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
