CSS Independent Transform Properties

CSS Independent Transform Properties

Implementation of CSS Transform Properties

1. Translate
2. Rotate
3. Scale

Link to Intent to implement:

https://groups.google.com/a/chromium.org/d/msg/blink-dev/-GfE73tlLX8/hv7Qitys6j8J

Link to w3c specifications:

http://dev.w3.org/csswg/css-transforms-2/

BUG=496550
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197547

[soonm, 2015-06-05 00:13:42]

soonm@google.com changed reviewers:
+ ericwilligers@chromium.org, shans@chromium.org

[Timothy Loh, 2015-06-05 00:55:58]

timloh@chromium.org changed reviewers:
+ timloh@chromium.org

[Timothy Loh, 2015-06-05 00:56:00]

I just skimmed some files at random. Lot's of stuff applies to all properties
but I only commented on one or two, I'm sure you can extrapolate.

https://codereview.chromium.org/1158603003/diff/1/Source/core/animation/css/C...
File Source/core/animation/css/CSSPropertyEquality.cpp (right):

https://codereview.chromium.org/1158603003/diff/1/Source/core/animation/css/C...
Source/core/animation/css/CSSPropertyEquality.cpp:293: return a.translate() ==
b.translate();
Probably not what you want, this does a pointer comparison

https://codereview.chromium.org/1158603003/diff/1/Source/core/css/resolver/An...
File Source/core/css/resolver/AnimatedStyleBuilder.cpp (right):

https://codereview.chromium.org/1158603003/diff/1/Source/core/css/resolver/An...
Source/core/css/resolver/AnimatedStyleBuilder.cpp:603: return;
unreachable?

https://codereview.chromium.org/1158603003/diff/1/Source/core/css/resolver/An...
Source/core/css/resolver/AnimatedStyleBuilder.cpp:605:
style->setTranslate(transform->x(), transform->y(), transform->z());
setTranslate(transform)?

https://codereview.chromium.org/1158603003/diff/1/Source/core/css/resolver/An...
Source/core/css/resolver/AnimatedStyleBuilder.cpp:613:
style->setRotate(transform->angle(), transform->x(), transform->y(),
transform->z());
setRotate(transform)?

https://codereview.chromium.org/1158603003/diff/1/Source/core/css/resolver/St...
File Source/core/css/resolver/StyleBuilderCustom.cpp (right):

https://codereview.chromium.org/1158603003/diff/1/Source/core/css/resolver/St...
Source/core/css/resolver/StyleBuilderCustom.cpp:843: if
(state.parentStyle()->hasTranslateProperty()) {
doesn't this work without the check?

https://codereview.chromium.org/1158603003/diff/1/Source/core/css/resolver/St...
Source/core/css/resolver/StyleBuilderCustom.cpp:853: Length ty = Length(0,
Fixed);
Length ty(0, Fixed)

https://codereview.chromium.org/1158603003/diff/1/Source/core/style/ComputedS...
File Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/1158603003/diff/1/Source/core/style/ComputedS...
Source/core/style/ComputedStyle.h:848: const
PassRefPtr<TranslateTransformOperation> translate() const { return
rareNonInheritedData->m_transform->m_translate; }
Maybe just return TranslateTransformOperation*

https://codereview.chromium.org/1158603003/diff/1/Source/core/style/StyleTran...
File Source/core/style/StyleTransformData.cpp (right):

https://codereview.chromium.org/1158603003/diff/1/Source/core/style/StyleTran...
Source/core/style/StyleTransformData.cpp:59: b |= m_translate ?
m_translate->is3DOperation() : false;
m_translate && m_translate->is3DOperation()

https://codereview.chromium.org/1158603003/diff/1/Source/platform/transforms/...
File Source/platform/transforms/RotateTransformOperation.h (right):

https://codereview.chromium.org/1158603003/diff/1/Source/platform/transforms/...
Source/platform/transforms/RotateTransformOperation.h:44: static
PassRefPtr<RotateTransformOperation> create(PassRefPtr<RotateTransformOperation>
o)
unused?

https://codereview.chromium.org/1158603003/diff/1/Source/platform/transforms/...
Source/platform/transforms/RotateTransformOperation.h:86:
RotateTransformOperation(const RotateTransformOperation& o)
unused?

[Eric Willigers, 2015-06-05 01:58:23]

https://codereview.chromium.org/1158603003/diff/1/LayoutTests/transforms/2d/t...
File LayoutTests/transforms/2d/transform-2d-properties.html (right):

https://codereview.chromium.org/1158603003/diff/1/LayoutTests/transforms/2d/t...
LayoutTests/transforms/2d/transform-2d-properties.html:34: //{ 'transform' :
'inherit', 'result' : 'none' }, // parent element doesn't have transform
Delete this commented out code. You might want a separate test file where (a) a
child element has properties explicitly inherited, and (b) a sibling child
element has properties never set (and so receives the initial values).

https://codereview.chromium.org/1158603003/diff/1/LayoutTests/transforms/2d/t...
LayoutTests/transforms/2d/transform-2d-properties.html:37: { 'translate' : '',  
         'result' : 'none' },
0px

https://codereview.chromium.org/1158603003/diff/1/LayoutTests/transforms/2d/t...
LayoutTests/transforms/2d/transform-2d-properties.html:39: { 'translate' : '100%
3em',   'result' : '200px, 90px' },  // depends on font size
Your test should set the font-size property.

https://codereview.chromium.org/1158603003/diff/1/LayoutTests/transforms/2d/t...
LayoutTests/transforms/2d/transform-2d-properties.html:43: { 'scale' : '',      
     'result' : 'none' },
1

https://codereview.chromium.org/1158603003/diff/1/LayoutTests/transforms/2d/t...
LayoutTests/transforms/2d/transform-2d-properties.html:49: { 'rotate' : '',     
         'result' : 'none' },
0deg

https://codereview.chromium.org/1158603003/diff/1/Source/core/animation/css/C...
File Source/core/animation/css/CSSAnimatableValueFactory.cpp (right):

https://codereview.chromium.org/1158603003/diff/1/Source/core/animation/css/C...
Source/core/animation/css/CSSAnimatableValueFactory.cpp:528: TransformOperations
op;
Repetition could be avoided with a new method (in anonymous namespace) that
accepts a single transform operation.

https://codereview.chromium.org/1158603003/diff/1/Source/core/css/ComputedSty...
File Source/core/css/ComputedStyleCSSValueMapping.cpp (right):

https://codereview.chromium.org/1158603003/diff/1/Source/core/css/ComputedSty...
Source/core/css/ComputedStyleCSSValueMapping.cpp:2597: return
cssValuePool().createIdentifierValue(CSSValueNone);
0px

https://codereview.chromium.org/1158603003/diff/1/Source/core/css/ComputedSty...
Source/core/css/ComputedStyleCSSValueMapping.cpp:2625: return
cssValuePool().createIdentifierValue(CSSValueNone);
0deg

https://codereview.chromium.org/1158603003/diff/1/Source/core/css/ComputedSty...
Source/core/css/ComputedStyleCSSValueMapping.cpp:2637: return
cssValuePool().createIdentifierValue(CSSValueNone);
1

https://codereview.chromium.org/1158603003/diff/1/Source/core/css/parser/CSSP...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1158603003/diff/1/Source/core/css/parser/CSSP...
Source/core/css/parser/CSSPropertyParser.cpp:1175: for (unsigned i = 0; i <
maxAxis - m_valueList->size(); i++)
Perhaps while (scalaList size < ...)

https://codereview.chromium.org/1158603003/diff/1/Source/core/css/resolver/An...
File Source/core/css/resolver/AnimatedStyleBuilder.cpp (right):

https://codereview.chromium.org/1158603003/diff/1/Source/core/css/resolver/An...
Source/core/css/resolver/AnimatedStyleBuilder.cpp:603: return;
Should we clear the translate? Or should we be asserting !op.size() is 1?

https://codereview.chromium.org/1158603003/diff/1/Source/core/css/resolver/St...
File Source/core/css/resolver/StyleBuilderCustom.cpp (right):

https://codereview.chromium.org/1158603003/diff/1/Source/core/css/resolver/St...
Source/core/css/resolver/StyleBuilderCustom.cpp:845: }
otherwise should we clear the translate property?

