RFR: 8210047 : api/overview-summary.html contains content outside of landmark region

[Jonathan Gibbons]

Wow, that turned out really nicely; well done.

I have one minor stylistic suggestion.

Code like this turns up in a number of places:

  119         Content header;
  120         if (configuration.allowTag(HtmlTag.HEADER)) {
  121             header = HtmlTree.HEADER();
  122         } else {
  123             header = new ContentBuilder();
  124         }

It ought to be possible to use method handles and a new shared utility 
method to simplify it
in all places to something like this:

  119         Content header = createIfAllowed(HtmlTag.HEADER,HtmlTree::HEADER,  ContentBuilder::new);

where the utility method is something like

          Content createIfAllowed(HtmlTag tag, Supplier<Content> ifSupported, Supplier<Content> ifNotSupported) {
  120         if (configuration.allowTag(tag)) {
  121             return ifSupported.get();
  122         } else {
  123             return ifNotSupported.get();
  124         }
          }


The name could maybe be improved. The method could reasonably be a 
protected method in HtmlDocletWriter.


For what it's worth, it could be simplified even more:

  119         Content header = createTreeOrContent(HtmlTag.HEADER);

where the utility method is something like

          Content createTreeOrContent(HtmlTag tag) {
  120         if (configuration.allowTag(tag)) {
  121             return new HtmlTree(tag);
  122         } else {
  123             return new ContentBuilder();
  124         }
          }





-- Jon


On 09/12/2018 04:57 AM, Priya Lakshmi Muthuswamy wrote:
> Hi Jon,
>
> updated the webrev with the changes for frames and the refactoring 
> done for handling the tags.
>
> webrev : http://cr.openjdk.java.net/~pmuthuswamy/8210047/webrev.00/
> [accidentally uploaded the changes in webrev.00 itself]
>
> Thanks,
> Priya
>
> On 8/31/2018 5:54 AM, Jonathan Gibbons wrote:
>>
>>
>> On 08/28/2018 01:25 AM, Priya Lakshmi Muthuswamy wrote:
>>> Hi,
>>>
>>> Kindly review the fix for 
>>> https://bugs.openjdk.java.net/browse/JDK-8210047
>>> webrev : http://cr.openjdk.java.net/~pmuthuswamy/8210047/webrev.00/
>>>
>>> Thanks,
>>> Priya
>>
>>
>> Just because we don't use frames in the JDK API docs doesn't mean 
>> they're
>> not a supported feature, for a while at least.
>>
>> So, shouldn't the fix also cover docs where frames -are- in use.
>>
>>
>>
>> Separately, the fix is "ugly" because it makes a (different) bad 
>> situation worse,
>> and I'm not sure at this stage what the best way forward is.
>>
>> The general bad situation, that needs cleaning up, is the overall 
>> handling
>> of "htmlTree" and HtmlTag.MAIN.  The existing code is pretty ugly in the
>> way that htmlTree is set up (too early) and then later handled with code
>> like
>>
>>  165             if (configuration.allowTag(HtmlTag.MAIN)) {
>>  166                 htmlTree.addContent(div);
>>  167             } else {
>>  168                 body.addContent(div);
>>  169             }
>>
>> It would be better to be building stuff in a more bottom up approach so
>> that you build the content, and then at a single place, decide whether
>> it needs to be wrapped in a MAIN tag.
>>
>> I need to think whether we should go with your fix, and make more places
>> that need to be cleaned up later, or whether we should just get it 
>> right, now.
>>
>> -- Jon
>