RFR: JDK-8190295: Introduce a new Table builder class

Mon Nov 6 21:48:01 UTC 2017

Hi Jon,

This is an enormous improvement and simplification!. Thanks!

Table.java:

304      * <li>The prefix should probably be a value such that the genenrated ids cannot

s/genenrated/generated/

The previous block is ternary operation, would make it nicer if this is 
also so.

             HtmlTree cell;
             if ((colIndex) == rowScopeColumnIndex) {
                 cell = HtmlTree.TH(cellStyle, "row", c);
             } else {
                 cell = HtmlTree.TD(cellStyle, c);
             }

final HtmlTree cell = HtmlTree.TH(cellStyle, "row", c)
             ? HtmlTree.TH(cellStyle, "row", c)
             : HtmlTree.TD(cellStyle, c);

AbstractMemberWriter.java

+                TypeElement te = utils.getEnclosingTypeElement(element); // is this different from typeElement?

I ran a quick test, and encountered several test failures, the reason is 
ClassUseWriter uses this method,
and it creates  AbstractMemberWriter with typeElement = null, via
     public AbstractMemberWriter(SubWriterHolderWriter writer) {
         this(writer, null);
     }
and thus typeElement could be a null, the following might save some cpu 
cycles,
  I verified all tests pass and the comparison tests also pass.

+                TypeElement te = typeElement == null
+                        ? utils.getEnclosingTypeElement(element)
+                        : typeElement;

There were other details we discussed offlist, and that clarifies the 
questions I had,
regarding Table.java. Most of this is in Bhavesh's expertise, I will 
leave it to him.

Kumar

> Please review the following change, to replace the code to create HTML 
> tables with a new
> builder-style Table class. With the exception of some minor 
> differences (bug fixes) on
> the deprecated-list page, the generated output should be unchanged for 
> now, although
> we may want to remove some backwards-compatibility hacks at some point 
> in the future
> to make all tables be more uniform when appropriate.
>
> Before reading the detailed changes in the modified files, you might 
> want to start by
> reviewing the new Table class.
>
> JBS: https://bugs.openjdk.java.net/browse/JDK-8190295
> Webrev: http://cr.openjdk.java.net/~jjg/8190295/webrev.00/
>
>
> Notes:
>
> (Various files)
> Stuff that has been deleted has generally moved in some form or other 
> into the new Table class
> or otherwise made redundant.
>
> AbstractMemberWriter:118-138
> Previously, the state defining the table was indirectly managed in the 
> MemberSummaryBuilder
> class via tableContents and counter (uugh.) Now, the state is kept in 
> the AbstractMemberWriter
> and lazily created by getSummaryTable, line 120.
>
> AbstractMemberWriter:433-437
> Here's a relatively simple example of creating a Table, new-style.
> The table is created (433-437), rows are added (463) and the HTML 
> generated (465)
> Other Tables are created in *WriterImpl classes.
>
> AbstractMemberWriter: old lines 535-553
> Anything to do with TableTabs is now mostly hidden inside the Table 
> class, driven by
> addTab calls when the Table is initialized.
>
> AbstractMemberWriter:583-592
> The lines are called by the MemberSummaryBuilder to conclude its work 
> on the table.
>
> DeprecatedListWriter:
> There is a small, deliberate (bug fix?) change in behavior here. 
> Previously, modules
> in the deprecated list used the colFirst style, and other types used 
> colDeprecatedItemName.
> Now, everything uses the same style, colDeprecatedItemName. It didn't 
> seem worth
> fixing for the old bad behavior. Also note that some cells were 
> omitted from the table,
> causing inconsistent visual appear of the striping. That is also fixed.
>
> HtmlDocletWriter:2548
> This is made available, for now. The script item itself is in 
> HtmlWriter (uugh). Long term,
> this should move into a new Head class (builder for <head> element)
>
> MethodWriterImpl.java:258-278
> This is definitely the most exciting part of this work. This is the 
> client-side  replacement for
> all the previous code to manage table tabs. Now, each tab is just a 
> method call with a
> name and a predicate.
>
> ModuleIndexWriter.java:135-141
> Following on from the previous comment, here in ModuleIndexWriter, and 
> corresponding code
> in PackageIndexWriter, we have a dynamically added set of tabs, based 
> on command line options.
>
> ModuleWriterImpl.java:635-648
> For now, for compatibility the rows are added in the same order as 
> before.
> Going forward, we might want to add the rows in overall sorted order.
>
> PropertyWriterImpl.java:234-245
> Because we create differently configures tables for properties and 
> methods, one without tabs, one with,
> it falls out of the design that the rows in the properties table don't 
> get "ids", thus avoiding the earlier
> "duplicate id" problem.
>
> Table.java:
> The new class that does all the work. It's reasonably well commented, 
> so nothing to add here.
>
> -- Jon
>