Bug 52852 - Flushing layer changes and rendering are intertwined in CACFLayerTreeHost, but shouldn't be
Summary: Flushing layer changes and rendering are intertwined in CACFLayerTreeHost, bu...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Adam Roben (:aroben)
URL:
Keywords: PlatformOnly
Depends on: 52894 52898
Blocks:
  Show dependency treegraph
 
Reported: 2011-01-20 16:23 PST by Adam Roben (:aroben)
Modified: 2011-01-21 11:36 PST (History)
2 users (show)

See Also:


Attachments
Separate compositing state syncing from rendering in WKCACFLayerRenderer (19.60 KB, patch)
2011-01-20 16:25 PST, Adam Roben (:aroben)
no flags Details | Formatted Diff | Diff
Separate flushing layer changes from rendering in CACFLayerView (22.47 KB, patch)
2011-01-21 09:07 PST, Adam Roben (:aroben)
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Roben (:aroben) 2011-01-20 16:23:11 PST
Syncing compositing state and rendering are intertwined in WKCACFLayerRenderer, but shouldn't be
Comment 1 Adam Roben (:aroben) 2011-01-20 16:25:07 PST
Created attachment 79664 [details]
Separate compositing state syncing from rendering in WKCACFLayerRenderer
Comment 2 Simon Fraser (smfr) 2011-01-20 22:56:52 PST
Comment on attachment 79664 [details]
Separate compositing state syncing from rendering in WKCACFLayerRenderer

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

> Source/WebCore/platform/graphics/ca/win/CompositingStateSyncer.cpp:92
> +    m_isCallingRenderers = true;
> +    for (size_t i = 0; i < renderers.size(); ++i)
> +        renderers[i]->syncCompositingStateNowIfNeeded();
> +    m_isCallingRenderers = false;

The name m_isCallingRenderers is a bit vague (and easily confused with Render objects).

> Source/WebCore/platform/graphics/ca/win/CompositingStateSyncer.h:39
> +class CompositingStateSyncer {

I'm not a big fan of this name. I started to use "flush pending layer changes" nomenclature in RenderLayerCompositor (we're not really synchronizing anything here).
Comment 3 Adam Roben (:aroben) 2011-01-21 05:24:33 PST
Comment on attachment 79664 [details]
Separate compositing state syncing from rendering in WKCACFLayerRenderer

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

>> Source/WebCore/platform/graphics/ca/win/CompositingStateSyncer.h:39
>> +class CompositingStateSyncer {
> 
> I'm not a big fan of this name. I started to use "flush pending layer changes" nomenclature in RenderLayerCompositor (we're not really synchronizing anything here).

The current name certainly seems more consistent with the current terminology (e.g., ChromeClient::scheduleCompositingStateSync). But I guess it's better to be forward-looking, if you're planning to change the existing names. Maybe LayerChangeFlusher is the way to go (along with flushLayerChangesSoon(WKCACFLayerRenderer*) and m_isFlushingLayerChanges).
Comment 4 Adam Roben (:aroben) 2011-01-21 05:52:29 PST
Comment on attachment 79664 [details]
Separate compositing state syncing from rendering in WKCACFLayerRenderer

This patch breaks <video>.
Comment 5 Adam Roben (:aroben) 2011-01-21 08:40:41 PST
I went ahead and got rid of uses of "sync compositing state" in Windows-specific code in bug 52894 and bug 52898. I'll upload a new patch that takes the new names into account and also fixes <video>.
Comment 6 Adam Roben (:aroben) 2011-01-21 09:07:40 PST
Created attachment 79744 [details]
Separate flushing layer changes from rendering in CACFLayerView
Comment 7 Adam Roben (:aroben) 2011-01-21 11:03:10 PST
Yet another rename in bug 52898 means that this patch will have to be updated before landing, but it will just be a mechanical rename change.
Comment 8 Adam Roben (:aroben) 2011-01-21 11:36:56 PST
Committed r76372: <http://trac.webkit.org/changeset/76372>