RFR: 8189405: More cleanup in HtmlWriter

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8189405: More cleanup in HtmlWriter

Jonathan Gibbons
Please review a medium-simple fix to continue the HtmlWriter cleanup
started in JDK-8160697.
This is about removing "constants" from the per-page HtmlWriter, such
that they are either moved
to the shared Contents class, or just created as needed.

This is done in part by introducing a new TableHeader class to help
create headers for tables.

This is cleanup only: no new tests or change in generated docs.

JBS: https://bugs.openjdk.java.net/browse/JDK-8189405
Webrev: http://cr.openjdk.java.net/~jjg/8189405/webrev.00/

-- Jon
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8189405: More cleanup in HtmlWriter

Kumar Srinivasan
Jon,

Looks ok to me, nice simplifications, this affects more on the rendering
side. Bhavesh will likely review it in detail, for the record, I did perform
a before/after test and it looks identical.

Thanks

Kumar

> Please review a medium-simple fix to continue the HtmlWriter cleanup
> started in JDK-8160697.
> This is about removing "constants" from the per-page HtmlWriter, such
> that they are either moved
> to the shared Contents class, or just created as needed.
>
> This is done in part by introducing a new TableHeader class to help
> create headers for tables.
>
> This is cleanup only: no new tests or change in generated docs.
>
> JBS: https://bugs.openjdk.java.net/browse/JDK-8189405
> Webrev: http://cr.openjdk.java.net/~jjg/8189405/webrev.00/
>
> -- Jon

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8189405: More cleanup in HtmlWriter

Bhavesh Patel

Nice cleanup. Just a couple of minor changes. Follow up review not
needed. Good to push.

ModuleWriterImpl.java

Lines 825 and 826 can be combined on the same line.

Lines 839 and 840 can be combined on the same line.


PackageUseWriter.java

Lines 97 and 98: Should we use "resources" instead of configuration here?

HtmlWriter.java

Not related to your change but to comment for method on line 77 is
missing @param configuration.

Regards,
Bhavesh.


On 10/17/2017 9:05 AM, Kumar Srinivasan wrote:

> Jon,
>
> Looks ok to me, nice simplifications, this affects more on the rendering
> side. Bhavesh will likely review it in detail, for the record, I did
> perform
> a before/after test and it looks identical.
>
> Thanks
>
> Kumar
>
>> Please review a medium-simple fix to continue the HtmlWriter cleanup
>> started in JDK-8160697.
>> This is about removing "constants" from the per-page HtmlWriter, such
>> that they are either moved
>> to the shared Contents class, or just created as needed.
>>
>> This is done in part by introducing a new TableHeader class to help
>> create headers for tables.
>>
>> This is cleanup only: no new tests or change in generated docs.
>>
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8189405
>> Webrev: http://cr.openjdk.java.net/~jjg/8189405/webrev.00/
>>
>> -- Jon
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8189405: More cleanup in HtmlWriter

Jonathan Gibbons
Thanks. I agree with and will incorporate all your feedback.

-- Jon

On 10/18/2017 05:12 PM, Bhavesh Patel wrote:

>
> Nice cleanup. Just a couple of minor changes. Follow up review not
> needed. Good to push.
>
> ModuleWriterImpl.java
>
> Lines 825 and 826 can be combined on the same line.
>
> Lines 839 and 840 can be combined on the same line.
>
>
> PackageUseWriter.java
>
> Lines 97 and 98: Should we use "resources" instead of configuration here?
>
> HtmlWriter.java
>
> Not related to your change but to comment for method on line 77 is
> missing @param configuration.
>
> Regards,
> Bhavesh.
>
>
> On 10/17/2017 9:05 AM, Kumar Srinivasan wrote:
>> Jon,
>>
>> Looks ok to me, nice simplifications, this affects more on the rendering
>> side. Bhavesh will likely review it in detail, for the record, I did
>> perform
>> a before/after test and it looks identical.
>>
>> Thanks
>>
>> Kumar
>>
>>> Please review a medium-simple fix to continue the HtmlWriter cleanup
>>> started in JDK-8160697.
>>> This is about removing "constants" from the per-page HtmlWriter,
>>> such that they are either moved
>>> to the shared Contents class, or just created as needed.
>>>
>>> This is done in part by introducing a new TableHeader class to help
>>> create headers for tables.
>>>
>>> This is cleanup only: no new tests or change in generated docs.
>>>
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8189405
>>> Webrev: http://cr.openjdk.java.net/~jjg/8189405/webrev.00/
>>>
>>> -- Jon
>>
>