Fwd: RFR: 8190552/8185985: Augment the Compiler API tree ; Html files in doc-files directories should be wrapped

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

Fwd: RFR: 8190552/8185985: Augment the Compiler API tree ; Html files in doc-files directories should be wrapped

Kumar Srinivasan
Hello,

Please review fix to augment the Compiler API tree to represent the
entire HTML content of a file, please see bug [1] and webrev [2].
The above are required to implement the bug [3] and webrev [4], please
review these as well.

Please note these patches exist as mq patches in my repo, and will be
pushed as two discrete changesets.

Thanks Kumar

[1] https://bugs.openjdk.java.net/browse/JDK-8190552
[2] http://cr.openjdk.java.net/~ksrini/8190552/webrev.00/

[3] https://bugs.openjdk.java.net/browse/JDK-8185985
[4] http://cr.openjdk.java.net/~ksrini/8185985/webrev.00/


Reply | Threaded
Open this post in threaded view
|

Re: Fwd: RFR: 8190552/8185985: Augment the Compiler API tree ; Html files in doc-files directories should be wrapped

Jonathan Gibbons

[1] https://bugs.openjdk.java.net/browse/JDK-8190552

src/jdk.compiler/share/classes/com/sun/source/doctree/DocTypeTree.java
line 32: typo: @lt; should be <    >  should be >

JavacTrees.java, 1041, 1042, duplicate call
> 1040         DocCommentTree docCommentTree = getDocCommentTree(jfo);
> 1041         docCommentTree = getDocCommentTree(jfo);

In DocCommentParser, the inPhase field can be reduced to a parameter on
blockComment.
Also, the following replacement for 191-208 is more robust in the face
of malformed docs:

  191                     if (isFileContent) {
  192                         switch (inPhase) {
  193                             case PREAMBLE:
                                     if (peek("body") == PeekKind.OPEN) {
  194                                     trees.add(html());
  195                                     if (textStart == -1) {
  196                                         textStart = bp;
  197                                         lastNonWhite = -1;
  198                                     }
  199                                     // mark this as the start, for processing purposes
  200                                     newline = true;
  201                                     break loop;
                                      }
                                      break;
  202                             case BLOCK:
                                      if(peek("body") == PeekKind.CLOSE) {
  203                                     addPendingText(trees, lastNonWhite);
  204                                     break loop;
                                      }
                                      break;
  205                             default:
  206                                 // fall through
  207                         }
  208                     }

(As a further simplification, you could even make "peek" be "boolean
peek(String)" and handle the / in the impl of peek, thus eliminating
PeekKind.)


DCTree: 161-165
Format the new lines like the preceding lines (3-line form).

DCTree: 303,305
Use "text" instead of "body" consistently

