Hmmm. The overload was intended to simplify the call in two places; it
hardly
seems worth it if it is only used in one place. But it can stay.
Suggest the following rewrite of the doc comment in HtmlDocletWriter.java
1309 /**
1310 * Converts inline tags and text to Content, expanding the
1311 * inline tags along the way. Called wherever text can contain
1312 * an inline tag, such as in comments or in free-form text arguments
1313 * to block tags.
1314 *
1315 * @param holderTag specific tag where comment resides
1316 * @param element specific element where comment resides
1317 * @param tags array of text tags and inline tags (often
alternating)
1318 * present in the text of interest for this element
1319 * @param isFirstSentence true if text is first sentence
1320 * @param inSummary true if the comment tags are added into the
summary section
1321 * @return a Content object
1322 */
Bonus points if you also do the equivalent clean up the comment in the
preceding method,
lines 1291-to 1303. :-)
No need to re-review if you just change this text.
-- Jon
On 09/20/2018 10:07 PM, Priya Lakshmi Muthuswamy wrote:
Hi Jon,
Yes as you mentioned, I didn't make a change between webrev.00 to
webrev.01 in ModuleWriterImpl.java.
The reason I mentioned it because, in the first review you had
suggested me to use an overload for commentTagsToContent and use that
in DocFilesHandlerImpl.java and ModuleWriterImpl.java, rather than
making changes in these two files.
So in webrev.01, I added the overload for commentTagsToContent. I
removed the changes made in DocFilesHandlerImpl.java, but retained the
change in ModuleWriterImpl.java.
To explained why I retained the change, I mentioned that comment.
/In ModuleWriterImpl, since we are writing to module summary, set the
inSummary parameter to true. /_
_I'm sorry if I was not clear in my explanation.
Thanks,
Priya
On 9/21/2018 5:40 AM, Jonathan Gibbons wrote:
Priya,
You have expanded more on what your comment means, but you've not
explained why
you are telling me this.
In particular, note the following two aspects of your revised review
request
1. You wrote:
updated webrev:
http://cr.openjdk.java.net/~pmuthuswamy/8202462/webrev.01/
In ModuleWriterImpl, since we are writing to module summary, set the
inSummary parameter to true.
2. But you did not modify ModuleWriterImpl in webrev.01 compared to
webrev.00.
In particular, this horribly long command shows you did not make any
additional changes to ModuleWriterImpl.java in webrev.01
$ diff
cr.openjdk.java.net/~pmuthuswamy/8202462/webrev.0{0,1}/raw_files/new/src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/ModuleWriterImpl.java
So, since you have made a point of telling me about that change, I'm
saying that I don't see that change in the updates you made to webrev.01.
Conversely, if you didn't mean to make any additional change to
ModuleWriterImpl.java, then why did you go out of your way to mention
the change?
-- Jon
On 09/19/2018 08:42 PM, Priya Lakshmi Muthuswamy wrote:
Hi Jon,
In ModuleWriterImpl, have set the last parameter(inSummary) to the
value true
providesTrees.put(t, commentTagsToContent(tree, mdle,
ch.getDescription(configuration, tree), false, true));
Since this method provides contents for the module summary file, set
the inSummary value to true.
Thanks,
Priya
On 9/20/2018 6:11 AM, Jonathan Gibbons wrote:
Priya,
Sort-of OK, but can you explain more about your comment about
ModuleWriterImpl:
In ModuleWriterImpl, since we are writing to module summary, set
the inSummary parameter to true.
I don't see any change between your two webrevs in
ModuleWriterImpl, and so I don't
understand what that comment means.
-- Jon
On 08/29/2018 06:07 AM, Priya Lakshmi Muthuswamy wrote:
Hi Jon,
Thanks for the comment.
updated webrev:
http://cr.openjdk.java.net/~pmuthuswamy/8202462/webrev.01/
In ModuleWriterImpl, since we are writing to module summary, set
the inSummary parameter to true.
Regards,
Priya
On 8/29/2018 5:42 AM, Jonathan Gibbons wrote:
DocFilesHandlerImpl.java
OK. It's a bit of a shame to have to edit this file. In
HtmlDOcletWriter, you added an overload for
getTagletWriterInstance, but not for commentTagsToContent.
HtmlDocletWriter.java
366: If you're going to add new javadoc comments, do it
properly/correctly; don't just cut-n-paste something that was
incomplete to begin with!
All the calls to addCommentTags are hard to read, with the
growing number of boolean parameters. This is OK for now, but
maybe we should used enums instead of booleans in some places to
aid readability.
1221 you've added a parameter but not updated the doc comment.
See next comment re: naming.
1233 summarySection ... suggest changing the name to be more like
a predicate, such as "inSummary" or "isSummary"
ModuleWriterImpl.java
Same comment as for DocFilesHandlerImpl.java
TagletWriter.java
Instead of having a non-final field, you're better to have the
first constructor call the second, like this:
74 private final boolean summarySection;
75
76 public TagletWriterImpl(HtmlDocletWriter htmlWriter, boolean
isFirstSentence) {
this(htmlWriter, isFirstSentence, false);
81 }
82
83 public TagletWriterImpl(HtmlDocletWriter htmlWriter, boolean
isFirstSentence, boolean summarySection) {
77 super(isFirstSentence);
78 this.htmlWriter = htmlWriter;
79 configuration = htmlWriter.configuration;
80 this.utils = configuration.utils;
85 this.summarySection = summarySection;
86 }
various: in this file you're unnecessarily qualifying member
references with "this.", as in code like this:
if (this.isFirstSentence && this.summarySection)
That's not a coding style for javadoc or even JDK. Please remove
unnecessary use of "this."
See previous comments about naming of "summarySection".
TestModules.java
OK
TestIndexTaglet.java
29: I'm mildly surprised that jtreg does not complain about the
"@" in "@index", but OK if it doesn't
Overall, it's a pretty weak test. This is especially so when you
look at lines 73-75.
73 checkOrder("pkg/A.html",
74 "<h3>Method Summary</h3>\n",
75 "a");
What that is saying is, make sure that after the "Method Summary"
heading, the character "a" appears. What are the chances of a
false positive there? :-) The indexed term needs to be much more
unique, such as "xyzzy" (look it up) or "Crumpets" or something.
And then in line 75, expand the check to include what the word
*should* be surrounded with. As written, the test would
incorrectly succeed if the doclet incorrectly was not changed,
because all you are checking for is the basic text, not the text
in context.
-- Jon
On 08/14/2018 01:58 AM, Priya Lakshmi Muthuswamy wrote:
Hi,
Kindly review the fix for
https://bugs.openjdk.java.net/browse/JDK-8202462
webrev : http://cr.openjdk.java.net/~pmuthuswamy/8202462/webrev.00/
Thanks,
Priya
