Bug 79708

Summary: [chromium] LayerChromium::contentChanged is redundant with setNeedsDisplay
Product: WebKit Reporter: James Robinson <jamesr>
Component: New BugsAssignee: James Robinson <jamesr>
Status: RESOLVED FIXED    
Severity: Normal CC: backer, cc-bugs, shawnsingh, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 79428    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch enne: review+

Description James Robinson 2012-02-27 15:21:29 PST
[chromium] LayerChromium::contentChanged is redundant with setNeedsDisplay
Comment 1 James Robinson 2012-02-27 15:25:34 PST
Created attachment 129111 [details]
Patch
Comment 2 Shawn Singh 2012-02-27 15:31:48 PST
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?
Comment 3 James Robinson 2012-02-27 15:38:41 PST
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.
Comment 4 James Robinson 2012-02-27 15:58:04 PST
Created attachment 129119 [details]
Patch
Comment 5 James Robinson 2012-02-27 16:00:02 PST
Shawn - does the updateRect logic have to be layer-specific, or would it be valid to set this in LayerChromium::setNeedsDisplayRect() ?
Comment 6 James Robinson 2012-02-27 16:30:44 PST
Comment on attachment 129119 [details]
Patch

Shawn had some good offline feedback.  Will revise and post a new patch.
Comment 7 James Robinson 2012-02-27 17:00:40 PST
Created attachment 129135 [details]
Patch
Comment 8 James Robinson 2012-02-27 17:02:54 PST
Created attachment 129136 [details]
Patch
Comment 9 James Robinson 2012-02-27 17:04:41 PST
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.
Comment 10 Shawn Singh 2012-02-27 17:10:39 PST
Yeah, this looks good to me. =)
Comment 11 Jonathan Backer 2012-02-28 08:39:25 PST
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.
Comment 12 James Robinson 2012-02-28 11:10:08 PST
Created attachment 129294 [details]
Patch
Comment 13 James Robinson 2012-02-28 11:12:12 PST
(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.
Comment 14 Jonathan Backer 2012-02-28 14:30:13 PST
SGTM. I can test tomorrow morning if you'd like (exercising the plugin path is a bit of a pain).
Comment 15 Jonathan Backer 2012-03-01 08:19:34 PST
(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.
Comment 16 Adrienne Walker 2012-03-01 11:45:58 PST
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?
Comment 17 James Robinson 2012-03-01 16:57:27 PST
Created attachment 129777 [details]
Patch
Comment 18 James Robinson 2012-03-01 17:02:02 PST
(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?
Comment 19 Adrienne Walker 2012-03-01 17:09:35 PST
Comment on attachment 129777 [details]
Patch

Looks good to me.  Thanks!
Comment 20 James Robinson 2012-03-01 17:14:30 PST
Committed r109469: <http://trac.webkit.org/changeset/109469>