Bug 91124 - [chromium] The root layer should not try create a second RenderSurface for itself
Summary: [chromium] The root layer should not try create a second RenderSurface for it...
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: Dana Jansens
URL:
Keywords:
Depends on:
Blocks: 91023
  Show dependency treegraph
 
Reported: 2012-07-12 11:22 PDT by Dana Jansens
Modified: 2012-07-12 14:42 PDT (History)
8 users (show)

See Also:


Attachments
Patch (3.68 KB, patch)
2012-07-12 11:23 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch for landing (3.74 KB, patch)
2012-07-12 11:57 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (3.77 KB, patch)
2012-07-12 14:40 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dana Jansens 2012-07-12 11:22:45 PDT
[chromium] The root layer should not try create a second RenderSurface for itself
Comment 1 Dana Jansens 2012-07-12 11:23:52 PDT
Created attachment 152009 [details]
Patch
Comment 2 Adrienne Walker 2012-07-12 11:48:10 PDT
Comment on attachment 152009 [details]
Patch

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

R=me.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:264
> +    // The root layer always has a surface already.

It's not that it has a surface already, it's that its surface is special (has a pre-set size, doesn't get a texture, is already in the surface list) so it doesn't go down the "separate surface" path.
Comment 3 Dana Jansens 2012-07-12 11:51:43 PDT
(In reply to comment #2)
> (From update of attachment 152009 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152009&action=review
> 
> R=me.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:264
> > +    // The root layer always has a surface already.
> 
> It's not that it has a surface already, it's that its surface is special (has a pre-set size, doesn't get a texture, is already in the surface list) so it doesn't go down the "separate surface" path.

Hm, kinda disagree. It has a RenderSurface object on it already, but agree I can word this better.
Comment 4 Dana Jansens 2012-07-12 11:57:12 PDT
Created attachment 152016 [details]
Patch for landing
Comment 5 Shawn Singh 2012-07-12 12:21:54 PDT
I agree more with Enne's original wording... to me, that wording pointed out the idiosyncrasies in the current code that motivate putting this if-statement here.  It might be true that the renderSurface existed already, but that's something we can (and may want to) change down the road.

Actually on the latest patch for landing, I like the comment wording even less.  It seems a bit misleading in my opinion.  I personally think the right thing to say here is to explicitly acknowledge that its a deviation from the intended clean concept.

something like: "even though technically the root layer's subtree does render to a separate surface, we want to force the root layer to go down the non-surface code path because it shares more common code with that case since the root surface is already expected to be initialized by outside code. This kink could eventually be fixed by reducing the special-ness of the root layer and its surface."

I know its wordy... but that's the spirit of the comment I think is appropriate =)
Comment 6 Dana Jansens 2012-07-12 13:05:18 PDT
This is why we shouldn't have comments :P
Comment 7 Dana Jansens 2012-07-12 14:40:33 PDT
Created attachment 152067 [details]
Patch
Comment 8 Dana Jansens 2012-07-12 14:42:48 PDT
Committed r122511: <http://trac.webkit.org/changeset/122511>