On Mon, 11 Jan 2021 23:55:16 GMT, Jonathan Gibbons <j...@openjdk.org> wrote:
>> Please review this change to centralize the management of HTML ids used by >> the standard doclet in a single new factory class, `HtmlIds`, which utilizes >> a new type-safe wrapper class around `String` called `HtmlId`. >> >> The new classes are used both when declaring ids (e.g. `HtmlTree.setId`) and >> when referencing them (e.g. `Links.createLink`). The enum `SectionName`, >> which declared a set of standard ids, goes away, as do methods in `Links` >> and other places to create "anchors". >> >> This is a simple refactoring: there is no intentional change of >> functionality, and no tests are affected or need to be updated. However, the >> changes do highlight the inconsistency of the use of ids: it would be good >> to rationalize these in separate, targeted follow-up work. > > Jonathan Gibbons has updated the pull request incrementally with one > additional commit since the last revision: > > tidy up merge Very nice work, Jon! +1 A few inline comments for an unused field and method and a couple of questions. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/markup/Links.java line 234: > 232: * @return a content tree for the link > 233: */ > 234: public Content createExternalLink(DocLink link, Content label) { The `createLink(DocLink, Content, boolan)` method above (line #221) that is replaced by this new method is not used anymore (and within it, the boolean parameter is not used). src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/markup/Table.java line 301: > 299: rowStyle = stripedStyles.get(rowIndex % 2); > 300: } > 301: Set<String> tabClasses = new HashSet<>(); // !! would be better > as a List I assume no bug has been filed for this? src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlIndexBuilder.java line 75: > 73: super(configuration, configuration.getOptions().noDeprecated()); > 74: this.configuration = configuration; > 75: links = new Links(DocPath.empty); It looks like `links` isn't used anywhere else in `HtmlIndexBuilder` and can be removed. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/markup/HtmlId.java line 34: > 32: * @see HtmlTree#setId(HtmlId) > 33: */ > 34: public interface HtmlId { Is there a reason for making this an interface instead of a plain class? ------------- Marked as reviewed by hannesw (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1951