<AWT Dev> [9] Review Request: 6815345 java.awt.Component.createImage(int width, int height) should remove behavioral optionality

Fri Sep 25 16:06:57 UTC 2015

On 25.09.15 18:38, Semyon Sadetsky wrote:
>
>
> On 9/25/2015 4:51 PM, Sergey Bylokhov wrote:
>> On 25.09.15 15:44, Semyon Sadetsky wrote:
>>>
>>>
>>> On 9/25/2015 2:46 PM, Sergey Bylokhov wrote:
>>>> On 25.09.15 10:13, Semyon Sadetsky wrote:
>>>>> Native container based components are disabled. But other native
>>>>> resources can be used if available.
>>>>
>>>> no hw components -> no backbuffers for them.
>>> In my understanding it depends on the component. If component capable to
>>> work in the headless mode it can continue to use its peer, can't it?
>>>>
>>>>> All **Peer interfaces are public and external. Peer is not a hidden or
>>>>> implementation specific term. It isn't allowed to application
>>>>> developers
>>>>> for implementation, but it is used for porting client libs to other
>>>>> platforms. Java specification has wider vision than one particular
>>>>> implementation. And for porting the headless mode is very sensitive
>>>>> topic because specific platforms are headless often.
>>>>
>>>> The java.awt.peer package is private and should be used by the awt
>>>> developers only. If the new port will be implemented then it should
>>>> follow the same specification as our implementation.
>>
>>> That's correct. And our specification should be reasonable in its turn.
>>> It must not be simply back-filled from our code otherwise it would
>>> unlikely have more than one implementation.
>>> JCK team likes direct specs in "if-then" style. But I see that the
>>> initial doc was soft enough because it just clarify that exceptions from
>>> the general rule are possible depending on the specific implementation
>>> concept to leave more possibilities for porting.
>>
>> But it was not strict enough, we always return null for a long time,
>> all other ports do the same because jck require this. So this is not a
>> problem for possible ports and this removes possible misleading
>> assumption for application developers. This fix is a spec
>> clarification that an application should not try to create a buffer
>> for the hw component in such situations.
>>
>> The article I send you
>>> also contains a lot of soft phrases.
>>>>
>>>>>>
>>>>>> Maybe
>>>>>>> Component#getImage() implementation should simply check for
>>>>>>> isHeadless()
>>>>>>> and return null?
>>>>> But what will happen if Unix DISPLAY variable is not set so
>>>>> isHeadlessInstance()=true?  Will peer be allowed? Should component
>>>>> buffering work smoothly?
>>>>
>>>> The peers are not allowed in the headless mode. The DISPLAY var is not
>>>> specified in the javadoc, this means that some implementation can work
>>>> when DISPLAY is empty/unset, but in this case they must return false
>>>> from the isHeadlessXX();
>>> isHeadlessInstance() just tests DISPLAY on Linux.
>>
>> This is our implementation, other can check other things. There are
>> some implementation that can work without display, but in this case
>> this environment is not headless.
>>
>> Can you specify what sentence you complain?
>> This cannot be removed since it was there already
>> "This will always happen if
>> * <code>GraphicsEnvironment.isHeadless()</code> returns
>> * <code>true</code>."
>
> "The return value may be null if the component is not displayable. This
> will always happen if GraphicsEnvironment.isHeadless() returns true."
>
> your new version:
>
> "Returns null value if the component is not displayable or
> GraphicsEnvironment.isHeadless() returns true"
>
> which invited me as a reviewer to find an "if" in the method body or
> deeper, because it sounds "It shall return null if ..." .
> But I didn't find it. Imagine if you get this spec to be implemented
> from scratch. Why 'if' is not added as the first line? That would
> eliminate all questions.

Is this check in the code is not clear?
(peer != null) ? peer.createImage(width, height) : null

>
> The previous version doesn't demand "if" because it is indirect and as
> developer I would never rely on the null value promised by such spec.
> But with the new version of the spec the null should be guaranteed for
> the specified conditions otherwise we receive a bug.

And if an application, which will tested on your code, will start on our 
jdk it will fail with npe.

>
> And I cannot see the reason why non-null value cannot be allowed because
> it doesn't brake anything.
> Explanation "we define this and developers should follow" is a bit
> frustrating because I see scenarios when component buffer could be
> useful in the headless mode.

