RFR: 8223378: CSS solution for navbar problem with <'a> elements is not ideal

Wed Nov 27 00:38:34 UTC 2019

Generally good, and OK to push.  But some comments follow:

It was nice to see various bits of code being "removed" from the 
left-hand side of the webrev. Yay!

In HtmlDocletWriter, is that a spurious new import (for Navigation)?

TreeWriter ... in most places you declare and initialize bodyContents in 
a single line;  In TreeWriter, you split the decl and init.

ClassBuilder ... it is conceptually wrong ... a breakdown of abstraction 
... for classes in the toolkit.** packages to depend on classes for a 
specific format, such as the classes in the formats.html.** packages.  
However, I think that in this case, they're just spurious left-over 
imports that can easily be removed.  The general intent is that 
"Content" is a general format-neutral container for use in the 
toolkit.** world; and all subtypes of Content (except maybe 
ContentBuilder?) belong and should only be used within the 
format-specific world to which they belong.  This is definitely an area 
where smaller modules (and module boundaries) would be helpful. 
Conceptually, there are 3 blobs of code in javadoc: the tool, the 
"abstract format-neutral doclet" and the "HTML-specific doclet".  In a 
different world, these would be in 3 separate modules.

I still think that the instances of BodyContents could be moved up into 
a single protected final declaration in HtmlDocletWriter.

I still think that (eventually, in another round of cleanup) that 
BodyContents can evolve all the way into Body and take care of 
generating the <body> tag as well. The remaining code to handle the 
<body> tag feels like a bit of an outlier than could be merged into 
BodyContents. But, there's already enough refactoring in this webrev!

Well done,

-- Jon

