Bug 66433 - [chromium] Remove LayerRendererChromium references from TiledLayerChromium
Summary: [chromium] Remove LayerRendererChromium references from TiledLayerChromium
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adrienne Walker
URL:
Keywords:
Depends on:
Blocks: 66430
  Show dependency treegraph
 
Reported: 2011-08-17 16:51 PDT by Adrienne Walker
Modified: 2011-08-23 11:27 PDT (History)
5 users (show)

See Also:


Attachments
Patch (30.09 KB, patch)
2011-08-19 14:49 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Patch (30.07 KB, patch)
2011-08-22 13:12 PDT, Adrienne Walker
jamesr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adrienne Walker 2011-08-17 16:51:27 PDT
LayerChromium-derived classes should not have direct access to LayerRendererChromium, as it lives on the compositor thread.  So, refactor it so it doesn't need it.  This also requires updating all of the texture updaters to not cache memeber pointers to GraphicsContext3D and instead take them as function args when needed.
Comment 1 Adrienne Walker 2011-08-19 14:49:35 PDT
Created attachment 104571 [details]
Patch
Comment 2 James Robinson 2011-08-19 17:32:43 PDT
Comment on attachment 104571 [details]
Patch

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

I gotta run, but I still owe you this review.

> Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:97
> +    ImageLayerTextureUpdater(bool useMapTexSubImage)

guess what keyword this constructor now gets!
Comment 3 Adrienne Walker 2011-08-19 18:31:35 PDT
(In reply to comment #2)
> (From update of attachment 104571 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=104571&action=review
> 
> I gotta run, but I still owe you this review.
> 
> > Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:97
> > +    ImageLayerTextureUpdater(bool useMapTexSubImage)
> 
> guess what keyword this constructor now gets!

I need to figure out how to hook this up in the style bot.
Comment 4 Adrienne Walker 2011-08-22 13:12:07 PDT
Created attachment 104721 [details]
Patch
Comment 5 Adrienne Walker 2011-08-22 13:12:32 PDT
(In reply to comment #4)
> Created an attachment (id=104721) [details]
> Patch

Now with more 'explicit'.
Comment 6 Nat Duca 2011-08-22 13:31:47 PDT
Comment on attachment 104721 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:132
> +    m_textureUpdater = LayerTextureUpdaterBitmap::create(ContentLayerPainter::create(m_owner), host->contextSupportsMapSub());

contextSuportsMapSub is moving to the LayerRendererSide. Is there any way to not need this at construction time?
Comment 7 Adrienne Walker 2011-08-22 13:38:40 PDT
(In reply to comment #6)
> (From update of attachment 104721 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=104721&action=review
> 
> > Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:132
> > +    m_textureUpdater = LayerTextureUpdaterBitmap::create(ContentLayerPainter::create(m_owner), host->contextSupportsMapSub());
> 
> contextSuportsMapSub is moving to the LayerRendererSide. Is there any way to not need this at construction time?

My expectation was that we would have an explicit init phase where we queried the graphics context for things like max texture sizes and support for features.  So, we would have those settings on both sides.  You're going to need to know those things before the first commit anyway.

Is this not feasible?
Comment 8 James Robinson 2011-08-22 13:39:25 PDT
We don't need to know if we have mapSub or not until updateCompositorResources()
Comment 9 James Robinson 2011-08-22 14:59:21 PDT
Comment on attachment 104721 [details]
Patch

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

Looks like a step in the right direction.

For max texture size it makes sense to grab that information during initialization and stash it. To be truly exhaustive, we should be refetching this on every lost context, since the new context could have a different limit (if the user changed their gfx driver on win7, or they switched to some other output device or whatever).  For mapSub support and bestTextureFormat, we shouldn't need that until upload time so I wonder if we can do something fancier.  No great harm in stashing it for now, though.

> Source/WebCore/platform/graphics/chromium/LayerTextureUpdaterCanvas.h:55
> +    LayerTextureUpdaterCanvas(PassOwnPtr<LayerPainterChromium>);

harro one-arg constructor~~~~~~
Comment 10 Adrienne Walker 2011-08-23 11:09:08 PDT
Committed r93615: <http://trac.webkit.org/changeset/93615>
Comment 11 Adrienne Walker 2011-08-23 11:27:41 PDT
(In reply to comment #9)
> (From update of attachment 104721 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=104721&action=review
> 
> Looks like a step in the right direction.
> 
> For max texture size it makes sense to grab that information during initialization and stash it. To be truly exhaustive, we should be refetching this on every lost context, since the new context could have a different limit (if the user changed their gfx driver on win7, or they switched to some other output device or whatever).  For mapSub support and bestTextureFormat, we shouldn't need that until upload time so I wonder if we can do something fancier.  No great harm in stashing it for now, though.

I'd categorize these with scroll offset, as properties we can pull from the compositor thread when requesting a frame.  And, if any of these properties change, we can just invalidate the world.

> > Source/WebCore/platform/graphics/chromium/LayerTextureUpdaterCanvas.h:55
> > +    LayerTextureUpdaterCanvas(PassOwnPtr<LayerPainterChromium>);
> 
> harro one-arg constructor~~~~~~

/me mumbles.