On Tue, 11 Jun 2024 13:26:28 GMT, Pavel Rappo <[email protected]> wrote:
>> Nizar Benalla has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains ten additional
>> commits since the last revision:
>>
>> - Null safety
>> - Merge branch 'master' into html-attributes
>> - remove trailing whitespace
>> - -Add a boolean attribute to the enum type
>> -Simply regex in `visitAttribute`
>> -Simplified the Test
>> -Added a negative test
>> - no longer print summary
>> - no longer print summary
>> - Add small comment
>> - Remove classpath exception
>> - Allow global variables
>
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/Checker.java line
> 695:
>
>> 693: // custom "data-*" attributes are also accepted
>> 694: var attrName=name.toString();
>> 695: if (!attrName.startsWith("on") &&
>> !attrName.matches("data-[a-z]+(-[a-z]+)*")){
>
> I understand that `startsWith("on")` precedes this PR. However, it's more
> permissive than it should be: in reality, there are only 70 global attributes
> that start with "on". On the other hand,
> `attrName.matches("data-[a-z]+(-[a-z]+)*")` is less permissive. For example,
> if I read the XML spec correctly, such an attribute can contain `_` or start
> with `data-.`.
>
> But maybe those checks are good enough?
I think like being slightly restrictive and safe.
> test/langtools/jdk/javadoc/doclet/TestGlobalHtml/TestGlobalHtml.java line 41:
>
>> 39: var tester = new TestGlobalHtml();
>> 40: tester.runTests();
>> 41: tester.printSummary();
>
> Why is this summary needed? Usually, we don't print it.
Fixed in
[cf4dab6](https://github.com/openjdk/jdk/pull/19652/commits/cf4dab6ceefb8625e9e7b281073cba49b894d13c).
I was using it locally.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/19652#discussion_r1635017243
PR Review Comment: https://git.openjdk.org/jdk/pull/19652#discussion_r1634982192
