Re: [9] Review request: 8170702: Document that javafx.graphics needs explicit access to application main class

2017-03-04 Thread Kevin Rushforth 

http://cr.openjdk.java.net/~kcr/8170702/webrev.01/


Kevin Rushforth wrote:



Mandy Chung wrote:
On Mar 3, 2017, at 10:36 PM, Kevin Rushforth 
 wrote:


[fixed subject line]

Please review the following to document that javafx.graphics needs 
explicit access to the Application class.


https://bugs.openjdk.java.net/browse/JDK-8170702
http://cr.openjdk.java.net/~kcr/8170702/webrev.00/



  69  * containing package must be {@link 
Module#isExported(String,Module) exported}


@linkplain instead?
  


I was following the pattern in Module.java, etc., which uses a regular 
@link in similar cases.


 239 StackTraceElement[] cause = 
Thread.currentThread().getStackTrace();


Good candidate to use StackWalker API.
  


This is pre-existing code (since JDK 7), and I don't want to change 
the implementation this late while fixing a doc bug. I will file a 
follow-on bug to consider improving this for JDK 10.


Is @throws RuntimeException an existing behavior?  I’d think CNFE and 
InaccessibleAE might be more appropriate.
  


Yes, this is the existing behavior and we are just documenting it. I 
agree that it might have been nicer to do something else, but the 
behavior w.r.t., exception is unchanged since JDK 8.


line 209 “It must be a subclass of Application or a RuntimeException 
will be thrown.”


I think this statement should be extended to cover if the class and 
its constructor are public and exported.
  


Yes, this seems like another good place to document the restriction. 
I'll post a .01 version of the webrev with this update.


-- Kevin



Mandy

Re: [9] Review request: 8170701: Update FXML documentation for setAccessible

2017-03-04 Thread Kevin Rushforth 

http://cr.openjdk.java.net/~kcr/8170701/webrev.01/


Kevin Rushforth wrote:



Mandy Chung wrote:
On Mar 3, 2017, at 10:29 PM, Kevin Rushforth 
 wrote:


Please review the following to update the FXML docs to document the 
requirement for a module that annotates non-public members with 
@FXML to "open" the containing package to the javafx.fxml module.


https://bugs.openjdk.java.net/browse/JDK-8170701
http://cr.openjdk.java.net/~kcr/8170701/webrev.00/



The doc change looks fine to me.

Minor suggestion.  You may refer "module-info class” to {@link 
ModuleDescriptor} which can be synthesized (although it may not be 
common).
  


I'll make this change.

You may consider referencing {@link Module#isOpened} where you may 
say “the containing package of the object is {@link Module#isOpened 
opened} to {@code javafx.fxml} module.
  


Good idea I will do that.

I'll post an updated webrev.

-- Kevin


Mandy

Re: [9] Review request: 8170702: Document that javafx.graphics needs explicit access to application main class

2017-03-04 Thread Mandy Chung 

> On Mar 4, 2017, at 1:04 AM, Kevin Rushforth  
> wrote:
> 
> 
> 
> Mandy Chung wrote:
>> 
>>> On Mar 3, 2017, at 10:36 PM, Kevin Rushforth  
>>>  wrote:
>>> 
>>> [fixed subject line]
>>> 
>>> Please review the following to document that javafx.graphics needs explicit 
>>> access to the Application class.
>>> 
>>> https://bugs.openjdk.java.net/browse/JDK-8170702 
>>> 
>>> http://cr.openjdk.java.net/~kcr/8170702/webrev.00/ 
>>> 
>>> 
>> 
>>   69  * containing package must be {@link Module#isExported(String,Module) 
>> exported}
>> 
>> @linkplain instead?
>>   
> 
> I was following the pattern in Module.java, etc., which uses a regular @link 
> in similar cases.
> 

Module.java should probably use @linkplain if that’s the case.  
>>  239 StackTraceElement[] cause = 
>> Thread.currentThread().getStackTrace();
>> 
>> Good candidate to use StackWalker API.
>>   
> 
> This is pre-existing code (since JDK 7), and I don't want to change the 
> implementation this late while fixing a doc bug. I will file a follow-on bug 
> to consider improving this for JDK 10.
> 

It's fine for JDK 10.
>> Is @throws RuntimeException an existing behavior?  I’d think CNFE and 
> 
> Yes, this seems like another good place to document the restriction. I'll 
> post a .01 version of the webrev with this update.

Sounds good.

Mandy

Re: [9] Review request: 8170702: Document that javafx.graphics needs explicit access to application main class

2017-03-04 Thread Mandy Chung 

> On Mar 4, 2017, at 5:15 PM, Kevin Rushforth  
> wrote:
> 
> http://cr.openjdk.java.net/~kcr/8170702/webrev.01/

Nit: @linkplain will show the normal font as text in javadoc rather than like 
the font for {@code…} 

 209  * be a public subclass of Application, in a package that is
 210  * {@link Module#isExported(String,Module) exported} to at least the
 211  * {@code javafx.graphics} module, or a RuntimeException will be 
thrown.

Should it mention the public constructor?  No need to have a new webrev if you 
update this before you push.

Mandy

Re: [9] Review request: 8170701: Update FXML documentation for setAccessible

2017-03-04 Thread Mandy Chung 

> On Mar 4, 2017, at 5:14 PM, Kevin Rushforth  
> wrote:
> 
> http://cr.openjdk.java.net/~kcr/8170701/webrev.01/
> 

  40  * object {@link Module#isOpen opens} the containing package to the

Nit: s/@link/@linkplain

  41  * {@code javafx.fxml} module, either in its {@link ModuleDescriptor}
  42  * (module-info.class) or by calling {@link Module#addOpens}.

Do you intend to take out “(module-info.class)”?  

  43  * An object is also reflectively accessible if it is declared as public,

Does “it” mean its constructor?

  44  * is in a public class, and the module containing that class
  45  * {@link Module#isExported(String,Module) exports}

Nit: s/@link/@linkplain

  46  * the containing package to the {@code javafx.fxml} module.

This is word-smithing and formatting nit.  No need to send a new webrev.

Mandy