Same for rotate, scale

https://codereview.chromium.org/1158603003/diff/1/Source/core/layout/composit...
File Source/core/layout/compositing/CompositingReasonFinder.cpp (right):

https://codereview.chromium.org/1158603003/diff/1/Source/core/layout/composit...
Source/core/layout/compositing/CompositingReasonFinder.cpp:146: return
layoutObject->hasTransformRelatedProperty() &&
(layoutObject->style()->transform().has3DOperation() ||
layoutObject->style()->has3DTransformProperties());
Why not ensure hasTransformRelatedProperty returns true if
rotate/translate/scale are set?

Does hasTransformRelatedProperty already consider motion path?

https://codereview.chromium.org/1158603003/diff/1/Source/core/style/StyleTran...
File Source/core/style/StyleTransformData.cpp (right):

https://codereview.chromium.org/1158603003/diff/1/Source/core/style/StyleTran...
Source/core/style/StyleTransformData.cpp:59: b |= m_translate ?
m_translate->is3DOperation() : false;
Perhaps
if (m_translate && m_translate->is3DOperation())
  return true;
..
return false;

OR

return (m_translate && m_translate->is3DOperation())
  || ...

[soonm, 2015-06-10 04:09:33]

https://codereview.chromium.org/1158603003/diff/1/Source/core/animation/css/C...
File Source/core/animation/css/CSSAnimatableValueFactory.cpp (right):

https://codereview.chromium.org/1158603003/diff/1/Source/core/animation/css/C...
Source/core/animation/css/CSSAnimatableValueFactory.cpp:528: TransformOperations
op;
On 2015/06/05 01:58:22, Eric Willigers wrote:
> Repetition could be avoided with a new method (in anonymous namespace) that
> accepts a single transform operation.

Done.

https://codereview.chromium.org/1158603003/diff/1/Source/core/animation/css/C...
File Source/core/animation/css/CSSPropertyEquality.cpp (right):

https://codereview.chromium.org/1158603003/diff/1/Source/core/animation/css/C...
Source/core/animation/css/CSSPropertyEquality.cpp:293: return a.translate() ==
b.translate();
On 2015/06/05 00:55:59, Timothy Loh wrote:
> Probably not what you want, this does a pointer comparison

Done.

https://codereview.chromium.org/1158603003/diff/1/Source/core/css/parser/CSSP...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1158603003/diff/1/Source/core/css/parser/CSSP...
Source/core/css/parser/CSSPropertyParser.cpp:1175: for (unsigned i = 0; i <
maxAxis - m_valueList->size(); i++)
On 2015/06/05 01:58:22, Eric Willigers wrote:
> Perhaps while (scalaList size < ...)

Changed - minimal css properties, so there's no need to fill 1.

https://codereview.chromium.org/1158603003/diff/1/Source/core/css/resolver/An...
File Source/core/css/resolver/AnimatedStyleBuilder.cpp (right):

https://codereview.chromium.org/1158603003/diff/1/Source/core/css/resolver/An...
Source/core/css/resolver/AnimatedStyleBuilder.cpp:603: return;
On 2015/06/05 01:58:22, Eric Willigers wrote:
> Should we clear the translate? Or should we be asserting !op.size() is 1?

Done.

https://codereview.chromium.org/1158603003/diff/1/Source/core/css/resolver/An...
Source/core/css/resolver/AnimatedStyleBuilder.cpp:605:
style->setTranslate(transform->x(), transform->y(), transform->z());
On 2015/06/05 00:55:59, Timothy Loh wrote:
> setTranslate(transform)?

Done.

https://codereview.chromium.org/1158603003/diff/1/Source/core/css/resolver/An...
Source/core/css/resolver/AnimatedStyleBuilder.cpp:613:
style->setRotate(transform->angle(), transform->x(), transform->y(),
transform->z());
On 2015/06/05 00:55:59, Timothy Loh wrote:
> setRotate(transform)?

Done.

https://codereview.chromium.org/1158603003/diff/1/Source/core/css/resolver/St...
File Source/core/css/resolver/StyleBuilderCustom.cpp (right):

https://codereview.chromium.org/1158603003/diff/1/Source/core/css/resolver/St...
Source/core/css/resolver/StyleBuilderCustom.cpp:843: if
(state.parentStyle()->hasTranslateProperty()) {
On 2015/06/05 00:55:59, Timothy Loh wrote:
> doesn't this work without the check?

Done.

https://codereview.chromium.org/1158603003/diff/1/Source/core/css/resolver/St...
Source/core/css/resolver/StyleBuilderCustom.cpp:845: }
On 2015/06/05 01:58:23, Eric Willigers wrote:
> otherwise should we clear the translate property?
> 
> Same for rotate, scale

The translate property is initialize to nullptr. If the parentStyle doesn't have
a translate property, then it should be correct.

https://codereview.chromium.org/1158603003/diff/1/Source/core/css/resolver/St...
Source/core/css/resolver/StyleBuilderCustom.cpp:853: Length ty = Length(0,
Fixed);
On 2015/06/05 00:55:59, Timothy Loh wrote:
> Length ty(0, Fixed)

Done. Transferred to StyleBuilderConverter

https://codereview.chromium.org/1158603003/diff/1/Source/core/layout/composit...
File Source/core/layout/compositing/CompositingReasonFinder.cpp (right):

https://codereview.chromium.org/1158603003/diff/1/Source/core/layout/composit...
Source/core/layout/compositing/CompositingReasonFinder.cpp:146: return
layoutObject->hasTransformRelatedProperty() &&
(layoutObject->style()->transform().has3DOperation() ||
layoutObject->style()->has3DTransformProperties());
On 2015/06/05 01:58:23, Eric Willigers wrote:
> Why not ensure hasTransformRelatedProperty returns true if
> rotate/translate/scale are set?
> 
> Does hasTransformRelatedProperty already consider motion path?

hasTransformProperty will return True because I modified hasTransform but
has3DOperations also needs to be true here.

