Bug 83415 - [chromium] LayerRendererChromium shouldn't know anything about CCLayerImpl
Summary: [chromium] LayerRendererChromium shouldn't know anything about CCLayerImpl
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Robinson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-06 18:39 PDT by James Robinson
Modified: 2012-04-15 19:45 PDT (History)
8 users (show)

See Also:


Attachments
Patch (29.85 KB, patch)
2012-04-06 18:44 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (6.38 MB, application/zip)
2012-04-09 03:32 PDT, WebKit Review Bot
no flags Details
Patch (30.02 KB, patch)
2012-04-10 11:26 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Patch (47.99 KB, patch)
2012-04-13 15:41 PDT, James Robinson
no flags Details | Formatted Diff | Diff
add PlatformCanvas.h include back to CCHeadsUpDisplay.cpp (47.97 KB, patch)
2012-04-13 17:37 PDT, James Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2012-04-06 18:39:47 PDT
[chromium] LayerRendererChromium shouldn't know anything about CCLayerImpl
Comment 1 James Robinson 2012-04-06 18:44:16 PDT
Created attachment 136118 [details]
Patch
Comment 2 WebKit Review Bot 2012-04-06 18:46:35 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 James Robinson 2012-04-06 18:47:15 PDT
(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).
Comment 4 James Robinson 2012-04-06 18:59:41 PDT
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
Comment 5 WebKit Review Bot 2012-04-09 03:32:40 PDT
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
Comment 6 WebKit Review Bot 2012-04-09 03:32:46 PDT
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
Comment 7 James Robinson 2012-04-10 11:26:15 PDT
Created attachment 136493 [details]
Patch
Comment 8 Adrienne Walker 2012-04-10 20:49:41 PDT
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.
Comment 9 James Robinson 2012-04-10 21:11:30 PDT
(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.
Comment 10 James Robinson 2012-04-11 17:22:41 PDT
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.
Comment 11 James Robinson 2012-04-13 15:41:14 PDT
Created attachment 137166 [details]
Patch
Comment 12 James Robinson 2012-04-13 15:42:19 PDT
Shawn - this has the ownership changes for CCFontAtlas that I was mentioning on the other bug.
Comment 13 WebKit Review Bot 2012-04-13 16:06:22 PDT
Comment on attachment 137166 [details]
Patch

Attachment 137166 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12400204
Comment 14 James Robinson 2012-04-13 16:07:25 PDT
Oops, I accidentally too many #includes. Will fix.
Comment 15 Shawn Singh 2012-04-13 16:12:11 PDT
(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. =)
Comment 16 James Robinson 2012-04-13 16:42:57 PDT
(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.
Comment 17 James Robinson 2012-04-13 17:37:45 PDT
Created attachment 137187 [details]
add PlatformCanvas.h include back to CCHeadsUpDisplay.cpp
Comment 18 Adrienne Walker 2012-04-13 17:47:55 PDT
Comment on attachment 137187 [details]
add PlatformCanvas.h include back to CCHeadsUpDisplay.cpp

R=me.  Nice!
Comment 19 WebKit Review Bot 2012-04-15 19:44:58 PDT
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>
Comment 20 WebKit Review Bot 2012-04-15 19:45:12 PDT
All reviewed patches have been landed.  Closing bug.