Jon, Thanks for the detailed explanation. I learned a few new things.
+1 Hannes > Am 23.07.2018 um 20:49 schrieb Jonathan Gibbons <[email protected]>: > > Hannes, > > Thanks for looking at this, and for your questions. Answers inline. > > -- Jon > > > On 07/20/2018 08:24 AM, Hannes Wallnöfer wrote: >> Hi Jon, >> >> I don’t quite all aspects of this fix, which is why I didn’t send a review >> in the first place. But since nobody else did either, I’ll just ask a few >> questions to help me better understand. >> >> My main area of confusion is related to what is considered linkable and what >> not. This should be well-defined, but from your description below (saying >> this is just a partial fix) it doesn’t seem to be. >> > > Yes, it's not well-defined, at least not in any moderately formal > documentation. This is another area in which javadoc uses and reuses > low-level constructs without wrapping them in higher level abstractions. The > point about this just being a partial fix was because I see the new method > isLinkable to be the place to encapsulate what it means to be linkable, where > "linkable" means "the ability the provide a non-broken, clickable link to a > useful and relevant target." As we investigate more broken links in the > docs, we may see the new method being enhanced to accommodate that cases. > >> Specific to your webrev, why are members of undocumented super types >> considered linkable? > > This is another undocumented "quirk" of javadoc, that is reasonable in > hindsight when you think about it. If a package-private superclass has > public methods, those public methods can be invoked by a client using a > public subtype. The question then becomes, where should those methods be > documented? If the immediately enclosing type is undocumented, then the > first public subtype becomes a "reasonable" place to document the methods. > Maybe there are other "reasonable" ways to present the information, but this > is the current (long-term) behavior of the doclet. > > Kumar found this behavior (the hard way) when writing the new > VisibleMemberTable, which replaces the old VisibleMemberMap. This sort of > behavior is the primary reason for comparing "before" and "after" versions of > generate docs when replacing or updating javadoc internals. "If you don't > think you're making any changes to the output, verify you're not making any > changes to the output!" > >> >> I also looked at the serialised form page of the current API docs and didn’t >> find the broken links mentioned in the JIRA issue, so I wasn’t able to >> observe the impact of the fix. >> > > Most links on the serialized form page are OK. Serialization is "weird" > because it includes the doc for private fields, and in a number of places, > the doc comment on the private fields contains @see references to other > private members which are not being documented. It is those links which were > broken and which now should not be broken, at least from a clickable-link > perspective. It is still "not great" that the doclet posts an (unlinked) > reference to an undocumented member. It could be an RFE to improve that, but > while I can imagine better behavior of @see links, I have no good suggestion > for the equivalent inline {@link} tags. > > Here is an example of the broken link: > > In the JDK 10 API docs, look at the Serialized form for java.awt.MenuItem: > https://docs.oracle.com/javase/10/docs/api/serialized-form.html#java.awt.MenuItem > Look at the last entry in the See Also list: > MenuItem.writeObject(ObjectOutputStream) > If you click on it you go to the page for MenuItem, but because there is no > id for the writeObject method, you just go to the top of the page. > > Now look at the corresponding page in the docs with this fix: > http://cr.openjdk.java.net/~jjg/8207214/api.00/serialized-form.html#java.awt.MenuItem > Look at that same last entry again. This time, it is just plain text, not > linked. > > Arguably, a better solution for a followup RFE would be to link the class > name (because the class is documented) but not the member name. But that is a > more tricky fix. > > -- Jon > >> >> Apart from that, the webrev looks good to me. >> >> Hannes >> >> >> >>> Am 13.07.2018 um 02:16 schrieb Jonathan Gibbons >>> <[email protected]> >>> : >>> >>> Please review a small but slightly tricky fix related to broken links on >>> the serialized-form page. >>> >>> The underlying problem was that the doclet was generating bad links when an >>> @see tag >>> was referencing a member that was not itself being documented. For example, >>> the following >>> will normally cause a broken link because privateMethod will not normally >>> be included in the >>> documentation. >>> >>> public class C { >>> /** Text. >>> * @see #privateMethod() >>> */ >>> public void publicMethod() { } >>> >>> private void privateMethod() { } >>> } >>> >>> >>> There is a spectrum of possible solutions, some more complicated than >>> others. >>> The approach here is to fix the perceived original intent of the code which >>> is to detect >>> when the link will not be generated, and in that case, just write the >>> label, without >>> any enclosing link. >>> >>> This is done by creating and using a new method in Utils that carefully >>> tries to determine >>> whether the link should be generated. It's not ideal, and may need to be >>> refined in due course, >>> as we identify more test cases, but it is arguably "good enough" for now, >>> in that all existing >>> tests continue to pass, as well as some new test cases that mimic the >>> problem as originally >>> discovered. It is possible that we might be able to simplify this code, in >>> time, by using >>> information in the recently-new VisibleMemberTable. >>> >>> With regard to the original problem of broken links on the serialized-form >>> page in the >>> JDK API, the number of broken links has gone down by about 15%. >>> >>> With respect to the webrev, there is some minor cleanup, but primarily, the >>> expression >>> on HtmlDocletWriter:916 has been replaced by a call to a new method in >>> Utils, that >>> provides a more nuanced determination of whether the link should be >>> generated >>> or not. >>> >>> The new test creates various test scenarios and runs them through javadoc. >>> It is worth >>> noting that by default *all* javadoc tests using JavadocTester now run a >>> link checker >>> (checkLinks) unless it is explicitly disabled, and that ensures no >>> regressions in the >>> output generated in any other of the javadoc tests. >>> >>> JBS: >>> https://bugs.openjdk.java.net/browse/JDK-8207214 >>> >>> Webrev: >>> http://cr.openjdk.java.net/~jjg/8207214/webrev.00/index.html >>> >>> >>> -- Jon >>> >>> >
