RFR: JDK-8240138: Cleanup HtmlTree

[Jonathan Gibbons]

Hi Pavel,

Thanks for your detailed feedback.  Some responses inline.  Most are 
discussion points that probably do not lead to another webrev. Some are 
about minor typos etc.

-- Jon


On 03/11/2020 06:30 AM, Pavel Rappo wrote:
> Hi Jon,
>
> 1. Some methods, constructors, enum constants, and unused imports have gone.
>
> 2. HtmlTree.HEADING(..., boolean printTitle, ...) has been split into 2 methods,
> HtmlTree.HEADING and HtmlTree.HEADING_TITLE.
>
> On a related note, it's satisfying to see that more and more calls to "new HtmlTree"
> are being substituted with calls to convenience static factory methods.

In another HTML-generator project in which I'm using similar HTML 
classes, I've made
the constructor for HtmlTree private, to "force" the use of the static 
factory methods.
If we did that, we might also consider moving/renaming HtmlTag to 
HtmlTree.Kind

>
> 3. The doc comments have been updated and cleaned up.
>
>    ***
>
> The bellow comments are for the preexisting issues in the code, incidentally
> highlighted by this change.
>
> 1. We could switch from Collections.{emptyMap(), emptyList()} to {Map, List}.of()
> in HtmlTree.

Yes, this code predates those methods.

>
> 2. HtmlTree.EMPTY should probably be of FixedStringContent instead. Maybe it can
> even be a specific final and immutable subtype of Content.

There's a bigger story here. EMPTY was initially just intended (in an 
over-abundance
of caution) as a marker to allow the creation of not-yet-populated 
container nodes,
and as a workaround for factory methods requiring a Content argument.

My sense is that we no longer need this mechanism, and if we do, it 
should be handled
by the client/generator code. In other words, we should be able to get 
rid of the
sentinel-use of HtmlTree.EMPTY, and change remaining uses to 
FixedStringContent.EMPTY.

But, that change needs careful verification, since it means removing the 
built-in check
for empty nodes in HtmlTree.add.

>
> 3. jdk.javadoc.internal.doclets.formats.html.markup.HtmlTree.add(java.lang.CharSequence)
> The following part of the doc looks like an implementation detail, and maybe
> should be an inline comment (or an @implNote) instead:
>
>      * If the last content member that was added is a {@code StringContent},
>      * append the string to that item; otherwise, create and use a new {@code StringContent}
>      * for the new text content.
Yeah, as is often the case, the comments predate other features, 
although strictly speaking,
@implNote (and friends) are not a javadoc feature, they are still just  
a JDK-makefiles feature.
I also think there can be a lower bar for internal API documentation, 
compared to public API
documentation.
>
> Also note, that the verbs are in imperative mood.
Yes, that is something we can make consistent.
>
> 4. Should we just inline Content.nullCheck to Objects.requireNonNull across
> the codebase at this stage?
I was wondering that when I wrote this changeset. I think it's a good 
and separable idea.
As is often the case, this part of javadoc predates Objects.requireNonNull.

>
> 5.  /**
>       * Creates an HTML {@code SPAN} element with the given style and some content.
>       *
>       * @param body  the content
>       * @return the element
>       */
>      public static HtmlTree SPAN(HtmlStyle styleClass, Content body) {}
>
> "@param styleClass" is missing
Oops. will fix.
>
> Nits
> ====
>
> Many of the below issues are also preexisting.
>
> 1. "Creates an {@code HTMLTree} object with a given kind of tag." should be {@code HtmlTree}
Noted.
>
> 2. "Apply percent-encoding to a URL." should be in declarative mood, "Applies"
Notes.
>
> 3. All the occurrences of "... and some content" read strangely to me
> (as opposed to "given/specified/provided content" or similar)
The phrasing was intended to cover a variable amount of content, 
including in some cases, no content.
>
> 4. Multiple occurrences of "* @param body  the content" (note an extra space)
probably cut-n-paste
>
> 5. "charSet" is cased strangely
Per-English words, it is correct; but I agree that by JDK standards it 
is weird.
>
> 6 "Adds additional content for the HTML element." smells like tautology

It is, I'll try and rephrase.
>
> 7. Do you think we should add a comment here?
>
>      else if (content == HtmlTree.EMPTY || content.isValid()) {  /* implicit nullcheck for content */
See earlier discussion about removing this check altogether.
>
> 8. Commas look strangely here:
>
>      Creates an HTML {@code TH} element with the given scope, and some content.
>      Creates an HTML {@code TH} element with the given style and scope, and some content.
I'll revisit phrasing of comments involving "some content".

>
> 9. The doc comment for HtmlTree.stripHtml is a bit messy.
>
> 10. Some conditional statements are missing curly braces.
That was an earlier code-style, and is even accepted in the unofficial 
Andreas Lundblad Code Style Guidelines.

>
>
> Otherwise looks good to me. Thanks for doing this!
>
> -Pavel
>
>> On 10 Mar 2020, at 23:06, Jonathan Gibbons <jonathan.gibbons at oracle.com> wrote:
>>
>> Please review another moderately simple cleanup, this time for the HtmlTree class.
>>
>> The cleanup started as a general improvement to the null checking, but ended up including improvements to the documentation, and to the way that headings are created, the last of which involved corresponding changes at use sites.
>>
>> Null-checking:
>>
>> When generally building data structures that should not contain null, it is generally better to check for null when creating the data structure, to prevent NPEs much later on when the data is accessed.  But the HtmlTree class did excessive checking, which can be simplified to the points at which items are stored in the attributes or content of the tree nodes.
>>
>> Cleaning up the null handling also involved cleaning up some overloaded methods in which null was being used to indicate an optional value, defeating the intent of the otherwise good null checking. This was most evident in the methods taking a 'style' parameter.
>>
>> Documentation:
>>
>> Previously, comments were added that used the word 'tag' as a way to avoid using the word 'element' to avoid confusion with the word 'element' as used to refer to an instance of javax.lang.model.element.Element. But that was an incorrect usage of 'tag'. The usage is corrected to used the term 'HTML element' instead. (The name of the HtmlTag class is also not ideal, but it is not proposed to change that at this time. If it were to be changed, it could be converted to HtmlTree.Kind)
>>
>> Headings:
>>
>> Some of the utility methods to create headings took a poorly named boolean parameter to indicate whether the title attribute should be set on the heading element. This lead to some clumsy implementation code. Instead, the overloads have been separated into two groups, with different names, ... those which do set the title attribute, and those which do not. This change affects use sites as well, which showed up other inconsistencies, such as not using the utility methods at all.
>>
>> It is a separate question whether the title attribute should be set on any heading element: the answer is "probably not", but that issue is not addressed here, and is left for a separate followup. Changing this will probably affect tests and the goal of this changeset is to not affect any tests.
>>
>> --------------
>>
>> I suggest that the changes in HtmlTree are reviewed first. Once the changes to the utility methods to create headings are understood, the changes in the other files should be relatively simple to understand as well. There are a few additional cleanups, like minor reformatting changes and deleting unused declarations.
>>
>> -- Jon
>>
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8240138
>> Webrev: http://cr.openjdk.java.net/~jjg/8240138/webrev.00/index.html
>>