Bug 105760

Summary: [TexMap] Rename current[Transform|Opacity|Filters] in TextureMapperLayer.
Product: WebKit Reporter: Dongseong Hwang <dongseong.hwang>
Component: Layout and RenderingAssignee: Dongseong Hwang <dongseong.hwang>
Status: RESOLVED FIXED    
Severity: Normal CC: kenneth, noam, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 105755, 105781    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Dongseong Hwang
Reported 2012-12-26 01:14:20 PST
We use effective members to keep changeable values that animations change. This patch puts 'effective' prefix before those members. In addition, add setPlatformTransform() to keep m_platformTransform after next calling setEffectiveTransform().
Attachments
Patch (17.05 KB, patch)
2012-12-26 01:26 PST, Dongseong Hwang 		no flags
DetailsFormatted DiffDiff
Patch (14.73 KB, patch)
2012-12-26 17:09 PST, Dongseong Hwang 		no flags
DetailsFormatted DiffDiff
Patch (16.34 KB, patch)
2012-12-26 17:26 PST, Dongseong Hwang 		no flags
DetailsFormatted DiffDiff
Patch (16.34 KB, patch)
2013-01-02 20:06 PST, Dongseong Hwang 		no flags
DetailsFormatted DiffDiff
Patch (15.71 KB, patch)
2013-01-07 22:34 PST, Dongseong Hwang 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Dongseong Hwang
Comment 1 2012-12-26 01:26:20 PST
Created attachment 180734 [details] Patch
Noam Rosenthal
Comment 2 2012-12-26 15:07:49 PST
Comment on attachment 180734 [details] Patch This seems unnecessary; platformTransform/opacity is only used for the root layer, which can't get a transform/opacity from anywhere else, which makes it redundant to multiply platformTransform with any other transform.
Dongseong Hwang
Comment 3 2012-12-26 17:09:10 PST
Created attachment 180767 [details] Patch
Dongseong Hwang
Comment 4 2012-12-26 17:12:22 PST
(In reply to comment #2) > (From update of attachment 180734 [details]) > This seems unnecessary; platformTransform/opacity is only used for the root layer, which can't get a transform/opacity from anywhere else, which makes it redundant to multiply platformTransform with any other transform. After reading your comment, I understand setTransform and setOpacity called from TextureMapperLayerClientQt require m_state.[transform|opacity] respectively. And I think effective prefix is still needed because I misunderstood the purpose of members a while. So I re-post renaming patch as well as some refactoring about setTransform and setOpacity.
Dongseong Hwang
Comment 5 2012-12-26 17:13:11 PST
(In reply to comment #4) > After reading your comment, I understand setTransform and setOpacity called from TextureMapperLayerClientQt require m_state.[transform|opacity] respectively. s/require/require to change/
Dongseong Hwang
Comment 6 2012-12-26 17:26:56 PST
Created attachment 180768 [details] Patch
Dongseong Hwang
Comment 7 2012-12-26 17:28:26 PST
(In reply to comment #6) > Created an attachment (id=180768) [details] > Patch I found we don't need to keep setOpacity and setTransform public api in TextureMapperLayer only for root layer. LayerTreeRenderer does not use both API.
Noam Rosenthal
Comment 8 2012-12-28 20:12:09 PST
Comment on attachment 180768 [details] Patch The name effectiveOpacity is confusing; effectiveOpacity is the opacity multiplied by all the ancestors, e.g. a layer with opacity .5 and a parent with opacity .6 will have an opacity of 0.3. Same goes for effectiveTransform. d
Dongseong Hwang
Comment 9 2012-12-28 22:38:51 PST
(In reply to comment #8) > (From update of attachment 180768 [details]) > The name effectiveOpacity is confusing; effectiveOpacity is the opacity multiplied by all the ancestors, e.g. a layer with opacity .5 and a parent with opacity .6 will have an opacity of 0.3. Same goes for effectiveTransform. It is why I want to change the name. Could you suggest better one?
Dongseong Hwang
Comment 10 2013-01-02 20:06:19 PST
Created attachment 181137 [details] Patch
Dongseong Hwang
Comment 11 2013-01-02 20:07:05 PST
(In reply to comment #10) > Created an attachment (id=181137) [details] > Patch rebase to upstream. Could you review this bug?
Noam Rosenthal
Comment 12 2013-01-07 20:09:53 PST
Comment on attachment 181137 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=181137&action=review Let's change from m_effectiveXXX to m_currentXXX. Effective is the multiplied value; current simply means that it's post-animation and not pre-animation. > Source/WebKit/qt/ChangeLog:10 > + It can reduce the number of public API of TextureMapperLayer. This removes unnecessary public API for TextureMapperLayer.
Dongseong Hwang
Comment 13 2013-01-07 22:34:58 PST
Created attachment 181643 [details] Patch
Dongseong Hwang
Comment 14 2013-01-07 22:37:43 PST
(In reply to comment #12) > (From update of attachment 181137 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=181137&action=review Thanks for review! > Let's change from m_effectiveXXX to m_currentXXX. > Effective is the multiplied value; current simply means that it's post-animation and not pre-animation. 'current' prefix is good although I revived effectiveXXX from old CoordinatedGraphicsLayer :) > This removes unnecessary public API for TextureMapperLayer. Done. In addition, I removed all setXXX methods because only setAnimatedXXX uses them.
Noam Rosenthal
Comment 15 2013-01-11 09:10:57 PST
> 'current' prefix is good although I revived effectiveXXX from old CoordinatedGraphicsLayer :) Yes, it was wrong there :)
WebKit Review Bot
Comment 16 2013-01-11 17:36:43 PST
Comment on attachment 181643 [details] Patch Clearing flags on attachment: 181643 Committed r139526: <http://trac.webkit.org/changeset/139526>
WebKit Review Bot
Comment 17 2013-01-11 17:36:48 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.