Bug 79708 – [chromium] LayerChromium::contentChanged is redundant with setNeedsDisplay

• Component: New Bugs
• Status: RESOLVED FIXED

[James Robinson]

[chromium] LayerChromium::contentChanged is redundant with setNeedsDisplay

[James Robinson]

Created attachment 129111 [details]
Patch

[Shawn Singh]

Just wondering how this patch interacts with backer's quick-fix in https://bugs.webkit.org/show_bug.cgi?id=79373 ?

Would we be able to remove contentChanged for video layers, too, and instead put the m_updateRect statement inside setNeedsDisplay?

[James Robinson]

Thanks, I missed that (why on earth does VideoLayerChromium.h not declare that function virtual?).

Yes, I believe that the WebMediaPlayerClientImpl should call setNeedsDisplay() and that call should set the correct updateRect.

[James Robinson]

Created attachment 129119 [details]
Patch

[James Robinson]

Shawn - does the updateRect logic have to be layer-specific, or would it be valid to set this in LayerChromium::setNeedsDisplayRect() ?

[James Robinson]

Comment on attachment 129119 [details]
Patch

Shawn had some good offline feedback.  Will revise and post a new patch.

[James Robinson]

Created attachment 129135 [details]
Patch

[James Robinson]

Created attachment 129136 [details]
Patch

[James Robinson]

This moves m_updateRect updates to the base class LayerChromium::setNeedsDisplayRect since it's a more reasonable default behavior - layers can override this in updateCompositorResources (and TiledLayerChromium does), but having this be the default should prevent more bugs like https://bugs.webkit.org/show_bug.cgi?id=79373 from popping up in the future.

[Shawn Singh]

Yeah, this looks good to me. =)

[Jonathan Backer]

Thanks for the cleanup. I tested this patch with HTML5 video, accelerated canvas2D, and WebGL on Aura.

A few comments:

I think that you can ditch the change to m_updateRect in WebGLLayerChromium::updateCompositorResources

I think that we can ditch the m_dirtyRect tracking introduced in https://bugs.webkit.org/show_bug.cgi?id=73485 for PluginLayerChromium.

[James Robinson]

Created attachment 129294 [details]
Patch

[James Robinson]

(In reply to comment #11)
> I think that you can ditch the change to m_updateRect in WebGLLayerChromium::updateCompositorResources
> 
> I think that we can ditch the m_dirtyRect tracking introduced in https://bugs.webkit.org/show_bug.cgi?id=73485 for PluginLayerChromium.

Cool, done.  Thanks for testing.

[Jonathan Backer]

SGTM. I can test tomorrow morning if you'd like (exercising the plugin path is a bit of a pain).

[Jonathan Backer]

(In reply to comment #14)
> SGTM. I can test tomorrow morning if you'd like (exercising the plugin path is a bit of a pain).

All good from my perspective.

[Adrienne Walker]

Comment on attachment 129294 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/PluginLayerChromium.cpp:105
>  void PluginLayerChromium::invalidateRect(const FloatRect& dirtyRect)
>  {
>      setNeedsDisplayRect(dirtyRect);
> -    m_dirtyRect.unite(dirtyRect);
>  }

While we're removing things, can you get rid of this function too and have WebPluginContainerImpl just call setNeedsDisplay directly?

[James Robinson]

Created attachment 129777 [details]
Patch

[James Robinson]

(In reply to comment #16)
> While we're removing things, can you get rid of this function too and have WebPluginContainerImpl just call setNeedsDisplay directly?

Sure can, it's gone as well. Mind taking another look?

[Adrienne Walker]

Comment on attachment 129777 [details]
Patch

Looks good to me.  Thanks!

[James Robinson]

Committed r109469: <http://trac.webkit.org/changeset/109469>