Bug 103215

Summary: Coordinated Graphics: Refactor code managing a backing store in LayerTreeRenderer.
Product: WebKit Reporter: Dongseong Hwang <dongseong.hwang>
Component: Layout and RenderingAssignee: Dongseong Hwang <dongseong.hwang>
Status: RESOLVED DUPLICATE    
Severity: Normal CC: noam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Dongseong Hwang
Reported 2012-11-25 22:19:42 PST
This patch makes LayerTreeRenderer assign a content backing store to TextureMapperLayer only in setLayerState(). Currently, createTile() can assign the backing store to TextureMapperLayer and it can break the invariant condition of TextureMapperLayer: TextureMapperLayer can have its own backing store only if the layer has following conditions: drawsContent, contentsVisible and non empty size. In addition, the modified code about creating and removing a backing store matches the same purpose code of CoordinatedGraphicsLayer and GraphicsLayerTextureMapepr.
Attachments
Patch (10.88 KB, patch)
2012-11-25 22:29 PST, Dongseong Hwang 		no flags
DetailsFormatted DiffDiff
Patch (11.36 KB, patch)
2012-11-25 23:03 PST, Dongseong Hwang 		no flags
DetailsFormatted DiffDiff
Patch (11.20 KB, patch)
2012-11-25 23:42 PST, Dongseong Hwang 		no flags
DetailsFormatted DiffDiff
Patch (11.20 KB, patch)
2012-11-26 00:31 PST, Dongseong Hwang 		no flags
DetailsFormatted DiffDiff
Patch (11.20 KB, patch)
2012-11-26 14:19 PST, Dongseong Hwang 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Dongseong Hwang
Comment 1 2012-11-25 22:29:22 PST
Created attachment 175925 [details] Patch
Noam Rosenthal
Comment 2 2012-11-25 22:41:11 PST
Comment on attachment 175925 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175925&action=review > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:416 > + if (graphicsLayer->drawsContent() && graphicsLayer->contentsAreVisible() && !graphicsLayer->size().isEmpty()) Put this in a static function layerShouldHaveBackingStore > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:419 > + else if (getBackingStore(graphicsLayer)) > + removeBackingStore(graphicsLayer); I would just have removeBackingStore return early if there is no backing store. > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:427 > + if (getBackingStore(graphicsLayer)) > + return; > + > TextureMapperLayer* layer = toTextureMapperLayer(graphicsLayer); I'd prefer it if this function didn't call getBackingStore, but returned early on if layer->backingStore() > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:439 > RefPtr<CoordinatedBackingStore> backingStore = static_cast<CoordinatedBackingStore*>(layer->backingStore().get()); > ASSERT(backingStore); This is not needed. > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:465 > + // LayerTreeRenderer::setLayerState removes a backing store. Unnecessary comment.
Dongseong Hwang
Comment 3 2012-11-25 23:03:27 PST
Created attachment 175929 [details] Patch
Dongseong Hwang
Comment 4 2012-11-25 23:05:08 PST
(In reply to comment #2) Thanks for review! > (From update of attachment 175925 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175925&action=review > > > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:416 > > + if (graphicsLayer->drawsContent() && graphicsLayer->contentsAreVisible() && !graphicsLayer->size().isEmpty()) > > Put this in a static function layerShouldHaveBackingStore Done. > > > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:419 > > + else if (getBackingStore(graphicsLayer)) > > + removeBackingStore(graphicsLayer); > > I would just have removeBackingStore return early if there is no backing store. Done. > > > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:427 > > + if (getBackingStore(graphicsLayer)) > > + return; > > + > > TextureMapperLayer* layer = toTextureMapperLayer(graphicsLayer); > > I'd prefer it if this function didn't call getBackingStore, but returned early on if layer->backingStore() Done. > > > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:439 > > RefPtr<CoordinatedBackingStore> backingStore = static_cast<CoordinatedBackingStore*>(layer->backingStore().get()); > > ASSERT(backingStore); > > This is not needed. You're right! Done. > > > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:465 > > + // LayerTreeRenderer::setLayerState removes a backing store. > > Unnecessary comment. Done.
Noam Rosenthal
Comment 5 2012-11-25 23:17:34 PST
Comment on attachment 175929 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175929&action=review > Source/WebKit2/ChangeLog:39 > + if needed. Moreover, we must sync a layer state prior to create a create -> creating. Also lines are too short... > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:422 > + if (!shouldHaveBackingStore && getBackingStore(graphicsLayer)) { The call to getBackingStore(graphicsLayer) is redundant. > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:427 > + if (shouldHaveBackingStore) This is not really needed if the above call is redundant.
Dongseong Hwang
Comment 6 2012-11-25 23:42:14 PST
Created attachment 175935 [details] Patch
Dongseong Hwang
Comment 7 2012-11-25 23:43:32 PST
(In reply to comment #5) > (From update of attachment 175929 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175929&action=review > > > Source/WebKit2/ChangeLog:39 > > + if needed. Moreover, we must sync a layer state prior to create a > > create -> creating. > Also lines are too short... Do you mean writing more words in lines? If so, done. > > > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:422 > > + if (!shouldHaveBackingStore && getBackingStore(graphicsLayer)) { > > The call to getBackingStore(graphicsLayer) is redundant. Ok, Done. > > > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:427 > > + if (shouldHaveBackingStore) > > This is not really needed if the above call is redundant. Ok, Done.
Dongseong Hwang
Comment 8 2012-11-26 00:31:30 PST
Created attachment 175937 [details] Patch
Dongseong Hwang
Comment 9 2012-11-26 00:31:53 PST
(In reply to comment #8) > Created an attachment (id=175937) [details] > Patch post for running ews.
Noam Rosenthal
Comment 10 2012-11-26 09:04:45 PST
Comment on attachment 175937 [details] Patch LGTM, but EWS is purple so it probably doesn't apply...
Dongseong Hwang
Comment 11 2012-11-26 14:19:24 PST
Created attachment 176063 [details] Patch
Dongseong Hwang
Comment 12 2012-11-26 14:20:24 PST
(In reply to comment #10) > (From update of attachment 175937 [details]) > LGTM, but EWS is purple so it probably doesn't apply... Thanks for review! I don't know why ews is purlple. I tried again.
Dongseong Hwang
Comment 13 2012-11-26 14:44:18 PST
(In reply to comment #12) > (In reply to comment #10) > > (From update of attachment 175937 [details] [details]) > > LGTM, but EWS is purple so it probably doesn't apply... > > Thanks for review! > > I don't know why ews is purlple. I tried again. EWS is purple too.. I think this bug has some problems... I'll make a duplicated new bug.
Dongseong Hwang
Comment 14 2012-11-26 14:46:26 PST
*** This bug has been marked as a duplicate of bug 103298 ***
Note You need to log in before you can comment on or make changes to this bug.