On 11/26/19 8:15 AM, Hannes Wallnöfer wrote:
> Thanks for the review, Jon. I uploaded a new webrev.
>
> http://cr.openjdk.java.net/~hannesw/8223378/webrev.03/
>
> Here’s an overview of changes compared to the previous webrev:
>
>   - BodyContent has been renamed to BodyContents, and
>   - Now uses „fluent“ BodyContents API wherever it makes sense. Instead of local variable declaration most MainContents are now created, built, and consumed in-place where they are needed.
>   - „top" content is added to a ContentBuilder before Navigation content instead of putting it inside the <nav>. There is also a new test to check presence of „top“ content with -nonavbar option (more details below)
>   - Removed unused contentTree fields and parameters
>   - There were a few cases where I passed a <main> tree to BodyContents.addMainContent. This resulted in duplicate <main> elements because BodyContents.toContent generates its own <main>. These are fixed now.
>   - Added the bug-id for this change to the changed tests
>
> More details on some of these changes inlined below.
>
>> Am 22.11.2019 um 02:20 schrieb Jonathan Gibbons <jonathan.gibbons at oracle.com>:
>>
>> I think its wrong to move the "top" content into the Navigation class. It doesn't help that the Navigation class is not well designed in the first place and needs to be overhauled anyway.  Instead of moving "top" into Navigation, I think there are a couple of possibilities.
>>
>> 1. Add a new Header class for the header of the page, containing the top text and the content of the Navigation object.
>>
>> 2. #1 may be overkill, in which case you could extend/generalize your new BodyContent(s) class so that instead of providind "setHeader"/"setFooter", you provide "addHeaderContent", "addFooterContent".  Then the code to "addTop" into the Navigation object becomes a pair of calls to add the top into the BodyContents header, followed by the contents of the Navigation object.
>>
>> 3. Create a ContentBuilder.  Call "addTop" to put the top content into that content builder, then add Navigation.toContent() into the content builder, and finally set the content builder  as the header of the bodyContents.   Of these 3 suggestions, I think I recommend this one.
>>
> I followed suggestion 3 as described above.
>
>
>>   
>> c. There is silliness in the code.  Look at the code-flow in Navigation.toContent.
>>      Content contentTree = new
>> ContentBuilder();
>>      
>> if (!configuration.nonavbar
>> ) {
>> 	// compute tree
>>          
>> return
>> tree;
>>      }
>>      
>> return
>> contentTree;
>>
>>
>> This could be simplified/improved.
>
> I fixed just this particular issue by immediately returning a new ContentBuilder if the -nonavbar option is enabled.
>
>> d. By moving the "top" text into Navigation, you have made it subject to the `-nonavbar` option, embodied by `configuration.nonavbar`. That's an inappropriate change in behavior. The top text is typically used for legal info and should not be hidden if `-nonavbar` is specified.
> Actually „top“ content was already dependent on the navbar in the current code, because HtmlDocletWriter.addTop ignored its htmlTree argument and added the top text to fixedNavDiv, which was in turn used by/embedded in the navbar. This is now fixed as described above, and I also added a test TestTopOption.testNoNavbar for it (fails with current code).
>
>> Should the various declarations of bodyContent in the various subtypes of HtmlDocletWriter be replaced by a single protected declaration in HtmlDocletWriter itself?
>>
> Not all HtmlDocletWriter subclasses need/use a field for BodyContents. Having just one declaration might make the code simpler and more uniform. For now I’ve left it as it was because I don’t want this change to become too big, but it’s something we should consider for future cleanup.
>
>> I noticed the following on PackageSummaryWriter/PackageWriterImpl, but there may be equivalent instances on other pages as well.  Look at the changes towards the end of the file, line 295, 307.
>> The parameter "contentTree" is now unused.  You've changed the semantics of the method from "add the 2nd argument to the 1st argument" to "add the 2nd argument to a class member". That in itself is not wrong, but it is not the semantics documented in the overriden method.  One way or another, you should align the spec/impl of the super method and the impl method.  That being said, I think this ties into some similar related issues, with respect to other add... methods and the use of the page-global bodyContent member.
>>
> There were a few cases of this passing around unused arguments, and actually it was quite a bit worse than that: Those classes had a contentTree field that was passed uninitialised to the main build* method, where the return value of the get*Header method was assigned to the parameter, which was then passed on as contentTree. Basically the field was never assigned or used.
>
> I fixed these classes by removing the field and parameter declarations, and only passing contentTree to the methods that actually use it, which is mostly printDocument.
>
>
>> I noticed this on SerializedFormWriterImpl.java; it may apply to other pages as well.  You've changed the impl of printDocument, which is supposed to (just) print the tree that is provided as an argument.  It seems wrong to be adding in bodyContent at this point, and related to the previous comment about adding nodes into page-global variables instead of the given arguments.
>>    
> It’s not ideal to add the BodyContents in printDocument, but it’s the safest thing to do with the current code layout. If we add the BodyContents before it is fully filled  some part will be missing in the resulting document. By adding it just before generating the document we are sure to have the full contents. Not great, but think it’s acceptable for now.
>
>> OK, now I see a root cause of the problems and why you have done some of what you have done.  I see that the issue is the historical split between the format-neutral toolkit world and the formats.html world ... and that `BodyContent` is a builder but not a subtype of Content.  The problem is exacerbated somewhat by the inconsistent asymmetry between the use of `get` methods that return Content and `add` methods that take a Content as an argument.
>>
>>
>> Temporary Summary:
>>
>> I don't have a good solution/suggestion at this point. I need to think about this. I'm sure we don't want to get into an big refactoring of the Builder/Writer/Impl APIs, but equally, it seems difficult to conform to those APIs as specified.
>>
>> That being said, I'm beginning to see a way forward in which the API is biased more towards to imperative `void addXyx(...)` style, at least at the outer levels of the page.  Taking PackageWriterImpl as an example, look at getPackageHeader.  It too does not really conform to its declared specification.  It creates a mostly empty `bodyTree`, then stashes most everything in member variables for `navBar` and `bodyContent` and finally returns the empty `bodyTree` ...   which is not the package header implied by the name of the method ... and then eventually, there's a second sleight-of-hand when the `bodyContent` is surreptiously slipped into the mostly empty body at the last minute in `printDocument`.  Perhaps we should     really go "all-in" on side-effect methodology and have *WriterImpl just build up <body>/bodyContent/navBar in member variables. In other words, use one instance of (a subtype of) HtmlDocletWriter per  page, which we already do, building up member variables for the page, resulting in a final parameter-less call of `printDocument()` when the content of the page has finally been prepared.
> I agree we should do a cleanup of the Builder/Writer code structure, but that’s not for 14.
>
> This change doesn’t really fix all things that should be fixed, but at think it slightly improves some parts of it and doesn’t make anything worse.
>
> Hannes
>
>
>