RFR: 8202462 : {@index} may cause duplicate labels

[Priya Lakshmi Muthuswamy]

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

[Jonathan Gibbons]

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

Post by Priya Lakshmi Muthuswamy
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

[Priya Lakshmi Muthuswamy]

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

Post by Jonathan Gibbons
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
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
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
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

Post by Priya Lakshmi Muthuswamy
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

[Jonathan Gibbons]

Priya,

Sort-of OK, but can you explain more about your comment about

Post by Priya Lakshmi Muthuswamy
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

Post by Priya Lakshmi Muthuswamy
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

Post by Jonathan Gibbons
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
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
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
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

Post by Priya Lakshmi Muthuswamy
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

[Priya Lakshmi Muthuswamy]

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

Post by Jonathan Gibbons
Priya,
Sort-of OK, but can you explain more about your comment about

Post by Priya Lakshmi Muthuswamy
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

Post by Priya Lakshmi Muthuswamy
Hi Jon,
Thanks for the comment.
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

Post by Jonathan Gibbons
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
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
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
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

Post by Priya Lakshmi Muthuswamy
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

[Jonathan Gibbons]

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

Post by Priya Lakshmi Muthuswamy
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

Post by Priya Lakshmi Muthuswamy
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

Post by Jonathan Gibbons
Priya,
Sort-of OK, but can you explain more about your comment about

Post by Priya Lakshmi Muthuswamy
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

Post by Priya Lakshmi Muthuswamy
Hi Jon,
Thanks for the comment.
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

Post by Jonathan Gibbons
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
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
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
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

Post by Priya Lakshmi Muthuswamy
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

[Priya Lakshmi Muthuswamy]

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
__

Post by Jonathan Gibbons
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

Post by Priya Lakshmi Muthuswamy
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

Post by Priya Lakshmi Muthuswamy
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

Post by Jonathan Gibbons
Priya,
Sort-of OK, but can you explain more about your comment about

Post by Priya Lakshmi Muthuswamy
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

Post by Priya Lakshmi Muthuswamy
Hi Jon,
Thanks for the comment.
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

Post by Jonathan Gibbons
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
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
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
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

Post by Priya Lakshmi Muthuswamy
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

[Jonathan Gibbons]

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

Post by Priya Lakshmi Muthuswamy
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

Post by Jonathan Gibbons
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

Post by Priya Lakshmi Muthuswamy
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

Post by Priya Lakshmi Muthuswamy
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

Post by Jonathan Gibbons
Priya,
Sort-of OK, but can you explain more about your comment about

Post by Priya Lakshmi Muthuswamy
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

Post by Priya Lakshmi Muthuswamy
Hi Jon,
Thanks for the comment.
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

Post by Jonathan Gibbons
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
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
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
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

Post by Priya Lakshmi Muthuswamy
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