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

[Kevin Rushforth]

Mandy Chung wrote:

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…} 
  


Right...see my other reply. I'm happy to make this change if you plan to 
do the same for the rest of the JDK.




 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.
  


A good point, yes, I will add this.

-- Kevin



Mandy

  

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

[Kevin Rushforth]

Mandy Chung wrote:


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. 


At least Module.java and Layer.java use @link rather than @linkplain. I 
didn't do an exhaustive search. If you plan to file a bug to fix the JDK 
docs, then I will change it here to match (and in FXML.java for the 
other bug).


-- Kevin



 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

[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

[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: 8170702: Document that javafx.graphics needs explicit access to application main class

[Kevin Rushforth]

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: 8170702: Document that javafx.graphics needs explicit access to application main class

[Mandy Chung]

> 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?

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

Good candidate to use StackWalker API.

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

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.

Mandy

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

[Kevin Rushforth]

[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/

-- Kevin