Bug 87602 - [BlackBerry] Make it possible to manipulate layers on the compositing thread
Summary: [BlackBerry] Make it possible to manipulate layers on the compositing thread
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit BlackBerry (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Arvid Nilsson
URL:
Keywords:
Depends on: 87601
Blocks: 87603
  Show dependency treegraph
 
Reported: 2012-05-27 15:05 PDT by Arvid Nilsson
Modified: 2012-05-28 15:23 PDT (History)
8 users (show)

See Also:


Attachments
Patch (12.57 KB, patch)
2012-05-27 15:42 PDT, Arvid Nilsson
no flags Details | Formatted Diff | Diff
Patch (18.40 KB, patch)
2012-05-27 16:01 PDT, Arvid Nilsson
no flags Details | Formatted Diff | Diff
Patch (13.75 KB, patch)
2012-05-28 14:26 PDT, Arvid Nilsson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Arvid Nilsson 2012-05-27 15:05:14 PDT
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.
Comment 1 Arvid Nilsson 2012-05-27 15:42:11 PDT
Created attachment 144244 [details]
Patch
Comment 2 Arvid Nilsson 2012-05-27 16:01:33 PDT
Created attachment 144247 [details]
Patch
Comment 3 Arvid Nilsson 2012-05-27 16:08:41 PDT
Comment on attachment 144244 [details]
Patch

Hmm, sorry about that, webkit-upload got the bug wrong
Comment 4 Yong Li 2012-05-28 09:48:08 PDT
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.
Comment 5 Rob Buis 2012-05-28 10:59:04 PDT
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.
Comment 6 Arvid Nilsson 2012-05-28 14:12:06 PDT
(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
Comment 7 Arvid Nilsson 2012-05-28 14:26:52 PDT
Created attachment 144397 [details]
Patch
Comment 8 Rob Buis 2012-05-28 14:30:31 PDT
Comment on attachment 144397 [details]
Patch

Looks good.
Comment 9 WebKit Review Bot 2012-05-28 14:53:25 PDT
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
Comment 10 WebKit Review Bot 2012-05-28 15:22:55 PDT
Comment on attachment 144397 [details]
Patch

Clearing flags on attachment: 144397

Committed r118709: <http://trac.webkit.org/changeset/118709>
Comment 11 WebKit Review Bot 2012-05-28 15:23:00 PDT
All reviewed patches have been landed.  Closing bug.