https://codereview.chromium.org/1158603003/diff/1/Source/core/style/ComputedS...
File Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/1158603003/diff/1/Source/core/style/ComputedS...
Source/core/style/ComputedStyle.h:848: const
PassRefPtr<TranslateTransformOperation> translate() const { return
rareNonInheritedData->m_transform->m_translate; }
On 2015/06/05 00:55:59, Timothy Loh wrote:
> Maybe just return TranslateTransformOperation*

Done.

https://codereview.chromium.org/1158603003/diff/1/Source/core/style/StyleTran...
File Source/core/style/StyleTransformData.cpp (right):

https://codereview.chromium.org/1158603003/diff/1/Source/core/style/StyleTran...
Source/core/style/StyleTransformData.cpp:59: b |= m_translate ?
m_translate->is3DOperation() : false;
On 2015/06/05 00:56:00, Timothy Loh wrote:
> m_translate && m_translate->is3DOperation()

Done.

https://codereview.chromium.org/1158603003/diff/1/Source/platform/transforms/...
File Source/platform/transforms/RotateTransformOperation.h (right):

https://codereview.chromium.org/1158603003/diff/1/Source/platform/transforms/...
Source/platform/transforms/RotateTransformOperation.h:44: static
PassRefPtr<RotateTransformOperation> create(PassRefPtr<RotateTransformOperation>
o)
On 2015/06/05 00:56:00, Timothy Loh wrote:
> unused?

Removed

https://codereview.chromium.org/1158603003/diff/1/Source/platform/transforms/...
Source/platform/transforms/RotateTransformOperation.h:86:
RotateTransformOperation(const RotateTransformOperation& o)
On 2015/06/05 00:56:00, Timothy Loh wrote:
> unused?

Removed

[Timothy Loh, 2015-06-10 04:13:27]

The CQ bit was checked by timloh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-10 04:13:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1158603003/20001

[commit-bot: I haz the power, 2015-06-10 04:17:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-10 04:17:25]

Dry run: Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)

[Timothy Loh, 2015-06-10 04:18:42]

The CQ bit was checked by timloh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-10 04:18:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1158603003/40001

[Timothy Loh, 2015-06-10 04:22:29]

https://codereview.chromium.org/1158603003/diff/40001/Source/core/css/parser/...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1158603003/diff/40001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:1112: if (!validUnit(value,
FLength) && !validUnit(value, FPercent))
validUnit(value, FLength | FPercent), also add tests with calc()

https://codereview.chromium.org/1158603003/diff/40001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:1153: if (value)
don't need, checked outside the switch statement

https://codereview.chromium.org/1158603003/diff/40001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:1170: if (value)
don't need, checked outside the switch statement

[commit-bot: I haz the power, 2015-06-10 06:04:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-10 06:04:57]

Dry run: Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6...)

[Eric Willigers, 2015-06-11 02:15:20]

https://codereview.chromium.org/1158603003/diff/40001/LayoutTests/animations/...
File
LayoutTests/animations/svg-attribute-interpolation/resources/interpolation-test.js
(right):

https://codereview.chromium.org/1158603003/diff/40001/LayoutTests/animations/...
LayoutTests/animations/svg-attribute-interpolation/resources/interpolation-test.js:472:
// The following collide with CSS properties or the Web Animations API (offset).
I recently made a change that retired svgPrefixedAttributes. The definition of
svgPrefixedAttributes can be deleted.

https://codereview.chromium.org/1158603003/diff/40001/LayoutTests/fast/css/ge...
File LayoutTests/fast/css/getComputedStyle/computed-style-listing-expected.txt
(right):

https://codereview.chromium.org/1158603003/diff/40001/LayoutTests/fast/css/ge...
LayoutTests/fast/css/getComputedStyle/computed-style-listing-expected.txt:235:
scale: 0
Why isn't this scale: 1 ?

Same for others e.g. LayoutTests/svg/css/getComputedStyle-listing-expected.txt

https://codereview.chromium.org/1158603003/diff/40001/LayoutTests/transforms/...
File LayoutTests/transforms/rotate.html (right):

https://codereview.chromium.org/1158603003/diff/40001/LayoutTests/transforms/...
LayoutTests/transforms/rotate.html:25: rotate: 30deg 1 -2.5 0;
Perhaps replace final 0, e.g. with 4.

https://codereview.chromium.org/1158603003/diff/40001/LayoutTests/transforms/...
File LayoutTests/transforms/transform-properties-mixed.html (right):

https://codereview.chromium.org/1158603003/diff/40001/LayoutTests/transforms/...
LayoutTests/transforms/transform-properties-mixed.html:53: transform:
translateZ(50px);
Where are the new transform properties being used?

https://codereview.chromium.org/1158603003/diff/40001/Source/core/animation/K...
File Source/core/animation/KeyframeEffect.cpp (left):

https://codereview.chromium.org/1158603003/diff/40001/Source/core/animation/K...
Source/core/animation/KeyframeEffect.cpp:247: 
applyEffects() also needs changing, in case 'transform' already has an animation
running on the compositor, and then a new property is set.

https://codereview.chromium.org/1158603003/diff/40001/Source/core/style/Style...
File Source/core/style/StyleTransformData.h (right):

https://codereview.chromium.org/1158603003/diff/40001/Source/core/style/Style...
Source/core/style/StyleTransformData.h:32: #include
"platform/transforms/TransformOperation.h"
I don't think we need to add this header.

https://codereview.chromium.org/1158603003/diff/40001/Source/platform/transfo...
File Source/platform/transforms/TranslateTransformOperation.h (left):

https://codereview.chromium.org/1158603003/diff/40001/Source/platform/transfo...
Source/platform/transforms/TranslateTransformOperation.h:58: virtual bool
operator==(const TransformOperation& o) const override
TranslateTransformOperation can be declared a final class. Same for other
operations.

[soonm, 2015-06-11 23:31:04]

https://codereview.chromium.org/1158603003/diff/40001/LayoutTests/animations/...
File
LayoutTests/animations/svg-attribute-interpolation/resources/interpolation-test.js
(right):

https://codereview.chromium.org/1158603003/diff/40001/LayoutTests/animations/...
LayoutTests/animations/svg-attribute-interpolation/resources/interpolation-test.js:472:
// The following collide with CSS properties or the Web Animations API (offset).
On 2015/06/11 02:15:19, Eric Willigers wrote:
> I recently made a change that retired svgPrefixedAttributes. The definition of
> svgPrefixedAttributes can be deleted.

Done - Reverted my change to match master.

https://codereview.chromium.org/1158603003/diff/40001/LayoutTests/fast/css/ge...
File LayoutTests/fast/css/getComputedStyle/computed-style-listing-expected.txt
(right):

https://codereview.chromium.org/1158603003/diff/40001/LayoutTests/fast/css/ge...
LayoutTests/fast/css/getComputedStyle/computed-style-listing-expected.txt:235:
scale: 0
On 2015/06/11 02:15:19, Eric Willigers wrote:
> Why isn't this scale: 1 ?
> 
> Same for others e.g. LayoutTests/svg/css/getComputedStyle-listing-expected.txt