This is not about a headless.

>
> I suspect the original spec didn't imply strict binding to the values,
> and this fix may simplify testing but distorts the original approach.

It is always good to specify strict and clear null behavior to suppress 
possible surprises.

> I just unsure that this issue should be resolved. Maybe other reviewers
> could add their opinion.
>>
>>
>>>>
>>>>> Note this is a common network server scenario which runs Java app from
>>>>> the text console. I cannot get from the spec is it completely
>>>>> different
>>>>> headless mode or it is the same as the global headless?
>>>>>>
>>>>>>>
>>>>>>> And more global question: Why should we disable the creating of
>>>>>>> component image buffer for the headless mode?It could be used for
>>>>>>> the
>>>>>>> same performance reason as in non-headless.
>>>>>>
>>>>>> in the headless mode we cannot create most of the hw components,
>>>>>> actually non of top level window can be created. And for lw
>>>>>> components
>>>>>> like in swing the one global buffer is used in the RepaintManager.
>>>>> One global buffer means one global repaint
>>>>
>>>> No this is not true. If one component is changed then only one
>>>> component is repainted.
>>>>
>>>> off all components for any
>>>>> single component change. Why can't we be isomorphic here?
>>>>>>
>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> The headless case is covered, heavyweight components cannot be
>>>>>>>>>> created
>>>>>>>>>> in such mode, so only lightweight buttons are checked.
>>>>>>>>>>
>>>>>>>>>> On 24.09.15 15:58, Semyon Sadetsky wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 9/24/2015 3:25 PM, Sergey Bylokhov wrote:
>>>>>>>>>>>> On 24.09.15 11:36, Semyon Sadetsky wrote:
>>>>>>>>>>>>> Hi Sergey,
>>>>>>>>>>>>>
>>>>>>>>>>>>>   isHeadless()=true must return null. If so please add the
>>>>>>>>>>>>> corresponding
>>>>>>>>>>>>> test case. It is not not obvious from the code.
>>>>>>>>>>>>
>>>>>>>>>>>> isHeadless()=true is headless mode where the frames are always
>>>>>>>>>>>> not
>>>>>>>>>>>> displayable, so everything is similar to the current test
>>>>>>>>>>>> except
>>>>>>>>>>>> that
>>>>>>>>>>>> in headless the pack() will be throw an exception and second
>>>>>>>>>>>> part of
>>>>>>>>>>>> the test in this mode is unnecessary.
>>>>>>>>>>> Then maybe simply do not call pack() for the headless test?
>>>>>>>>>>> Okay, let me rephrase what I meant. Since isHeadless()=true
>>>>>>>>>>> case is
>>>>>>>>>>> mentioned in those 3 specs so explicitly it must be guarantied
>>>>>>>>>>> that
>>>>>>>>>>> the
>>>>>>>>>>> specified behavior works for the case as described. I cannot
>>>>>>>>>>> trace
>>>>>>>>>>> the
>>>>>>>>>>> result by reading the code of the createImage(), isHeadless()
>>>>>>>>>>> method is
>>>>>>>>>>> not even called there.  So the test case should be added. Or you
>>>>>>>>>>> could
>>>>>>>>>>> remove isHeadless() references from the specs. Or write
>>>>>>>>>>> something
>>>>>>>>>>> like
>>>>>>>>>>> "the result is non-deterministic if isHeadless() is not
>>>>>>>>>>> false..."
>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> --Semyon
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 9/23/2015 9:14 PM, Sergey Bylokhov wrote:
>>>>>>>>>>>>>> Hello.
>>>>>>>>>>>>>> Please review the fix for jdk9.
>>>>>>>>>>>>>> The specification is updated as suggested in JDK-6186530 and
>>>>>>>>>>>>>> JDK-6815345. The test is added to prove that we always return
>>>>>>>>>>>>>> null
>>>>>>>>>>>>>> when the component is not displayable.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> ccc request will be created after the technical review. One
>>>>>>>>>>>>>> additional
>>>>>>>>>>>>>> bug filed https://bugs.openjdk.java.net/browse/JDK-8137047
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-6815345
>>>>>>>>>>>>>> Webrev can be found at:
>>>>>>>>>>>>>> http://cr.openjdk.java.net/~serb/6815345/webrev.04
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
>

-- 
Best regards, Sergey.