Bug 66433 – [chromium] Remove LayerRendererChromium references from TiledLayerChromium

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Adrienne Walker]

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.

[Adrienne Walker]

Created attachment 104571 [details]
Patch

[James Robinson]

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!

[Adrienne Walker]

(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.

[Adrienne Walker]

Created attachment 104721 [details]
Patch

[Adrienne Walker]

(In reply to comment #4)
> Created an attachment (id=104721) [details]
> Patch

Now with more 'explicit'.

[Nat Duca]

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?

[Adrienne Walker]

(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?

[James Robinson]

We don't need to know if we have mapSub or not until updateCompositorResources()

[James Robinson]

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~~~~~~

[Adrienne Walker]

Committed r93615: <http://trac.webkit.org/changeset/93615>

[Adrienne Walker]

(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.