Bug 103297 - Coordinated Graphics: Refactor code related to mask and replication.
Summary: Coordinated Graphics: Refactor code related to mask and replication.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dongseong Hwang
URL:
Keywords:
Depends on: 103368
Blocks:
  Show dependency treegraph
 
Reported: 2012-11-26 14:40 PST by Dongseong Hwang
Modified: 2012-11-27 00:22 PST (History)
3 users (show)

See Also:


Attachments
Patch (9.79 KB, patch)
2012-11-26 14:43 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff
Patch (3.31 KB, patch)
2012-11-26 18:42 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff
Patch. Coordinated Graphics: traverse a mask layer and a replication layer too. (4.14 KB, patch)
2012-11-26 18:49 PST, Dongseong Hwang
noam: review-
noam: commit-queue-
Details | Formatted Diff | Diff
Patch4 (3.05 KB, patch)
2012-11-26 18:51 PST, Dongseong Hwang
noam: review+
noam: commit-queue-
Details | Formatted Diff | Diff
Patch (4.78 KB, patch)
2012-11-26 19:31 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff
Patch (3.12 KB, patch)
2012-11-26 21:53 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff
Patch7 that changed changelog of Patch4 (3.12 KB, patch)
2012-11-26 22:11 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dongseong Hwang 2012-11-26 14:40:59 PST
This patch makes code related to mask and replication in
GraphicsLayerTextureMapper be consistent with them in CoordinatedGraphicsLayer.

1. setSize().
GraphicsLayerTextureMapper::setSize() also sets the size of a mask layer like
CoordinatedGraphicsLayer. setMaksLayer ditto.

2. setContentsVisible().
GraphicsLayerTextureMapper::setContentsVisible() and
CoordinatedGraphicsLayer::setContentsVisible() set the visibility of a mask layer
like setSize().

3. recursive traverse.
We sometime miss traversing a mask or replication layer. This patch fixs them.

4. remove maskTarget in CoordinatedGraphicsLayer.
It is not used.
Comment 1 Dongseong Hwang 2012-11-26 14:43:00 PST
Created attachment 176071 [details]
Patch
Comment 2 Noam Rosenthal 2012-11-26 17:22:09 PST
Comment on attachment 176071 [details]
Patch

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

Those fixes are different, and should come in separate patches. You don't need new bug reports, simply attach different patches to the same bug.

> Source/WebKit2/ChangeLog:24
> +        4. remove maskTarget in CoordinatedGraphicsLayer.
> +        It is not used.

This is the only part that should apply to this changelog.
Comment 3 Dongseong Hwang 2012-11-26 18:42:02 PST
Created attachment 176141 [details]
Patch
Comment 4 Dongseong Hwang 2012-11-26 18:49:14 PST
Created attachment 176145 [details]
Patch. Coordinated Graphics: traverse a mask layer and a replication layer too.
Comment 5 Dongseong Hwang 2012-11-26 18:51:57 PST
Created attachment 176146 [details]
Patch4
Comment 6 Dongseong Hwang 2012-11-26 19:31:32 PST
Created attachment 176150 [details]
Patch
Comment 7 Dongseong Hwang 2012-11-26 19:32:03 PST
(In reply to comment #2)
> (From update of attachment 176071 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176071&action=review
> 
> Those fixes are different, and should come in separate patches. You don't need new bug reports, simply attach different patches to the same bug.
> 
> > Source/WebKit2/ChangeLog:24
> > +        4. remove maskTarget in CoordinatedGraphicsLayer.
> > +        It is not used.
> 
> This is the only part that should apply to this changelog.

Thanks for review!

I separated 4 patches.
Comment 8 WebKit Review Bot 2012-11-26 20:41:54 PST
Comment on attachment 176141 [details]
Patch

Clearing flags on attachment: 176141

Committed r135814: <http://trac.webkit.org/changeset/135814>
Comment 9 Noam Rosenthal 2012-11-26 21:25:33 PST
Comment on attachment 176145 [details]
Patch. Coordinated Graphics: traverse a mask layer and a replication layer too.

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

We shouldn't traverse mask and replica layers in this way. 
Backing store for masks depend on their owning layer, and replica layers don't have a backing store. We should make more conscious decisions about this rather than traverse everything.

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:57
> +    if (m_state.replicaLayer)
> +        m_state.replicaLayer->clearBackingStoresRecursive();

A replica layer never has a backing store.
Comment 10 Noam Rosenthal 2012-11-26 21:26:11 PST
Comment on attachment 176146 [details]
Patch4

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

> Source/WebKit2/ChangeLog:8
> +        The member is not used.

Remove the maskTarget member of CoordinatedGraphicsLayer, which is not used.
Comment 11 Dongseong Hwang 2012-11-26 21:50:24 PST
(In reply to comment #9)
> (From update of attachment 176145 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176145&action=review
> 
> We shouldn't traverse mask and replica layers in this way. 
> Backing store for masks depend on their owning layer, and replica layers don't have a backing store. We should make more conscious decisions about this rather than traverse everything.
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:57
> > +    if (m_state.replicaLayer)
> > +        m_state.replicaLayer->clearBackingStoresRecursive();
> 
> A replica layer never has a backing store.

Thanks for explanation. My lack of understanding made this patch.
The part of Coordinated Graphics is also useless, right?
Comment 12 Dongseong Hwang 2012-11-26 21:53:12 PST
Created attachment 176168 [details]
Patch
Comment 13 Dongseong Hwang 2012-11-26 21:55:25 PST
(In reply to comment #10)
> (From update of attachment 176146 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176146&action=review
> 
> > Source/WebKit2/ChangeLog:8
> > +        The member is not used.
> 
> Remove the maskTarget member of CoordinatedGraphicsLayer, which is not used.

Thanks for rephrasing. It is done in following patch.

(In reply to comment #12)
> Created an attachment (id=176168) [details]
> Patch
Comment 14 Dongseong Hwang 2012-11-26 22:11:34 PST
Created attachment 176172 [details]
Patch7 that changed changelog of Patch4
Comment 15 WebKit Review Bot 2012-11-26 22:16:32 PST
Comment on attachment 176150 [details]
Patch

Clearing flags on attachment: 176150

Committed r135818: <http://trac.webkit.org/changeset/135818>
Comment 16 WebKit Review Bot 2012-11-27 00:21:56 PST
Comment on attachment 176172 [details]
Patch7 that changed changelog of Patch4

Clearing flags on attachment: 176172

Committed r135830: <http://trac.webkit.org/changeset/135830>
Comment 17 WebKit Review Bot 2012-11-27 00:22:00 PST
All reviewed patches have been landed.  Closing bug.