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 <[email protected]>: > > 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 >>> <[email protected]>: >>> >>> 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 >
