will-change on SVG elements should not reset transforms.

This CL is to fix will-change bug in SVG.
Because if set will-change in SVG Element which has |transform|
property, transform matrix applied |transform| property change to
identity matrix.

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

[hyunjunekim2, 2015-04-10 03:59:05]

hyunjune.kim@samsung.com changed reviewers:
+ fs@opera.com, mkwst@chromium.org, pdr@chromium.org

[hyunjunekim2, 2015-04-10 04:01:27]

pdr, I fixed issue 475573, Could you give me feedback? Thank you :)

[pdr., 2015-04-10 04:55:11]

Lets update the change title (and first line of the description) to be more
descriptive. Something like "will-change on SVG elements should not reset
transforms". We also try to wrap the change description to 72 characters.

Please add a test.

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

https://codereview.chromium.org/1075093002/diff/1/Source/core/css/resolver/St...
Source/core/css/resolver/StyleAdjuster.cpp:200: if (!e->isSVGElement() &&
!style.hasTransform() &&
(style.willChangeProperties().contains(CSSPropertyWebkitTransform) ||
style.willChangeProperties().contains(CSSPropertyTransform))) {
This will crash when e is null so you'll need an extra check for that case.

Because we also check e->isSVGElement below, lets move this into a local
variable:
bool isSVGElement = e && e->isSVGElement();
You can use this in the svg check below (line 231).

Lets also add a comment:
// will-change:transform should result in the same rendering behavior as having
a transform,
// including the creation of a containing block for fixed position descendants.
SVG elements
// can skip this because they implicitly have transforms.
bool isSVGElement = e && e->isSVGElement();
if (!isSVGElement && !style.hasTransform() && (style.wil... etc

[hyunjunekim2, 2015-04-10 05:36:59]

On 2015/04/10 04:55:11, pdr wrote:
> Lets update the change title (and first line of the description) to be more
> descriptive. Something like "will-change on SVG elements should not reset
> transforms". We also try to wrap the change description to 72 characters.
> 
> Please add a test.
> 
>
https://codereview.chromium.org/1075093002/diff/1/Source/core/css/resolver/St...
> File Source/core/css/resolver/StyleAdjuster.cpp (right):
> 
>
https://codereview.chromium.org/1075093002/diff/1/Source/core/css/resolver/St...
> Source/core/css/resolver/StyleAdjuster.cpp:200: if (!e->isSVGElement() &&
> !style.hasTransform() &&
> (style.willChangeProperties().contains(CSSPropertyWebkitTransform) ||
> style.willChangeProperties().contains(CSSPropertyTransform))) {
> This will crash when e is null so you'll need an extra check for that case.
> 
> Because we also check e->isSVGElement below, lets move this into a local
> variable:
> bool isSVGElement = e && e->isSVGElement();
> You can use this in the svg check below (line 231).
> 
> Lets also add a comment:
> // will-change:transform should result in the same rendering behavior as
having
> a transform,
> // including the creation of a containing block for fixed position
descendants.
> SVG elements
> // can skip this because they implicitly have transforms.
> bool isSVGElement = e && e->isSVGElement();
> if (!isSVGElement && !style.hasTransform() && (style.wil... etc

Could you check this one? Thank you. :)

[pdr., 2015-04-10 05:39:40]

On 2015/04/10 at 05:36:59, hyunjune.kim wrote:
> On 2015/04/10 04:55:11, pdr wrote:
> > Lets update the change title (and first line of the description) to be more
> > descriptive. Something like "will-change on SVG elements should not reset
> > transforms". We also try to wrap the change description to 72 characters.
> > 
> > Please add a test.
> > 
> >
https://codereview.chromium.org/1075093002/diff/1/Source/core/css/resolver/St...
> > File Source/core/css/resolver/StyleAdjuster.cpp (right):
> > 
> >
https://codereview.chromium.org/1075093002/diff/1/Source/core/css/resolver/St...
> > Source/core/css/resolver/StyleAdjuster.cpp:200: if (!e->isSVGElement() &&
> > !style.hasTransform() &&
> > (style.willChangeProperties().contains(CSSPropertyWebkitTransform) ||
> > style.willChangeProperties().contains(CSSPropertyTransform))) {
> > This will crash when e is null so you'll need an extra check for that case.
> > 
> > Because we also check e->isSVGElement below, lets move this into a local
> > variable:
> > bool isSVGElement = e && e->isSVGElement();
> > You can use this in the svg check below (line 231).
> > 
> > Lets also add a comment:
> > // will-change:transform should result in the same rendering behavior as
having
> > a transform,
> > // including the creation of a containing block for fixed position
descendants.
> > SVG elements
> > // can skip this because they implicitly have transforms.
> > bool isSVGElement = e && e->isSVGElement();
> > if (!isSVGElement && !style.hasTransform() && (style.wil... etc
> 
> Could you check this one? Thank you. :)

Please read my previous reply more closely. There are several comments not
addressed in this patch.

[hyunjunekim2, 2015-04-10 07:18:46]

On 2015/04/10 05:39:40, pdr wrote:
> On 2015/04/10 at 05:36:59, hyunjune.kim wrote:
> > On 2015/04/10 04:55:11, pdr wrote:
> > > Lets update the change title (and first line of the description) to be
more
> > > descriptive. Something like "will-change on SVG elements should not reset
> > > transforms". We also try to wrap the change description to 72 characters.
> > > 
> > > Please add a test.
> > > 
> > >
>
https://codereview.chromium.org/1075093002/diff/1/Source/core/css/resolver/St...
> > > File Source/core/css/resolver/StyleAdjuster.cpp (right):
> > > 
> > >
>
https://codereview.chromium.org/1075093002/diff/1/Source/core/css/resolver/St...
> > > Source/core/css/resolver/StyleAdjuster.cpp:200: if (!e->isSVGElement() &&
> > > !style.hasTransform() &&
> > > (style.willChangeProperties().contains(CSSPropertyWebkitTransform) ||
> > > style.willChangeProperties().contains(CSSPropertyTransform))) {
> > > This will crash when e is null so you'll need an extra check for that
case.
> > > 
> > > Because we also check e->isSVGElement below, lets move this into a local
> > > variable:
> > > bool isSVGElement = e && e->isSVGElement();
> > > You can use this in the svg check below (line 231).
> > > 
> > > Lets also add a comment:
> > > // will-change:transform should result in the same rendering behavior as
> having
> > > a transform,
> > > // including the creation of a containing block for fixed position
> descendants.
> > > SVG elements
> > > // can skip this because they implicitly have transforms.
> > > bool isSVGElement = e && e->isSVGElement();
> > > if (!isSVGElement && !style.hasTransform() && (style.wil... etc
> > 
> > Could you check this one? Thank you. :)
> 
> Please read my previous reply more closely. There are several comments not
> addressed in this patch.

pdr, Sorry. Now, modfied subject, description and added test-case. Could you
review this issue?
Thank you.

[Mike West, 2015-04-10 11:21:55]

mkwst@chromium.org changed reviewers:
- mkwst@chromium.org

[Mike West, 2015-04-10 11:21:56]

Removing myself as a reviewer; I'm not a good choice for SVG-related code.

[pdr., 2015-04-10 20:19:21]

Getting closer

https://codereview.chromium.org/1075093002/diff/40001/LayoutTests/svg/transfo...
File LayoutTests/svg/transforms/will-change-transform-expected.svg (right):

https://codereview.chromium.org/1075093002/diff/40001/LayoutTests/svg/transfo...
LayoutTests/svg/transforms/will-change-transform-expected.svg:1: <svg
xmlns="http://www.w3.org/2000/svg" width="700" height="500">
With the above comment, this will become something like:
<svg xmlns="http://www.w3.org/2000/svg" width="200" height="200">
  <rect width="100" height="100" fill="green" />
</svg>

Again, please double-check that this fails without your patch and passes with
it.

https://codereview.chromium.org/1075093002/diff/40001/LayoutTests/svg/transfo...
File LayoutTests/svg/transforms/will-change-transform.svg (right):

https://codereview.chromium.org/1075093002/diff/40001/LayoutTests/svg/transfo...
LayoutTests/svg/transforms/will-change-transform.svg:2: <rect x="0" y="0"
width="100" height="100" fill="red" transform="translate(50,50) scale(2,2)"
style="will-change: transform;"/>
We try to have red squares for test failures and green squares for test passes.

Lets refactor the test to do something like this (please verify it fails without
your patch and passes with it):
<svg xmlns="http://www.w3.org/2000/svg" width="200" height="200">
  <rect width="100" height="100" fill="red" />
  <g transform="translate(-100,-100)">
    <rect width="100" height="100" fill="green" transform="translate(100, 100)"
style="will-change: transform;"/>
  </g>
</svg>

https://codereview.chromium.org/1075093002/diff/40001/Source/core/css/resolve...
File Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/1075093002/diff/40001/Source/core/css/resolve...
Source/core/css/resolver/StyleAdjuster.cpp:199: // including the creation of a
containing block for fixed position descendants.
Please add the comment I suggested.

[hyunjunekim2, 2015-04-11 07:02:50]

On 2015/04/10 20:19:21, pdr wrote:
> Getting closer
> 
>
https://codereview.chromium.org/1075093002/diff/40001/LayoutTests/svg/transfo...
> File LayoutTests/svg/transforms/will-change-transform-expected.svg (right):
> 
>
https://codereview.chromium.org/1075093002/diff/40001/LayoutTests/svg/transfo...
> LayoutTests/svg/transforms/will-change-transform-expected.svg:1: <svg
> xmlns="http://www.w3.org/2000/svg" width="700" height="500">
> With the above comment, this will become something like:
> <svg xmlns="http://www.w3.org/2000/svg" width="200" height="200">
>   <rect width="100" height="100" fill="green" />
> </svg>
> 
> Again, please double-check that this fails without your patch and passes with
> it.
> 
>
https://codereview.chromium.org/1075093002/diff/40001/LayoutTests/svg/transfo...
> File LayoutTests/svg/transforms/will-change-transform.svg (right):
> 
>
https://codereview.chromium.org/1075093002/diff/40001/LayoutTests/svg/transfo...
> LayoutTests/svg/transforms/will-change-transform.svg:2: <rect x="0" y="0"
> width="100" height="100" fill="red" transform="translate(50,50) scale(2,2)"
> style="will-change: transform;"/>
> We try to have red squares for test failures and green squares for test
passes.
> 
> Lets refactor the test to do something like this (please verify it fails
without
> your patch and passes with it):
> <svg xmlns="http://www.w3.org/2000/svg" width="200" height="200">
>   <rect width="100" height="100" fill="red" />
>   <g transform="translate(-100,-100)">
>     <rect width="100" height="100" fill="green" transform="translate(100,
100)"
> style="will-change: transform;"/>
>   </g>
> </svg>
> 
>
https://codereview.chromium.org/1075093002/diff/40001/Source/core/css/resolve...
> File Source/core/css/resolver/StyleAdjuster.cpp (right):
> 
>
https://codereview.chromium.org/1075093002/diff/40001/Source/core/css/resolve...
> Source/core/css/resolver/StyleAdjuster.cpp:199: // including the creation of a
> containing block for fixed position descendants.
> Please add the comment I suggested.

pdr, Could you check this one? Thank you.

[pdr., 2015-04-11 17:04:25]

https://codereview.chromium.org/1075093002/diff/80001/Source/core/css/resolve...
File Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/1075093002/diff/80001/Source/core/css/resolve...
Source/core/css/resolver/StyleAdjuster.cpp:200: // can skip this because they
implicitly have transforms.
Almost there but this is still not correct. Please read my first reply more
carefully. You'll need to add the words "SVG elements" to the previous line so
this is a complete sentence.

[hyunjunekim2, 2015-04-11 23:13:40]

On 2015/04/11 17:04:25, pdr wrote:
>
https://codereview.chromium.org/1075093002/diff/80001/Source/core/css/resolve...
> File Source/core/css/resolver/StyleAdjuster.cpp (right):
> 
>
https://codereview.chromium.org/1075093002/diff/80001/Source/core/css/resolve...
> Source/core/css/resolver/StyleAdjuster.cpp:200: // can skip this because they
> implicitly have transforms.
> Almost there but this is still not correct. Please read my first reply more
> carefully. You'll need to add the words "SVG elements" to the previous line so
> this is a complete sentence.

pdr, I modified, Could you check this? Thank you.

[pdr., 2015-04-11 23:22:01]

The CQ bit was checked by pdr@chromium.org

[pdr., 2015-04-11 23:22:02]

lgtm

[commit-bot: I haz the power, 2015-04-11 23:22:05]

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

[commit-bot: I haz the power, 2015-04-12 02:32:18]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193597