Hi Jon,
[changed subj line to reflect the current review]
Full webrev:
http://cr.openjdk.java.net/~ksrini/8185985/webrev.01/
Delta webrev:
http://cr.openjdk.java.net/~ksrini/8185985/webrev.01/webrev.delta/
Here's comments on the code (not looked at the tests yet).
On 11/06/2017 06:04 PM, Kumar Srinivasan wrote:
[3] https://bugs.openjdk.java.net/browse/JDK-8185985
[4] http://cr.openjdk.java.net/~ksrini/8185985/webrev.00/
HtmlDocletWriter: line 451-493
I accept you have put the new code after the existing like-named
method,
but we're still trying to de-clutter classes like HtmlDocletWriter.
The new method seems like a combination of two methods ...
one is new functionality to extract the TITLE from an HTML document,
and the other is a simple call onto the existing getWindowTitle.
I'm also not sure that the filename is a good default. But that
being said,
TITLE is not an optional element in a HEAD section, and should
always be
provided, and so a non-empty default may never normally be required.
I have changed it to use an empty string.
HtmlDocletWriter: line 2495,2496
Suggest increasing indent of parameter list
Done
WriterFactoryImpl: 244
You're in a class called "WriterFactoryImpl" and the method is named
"getDocFilesWriter"
and yet it returns an object whose type is DocFilesHandler, instead
of DocFilesWriter.
Just sayin'....
Fixed
BaseConfiguration:378
Were you going to move this assignment later in the execution?
Done
CommentUtils:183
When is the returned type not a PackageElement?
Why is the return type of the method not PackageElement?
If it can be something else, like a module element, should there be
a check here?
Fixed
CommentUtil:219-221
The comment requires some mental gymnastics to guess any possible
meaning.
If you know what it means, can you rewrite it please?
Fixed
OverviewElement
We definitely talked about removing any direct dependence on and
storage of
BaseConfiguration.
Fixed
WriterFactory
See comments for WriterFactoryImpl re: naming.
AnnotationTypeBuilder:31
ClassBuilder:31
ModuleSummaryBuilder:30
PackageSummaryBuilder:34
Illegal import. builders cannot depend on types in format-specific
packages
(i.e. formats.html) There needs to be an interface in the toolkit
world, and
an impl of the interface in the formats.html world, the same as is
done for
other Writer/WriterImpl pairs.
Done
ModuleSummaryBuilder:120
You're very close to being able to support this. The PackageElement for
the DocletElement would be the unnamed package for the module.
It all seems to work, for modules, however I have disabled it for
now, added suitable
comments.
DocFile:106
Grumble, why do you need this? In general, the doclet world uses
DocFile objects
not FileObjects
FO needed for DocTrees.
DocPaths:173-196
This class used to be sorted in case-equivalent alpha order.
Somehow, these 'M'
fields and methods have ended up between 'P' and 'R'
Fixed.
StandardDocFileFactory
Again grumble that this is necessary, but I can see where this going
Utils
Who is now using these copyDirectory methods, since they don't check
for HTML files?
The doclet support files such as resources, jquery etc. Howeve please
note the doc-files
are copied by its own copyDirectory logic.
DocFilesHandler.java
What about .HtMl ... in other words, get the extension and do a
ignore-case check.
line 153: in time we'll want more from the <head> than just the
title, so I think
getTitle belongs here. Also, I don't think the title gets properly
propagated to
the TITLE in the generated file.
moved it here and converted the file extension check as suggested.
And it seems
to be propagated to the output, there are tests to check this.
DocFileElement:64
You want the unnamed package for the specific module (each module
has its own unnamed package.)
added a comment for future use.
DocletElement:83,84
You can't put types in the unnamed package of a named module, but
javac (Symbol, etc)
does understand the concept, and so it is reasonable (in the API) to
get the unnamed package
for a named module. You might need a workaround to get it, though.
Fixed comment
Thanks
Kumar
-- Jon
