DocFilesHandlerImpl.java:198
Why is the `if` statement necessary? Why would the call on 201 behave
differently to the call on 199 when localTagsContent is empty? As I aid
in an earlier review, I think you can replace 198-202 with just 199.
HtmlDocletWriter: 431
Use Collections.emptyList instead of null, to simplify 458
HtmlDocletWriter:442
Please don't use the word "header" in this context. I suggest you
follow the convention in Head.java and call the parameter "extraContent".
HtmlDocletWriter: 458
Remove null check, and ensure that null is never passed in on 431.
Head.java
OK, although the old arrangement was deliberate/intentional, looking
down the road at better support of stylesheets, I guess the revised
version is more logical.
-- Jon
On 12/25/2018 08:05 PM, Priya Lakshmi Muthuswamy wrote:
Hi Jon,
Javadoc styles for header/navbar will appear as it is and will not get
disturbed.
You're assuming that the javadoc styles for the header/navbar will not
be affected by the user styles ;-)
I meant for any html content in doc-files, if there is any
user-defined style, then those should override the javadoc styles.
In html head element, default(javadoc) styles will be placed first and
then those defined in doc-file.
Thanks,
Priya
On 12/21/2018 9:24 PM, Jonathan Gibbons wrote:
I've not yet read the full webrev, but I do have one immediate
question, given below.
-- Jon
On 12/21/18 2:13 AM, Priya Lakshmi Muthuswamy wrote:
Hi Jon,
Thanks for the review.
I have made the following changes:
1)Added style tag in HtmlTag.java, before we were using style
attribute for comparison.
2)utils.getPreamble(element) ignores new lines after html tags, so I
can included them while fetching the localHeadTags.
3)In Head.java, used addContent() itself instead of new methods.
I had seen the addContent(), but it was adding the content before
the scripts/styles tags.
The doc-files script/style needed to added at the end of the other
styles it that it overrides them.
This is a red flag. Why is it important that the user styles
override the javadoc styles? Given that we're adding javadoc styles
for the javadoc header/navbar, isn't it important that those should
appear as intended?
When I checked the usage of addContent, i see that its been used
only by IndexRedirectWriter to include script/noscript tags.
I think that can be added at the end of existing tags as well.
updated webrev:
http://cr.openjdk.java.net/~pmuthuswamy/8214738/webrev.01/
api: http://cr.openjdk.java.net/~pmuthuswamy/8214738/api/
some of the doc-file with user specific styles.
api/jdk.jdi/com/sun/jdi/doc-files/signature.html
api/java.base/java/lang/doc-files/threadPrimitiveDeprecation.html
api/java.desktop/java/awt/doc-files/Modality.html
Thanks,
Priya
On 12/21/2018 3:16 AM, Jonathan Gibbons wrote:
Priya,
In DocFilesHandlerImpl, you have a boolean flag titleOrMetaTags.
Since <meta> elements never have content, and so never have end
elements, it seems misleading/incorrect to set the flag to true.
Also, you omit to handle ENTITY in the <title> element, so I added
that in as well. You could also write this code as a visitor if
you wanted, but the switch form is not too bad.
I suggest the code should look something like this:
205 List<DocTree> localTags = new ArrayList<>(); 206 boolean
inHead = false;
207 boolean inTitle = false; loop: 208 for (DocTree dt : dtrees) {
209 switch (dt.getKind()) {
210 case START_ELEMENT:
211 StartElementTree startElem = (StartElementTree) dt; switch
(HtmlTag.get(startElem)) { case HEAD: inHead = true; break;case
META: break; case TITLE: inTitle=true; break; default: if (inHead)
{ localTags.add(startElem); } }
223 break; 224 case END_ELEMENT: 211 EndElementTree endElem =
(EndElementTree) dt; switch (HtmlTag.get(endElem)) { case HEAD:
break loop; case TITLE:
inTitle = false;
break loop;default: if (inHead) {
localTags.add(startElem); } }
235 break; case ENTITY: 236 case TEXT:
237 if (inHead && !inTitle) {
238 localTags.add(dt);
239 }
240 break;
241 }
-- Jon
On 12/20/2018 11:31 AM, Jonathan Gibbons wrote:
On 12/19/2018 08:38 PM, Priya Lakshmi Muthuswamy wrote:
Hi,
Kindly review the fix for
https://bugs.openjdk.java.net/browse/JDK-8214738
webrev : http://cr.openjdk.java.net/~pmuthuswamy/8214738/webrev.00/
Thanks,
Priya
I don't think you need to add anything (userHeaderTags etc) to Head.
Head already has the requisite feature, "extraContent" and
"addContent"
which can be used to add content into the <head> element.
Re:
Utils.toLowerCase(nodeEnd.getName().toString()).equals(HtmlTag.HEAD.getText())
Don't use long-winded String comparison when you have type-safe
comparison.
In this case, you should be able to do something like
HtmlTag.get(nodeEnd.getName()) == HtmlTag.HEAD
getLocalHeaderTags needlessly walks through all the <body>
element. You can
break out of the loop once you hit </head>.
DocFilesHandlerImpl:196.
The "if" statement is a waste of time and effort. The code will
work perfectly
well when localTagsContent is an empty list, so you can simplify
196-199 to just 197.
TestCopyFiles.java
(style) re: indenting the continuation of multi-line strings
The general Java style for breaking long expressions is to break
before a binary operator, not after.
The style in other javadoc tests is to not indent the continuation
expressions, so that it is easier
to see how the first line and subsequent lines are aligned.
-- Jon
