Bug 87602 – [BlackBerry] Make it possible to manipulate layers on the compositing thread

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit BlackBerry

[Arvid Nilsson]

Normally, layers are manipulated on the WebKit thread, and the changes are
    synced to the compositing thread during the next accelerated compositing commit
    operation.
    
    However, for overlay layers the ability to manipulate layers on the compositing
    thread is necessary for adequate user interface responsiveness.
    
    Two mechanisms are added:
    1. For a layer with a WebKit-thread counterpart, you can temporarily override
    attributes or add animations whose output override attributes.
    2. For a layer with no WebKit-thread counterpart, you can now directly set the
    compositing thread values for attributes. If you attempt to do this for layers
    that do have a WebKit-thread counterpart, the compositing thread values will
    be overwritten at the time of the next commit, which makes the override
    mechanism more useful there.
    
    PR #156812


Please note that this has not been internally reviewed yet, so it needs actual review. And also, it has a dependency that needs to be landed first.

[Arvid Nilsson]

Created attachment 144244 [details]
Patch

[Arvid Nilsson]

Created attachment 144247 [details]
Patch

[Arvid Nilsson]

Comment on attachment 144244 [details]
Patch

Hmm, sorry about that, webkit-upload got the bug wrong

[Yong Li]

Comment on attachment 144244 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144244&action=review

> Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.h:77
> +    FloatPoint position;
> +    FloatPoint anchorPoint;
> +    IntSize bounds;
> +    TransformationMatrix transform;
> +    float opacity;
> +    Vector<RefPtr<LayerAnimation> > m_animations;

All members don't have "m_" prefix but m_animations. Should be in same style?

More thought: how to avoid people from setting those members directly and forgetting to set the flags?

I would rather make them private, and provide read-only get() methods for them.

[Rob Buis]

Comment on attachment 144244 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144244&action=review

Ok, can be cleaned up a bit more I think.

> Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:394
> +    layer->setSuperlayer(this);

Would it make sense for setSuperlayer to call removeFromSuperlayer internally?

> Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:498
> +    if (m_override) {

Should probably use early return style here, code will be more readable.

>> Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.h:77
>> +    Vector<RefPtr<LayerAnimation> > m_animations;
> 
> All members don't have "m_" prefix but m_animations. Should be in same style?
> 
> More thought: how to avoid people from setting those members directly and forgetting to set the flags?
> 
> I would rather make them private, and provide read-only get() methods for them.

Agree with Yong, m_ is a good prefix to use.

[Arvid Nilsson]

(In reply to comment #5)
> (From update of attachment 144244 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144244&action=review
> 
> Ok, can be cleaned up a bit more I think.
> 
> > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:394
> > +    layer->setSuperlayer(this);
> 
> Would it make sense for setSuperlayer to call removeFromSuperlayer internally?

That's a great suggestion! However, normally the layer hierarchy on the compositing thread is updated in void LayerWebKitThread::updateLayerHierarchy(), by forcing the compositing thread hierarchy to be equal to the webkit thread hieararchy. That's why there's no sanity checks in the set methods (setsuperlayer, setsublayers). Cleaning that up should be a separate patch I think.

> 
> > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:498
> > +    if (m_override) {
> 
> Should probably use early return style here, code will be more readable.
> 
> >> Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.h:77
> >> +    Vector<RefPtr<LayerAnimation> > m_animations;
> > 
> > All members don't have "m_" prefix but m_animations. Should be in same style?
> > 
> > More thought: how to avoid people from setting those members directly and forgetting to set the flags?
> > 
> > I would rather make them private, and provide read-only get() methods for them.
> 
> Agree with Yong, m_ is a good prefix to use.

New patch forthcoming

[Arvid Nilsson]

Created attachment 144397 [details]
Patch

[Rob Buis]

Comment on attachment 144397 [details]
Patch

Looks good.

[WebKit Review Bot]

Comment on attachment 144397 [details]
Patch

Rejecting attachment 144397 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

Last 500 characters of output:
  return self.open(self.click(*args, **kwds))
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_mechanize.py", line 203, in open
    return self._mech_open(url, data, timeout=timeout)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_mechanize.py", line 255, in _mech_open
    raise response
webkitpy.thirdparty.autoinstalled.mechanize._response.httperror_seek_wrapper: HTTP Error 500: Internal Server Error

Full output: http://queues.webkit.org/results/12846288

[WebKit Review Bot]

Comment on attachment 144397 [details]
Patch

Clearing flags on attachment: 144397

Committed r118709: <http://trac.webkit.org/changeset/118709>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.