Yes this should be 1 instead of 0 here. Done.

https://codereview.chromium.org/1158603003/diff/40001/LayoutTests/transforms/...
File LayoutTests/transforms/rotate.html (right):

https://codereview.chromium.org/1158603003/diff/40001/LayoutTests/transforms/...
LayoutTests/transforms/rotate.html:25: rotate: 30deg 1 -2.5 0;
On 2015/06/11 02:15:19, Eric Willigers wrote:
> Perhaps replace final 0, e.g. with 4.

Done.

https://codereview.chromium.org/1158603003/diff/40001/Source/core/animation/K...
File Source/core/animation/KeyframeEffect.cpp (left):

https://codereview.chromium.org/1158603003/diff/40001/Source/core/animation/K...
Source/core/animation/KeyframeEffect.cpp:247: 
On 2015/06/11 02:15:19, Eric Willigers wrote:
> applyEffects() also needs changing, in case 'transform' already has an
animation
> running on the compositor, and then a new property is set.

Done.

https://codereview.chromium.org/1158603003/diff/40001/Source/core/css/parser/...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1158603003/diff/40001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:1112: if (!validUnit(value,
FLength) && !validUnit(value, FPercent))
On 2015/06/10 04:22:29, Timothy Loh wrote:
> validUnit(value, FLength | FPercent), also add tests with calc()

Done.

https://codereview.chromium.org/1158603003/diff/40001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:1153: if (value)
On 2015/06/10 04:22:29, Timothy Loh wrote:
> don't need, checked outside the switch statement

Done - Removed

https://codereview.chromium.org/1158603003/diff/40001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:1170: if (value)
On 2015/06/10 04:22:29, Timothy Loh wrote:
> don't need, checked outside the switch statement

Done - Removed

https://codereview.chromium.org/1158603003/diff/40001/Source/core/style/Style...
File Source/core/style/StyleTransformData.h (right):

https://codereview.chromium.org/1158603003/diff/40001/Source/core/style/Style...
Source/core/style/StyleTransformData.h:32: #include
"platform/transforms/TransformOperation.h"
On 2015/06/11 02:15:19, Eric Willigers wrote:
> I don't think we need to add this header.

Done - Removed.

https://codereview.chromium.org/1158603003/diff/40001/Source/platform/transfo...
File Source/platform/transforms/TranslateTransformOperation.h (left):

https://codereview.chromium.org/1158603003/diff/40001/Source/platform/transfo...
Source/platform/transforms/TranslateTransformOperation.h:58: virtual bool
operator==(const TransformOperation& o) const override
On 2015/06/11 02:15:19, Eric Willigers wrote:
> TranslateTransformOperation can be declared a final class. Same for other
> operations.

Done.

[dstockwell, 2015-06-12 00:17:56]

dstockwell@chromium.org changed reviewers:
+ dstockwell@chromium.org

[dstockwell, 2015-06-12 00:17:59]

