Summary: | [chromium] LayerChromium::contentChanged is redundant with setNeedsDisplay | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | James Robinson <jamesr> | ||||||||||||||
Component: | New Bugs | Assignee: | 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
James Robinson
2012-02-27 15:21:29 PST
Created attachment 129111 [details]
Patch
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? 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. Created attachment 129119 [details]
Patch
Shawn - does the updateRect logic have to be layer-specific, or would it be valid to set this in LayerChromium::setNeedsDisplayRect() ? Comment on attachment 129119 [details]
Patch
Shawn had some good offline feedback. Will revise and post a new patch.
Created attachment 129135 [details]
Patch
Created attachment 129136 [details]
Patch
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. Yeah, this looks good to me. =) 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. Created attachment 129294 [details]
Patch
(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. SGTM. I can test tomorrow morning if you'd like (exercising the plugin path is a bit of a pain). (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 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? Created attachment 129777 [details]
Patch
(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 on attachment 129777 [details]
Patch
Looks good to me. Thanks!
Committed r109469: <http://trac.webkit.org/changeset/109469> |