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

[Priya Lakshmi Muthuswamy]

Thanks Jon. Updated the fix with the change.

webrev : http://cr.openjdk.java.net/~pmuthuswamy/8210047/webrev.01/

Thanks,
Priya

On 9/13/2018 3:47 AM, Jonathan Gibbons wrote:
> 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
>>
>