The new method on DocTrees does not have @since and needs a CSR.
I ran the new test to confirm all parts of the link. Nice, although
there are lots of "external link" icons.
The test has worn out a bit, because of independent changes. I sent you
(separately) a patchy for the new test. With that patch, the test works
for me.
OK to push if you patch the test and when you have got the appropriate
CSR approvals.
-- Jon
On 02/27/2020 02:56 AM, Hannes Wallnöfer wrote:
Thanks for the review, Jon.
New webrev: http://cr.openjdk.java.net/~hannesw/8177280/webrev.03/
Comments inline below.
Am 25.02.2020 um 00:11 schrieb Jonathan Gibbons <jonathan.gibb...@oracle.com>:
HtmlDocletWriter:1044
Changing RawHtml to StringContent is a significant behavioral change. It's not
explicitly stated in the doc comment spec whether the text may contain HTML,
but it is reasonable to infer from the descriptions of {@code} and {@literal}
that it may be HTML content. Is this change necessary?
You are right, that change was mostly driven by personal preference, which is
not a good guideline.
I reversed it in the new webrev.
CommentHelper ... clever changes, I think ;-)
In the new test, the direct/explicit use of
https://docs.oracle.com/en/java/javase/12/docs/api/ may cause issues later, in that
"sanity scripts" may discover the string and wonder if it is an out of date
reference. At a minimum, the use of the URL should be significantly commented in the
test. But, despite the comment on the `testValidLinks` method, I don't think it actually
checks that the links exist (as compared to 404-Not Found). Since you are using
-linkoffline, it may be sufficient to just do a global replace of
https://docs.oracle.com/en/java/javase/12/docs/api
to
http://example.com/docs/api
or something like that.
I replaced the URLs as suggested, indeed the test is passing.
Hannes
-- Jon
On 02/18/2020 03:26 AM, Hannes Wallnöfer wrote:
I have updated the patch to recent changes in CommentHelper, no changes
otherwise.
http://cr.openjdk.java.net/~hannesw/8177280/webrev.01/
Hannes
Am 07.02.2020 um 19:34 schrieb Hannes Wallnöfer <hannes.wallnoe...@oracle.com>:
Please review:
JBS: https://bugs.openjdk.java.net/browse/JDK-8177280
Webrev: http://cr.openjdk.java.net/~hannesw/8177280/webrev.00/
As I said previously there are some things in this patch I’m unsure or not
quite happy about.
Some notes:
- While the implementation of the new DocTrees method is quite simple, I’m not
sure if I should install a new Log.DeferredDiagnosticHandler for the runtime of
the method, like the attributeDocReference method in the same class does.
- In HtmlDocletWriter.seeTagToContent I changed the handling of the tag text to
escape HTML characters if no label is specified. This causes „<„ and „>“ to be
displayed in the browser instead of being interpreted as HTML tag if a generic link
target can’t be resolved. This is potentially problematic since @see and @link can
contain plain HTML content, but I think it’s ok since those cases are handled further
up in HtmlDocletWriter.seeTagToContent.
- That same method in HtmlDocletWriter contained some never-used code
(dependent on the BaseConfiguration.backwardCompatibility flag which is always
true) to use the fully qualified class name for links in some cases. I removed
that code and the field in BaseConfiguration since it adds to that already
long-winding method.
- Setting the label on a LinkInfoImpl basically disables rendering of type
arguments. While I understand the rationale behind this, it might still be
useful to set the label for the main link of a generic type. I’ve tried to
remove the restriction, but ran into a lot of problems (i.e. failing tests) in
other places. Since the current behaviour does what we need I decided to not
change this.
- There was a potential infinite recursion in
LinkFactoryImpl.getTypeParameterLinks caused by following the type parameters
of linkInfo.typeElement when linkInfo.type is set. The problem was that
linkInfo.typeElement may be set to the owner of linkInfo.type, and the solution
is to only follow that path if linkInfo.type is null.
- I removed two unused LinkInfo Kind enum values.
- I CommentHelper there were previously two and now three Visitors to extract
ReferenceTrees, so I added a common base class called ReferenceDocTreeVisitor.
- As an completely unrelated cleanup I removed the unused javafx field in
IndexBuilder.
Thanks,
Hannes
