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
