I forgot to add the link to the new webrev. Here it is:
http://cr.openjdk.java.net/~prappo/8251550/webrev.01/
> On 17 Aug 2020, at 13:30, Pavel Rappo <[email protected]> wrote:
>
> Thanks for having a good look at it, Jon. My replies are inline.
>
>> On 14 Aug 2020, at 17:49, Jonathan Gibbons <[email protected]>
>> wrote:
>>
>> Good cleanup.
>>
>> Some systemic changes needed. Details below.
>>
>> -- Jon
>>
>> On 8/13/20 11:48 AM, Pavel Rappo wrote:
>>> Hello,
>>>
>>> Please review the below change for
>>> https://bugs.openjdk.java.net/browse/JDK-8251550
>>>
>>>
>>>
>>> http://cr.openjdk.java.net/~prappo/8251550/webrev.00/
>>>
>>>
>>> This represents squashed commits that have accumulated in my git branch.
>>> Since the changeset has started to look dangerously big, I decided not to
>>> wait until we finish the migration to Git and GitHub, and push it sooner.
>>> The less the others have to merge, the better.
>>>
>>> -Pavel
>>>
>>>
>>
>>
>> Some of the doctree changes contain this sequence:
>>
>> <p>
>> <pre>
>>
>> That will trigger a doccheck warning for a redundant <p> tag.
>
> Fixed.
>
>> Since you're improving the doc comments for the DocTree nodes, it would be
>> nice to use <i>...</i> or (better) <var>,,,</var> around the variables in
>> the templates. For example:
>>
>> 34 * <pre>
>> 35 * @author <var>name-text</var>
>> 36 * </pre>
>>
>> 37 *
>
> Understood. However, for now I would prefer consistency (with the interfaces
> of the com.sun.source.tree package) over further improvement.
>
>> DocRootTree:
>>
>> I think you need {@literal ...} on line 33.
>
> Why? The doc comment uses an HTML entity, not a naked "@" symbol:
>
> * <p>
> * <pre>
> * {@docroot}
> * </pre>
>
> Are you looking at the actual *diff* rather than at a *view* of the webrev?
> Webrev is infamous for producing a certain type of illusions.
>
>> DocTypeTree:
>> As an enhancement, given that the tools only support HTML 5 these days, It
>> might be helpful to give the specific text for an HTML 5 document
>>
>> 32 * <pre>
>>
>> 33 * <!doctype text>
>>
>> 34 * </pre>
>> + * For HTML5 documents, the correct form is {@code <!doctype html>}.
>
> Added the proposed line.
>
>> EntityTree:
>> Maybe remove the "misleading" spaces in the examples?
>
> Removed the spaces.
>
>> General:
>>
>> Thinking aloud, for a future update when we have the `{@spec}` tag. In the
>> introductory sentence for each tag, replace the use of `{@code}` by
>> `{@spec}` linking to the right place in the doc-comment tag specification. I
>> guess for many, it's not necessary, but the idea came to mind reading
>> IndexTree, where more information may be helpful. That being said,
>> additional info belongs in the tag spec, not the tree node spec.
>
> Agreed.
>
>> DocSourcePositions:57,89
>> replace `CompilationUnit` with `compilation unit`
>
> Replaced.
>
>> DocTreeFactory:112
>> We are inconsistent about whether to use `{..}` for inline tags, which will
>> be worse when we add our first bimodal standard tag, `@return`, but using
>> `{...}` for `{@deprecated}` is definitely wrong
>
> Yes, I have that thought too. I fixed "@deprecated" along with other block
> tags in DocTreeFactory.java. On the other hand, I added missing curly braces
> around "@summary".
>
>> General: (question)
>> Does a trailing space inside `{@code something }` cause extra whitespace in
>> the rendered HTML?
>
> Yes, it does. I removed those *outer* trailing whitespace characters.
>
>> DocTreePath:37,96,98,99
>> More un-code-d type names. Follow existing conventions for either using
>> `{@code...}` or the descriptive equivalent (e.g. "tree path")
>
> @code-d those and some others.
>
>> Trees:239,240
>> replace "The" by "the"
>
> Replaced there and in one other place.
>
>> remove package name from TypeMirror, and use @code for that and ErrorType
>
> Removed the package, but will NOT do the rest: it would be completely
> inconsistent with everything else in that file.
>
>> DocCommentParser:
>> avoid Latin abbreviations; use "for example, such as"
>
> Fixed. Although, I don't get the rationale behind this piece of advice. One
> reason not to use any abbreviations would be the period characters: periods
> interfere with detecting first sentences in doc comments.
>
>> 1103: another candidate for `{@spec}` !!
>
> Yes. I just felt I have to provide something more fresh and immediately
> accessible.
>
>> Pretty: removing import
>> In general, this is a dangerous edit; throughout javac, you will see imports
>> of javac.util.List in preference to java.util.List. I assume you have
>> compiled and run the tests, to validate this change.
>
> Correct, before posting the initial webrev I ran the tiers 1 through 3 in our
> CI. It was fine.
>
>> WriterFactory:135
>> Trailing period.
>
> Removed there and in some other places.
>
>> BaseTaglet
>> Yay for removing redundant doc comments from overridden methods!
>>
>> toolkit/taglets/Taglet.java
>> 41,143:
>> If you think "tag" refers to instances in doc comments, the original was
>> correct
>> If you think "tag" refers to the class of the instances, the new text should
>> use "the block tag"
>
> Those terms have to be better defined in the spec.
>
>> 142: consider remove "all" from end of line
>
> Removed.
>
> -Pavel
>
>> -- Jon
