On Thu, 29 Aug 2024 20:10:13 GMT, Jonathan Gibbons <j...@openjdk.org> wrote:
> Please review an update to "clean up" the direct use of HtmlTree constructors. > > Hitherto, many/most instances of `HtmlTree` were created by static factory > methods. This update extends that convention. > In most cases, this is by providing either simple no-arg factory methods or > commonly used overloads that take an `HtmlId` or `HtmlStyle`. > > For some tags, (`br`, `hr`, `wbr`) this allows a singleton instance to be > used. > For some of the more obscure cases, a more generic `HtmlTree.of(HtmlTag)` > method was used. > > Notes: > * some significant block-level nodes, like `pre`, should probably always set > a style, which could be enforced by suitable factory methods. That is > currently not the case and could be a future cleanup. > * some lists put the same style info on each list item, but might be better > placed on the enclosing list. That could be a future cleanup This is a very nice cleanup, Jon. The inconsistent use of `HtmlTree` has bugged me for a long time. The only major issue are the missing `final` modifiers on the new static fields in `HtmlTree` which also cause a javac test to fail as noted in a separate comment. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/IndexRedirectWriter.java line 107: > 105: > 106: var body = HtmlTree.BODY(HtmlStyles.indexRedirectPage) > 107: .add(bodyContent); I assume the `<main>` element is omitted intentially here? If so I'm fine with that, but I just want to make sure it's not an accidental change. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/SearchWriter.java line 114: > 112: .add(HtmlTree.of(HtmlTag.P) > 113: .setId(HtmlId.of("page-search-notify")) > 114: > .add(contents.getContent("doclet.search.loading"))) We could combine `.of` and `.add` into `HtmlTree.P(Content)` here. src/jdk.javadoc/share/classes/jdk/javadoc/internal/html/HtmlTree.java line 401: > 399: * Creates an HTML {@code BUTTON} element with the given id. > 400: * > 401: * @return the element Missing `@param` for `id`. src/jdk.javadoc/share/classes/jdk/javadoc/internal/html/HtmlTree.java line 410: > 408: * Creates an HTML {@code BUTTON} element with the given style. > 409: * > 410: * @return the element Missing `@param` for `style`. ------------- Changes requested by hannesw (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20778#pullrequestreview-2275308076 PR Review Comment: https://git.openjdk.org/jdk/pull/20778#discussion_r1740712485 PR Review Comment: https://git.openjdk.org/jdk/pull/20778#discussion_r1740718413 PR Review Comment: https://git.openjdk.org/jdk/pull/20778#discussion_r1740733779 PR Review Comment: https://git.openjdk.org/jdk/pull/20778#discussion_r1740734089