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

Wed Nov 22 21:19:03 UTC 2017

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