RFR : 8213354 : Support package-specific stylesheets

[Priya Lakshmi Muthuswamy]

Hi Jon,

I have updated the fix based on the comments.

Regarding throwing broader DocletException in getStylesheets, 
printDocument in various writer classes throw DocFileIOException,
If we have to change, then we might make changes in all these methods.

I will log a separate bug for supporting module specific stylesheets.

webrev : http://cr.openjdk.java.net/~pmuthuswamy/8213354/webrev.02/

Thanks,
Priya

On 2/15/2019 7:28 AM, Jonathan Gibbons wrote:
> It's looking good but could still be improved.
>
> 1. You haven't followed the typical coding patter for managing a cache 
> -- in this case the cache of stylesheets.
>
> The pattern is typically:
>
>     Foo foo = fooCache.get(key);
>     if (foo == null) {
>         foo = // compute foo
>         fooCache.put(key, foo);
>    }
>     // use foo
>
> In your case, you have most of that pattern in HtmlDocletWriter, but 
> the call to update the cache is buried inside DocFilesHandlerImpl.
>
> 2. I still don't really like the reference to stylesheets in 
> DocFilesHandler, but I can live with it, but you should follow the 
> style of the other method and declare that you throw DocletException 
> instead of the more specific DocFileIOException. That saves needing 
> the more specific import, and allows any future subtypes or upgrades 
> to use other subtypes of DocletException if desired. Note that you can 
> change the exception here without changing the throws on the 
> implementation method.
>
> 3. You are very close to supporting module-specifc stylesheets as well 
> as package-specific ones. I'm not suggesting you go all the way for 
> this, unless you want to, but I think you should at least generalize 
> the cache in HtmlConfiguration to be a Map<Element,List<DocPath>> and 
> change/generalize some of the method names as well (e.g. by removing 
> the Package word), so that they can be reused if we choose to support 
> module-specific stylesheets.
>
> It would be pretty easy to have what is currently 
> getPackageLocalStylesheetContent(PackageElement) automatically check 
> if the package has an enclosing module and look up the style sheets 
> for the enclosing module as well as the package.
>
> -- Jon
>
> On 02/12/2019 12:40 AM, Priya Lakshmi Muthuswamy wrote:
>> Hi Jon,
>>
>> Thanks for the review.
>>
>> I have modified the DocFilesHandlerImpl to get all the .css files, so 
>> that user is not restricted to one name.
>> Also included the Map in HtmlConfiguration.
>>
>> updated webrev : 
>> http://cr.openjdk.java.net/~pmuthuswamy/8213354/webrev.01/
>>
>> Thanks,
>> Priya
>>
>> On 2/9/2019 6:24 AM, Jonathan Gibbons wrote:
>>> Hi Priya,
>>>
>>> There are two significant issues that need to be addressed.
>>>
>>> 1.
>>>
>>> In DocFilesHandlerImpl you randomly take the first .css file you 
>>> find when listing the directory. If there should only be one, we 
>>> should specify the name (e.g. stylesheet.css). Otherwise, I think 
>>> you should honor all files found. Note that the order of iteration 
>>> of a directory is undefined, and may depend on the operating system, 
>>> and may vary over time. That is a very strong reason not to just 
>>> take the first file found when listing the directory.
>>>
>>> In terms of naming, "check..." is not a good name for this method. 
>>> I'd suggest either getStylesheet (if you only want to handle one) or 
>>> getStyleSheets() returning a List<DocPath> if you support more than 
>>> one.  Note that using a list allows you to easily handle no files 
>>> found (i.e. an empty list) as well as 1 or more files found.
>>>
>>> Minor nit: please ensure a space between a keyword and an open 
>>> parenthesis. You should be able to configure your IDE for that 
>>> style; it is the standard style for JDK code.
>>>
>>> 2.
>>>
>>> You should not be modifying anything for this feature in the 
>>> jdk/javadoc/internal/doclets/toolkit/ directory. The use of 
>>> stylesheets and CSS is specific to the HTML format, and any changes 
>>> should be limited to the directory 
>>> jdk/javadoc/internal/doclets/formats/html/ (and any subdirectories.)
>>>
>>> I realize you need to get information into the individual 
>>> *WriterImpl classes.  In general, the way we do that is to build and 
>>> use a table in the HtmlConfiguration object. Some class like 
>>> HtmlDoclet can stash information in that table; other Html classes 
>>> can access it.  The table can either be a 
>>> Map<PackageElement,DocPath> or a Map<PackageElement,List<DocPath>> 
>>> depending on whether you support at most one stylesheet per package 
>>> or many.
>>>
>>> If you keep a Map in HtmlConfiguration, you could also compute 
>>> entries lazily, and not bother with editing HtmlDoclet. For example, 
>>> no entry in the cache means it has not been computed yet, an empty 
>>> list in the cache means no package-specific stylesheets, a non-empty 
>>> list means you have local stylesheets. This computation can all be 
>>> done in DocFilesHandlerImpl, with just the cache object itself being 
>>> retained in HtmlConfiguration.
>>>
>>> -- Jon
>>>
>>>
>>> On 01/30/2019 04:18 AM, Priya Lakshmi Muthuswamy wrote:
>>>> Hi,
>>>>
>>>> Kindly review the fix for 
>>>> https://bugs.openjdk.java.net/browse/JDK-8213354
>>>> Package specific stylesheets needs to placed under doc-files 
>>>> directory of a package.
>>>> DocFilesHandler looks for any css file and adds them to the 
>>>> generated html pages.
>>>>
>>>> webrev : http://cr.openjdk.java.net/~pmuthuswamy/8213354/webrev.00/
>>>>
>>>> Thanks,
>>>> Priya
>>>>
>>>
>