RFR: 8283269: Improve definition and use of jdk.javadoc.internal.doclets.toolkit.Content

Fri Mar 25 18:28:48 UTC 2022

On Wed, 16 Mar 2022 16:32:09 GMT, Pavel Rappo <prappo at openjdk.org> wrote:

> This PR started in a draft mode as a refactoring effort. After a few commits and chats with Jonathan Gibbons, I decided that further refactoring belongs to follow-up PRs and that this PR could be marked as "Ready for review".
> 
> The motivation for the initial refactoring effort was as follows. The word "tree" is heavily overloaded in the JavaDoc codebase, both in comments and code. Here are some of the contexts this word appears in:  
> 
>   * Hierarchy Tree (i.e. overview-tree.html and package-tree.html)
>   * DocTree (AST nodes)
>   * ClassTree, *TreeWriter (data structures and entities related to supertype-subtype relationship)
>   * HtmlTree (HTML nodes) and specifically UL/OL elements which are nested lists
> 
> Sometimes contexts overlap, making the word "tree" ambiguous. To reduce this ambiguity, the word "tree" should be dropped in favor of a more specific word of phrase where possible.
> 
> In the case where the context is `jdk.javadoc.internal.doclets.toolkit.Content`, the programmer is already aware that they are dealing with trees in the sense that `Content` objects support recursive composition. There's no need to have the phrase "content tree" where "content" would do. Moreover, in the context that is exclusively about `Content` objects, the word "content" can be dropped too, since the type information is assumed.
> 
> As an example of content overlap, have a look at the source of `jdk.javadoc.internal.doclets.toolkit.builders.MemberSummaryBuilder::buildSummary`. This method used to needlessly mix DocTree with Content tree.

OK ...

Well, there's a huge amount of work here, most of which is a genuine improvement, so well done for all that, but I get the sense this work got out of control a bit, given there are many "themes" in this work, each of which would have been a worthy and significant PR in its own right.  As a result, the whole is much harder to review than the sum of its parts.

The primary theme seems to be to remove the word `tree` from method names and comments when it is either in appropriate or somewhat unnecessary.

Amongst the additional themes that are not directly related to the JBS issue:

* use of `var` 
* removing unnecessary comments from overriding methods
* use of `{@return}`
* minor grammar 

Don't get me wrong: these are all good improvements. It's just that doing them all together makes it harder to see the trees in the forest. (sic) 

You are somewhat fortunate in that there is not a lot of major concurrent development in javadoc right now; the merge could have been horrible!

So, to summarize, thanks for taking all this on; I'll approve it, but please file JBS issues for any follow-up cleanup that you consider still needs to be done.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AbstractExecutableMemberWriter.java line 268:

> 266:      *
> 267:      * @param member the member to get the exception information for
> 268:      * @return the exception information

For later ... use `Returns`

Also, maybe better grammar 
@param member the member for which to get/obtain/ the exception information

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AbstractMemberWriter.java line 190:

> 188:      * @param target the content to which the modifiers and type will be added
> 189:      */
> 190:     protected void addModifierAndType(Element member, TypeMirror type,

For later: maybe adjust method name as well

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AbstractOverviewIndexWriter.java line 134:

> 132:         String doctitle = configuration.getOptions().docTitle();
> 133:         if (!doctitle.isEmpty()) {
> 134:             RawHtml title = new RawHtml(doctitle);

why not `var`?

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/ConstantsSummaryWriterImpl.java line 71:

> 69: 
> 70:     /**
> 71:      * The HTML node for constant values summary currently being written.

This change seems unnecessary and just in produces an unnecessary synonym

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/Contents.java line 346:

> 344:      *
> 345:      * @param key the key for the desired string
> 346:      * @return the string

No, that suggests it returns the String itself

Suggest `content containing the string`

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/Contents.java line 359:

> 357:      * @param key the key for the desired string
> 358:      * @param o0  string or content argument to be formatted into the result
> 359:      * @return the string

see previous

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/Contents.java line 373:

> 371:      * @param o0  string or content argument to be formatted into the result
> 372:      * @param o1  string or content argument to be formatted into the result
> 373:      * @return the string

see previous

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/Contents.java line 388:

> 386:      * @param o1  string or content argument to be formatted into the result
> 387:      * @param o2  string or content argument to be formatted into the result
> 388:      * @return the string

see previous

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/Contents.java line 449:

> 447:      *
> 448:      * @param key the key for the desired string
> 449:      * @return the string

see similar

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/Contents.java line 461:

> 459:      *
> 460:      * @param text the string
> 461:      * @return the string content

interesting/inconsistent ;-)

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java line 321:

> 319:      * Adds the tags information.
> 320:      *
> 321:      * @param e the Element for which the tags will be generated

`Element` should be `element`

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java line 1906:

> 1904: 
> 1905:     /**
> 1906:      * {@return the annotation types info for the given element}

I would be surprised if this returns content containing the annotation types, as compared to the annotations themselves

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/PackageTreeWriter.java line 93:

> 91: 
> 92:     /**
> 93:      * Generate a separate tree file.

Generates

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/PackageWriterImpl.java line 80:

> 78: 
> 79:     /**
> 80:      * The HTML element for the section tag being written.

Hmm, it wasn't wrong to use `tree` here;   the goal should not be to remove as many uses of the word `tree` as possible!   Future developers will wonder, why does the code avoid all use of the word `tree`

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/Signatures.java line 393:

> 391:          * Set the return type for an executable member.
> 392:          *
> 393:          * @param returnType the return type to add.

remove period

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/Signatures.java line 415:

> 413:          * Set the parameter information of an executable member.
> 414:          *
> 415:          * @param content the parameter information.

maybe a general cleanup later on fixing trailing periods?

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/Signatures.java line 435:

> 433: 
> 434:         /**
> 435:          * Set the annotation information of a member.

Sets

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/TreeWriter.java line 166:

> 164:         String title = resources.getText("doclet.Window_Class_Hierarchy");
> 165:         HtmlTree bodyTree = getBody(getWindowTitle(title));
> 166:         bodyContents.setHeader(getHeader(PageMode.TREE)); // idempotent

why is the comment significant?

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/markup/Script.java line 102:

> 100: 
> 101:     /**
> 102:      * Returns a "live" view of the script as a {@code Content} object.

Don't change it now, but the comment abut a "live view" is sort-of unnecessary because in general most HTMLTree objects are mutable.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/ModuleSummaryWriter.java line 64:

> 62: 
> 63:     /**
> 64:      * Wrap the content into summary section.

Wraps

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/SerializedFormWriter.java line 71:

> 69: 
> 70:     /**
> 71:      * Add the serialized package to the serialized summaries.

Adds

-------------

Marked as reviewed by jjg (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/7843