Re: RFR:8185985:Html files in doc-files directories should be wrapped

[Jonathan Gibbons]

OK, but I also note there is a minor visual regression in the doc-file 
pages.

Previously, the pages relied on the default style, which gave some left 
margin.  Now the pages are picking up elements of the doclet style, 
which has no left margin. This needs to be fixed at some point, perhaps 
as part of the overall stylesheet cleanup, and the subgoal within that 
to better separate the styles for user-written content from the styles 
for doclet-generated content.

-- Jon

On 11/22/2017 11:40 AM, Kumar Srinivasan wrote:
Jon,

DocletElement:84

  83      * Returns the anchoring package element, in the case of a
  84      * module element, this could be an unnamed package.

Change to:     this is the module's unnamed package

Fixed

DocFilesHandlerImpl

 199         sb.trimToSize();
 200         return docletWriter.getWindowTitle(sb.toString().trim());

No need for line 199; remove it

Fixed


DocFilesHandler.java

  39     void copyDocFiles() throws DocletException ;

Space before ';'

Fixed

It would be nice if the MODULE and PACKAGE links were active.

Done and thanks for the override tip!.

Also added more tests for single and multi modules, the latter the 
cause behind
links not appearing.

The corrected doc-file:
http://cr.openjdk.java.net/~ksrini/8185985/api-docs/api/java/util/doc-files/coll-index.html 


Delta webrev:
http://cr.openjdk.java.net/~ksrini/8185985/webrev.02/webrev.delta/


Thanks
Kumar





-- Jon


On 11/09/2017 03:09 PM, Kumar Srinivasan wrote:
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