Bug 41954 - [Qt] GraphicsLayerQt must have syncCompositingStateForThisLayerOnly() implemented
Summary: [Qt] GraphicsLayerQt must have syncCompositingStateForThisLayerOnly() impleme...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Jesus Sanchez-Palencia
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks: 38744
  Show dependency treegraph
 
Reported: 2010-07-09 05:18 PDT by Jesus Sanchez-Palencia
Modified: 2010-07-14 06:03 PDT (History)
3 users (show)

See Also:


Attachments
Patch (2.20 KB, patch)
2010-07-09 05:32 PDT, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff
Patch (5.68 KB, patch)
2010-07-13 13:29 PDT, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff
Patch (1.67 KB, patch)
2010-07-13 14:59 PDT, Jesus Sanchez-Palencia
kenneth: review+
jesus: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jesus Sanchez-Palencia 2010-07-09 05:18:22 PDT
[Qt] GraphicsLayerQt must have syncCompositingStateForThisLayerOnly() implemented
Comment 1 Jesus Sanchez-Palencia 2010-07-09 05:32:37 PDT
Created attachment 61036 [details]
Patch
Comment 2 Jesus Sanchez-Palencia 2010-07-09 05:37:59 PDT
Noam and Kenneth, could you please take at this? We need to implement this in order to get AC working with WebKit2.

Thanks in advance!
Comment 3 Noam Rosenthal 2010-07-09 06:40:48 PDT
the first think I'd do for that is to make sure only flushChanges calls m_layer and m_pendingContent, and everyone else calls m_state and m_currentContent.
 I think maybe updateTransform still calls m_layer directly - also the animations, but that's fine because animations work on an already synced layer.

~ m_layer and m_pendingContent contain the non-synced state, m_state and m_currentContent contain the synced state.
Comment 4 WebKit Commit Bot 2010-07-09 15:21:17 PDT
Comment on attachment 61036 [details]
Patch

Clearing flags on attachment: 61036

Committed r63001: <http://trac.webkit.org/changeset/63001>
Comment 5 WebKit Commit Bot 2010-07-09 15:21:22 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Jesus Sanchez-Palencia 2010-07-13 13:29:21 PDT
Created attachment 61410 [details]
Patch
Comment 7 Jesus Sanchez-Palencia 2010-07-13 13:36:12 PDT
Comment on attachment 61410 [details]
Patch

I'm not going to ask for review it.

No'am, could you please take a look at this follow-up patch? I'm addressing your last comments with a few exceptions.

I noticed that modifying GraphicsLayerQtImpl::recache(), 
GraphicsLayerQtImpl::notifySyncRequired() and GraphicsLayerQt::setContentsToImage() leaves Accelerated Compositing with a strange behavior, crashing often and not working on some examples (i.e.g.: falling leaves).

It would help to have more info on whether a function should use the synced state or the non-synced one. Maybe I missed something...

thanks in advance!
Comment 8 Noam Rosenthal 2010-07-13 13:43:20 PDT
Comment on attachment 61410 [details]
Patch

> -    if (!m_transformAnimationRunning)
> -        m_baseTransform = m_layer->transform();
> +    if (!m_transformAnimationRunning && m_state.maskLayer)
> +        m_baseTransform = m_state.maskLayer->transform();

should be m_state.transform, not m_state.maskLayer->transform. There's no mask layer involved

>  void GraphicsLayerQt::setNeedsDisplay()
>  {
> -    m_impl->m_pendingContent.regionToUpdate = QRegion(QRect(QPoint(0, 0), QSize(size().width(), size().height())));
> +    m_impl->m_currentContent.regionToUpdate = QRegion(QRect(QPoint(0, 0), QSize(size().width(), size().height())));
>      m_impl->notifyChange(GraphicsLayerQtImpl::DisplayChange);
>  }

No need for all that :) it needs to update m_pendingContent and not m_currentContent. The only function we really need to change is updateTransform(), to not read m_layer->transform().
Comment 9 Jesus Sanchez-Palencia 2010-07-13 14:59:54 PDT
Created attachment 61422 [details]
Patch
Comment 10 Noam Rosenthal 2010-07-13 15:14:30 PDT
LGTM