Bug 83415 – [chromium] LayerRendererChromium shouldn't know anything about CCLayerImpl

• Component: New Bugs
• Status: RESOLVED FIXED

[James Robinson]

[chromium] LayerRendererChromium shouldn't know anything about CCLayerImpl

[James Robinson]

Created attachment 136118 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[James Robinson]

(note: this currently fails some webkit_unit_tests. I'm not sure why yet, but I want to check that my EWS changes to run unit tests work so I'ma let this patch fail).

[James Robinson]

Comment on attachment 136118 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:-398
> -    m_layerRenderer->beginDrawingFrame();

whoops, this call was kind of important!  will readd, but i want to see this fail EWS first

[WebKit Review Bot]

Comment on attachment 136118 [details]
Patch

Attachment 136118 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12371156

New failing tests:
compositing/checkerboard.html
compositing/fixed-position-changed-within-composited-parent-layer.html
animations/3d/matrix-transform-type-animation.html
animations/3d/state-at-end-event-transform.html
compositing/direct-image-compositing.html
compositing/video-page-visibility.html
animations/additive-transform-animations.html
animations/3d/replace-filling-transform.html
compositing/scrollbar-painting.html
compositing/backface-visibility.html
CCLayerTreeHostImplTest.reshapeNotCalledUntilDraw
compositing/text-on-large-layer.html
compositing/layers-inside-overflow-scroll.html
compositing/animation/state-at-end-event-transform-layer.html
compositing/sibling-positioning.html
compositing/generated-content.html
compositing/fixed-position-changed-in-composited-layer.html
compositing/self-painting-layers.html
compositing/culling/filter-occlusion-alpha-large.html
animations/3d/change-transform-in-end-event.html
compositing/absolute-position-changed-with-composited-parent-layer.html
compositing/absolute-position-changed-in-composited-layer.html
compositing/flat-with-transformed-child.html
accessibility/aria-disabled.html
compositing/backface-visibility-hierarchical-transform.html
compositing/culling/filter-occlusion-alpha.html
compositing/culling/clear-fixed-iframe.html
compositing/compositing-visible-descendant.html
compositing/culling/filter-occlusion-blur-large.html
CCLayerTreeHostTestAtomicCommitWithPartialUpdate.runMultiThread
compositing/color-matching/image-color-matching.html
CCLayerTreeHostTestAtomicCommit.runMultiThread
compositing/color-matching/pdf-image-match.html

[WebKit Review Bot]

Created attachment 136208 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[James Robinson]

Created attachment 136493 [details]
Patch

[Adrienne Walker]

Comment on attachment 136493 [details]
Patch

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

> Source/Platform/ChangeLog:6
> +        Remove unused compositeOffscreen setting.

...how is this change related?

> Source/WebCore/ChangeLog:10
> +        LayerRendererChromium shouldn't reach back up to CCLayerImpl. This gets rid of all CCLayerImpl references in
> +        LayerRendererChromium except for one which is being fixed in https://bugs.webkit.org/show_bug.cgi?id=83287.

This patch has already landed.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:402
> +    m_layerRenderer->beginDrawingFrame(rootLayer()->renderSurface());
> +
> +    // FIXME: use the frame begin time from the overall compositor scheduler.
> +    // This value is currently inaccessible because it is up in Chromium's
> +    // RenderWidget.
> +    m_headsUpDisplay->onFrameBegin(currentTime());

I think this should go before beginDrawingFrame.

[James Robinson]

(In reply to comment #8)
> (From update of attachment 136493 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136493&action=review
> 
> > Source/Platform/ChangeLog:6
> > +        Remove unused compositeOffscreen setting.
> 
> ...how is this change related?
> 

The compositeOffscreen touches the root layer in a few places in LayerRendererChromium.  Keeping it working would require plumbing lots of data down to LRC.  Since we've never used this path and don't plan to ever use it, seemed better to just remove it.

I could rip this out as a separate prerequisite patch to this one.  That's probably a good idea anyway.

> > Source/WebCore/ChangeLog:10
> > +        LayerRendererChromium shouldn't reach back up to CCLayerImpl. This gets rid of all CCLayerImpl references in
> > +        LayerRendererChromium except for one which is being fixed in https://bugs.webkit.org/show_bug.cgi?id=83287.
> 
> This patch has already landed.

Good point! Will update.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:402
> > +    m_layerRenderer->beginDrawingFrame(rootLayer()->renderSurface());
> > +
> > +    // FIXME: use the frame begin time from the overall compositor scheduler.
> > +    // This value is currently inaccessible because it is up in Chromium's
> > +    // RenderWidget.
> > +    m_headsUpDisplay->onFrameBegin(currentTime());
> 
> I think this should go before beginDrawingFrame.

Yep, probably should.

[James Robinson]

Comment on attachment 136493 [details]
Patch

I'd missed some of the compositeOffscreen support code, moved that patch (now patch series) out into https://bugs.webkit.org/show_bug.cgi?id=83733.  I'll refresh this after that lands.

[James Robinson]

Created attachment 137166 [details]
Patch

[James Robinson]

Shawn - this has the ownership changes for CCFontAtlas that I was mentioning on the other bug.

[WebKit Review Bot]

Comment on attachment 137166 [details]
Patch

Attachment 137166 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12400204

[James Robinson]

Oops, I accidentally too many #includes. Will fix.

[Shawn Singh]

(In reply to comment #12)
> Shawn - this has the ownership changes for CCFontAtlas that I was mentioning on the other bug.

So I took a look.  That's exactly what I had in mind when we discussed before, seems correct to me.

Sorry to have made you do this extra work, I should have waited for this to land before landing my other patch. =)

[James Robinson]

(In reply to comment #15)
> Sorry to have made you do this extra work, I should have waited for this to land before landing my other patch. =)

Don't worry about it - this patch had other dependencies that I had to take care of first, so you waiting wouldn't really have made anything easier.

[James Robinson]

Created attachment 137187 [details]
add PlatformCanvas.h include back to CCHeadsUpDisplay.cpp

[Adrienne Walker]

Comment on attachment 137187 [details]
add PlatformCanvas.h include back to CCHeadsUpDisplay.cpp

R=me.  Nice!

[WebKit Review Bot]

Comment on attachment 137187 [details]
add PlatformCanvas.h include back to CCHeadsUpDisplay.cpp

Clearing flags on attachment: 137187

Committed r114218: <http://trac.webkit.org/changeset/114218>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.