Fwd: RFR: 8190552/8185985: Augment the Compiler API tree ; Html files in doc-files directories should be wrapped

Thu Nov 9 01:12:22 UTC 2017

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.

HtmlDocletWriter: line 2495,2496
Suggest increasing indent of parameter list

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'....

BaseConfiguration:378
Were you going to move this assignment later in the execution?

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?

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?

OverviewElement
We definitely talked about removing any direct dependence on and storage of
BaseConfiguration.

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.

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.

DocFile:106
Grumble, why do you need this?  In general, the doclet world uses 
DocFile objects
not FileObjects

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'

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?

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.

DocFileElement:64
You want the unnamed package for the specific module (each module has 
its own unnamed package.)

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.

-- Jon