https://codereview.chromium.org/1158603003/diff/80001/Source/core/style/Compu...
File Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/1158603003/diff/80001/Source/core/style/Compu...
Source/core/style/ComputedStyle.cpp:937: if (applyTransformProperty ==
IncludeTransformProperty) {
It's confusing that IncludeTransformProperty doesn't refer to the 'transform'
property.

https://codereview.chromium.org/1158603003/diff/80001/Source/core/style/Compu...
File Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/1158603003/diff/80001/Source/core/style/Compu...
Source/core/style/ComputedStyle.h:857: bool hasTransformProperties() const {
return hasTranslateProperty() || hasRotateProperty() || hasScaleProperty(); }
Confusing that it doesn't include the transform property. Do we really need this
function?

https://codereview.chromium.org/1158603003/diff/80001/Source/core/style/Style...
File Source/core/style/StyleTransformData.cpp (right):

https://codereview.chromium.org/1158603003/diff/80001/Source/core/style/Style...
Source/core/style/StyleTransformData.cpp:59: return (m_translate &&
m_translate->z() != 0)
Why doesn't this care about m_operations?

https://codereview.chromium.org/1158603003/diff/80001/Source/core/style/Style...
File Source/core/style/StyleTransformData.h (right):

https://codereview.chromium.org/1158603003/diff/80001/Source/core/style/Style...
Source/core/style/StyleTransformData.h:49: void
setTranslate(PassRefPtr<TranslateTransformOperation>);
Do we need setters when the members are public?

https://codereview.chromium.org/1158603003/diff/80001/Source/platform/Runtime...
File Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/1158603003/diff/80001/Source/platform/Runtime...
Source/platform/RuntimeEnabledFeatures.in:45: CSSIndependentTransformProperties
status=experimental
This is not the same flag mentioned in the intent to implement.

https://codereview.chromium.org/1158603003/diff/80001/Source/platform/transfo...
File Source/platform/transforms/TransformOperation.h (right):

https://codereview.chromium.org/1158603003/diff/80001/Source/platform/transfo...
Source/platform/transforms/TransformOperation.h:85: bool
isRotateTransformOperation() const
just isRotate, etc.

[soonm, 2015-06-12 01:54:13]

https://codereview.chromium.org/1158603003/diff/80001/Source/core/style/Compu...
File Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/1158603003/diff/80001/Source/core/style/Compu...
Source/core/style/ComputedStyle.cpp:937: if (applyTransformProperty ==
IncludeTransformProperty) {
On 2015/06/12 00:17:58, dstockwell wrote:
> It's confusing that IncludeTransformProperty doesn't refer to the 'transform'
> property.

Change to IncludeTransformL2

https://codereview.chromium.org/1158603003/diff/80001/Source/core/style/Compu...
File Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/1158603003/diff/80001/Source/core/style/Compu...
Source/core/style/ComputedStyle.h:857: bool hasTransformProperties() const {
return hasTranslateProperty() || hasRotateProperty() || hasScaleProperty(); }
On 2015/06/12 00:17:58, dstockwell wrote:
> Confusing that it doesn't include the transform property. Do we really need
this
> function?

Merge this function into hasTransform()

https://codereview.chromium.org/1158603003/diff/80001/Source/core/style/Style...
File Source/core/style/StyleTransformData.cpp (right):

https://codereview.chromium.org/1158603003/diff/80001/Source/core/style/Style...
Source/core/style/StyleTransformData.cpp:59: return (m_translate &&
m_translate->z() != 0)
On 2015/06/12 00:17:58, dstockwell wrote:
> Why doesn't this care about m_operation?

I wanted to separate the 3D checks for 'transform' and the individual ones
initially, but I think I can merge them.

CompositingReasonFinder.cpp:146 now uses this function instead of checking both
separatedly

https://codereview.chromium.org/1158603003/diff/80001/Source/platform/Runtime...
File Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/1158603003/diff/80001/Source/platform/Runtime...
Source/platform/RuntimeEnabledFeatures.in:45: CSSIndependentTransformProperties
status=experimental
On 2015/06/12 00:17:58, dstockwell wrote:
> This is not the same flag mentioned in the intent to implement.

I made the change during the design doc because there was a comment that
CSSTransformL2 wasn't really informative.

Should I revert and use the one in the Intent To Implement?

https://codereview.chromium.org/1158603003/diff/80001/Source/platform/transfo...
File Source/platform/transforms/TransformOperation.h (right):

https://codereview.chromium.org/1158603003/diff/80001/Source/platform/transfo...
Source/platform/transforms/TransformOperation.h:85: bool
isRotateTransformOperation() const
On 2015/06/12 00:17:58, dstockwell wrote:
> just isRotate, etc.

Done - for all 3 predicates

[Timothy Loh, 2015-06-12 04:48:35]

https://codereview.chromium.org/1158603003/diff/80001/Source/platform/transfo...
File Source/platform/transforms/ScaleTransformOperation.h (right):

https://codereview.chromium.org/1158603003/diff/80001/Source/platform/transfo...
Source/platform/transforms/ScaleTransformOperation.h:49: virtual OperationType
type() const override { return m_type; }
Can this (and the one for Translate) stay private?

https://codereview.chromium.org/1158603003/diff/80001/Source/platform/transfo...
File Source/platform/transforms/TransformOperation.h (right):

https://codereview.chromium.org/1158603003/diff/80001/Source/platform/transfo...
Source/platform/transforms/TransformOperation.h:85: bool
isRotateTransformOperation() const
On 2015/06/12 01:54:13, soonm wrote:
> On 2015/06/12 00:17:58, dstockwell wrote:
> > just isRotate, etc.
> 
> Done - for all 3 predicates

Are these even used in this patch though?

[Timothy Loh, 2015-06-12 05:38:33]

timloh@chromium.org changed reviewers:
+ alancutter@chromium.org

[Timothy Loh, 2015-06-12 05:38:34]

https://codereview.chromium.org/1158603003/diff/80001/Source/core/css/Compute...
File Source/core/css/ComputedStyleCSSValueMapping.cpp (right):

https://codereview.chromium.org/1158603003/diff/80001/Source/core/css/Compute...
Source/core/css/ComputedStyleCSSValueMapping.cpp:2643: return
cssValuePool().createValue(0, CSSPrimitiveValue::CSS_PX);
+alancutter

Do these get fed back into the StyleBuilder for animations?

[soonm, 2015-06-12 06:18:49]

https://codereview.chromium.org/1158603003/diff/80001/Source/core/style/Style...
File Source/core/style/StyleTransformData.h (right):

https://codereview.chromium.org/1158603003/diff/80001/Source/core/style/Style...
Source/core/style/StyleTransformData.h:49: void
setTranslate(PassRefPtr<TranslateTransformOperation>);
On 2015/06/12 00:17:58, dstockwell wrote:
> Do we need setters when the members are public?

Removed setters.

https://codereview.chromium.org/1158603003/diff/80001/Source/platform/transfo...
File Source/platform/transforms/ScaleTransformOperation.h (right):

https://codereview.chromium.org/1158603003/diff/80001/Source/platform/transfo...
Source/platform/transforms/ScaleTransformOperation.h:49: virtual OperationType
type() const override { return m_type; }
On 2015/06/12 04:48:35, Timothy Loh wrote:
> Can this (and the one for Translate) stay private?

Yep they can be private. Reverted.

https://codereview.chromium.org/1158603003/diff/80001/Source/platform/transfo...
File Source/platform/transforms/TransformOperation.h (right):

https://codereview.chromium.org/1158603003/diff/80001/Source/platform/transfo...
Source/platform/transforms/TransformOperation.h:85: bool
isRotateTransformOperation() const
On 2015/06/12 04:48:35, Timothy Loh wrote:
> On 2015/06/12 01:54:13, soonm wrote:
> > On 2015/06/12 00:17:58, dstockwell wrote:
> > > just isRotate, etc.
> > 
> > Done - for all 3 predicates
> 
> Are these even used in this patch though?

Should not be in this patch. Reverted.

[alancutter (OOO until 2018), 2015-06-12 07:56:32]

https://codereview.chromium.org/1158603003/diff/1/Source/core/style/StyleTran...
File Source/core/style/StyleTransformData.cpp (left):

https://codereview.chromium.org/1158603003/diff/1/Source/core/style/StyleTran...
Source/core/style/StyleTransformData.cpp:24: 
These two headers should always be separated from the others by a blank line.

https://codereview.chromium.org/1158603003/diff/80001/Source/core/css/Compute...
File Source/core/css/ComputedStyleCSSValueMapping.cpp (right):

https://codereview.chromium.org/1158603003/diff/80001/Source/core/css/Compute...
Source/core/css/ComputedStyleCSSValueMapping.cpp:2643: return
cssValuePool().createValue(0, CSSPrimitiveValue::CSS_PX);
On 2015/06/12 at 05:38:34, Timothy Loh wrote:
> +alancutter
> 
> Do these get fed back into the StyleBuilder for animations?

No, animations won't be using the getComputedStyle hack if I can help it.

[soonm, 2015-06-15 01:50:52]

The CQ bit was checked by soonm@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-06-15 01:51:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1158603003/100001

[commit-bot: I haz the power, 2015-06-15 01:51:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-15 01:51:07]

Dry run: No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[soonm, 2015-06-15 05:07:17]

https://codereview.chromium.org/1158603003/diff/1/Source/core/style/StyleTran...
File Source/core/style/StyleTransformData.cpp (left):

https://codereview.chromium.org/1158603003/diff/1/Source/core/style/StyleTran...
Source/core/style/StyleTransformData.cpp:24: 
On 2015/06/12 07:56:31, alancutter wrote:
> These two headers should always be separated from the others by a blank line.

Done.

[Timothy Loh, 2015-06-15 06:17:34]

random comments

https://codereview.chromium.org/1158603003/diff/120001/Source/core/css/resolv...
File Source/core/css/resolver/StyleBuilderConverter.cpp (right):

https://codereview.chromium.org/1158603003/diff/120001/Source/core/css/resolv...
Source/core/css/resolver/StyleBuilderConverter.cpp:977: if (list->length() >= 2
&& list->length() <= 3)
why check against 3?

https://codereview.chromium.org/1158603003/diff/120001/Source/core/style/Comp...
File Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/1158603003/diff/120001/Source/core/style/Comp...
Source/core/style/ComputedStyle.cpp:936: RefPtr<TransformOperation> translateOp
= translate();
This adds ref churn, just translate()->apply(..) and so on.

https://codereview.chromium.org/1158603003/diff/120001/Source/core/style/Styl...
File Source/core/style/StyleTransformData.cpp (right):

https://codereview.chromium.org/1158603003/diff/120001/Source/core/style/Styl...
Source/core/style/StyleTransformData.cpp:51: && m_operations == o.m_operations
indentation looks wrong

https://codereview.chromium.org/1158603003/diff/120001/Source/core/style/Styl...
Source/core/style/StyleTransformData.cpp:62: || (m_rotate && (m_rotate->x() != 0
|| m_rotate->y() != 0 || m_rotate->z() != 1))
probably don't need to check z here?

[Eric Willigers, 2015-06-15 06:45:38]

lgtm after Tim's nits are addressed.

[soonm, 2015-06-15 06:59:19]

https://codereview.chromium.org/1158603003/diff/120001/Source/core/css/resolv...
File Source/core/css/resolver/StyleBuilderConverter.cpp (right):

https://codereview.chromium.org/1158603003/diff/120001/Source/core/css/resolv...
Source/core/css/resolver/StyleBuilderConverter.cpp:977: if (list->length() >= 2
&& list->length() <= 3)
On 2015/06/15 06:17:34, Timothy Loh wrote:
> why check against 3?

Do not need to - Removed

https://codereview.chromium.org/1158603003/diff/120001/Source/core/style/Comp...
File Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/1158603003/diff/120001/Source/core/style/Comp...
Source/core/style/ComputedStyle.cpp:936: RefPtr<TransformOperation> translateOp
= translate();
On 2015/06/15 06:17:34, Timothy Loh wrote:
> This adds ref churn, just translate()->apply(..) and so on.

Done. Made the apply function on the derived TransformOperation public.

https://codereview.chromium.org/1158603003/diff/120001/Source/core/style/Styl...
File Source/core/style/StyleTransformData.cpp (right):

https://codereview.chromium.org/1158603003/diff/120001/Source/core/style/Styl...
Source/core/style/StyleTransformData.cpp:51: && m_operations == o.m_operations
On 2015/06/15 06:17:34, Timothy Loh wrote:
> indentation looks wrong

Done - Fixed

https://codereview.chromium.org/1158603003/diff/120001/Source/core/style/Styl...
Source/core/style/StyleTransformData.cpp:62: || (m_rotate && (m_rotate->x() != 0
|| m_rotate->y() != 0 || m_rotate->z() != 1))
On 2015/06/15 06:17:34, Timothy Loh wrote:
> probably don't need to check z here?

Done - Removed

[alancutter (OOO until 2018), 2015-06-17 07:38:33]

https://codereview.chromium.org/1158603003/diff/140001/LayoutTests/transforms...
File LayoutTests/transforms/combine-transforms-properties-motion-path.html
(right):

https://codereview.chromium.org/1158603003/diff/140001/LayoutTests/transforms...
LayoutTests/transforms/combine-transforms-properties-motion-path.html:10: <!--
Note the order of the scaling operation affects the path coordinates -->
/* */

https://codereview.chromium.org/1158603003/diff/140001/LayoutTests/transforms...
File LayoutTests/transforms/rotate-parsing.html (right):

https://codereview.chromium.org/1158603003/diff/140001/LayoutTests/transforms...
LayoutTests/transforms/rotate-parsing.html:23: .isComputedTo('450deg 0 1 1');
One line plz.

https://codereview.chromium.org/1158603003/diff/140001/LayoutTests/transforms...
File LayoutTests/transforms/scale.html (right):

https://codereview.chromium.org/1158603003/diff/140001/LayoutTests/transforms...
LayoutTests/transforms/scale.html:12: scale: 0.5 1;
No test for scale with a single value?

https://codereview.chromium.org/1158603003/diff/140001/Source/core/css/resolv...
File Source/core/css/resolver/StyleBuilderConverter.cpp (right):

https://codereview.chromium.org/1158603003/diff/140001/Source/core/css/resolv...
Source/core/css/resolver/StyleBuilderConverter.cpp:940: CSSValueList* list =
toCSSValueList(value);
Nit: Dereference value here.

https://codereview.chromium.org/1158603003/diff/140001/Source/core/css/resolv...
Source/core/css/resolver/StyleBuilderConverter.cpp:941: ASSERT(list->length() >=
1 && list->length() <= 3);
list->length() >= 1 is implicitly asserted when you access the items, you can
just assert <= 3 here.

https://codereview.chromium.org/1158603003/diff/140001/Source/core/css/resolv...
File Source/core/css/resolver/StyleBuilderConverter.h (right):

https://codereview.chromium.org/1158603003/diff/140001/Source/core/css/resolv...
Source/core/css/resolver/StyleBuilderConverter.h:44: #include
"platform/transforms/TranslateTransformOperation.h"
Can these just be forward declared instead?

https://codereview.chromium.org/1158603003/diff/140001/Source/core/style/Comp...
File Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/1158603003/diff/140001/Source/core/style/Comp...
Source/core/style/ComputedStyle.cpp:888: const
Vector<RefPtr<TransformOperation>>& transformOperations =
rareNonInheritedData->m_transform->m_operations.operations();
This can just call transform().

https://codereview.chromium.org/1158603003/diff/140001/Source/core/style/Comp...
Source/core/style/ComputedStyle.cpp:922: const
Vector<RefPtr<TransformOperation>>& transformOperations =
rareNonInheritedData->m_transform->m_operations.operations();
This can just call transform() and be moved down to where it's actually used.
The transform parameter should be renamed to result for clarity and to avoid the
name collision.

https://codereview.chromium.org/1158603003/diff/140001/Source/core/style/Comp...
File Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/1158603003/diff/140001/Source/core/style/Comp...
Source/core/style/ComputedStyle.h:885: enum ApplyTransformL2 {
IncludeTransformL2, ExcludeTransformL2 };
ApplyIndependentTransformProperties for consistency with the runtime flag?

https://codereview.chromium.org/1158603003/diff/140001/Source/core/style/Styl...
File Source/core/style/StyleTransformData.h (right):

https://codereview.chromium.org/1158603003/diff/140001/Source/core/style/Styl...
Source/core/style/StyleTransformData.h:49: bool has3DTransformProperties()
const;
I'm not sure if has3DTransformProperties() makes sense, perhaps
has3DTransform()?

[soonm, 2015-06-17 23:56:54]

https://codereview.chromium.org/1158603003/diff/140001/LayoutTests/transforms...
File LayoutTests/transforms/combine-transforms-properties-motion-path.html
(right):

https://codereview.chromium.org/1158603003/diff/140001/LayoutTests/transforms...
LayoutTests/transforms/combine-transforms-properties-motion-path.html:10: <!--
Note the order of the scaling operation affects the path coordinates -->
On 2015/06/17 07:38:32, alancutter wrote:
> /* */

Done.

https://codereview.chromium.org/1158603003/diff/140001/LayoutTests/transforms...
File LayoutTests/transforms/rotate-parsing.html (right):

https://codereview.chromium.org/1158603003/diff/140001/LayoutTests/transforms...
LayoutTests/transforms/rotate-parsing.html:23: .isComputedTo('450deg 0 1 1');
On 2015/06/17 07:38:32, alancutter wrote:
> One line plz.

Done.

https://codereview.chromium.org/1158603003/diff/140001/LayoutTests/transforms...
File LayoutTests/transforms/scale.html (right):

https://codereview.chromium.org/1158603003/diff/140001/LayoutTests/transforms...
LayoutTests/transforms/scale.html:12: scale: 0.5 1;
On 2015/06/17 07:38:32, alancutter wrote:
> No test for scale with a single value?

Done. It currently assumes that x and y use the same scale if only 1 number is
defined.

https://codereview.chromium.org/1158603003/diff/140001/Source/core/css/resolv...
File Source/core/css/resolver/StyleBuilderConverter.cpp (right):

https://codereview.chromium.org/1158603003/diff/140001/Source/core/css/resolv...
Source/core/css/resolver/StyleBuilderConverter.cpp:940: CSSValueList* list =
toCSSValueList(value);
On 2015/06/17 07:38:32, alancutter wrote:
> Nit: Dereference value here.

Done.

https://codereview.chromium.org/1158603003/diff/140001/Source/core/css/resolv...
Source/core/css/resolver/StyleBuilderConverter.cpp:941: ASSERT(list->length() >=
1 && list->length() <= 3);
On 2015/06/17 07:38:32, alancutter wrote:
> list->length() >= 1 is implicitly asserted when you access the items, you can
> just assert <= 3 here.

Done.

https://codereview.chromium.org/1158603003/diff/140001/Source/core/css/resolv...
File Source/core/css/resolver/StyleBuilderConverter.h (right):

https://codereview.chromium.org/1158603003/diff/140001/Source/core/css/resolv...
Source/core/css/resolver/StyleBuilderConverter.h:44: #include
"platform/transforms/TranslateTransformOperation.h"
On 2015/06/17 07:38:32, alancutter wrote:
> Can these just be forward declared instead?

Done.

https://codereview.chromium.org/1158603003/diff/140001/Source/core/style/Comp...
File Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/1158603003/diff/140001/Source/core/style/Comp...
Source/core/style/ComputedStyle.cpp:888: const
Vector<RefPtr<TransformOperation>>& transformOperations =
rareNonInheritedData->m_transform->m_operations.operations();
On 2015/06/17 07:38:33, alancutter wrote:
> This can just call transform().

Done.

https://codereview.chromium.org/1158603003/diff/140001/Source/core/style/Comp...
Source/core/style/ComputedStyle.cpp:922: const
Vector<RefPtr<TransformOperation>>& transformOperations =
rareNonInheritedData->m_transform->m_operations.operations();
On 2015/06/17 07:38:33, alancutter wrote:
> This can just call transform() and be moved down to where it's actually used.
> The transform parameter should be renamed to result for clarity and to avoid
the
> name collision.

Done.

https://codereview.chromium.org/1158603003/diff/140001/Source/core/style/Comp...
File Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/1158603003/diff/140001/Source/core/style/Comp...
Source/core/style/ComputedStyle.h:885: enum ApplyTransformL2 {
IncludeTransformL2, ExcludeTransformL2 };
On 2015/06/17 07:38:33, alancutter wrote:
> ApplyIndependentTransformProperties for consistency with the runtime flag?

Done.

https://codereview.chromium.org/1158603003/diff/140001/Source/core/style/Styl...
File Source/core/style/StyleTransformData.h (right):

https://codereview.chromium.org/1158603003/diff/140001/Source/core/style/Styl...
Source/core/style/StyleTransformData.h:49: bool has3DTransformProperties()
const;
On 2015/06/17 07:38:33, alancutter wrote:
> I'm not sure if has3DTransformProperties() makes sense, perhaps
> has3DTransform()?

Done - Yea I agree since it should be simplified since it check both transform
and the independent properties.

[alancutter (OOO until 2018), 2015-06-18 13:03:25]

lgtm with nits.

https://codereview.chromium.org/1158603003/diff/180001/Source/core/css/Comput...
File Source/core/css/ComputedStyleCSSValueMapping.cpp (right):

https://codereview.chromium.org/1158603003/diff/180001/Source/core/css/Comput...
Source/core/css/ComputedStyleCSSValueMapping.cpp:2651:
list->append(zoomAdjustedPixelValue(floatValueForLength(style.translate()->y(),
box.height().toFloat()), style));
Add tests for absolute lengths from getComputedStyle().

https://codereview.chromium.org/1158603003/diff/180001/Source/core/layout/com...
File Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.cpp
(right):

https://codereview.chromium.org/1158603003/diff/180001/Source/core/layout/com...
Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.cpp:272:
style.applyTransform(t,
LayoutSize(toLayoutBox(layoutObject())->pixelSnappedSize()),
ComputedStyle::ExcludeTransformOrigin, ComputedStyle::IncludeMotionPath,
ComputedStyle::IncludeIndependentTransformProperties);
Looks like you'll need to rebase again on the change you made earlier now that
it's landed.

https://codereview.chromium.org/1158603003/diff/180001/Source/core/style/Comp...
File Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/1158603003/diff/180001/Source/core/style/Comp...
Source/core/style/ComputedStyle.h:83: #include
"platform/transforms/TranslateTransformOperation.h"
Can these be forward declared?

https://codereview.chromium.org/1158603003/diff/180001/Source/core/style/Comp...
Source/core/style/ComputedStyle.h:851: ScaleTransformOperation* scale() const {
return rareNonInheritedData->m_transform->m_scale.get(); }
The returned pointer should be const.

[Timothy Loh, 2015-06-19 01:16:45]

https://codereview.chromium.org/1158603003/diff/180001/LayoutTests/transforms...
File LayoutTests/transforms/transform-properties-mixed-expected.html (right):

https://codereview.chromium.org/1158603003/diff/180001/LayoutTests/transforms...
LayoutTests/transforms/transform-properties-mixed-expected.html:40: transform:
translateX(50px) rotate3d(0, 1, 0, 90deg);
not a big deal but it seems weird to change the translate to a translateX but
not make the rotate a rotateY here (and all the other places in this file)

https://codereview.chromium.org/1158603003/diff/180001/Source/core/css/Comput...
File Source/core/css/ComputedStyleCSSValueMapping.cpp (right):

https://codereview.chromium.org/1158603003/diff/180001/Source/core/css/Comput...
Source/core/css/ComputedStyleCSSValueMapping.cpp:2655:
list->append(zoomAdjustedPixelValueForLength(style.translate()->x(), style));
is this branch tested?

https://codereview.chromium.org/1158603003/diff/180001/Source/core/css/resolv...
File Source/core/css/resolver/StyleBuilderConverter.h (right):

https://codereview.chromium.org/1158603003/diff/180001/Source/core/css/resolv...
Source/core/css/resolver/StyleBuilderConverter.h:46: class
RotateTransformOperation;
Probably better above the comment otherwise it reads weirdly

https://codereview.chromium.org/1158603003/diff/180001/Source/core/style/Comp...
File Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/1158603003/diff/180001/Source/core/style/Comp...
Source/core/style/ComputedStyle.h:1660: static const
PassRefPtr<TranslateTransformOperation> initialTranslate() { return
TranslateTransformOperation::create(Length(0, Fixed), Length(0, Fixed), 0,
TransformOperation::Translate3D); }
returning const values is... weird. I'm pretty sure there's no reason for the
const.

[soonm, 2015-06-19 04:39:53]

https://codereview.chromium.org/1158603003/diff/180001/LayoutTests/transforms...
File LayoutTests/transforms/transform-properties-mixed-expected.html (right):

https://codereview.chromium.org/1158603003/diff/180001/LayoutTests/transforms...
LayoutTests/transforms/transform-properties-mixed-expected.html:40: transform:
translateX(50px) rotate3d(0, 1, 0, 90deg);
On 2015/06/19 01:16:45, Timothy Loh wrote:
> not a big deal but it seems weird to change the translate to a translateX but
> not make the rotate a rotateY here (and all the other places in this file)

Done - I should make this consistent and use transform3d, rotate3d and scale3d.

https://codereview.chromium.org/1158603003/diff/180001/Source/core/css/Comput...
File Source/core/css/ComputedStyleCSSValueMapping.cpp (right):

https://codereview.chromium.org/1158603003/diff/180001/Source/core/css/Comput...
Source/core/css/ComputedStyleCSSValueMapping.cpp:2651:
list->append(zoomAdjustedPixelValue(floatValueForLength(style.translate()->y(),
box.height().toFloat()), style));
On 2015/06/18 13:03:24, alancutter wrote:
> Add tests for absolute lengths from getComputedStyle().

Done.

https://codereview.chromium.org/1158603003/diff/180001/Source/core/css/Comput...
Source/core/css/ComputedStyleCSSValueMapping.cpp:2655:
list->append(zoomAdjustedPixelValueForLength(style.translate()->x(), style));
On 2015/06/19 at 01:16:45, Timothy Loh wrote:
> is this branch tested?

Done - Add tests in translate-parsing with display: none

https://codereview.chromium.org/1158603003/diff/180001/Source/core/css/resolv...
File Source/core/css/resolver/StyleBuilderConverter.h (right):

https://codereview.chromium.org/1158603003/diff/180001/Source/core/css/resolv...
Source/core/css/resolver/StyleBuilderConverter.h:46: class
RotateTransformOperation;
On 2015/06/19 at 01:16:45, Timothy Loh wrote:
> Probably better above the comment otherwise it reads weirdly

Moved comment down

https://codereview.chromium.org/1158603003/diff/180001/Source/core/layout/com...
File Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.cpp
(right):

https://codereview.chromium.org/1158603003/diff/180001/Source/core/layout/com...
Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.cpp:272:
style.applyTransform(t,
LayoutSize(toLayoutBox(layoutObject())->pixelSnappedSize()),
ComputedStyle::ExcludeTransformOrigin, ComputedStyle::IncludeMotionPath,
ComputedStyle::IncludeIndependentTransformProperties);
On 2015/06/18 13:03:24, alancutter wrote:
> Looks like you'll need to rebase again on the change you made earlier now that
> it's landed.

Done.

https://codereview.chromium.org/1158603003/diff/180001/Source/core/style/Comp...
File Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/1158603003/diff/180001/Source/core/style/Comp...
Source/core/style/ComputedStyle.h:851: ScaleTransformOperation* scale() const {
return rareNonInheritedData->m_transform->m_scale.get(); }
On 2015/06/18 13:03:24, alancutter wrote:
> The returned pointer should be const.

As discussed, there is a use case for non-const raw ptr.

https://codereview.chromium.org/1158603003/diff/180001/Source/core/style/Comp...
Source/core/style/ComputedStyle.h:1660: static const
PassRefPtr<TranslateTransformOperation> initialTranslate() { return
TranslateTransformOperation::create(Length(0, Fixed), Length(0, Fixed), 0,
TransformOperation::Translate3D); }
On 2015/06/19 at 01:16:45, Timothy Loh wrote:
> returning const values is... weird. I'm pretty sure there's no reason for the
const.

Done - Yep no need to be const here

[Timothy Loh, 2015-06-19 05:09:57]

lgtm

[soonm, 2015-06-19 05:15:03]

soonm@google.com changed reviewers:
+ noel@chromium.org

[soonm, 2015-06-19 06:16:51]

+ Noel for platform owners review

[dstockwell, 2015-06-19 06:20:45]

On 2015/06/19 at 06:16:51, soonm wrote:
> + Noel for platform owners review

I don't think you need to make any changes in platform/... Isn't the apply
function public in TransformOperation?

[dstockwell, 2015-06-19 06:21:40]

On 2015/06/19 at 06:20:45, dstockwell wrote:
> On 2015/06/19 at 06:16:51, soonm wrote:
> > + Noel for platform owners review
> 
> I don't think you need to make any changes in platform/... Isn't the apply
function public in TransformOperation?

But, I suppose you need the RuntimeEnabledFeatures change :P

[Timothy Loh, 2015-06-19 06:23:26]

On 2015/06/19 06:20:45, dstockwell wrote:
> On 2015/06/19 at 06:16:51, soonm wrote:
> > + Noel for platform owners review
> 
> I don't think you need to make any changes in platform/... Isn't the apply
> function public in TransformOperation?

Public on base class but private on derived class is a really bad pattern though
since you'd need to upcast a pointer/reference to the derived class to call the
function (it breaks LSP :/)

[Noel Gordon, 2015-06-19 06:36:41]

On 2015/06/19 06:23:26, Timothy Loh wrote:
> On 2015/06/19 06:20:45, dstockwell wrote:
> > On 2015/06/19 at 06:16:51, soonm wrote:
> > > + Noel for platform owners review

Let me know when it's ready then.

[dstockwell, 2015-06-19 06:45:17]

On 2015/06/19 at 06:23:26, timloh wrote:
> On 2015/06/19 06:20:45, dstockwell wrote:
> > On 2015/06/19 at 06:16:51, soonm wrote:
> > > + Noel for platform owners review
> > 
> > I don't think you need to make any changes in platform/... Isn't the apply
> > function public in TransformOperation?
> 
> Public on base class but private on derived class is a really bad pattern
though since you'd need to upcast a pointer/reference to the derived class to
call the function (it breaks LSP :/)

ok lgtm

[Noel Gordon, 2015-06-19 06:53:21]

rs-lgtm

[soonm, 2015-06-19 06:56:33]

On 2015/06/19 at 06:36:41, noel wrote:
> On 2015/06/19 06:23:26, Timothy Loh wrote:
> > On 2015/06/19 06:20:45, dstockwell wrote:
> > > On 2015/06/19 at 06:16:51, soonm wrote:
> > > > + Noel for platform owners review
> 
> Let me know when it's ready then.

There's no changes on that problematic pattern in this patch, so you can review
it.

[Noel Gordon, 2015-06-19 07:01:14]

On 2015/06/19 06:56:33, soonm wrote:

> There's no changes on that problematic pattern in this patch, so you can
review
> it.

see #47 :)

[soonm, 2015-06-21 23:49:59]

The CQ bit was checked by soonm@google.com

[soonm, 2015-06-21 23:50:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
ericwilligers@chromium.org, alancutter@chromium.org
Link to the patchset: https://codereview.chromium.org/1158603003/#ps220001
(title: "Rebase master")

[commit-bot: I haz the power, 2015-06-21 23:50:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1158603003/220001

[commit-bot: I haz the power, 2015-06-22 01:39:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-22 01:39:33]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/59975)

[soonm, 2015-06-22 01:54:17]

The CQ bit was checked by soonm@google.com

[commit-bot: I haz the power, 2015-06-22 01:54:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1158603003/220001

[commit-bot: I haz the power, 2015-06-22 02:27:57]

Committed patchset #12 (id:220001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197547