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

[Jonathan Gibbons]

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

DocFilesHandlerImpl

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

No need for line 199; remove it

DocFilesHandler.java

   39     void copyDocFiles() throws DocletException ;

Space before ';'


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

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