test/langtools/tools/javac/doctree/dcapi/DocCommentTreeApiTester.java
line 196:  do you mean "incorrect" here?   do you mean"invalid"?
     (I don't know what it means to say "incorrect input document")
line 239: ditto

test/langtools/tools/javac/doctree/dcapi/overview0.html and friends
why do you still need the partial comment "<!--EXPECT_START"

test/langtools/tools/javac/doctree/dcapi/package.html
<!DOCTYPE...> must be the _first_ line of the file

On 11/06/2017 06:09 PM, Kumar Srinivasan wrote:

> Hello,
>
> Please review fix to augment the Compiler API tree to represent the
> entire HTML content of a file, please see bug [1] and webrev [2].
> The above are required to implement the bug [3] and webrev [4], please
> review these as well.
>
> Please note these patches exist as mq patches in my repo, and will be
> pushed as two discrete changesets.
>
> Thanks Kumar
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8190552
> [2] http://cr.openjdk.java.net/~ksrini/8190552/webrev.00/
>
> [3] https://bugs.openjdk.java.net/browse/JDK-8185985
> [4] http://cr.openjdk.java.net/~ksrini/8185985/webrev.00/
>
>

Reply | Threaded
Open this post in threaded view
|

Re: Fwd: RFR: 8190552/8185985: Augment the Compiler API tree ; Html files in doc-files directories should be wrapped

Kumar Srinivasan
Jon,

Fixed your comments noted here:

updated full webrev:
http://cr.openjdk.java.net/~ksrini/8190552/webrev.01/

updated delta webrev
http://cr.openjdk.java.net/~ksrini/8190552/webrev.01/webrev.delta/


Thanks
Kumar

On 11/7/2017 3:15 PM, Jonathan Gibbons wrote:

>
> [1] https://bugs.openjdk.java.net/browse/JDK-8190552
>
> src/jdk.compiler/share/classes/com/sun/source/doctree/DocTypeTree.java
> line 32: typo: @lt; should be &lt;    >  should be &gt;
>
> JavacTrees.java, 1041, 1042, duplicate call
>> 1040         DocCommentTree docCommentTree = getDocCommentTree(jfo);
>> 1041         docCommentTree = getDocCommentTree(jfo);
>
> In DocCommentParser, the inPhase field can be reduced to a parameter
> on blockComment.
> Also, the following replacement for 191-208 is more robust in the face
> of malformed docs:
>
>  191                     if (isFileContent) {
>  192                         switch (inPhase) {
>  193                             case PREAMBLE:
>                      if (peek("body") == PeekKind.OPEN) {
>  194                                     trees.add(html());
>  195                                     if (textStart == -1) {
>  196                                         textStart = bp;
>  197                                         lastNonWhite = -1;
>  198                                     }
>  199                                     // mark this as the start,
> for processing purposes
>  200                                     newline = true;
>  201                                     break loop;
>                                      }
>                                      break;
>  202                             case BLOCK:
>                                      if(peek("body") == PeekKind.CLOSE) {
>  203                                     addPendingText(trees,
> lastNonWhite);
>  204                                     break loop;
>                                      }
>                                      break;
>  205                             default:
>  206                                 // fall through
>  207                         }
>  208                     }
>
> (As a further simplification, you could even make "peek" be "boolean
> peek(String)" and handle the / in the impl of peek, thus eliminating
> PeekKind.)
>
>
> DCTree: 161-165
> Format the new lines like the preceding lines (3-line form).
>
> DCTree: 303,305
> Use "text" instead of "body" consistently
>
> test/langtools/tools/javac/doctree/dcapi/DocCommentTreeApiTester.java
> line 196:  do you mean "incorrect" here?   do you mean"invalid"?
>     (I don't know what it means to say "incorrect input document")
> line 239: ditto
>
> test/langtools/tools/javac/doctree/dcapi/overview0.html and friends
> why do you still need the partial comment "<!--EXPECT_START"
>
> test/langtools/tools/javac/doctree/dcapi/package.html
> <!DOCTYPE...> must be the _first_ line of the file
>
> On 11/06/2017 06:09 PM, Kumar Srinivasan wrote:
>> Hello,
>>
>> Please review fix to augment the Compiler API tree to represent the
>> entire HTML content of a file, please see bug [1] and webrev [2].
>> The above are required to implement the bug [3] and webrev [4],
>> please review these as well.
>>
>> Please note these patches exist as mq patches in my repo, and will be
>> pushed as two discrete changesets.
>>
>> Thanks Kumar
>>
>> [1] https://bugs.openjdk.java.net/browse/JDK-8190552
>> [2] http://cr.openjdk.java.net/~ksrini/8190552/webrev.00/
>>
>> [3] https://bugs.openjdk.java.net/browse/JDK-8185985
>> [4] http://cr.openjdk.java.net/~ksrini/8185985/webrev.00/
>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: Fwd: RFR: 8190552/8185985: Augment the Compiler API tree ; Html files in doc-files directories should be wrapped

Jonathan Gibbons
Looks good.  I like the cleanup in DocCommentParser.

The line line of the last file looks inconsistently weird, but as long
as the checked in file is good, that is what matters.

-- Jon

On 11/07/2017 08:09 PM, Kumar Srinivasan wrote:

> Jon,
>
> Fixed your comments noted here:
>
> updated full webrev:
> http://cr.openjdk.java.net/~ksrini/8190552/webrev.01/
>
> updated delta webrev
> http://cr.openjdk.java.net/~ksrini/8190552/webrev.01/webrev.delta/
>
>
> Thanks
> Kumar
>
> On 11/7/2017 3:15 PM, Jonathan Gibbons wrote:
>>
>> [1] https://bugs.openjdk.java.net/browse/JDK-8190552
>>
>> src/jdk.compiler/share/classes/com/sun/source/doctree/DocTypeTree.java
>> line 32: typo: @lt; should be &lt;    >  should be &gt;
>>
>> JavacTrees.java, 1041, 1042, duplicate call
>>> 1040         DocCommentTree docCommentTree = getDocCommentTree(jfo);
>>> 1041         docCommentTree = getDocCommentTree(jfo);
>>
>> In DocCommentParser, the inPhase field can be reduced to a parameter
>> on blockComment.
>> Also, the following replacement for 191-208 is more robust in the
>> face of malformed docs:
>>
>>  191                     if (isFileContent) {
>>  192                         switch (inPhase) {
>>  193                             case PREAMBLE:
>>                      if (peek("body") == PeekKind.OPEN) {
>>  194                                     trees.add(html());
>>  195                                     if (textStart == -1) {
>>  196                                         textStart = bp;
>>  197                                         lastNonWhite = -1;
>>  198                                     }
>>  199                                     // mark this as the start,
>> for processing purposes
>>  200                                     newline = true;
>>  201                                     break loop;
>>                                      }
>>                                      break;
>>  202                             case BLOCK:
>>                                      if(peek("body") ==
>> PeekKind.CLOSE) {
>>  203                                     addPendingText(trees,
>> lastNonWhite);
>>  204                                     break loop;
>>                                      }
>>                                      break;
>>  205                             default:
>>  206                                 // fall through
>>  207                         }
>>  208                     }
>>
>> (As a further simplification, you could even make "peek" be "boolean
>> peek(String)" and handle the / in the impl of peek, thus eliminating
>> PeekKind.)
>>
>>
>> DCTree: 161-165
>> Format the new lines like the preceding lines (3-line form).
>>
>> DCTree: 303,305
>> Use "text" instead of "body" consistently
>>
>> test/langtools/tools/javac/doctree/dcapi/DocCommentTreeApiTester.java
>> line 196:  do you mean "incorrect" here?   do you mean"invalid"?
>>     (I don't know what it means to say "incorrect input document")
>> line 239: ditto
>>
>> test/langtools/tools/javac/doctree/dcapi/overview0.html and friends
>> why do you still need the partial comment "<!--EXPECT_START"
>>
>> test/langtools/tools/javac/doctree/dcapi/package.html
>> <!DOCTYPE...> must be the _first_ line of the file
>>
>> On 11/06/2017 06:09 PM, Kumar Srinivasan wrote:
>>> Hello,
>>>
>>> Please review fix to augment the Compiler API tree to represent the
>>> entire HTML content of a file, please see bug [1] and webrev [2].
>>> The above are required to implement the bug [3] and webrev [4],
>>> please review these as well.
>>>
>>> Please note these patches exist as mq patches in my repo, and will
>>> be pushed as two discrete changesets.
>>>
>>> Thanks Kumar
>>>
>>> [1] https://bugs.openjdk.java.net/browse/JDK-8190552
>>> [2] http://cr.openjdk.java.net/~ksrini/8190552/webrev.00/
>>>
>>> [3] https://bugs.openjdk.java.net/browse/JDK-8185985
>>> [4] http://cr.openjdk.java.net/~ksrini/8185985/webrev.00/
>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: Fwd: RFR: 8190552/8185985: Augment the Compiler API tree ; Html files in doc-files directories should be wrapped

Jonathan Gibbons
In reply to this post by Kumar Srinivasan
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.


HtmlDocletWriter: line 2495,2496
Suggest increasing indent of parameter list

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

BaseConfiguration:378
Were you going to move this assignment later in the execution?

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?

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?

OverviewElement
We definitely talked about removing any direct dependence on and storage of
BaseConfiguration.

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.

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.

DocFile:106
Grumble, why do you need this?  In general, the doclet world uses
DocFile objects
not FileObjects

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'

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?

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.

DocFileElement:64
You want the unnamed package for the specific module (each module has
its own unnamed package.)

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.


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

Re: Fwd: RFR: 8190552/8185985: Augment the Compiler API tree ; Html files in doc-files directories should be wrapped

Kumar Srinivasan
In reply to this post by Jonathan Gibbons
Hi Jan, Jon,

Please review:
http://cr.openjdk.java.net/~ksrini/8190552/webrev.02/webrev.delta/
I have included Jan's patch to fix jshell test, add bugid to the tests,
I had missed adding that for the DocCommentParserApi test.

The full webrev is here containing everything for reference
http://cr.openjdk.java.net/~ksrini/8190552/webrev.02

Thanks
Kumar

On 11/8/2017 3:15 PM, Jonathan Gibbons wrote:

> Looks good.  I like the cleanup in DocCommentParser.
>
> The line line of the last file looks inconsistently weird, but as long
> as the checked in file is good, that is what matters.
>
> -- Jon
>
> On 11/07/2017 08:09 PM, Kumar Srinivasan wrote:
>> Jon,
>>
>> Fixed your comments noted here:
>>
>> updated full webrev:
>> http://cr.openjdk.java.net/~ksrini/8190552/webrev.01/
>>
>> updated delta webrev
>> http://cr.openjdk.java.net/~ksrini/8190552/webrev.01/webrev.delta/
>>
>>
>> Thanks
>> Kumar
>>
>> On 11/7/2017 3:15 PM, Jonathan Gibbons wrote:
>>>
>>> [1] https://bugs.openjdk.java.net/browse/JDK-8190552
>>>
>>> src/jdk.compiler/share/classes/com/sun/source/doctree/DocTypeTree.java
>>> line 32: typo: @lt; should be &lt;    >  should be &gt;
>>>
>>> JavacTrees.java, 1041, 1042, duplicate call
>>>> 1040         DocCommentTree docCommentTree = getDocCommentTree(jfo);
>>>> 1041         docCommentTree = getDocCommentTree(jfo);
>>>
>>> In DocCommentParser, the inPhase field can be reduced to a parameter
>>> on blockComment.
>>> Also, the following replacement for 191-208 is more robust in the
>>> face of malformed docs:
>>>
>>>  191                     if (isFileContent) {
>>>  192                         switch (inPhase) {
>>>  193                             case PREAMBLE:
>>>                      if (peek("body") == PeekKind.OPEN) {
>>>  194                                     trees.add(html());
>>>  195                                     if (textStart == -1) {
>>>  196                                         textStart = bp;
>>>  197                                         lastNonWhite = -1;
>>>  198                                     }
>>>  199                                     // mark this as the start,
>>> for processing purposes
>>>  200                                     newline = true;
>>>  201                                     break loop;
>>>                                      }
>>>                                      break;
>>>  202                             case BLOCK:
>>>                                      if(peek("body") ==
>>> PeekKind.CLOSE) {
>>>  203                                     addPendingText(trees,
>>> lastNonWhite);
>>>  204                                     break loop;
>>>                                      }
>>>                                      break;
>>>  205                             default:
>>>  206                                 // fall through
>>>  207                         }
>>>  208                     }
>>>
>>> (As a further simplification, you could even make "peek" be "boolean
>>> peek(String)" and handle the / in the impl of peek, thus eliminating
>>> PeekKind.)
>>>
>>>
>>> DCTree: 161-165
>>> Format the new lines like the preceding lines (3-line form).
>>>
>>> DCTree: 303,305
>>> Use "text" instead of "body" consistently
>>>
>>> test/langtools/tools/javac/doctree/dcapi/DocCommentTreeApiTester.java
>>> line 196:  do you mean "incorrect" here?   do you mean"invalid"?
>>>     (I don't know what it means to say "incorrect input document")
>>> line 239: ditto
>>>
>>> test/langtools/tools/javac/doctree/dcapi/overview0.html and friends
>>> why do you still need the partial comment "<!--EXPECT_START"
>>>
>>> test/langtools/tools/javac/doctree/dcapi/package.html
>>> <!DOCTYPE...> must be the _first_ line of the file
>>>
>>> On 11/06/2017 06:09 PM, Kumar Srinivasan wrote:
>>>> Hello,
>>>>
>>>> Please review fix to augment the Compiler API tree to represent the
>>>> entire HTML content of a file, please see bug [1] and webrev [2].
>>>> The above are required to implement the bug [3] and webrev [4],
>>>> please review these as well.
>>>>
>>>> Please note these patches exist as mq patches in my repo, and will
>>>> be pushed as two discrete changesets.
>>>>
>>>> Thanks Kumar
>>>>
>>>> [1] https://bugs.openjdk.java.net/browse/JDK-8190552
>>>> [2] http://cr.openjdk.java.net/~ksrini/8190552/webrev.00/
>>>>
>>>> [3] https://bugs.openjdk.java.net/browse/JDK-8185985
>>>> [4] http://cr.openjdk.java.net/~ksrini/8185985/webrev.00/
>>>>
>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

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

Kumar Srinivasan
In reply to this post by Jonathan Gibbons
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

Reply | Threaded
Open this post in threaded view
|

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

Reply | Threaded
Open this post in threaded view
|

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

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

Reply | Threaded
Open this post in threaded view
|

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

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