Re: RFR: 8322708: Global HTML attributes are not allowed [v2]
On Tue, 11 Jun 2024 13:24:48 GMT, Nizar Benalla wrote: >> Can I please get a review for this change, that aims to add support for >> Global HTML tags. >> Here is the >> [link](https://cr.openjdk.org/~nbenalla/javadocGlobalPR/pkg1/package-summary.html) >> to the generated docs. >> Thanks in advance. > > Nizar Benalla has updated the pull request incrementally with one additional > commit since the last revision: > > Remove classpath exception I've ran tests locally for doclint and they all passed. Waiting on new results from tier 1-3 but there shouldn't be anything else that breaks. - PR Comment: https://git.openjdk.org/jdk/pull/19652#issuecomment-2167889419
Re: RFR: 8322708: Global HTML attributes are not allowed [v2]
On Tue, 11 Jun 2024 13:35:31 GMT, Chen Liang wrote:
>> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/HtmlTag.java line
>> 556:
>>
>>> 554: }
>>> 555:
>>> 556: private final EnumSet GLOBAL_ATTRS = EnumSet.of(
>>
>> I get it, you cannot make it static because it's used in an enum ctor. But
>> attaching it to every single enum constant seems wasteful.
>
> If we are ordering the entries, we can use comparable to check that an attr
> is greater than the start of the global attr, something like
>
> private static boolean isGlobalAttr(Attr value) {
> return value.compareTo(Attr.ACCESSKEY) >= 0;
> }
> If we are ordering the entries, we can use comparable to check that an attr
> is greater than the start of the global attr, something like
>
> ```java
> private static boolean isGlobalAttr(Attr value) {
> return value.compareTo(Attr.ACCESSKEY) >= 0;
> }
> ```
That would also work, yes. I still avoid relying on the order of enum
constants; but it's me.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19652#discussion_r1634916196
Re: RFR: 8322708: Global HTML attributes are not allowed [v2]
On Tue, 11 Jun 2024 12:45:10 GMT, Pavel Rappo wrote:
>> Nizar Benalla has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Remove classpath exception
>
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/HtmlTag.java line
> 556:
>
>> 554: }
>> 555:
>> 556: private final EnumSet GLOBAL_ATTRS = EnumSet.of(
>
> I get it, you cannot make it static because it's used in an enum ctor. But
> attaching it to every single enum constant seems wasteful.
If we are ordering the entries, we can use comparable to check that an attr is
greater than the start of the global attr, something like
private static boolean isGlobalAttr(Attr value) {
return value.compareTo(Attr.ACCESSKEY) >= 0;
}
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19652#discussion_r1634905873
Re: RFR: 8322708: Global HTML attributes are not allowed [v2]
On Tue, 11 Jun 2024 13:24:48 GMT, Nizar Benalla wrote:
>> Can I please get a review for this change, that aims to add support for
>> Global HTML tags.
>> Here is the
>> [link](https://cr.openjdk.org/~nbenalla/javadocGlobalAttrs/pkg1/package-summary.html)
>> to the generated docs.
>> Thanks in advance.
>
> Nizar Benalla has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove classpath exception
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?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19652#discussion_r1634890608
Re: RFR: 8322708: Global HTML attributes are not allowed [v2]
> Can I please get a review for this change, that aims to add support for > Global HTML tags. > Here is the > [link](https://cr.openjdk.org/~nbenalla/javadocGlobalAttrs/pkg1/package-summary.html) > to the generated docs. > Thanks in advance. Nizar Benalla has updated the pull request incrementally with one additional commit since the last revision: Remove classpath exception - Changes: - all: https://git.openjdk.org/jdk/pull/19652/files - new: https://git.openjdk.org/jdk/pull/19652/files/bbd9e017..a1292496 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19652&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19652&range=00-01 Stats: 6 lines in 1 file changed: 0 ins; 5 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19652.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19652/head:pull/19652 PR: https://git.openjdk.org/jdk/pull/19652
