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

[Jonathan Gibbons]

Ok, close enough.

Minor,

In HtmlDocWriter:

2110     /**
2111      * creates the HTML tag if the tag is supported by this specific HTML version
2112      * otherwise return the Content instance provided by Supplier ifNotSupported
2113      * @param tag the HTML tag
2114      * @param ifSupported create this instance if HTML tag is supported
2115      * @param ifNotSupported create this instance if HTML tag is not supported
2116      * @return
2117      */

2111: Should be "Creates" (starts with capital letter.)
2112: Should end with "." (period)

No need to re-review.

-- Jon

On 09/14/2018 01:14 AM, Priya Lakshmi Muthuswamy wrote:
> 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
>>>
>>
>