Don't force layout of descendants of SVG containers needing self-layout

Don't force layout of descendants of SVG containers needing self-layout

There isn't much "layout" required for a generic SVG container - it is
sensitive to changes to its descendants and any impact a filter it
itself references has. We would however always relayout the entire
subtree of a container that had been marked as needing layout.
Like he code removed by https://codereview.chromium.org/1907273002, this
remaining part of the condition is primarily a remnant of the old way of
performing paint invalidation.
This CL changes to not force layout unconditionally based on the
self-needs-layout flag, but instead relies on the scale-factor-changed
flag (and of course specific marking of descendants) to handle this job.
To achieve this, rudimentary scale-factor-changed "detection" is added
to LayoutSVGRoot, as well as code to propagate this state to
descendants. LayoutSVGResourceMarker is modified in a similar fashion.
This is needed to counter-act the removal of the forced layout, since
previously this would propagate to descendants (unconditionally) by
forcing layout in both the root and containers. This effect is now
achieved (for the root) by using the scale-factor-changed signal
instead. This signal will be improved in future CLs.

BUG=603956
Committed: https://crrev.com/6260c0fbdec366ad2c78c3ba5cb9be297859dbef
Cr-Commit-Position: refs/heads/master@{#389742}

[fs, 2016-04-25 14:26:41]

fs@opera.com changed reviewers:
+ pdr@chromium.org, schenney@chromium.org

[fs, 2016-04-25 15:12:20]

Some additional context/background:
https://bugs.webkit.org/show_bug.cgi?id=14003

[pdr., 2016-04-26 00:07:42]

This is a little messy but I see the light at the end of the tunnel.

LGTM

https://codereview.chromium.org/1920833002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceMarker.cpp
(right):

https://codereview.chromium.org/1920833002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceMarker.cpp:157: //
TODO(fs): Tweak this condition to be more precise and factor in actual
This is correct but was pretty confusing at first glance.

Can you update the comment to be something like (here and below):
// TODO(fs): Temporarily, needing a layout implies that the local transform has
// changed. This should be updated to be more precise and factor in the
// actual (relevant) changes to the computed user-space transform.

https://codereview.chromium.org/1920833002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceMarker.h
(right):

https://codereview.chromium.org/1920833002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceMarker.h:48: bool
calculateLocalTransform() override;
Should this be private?

[pdr., 2016-04-26 00:09:10]

On 2016/04/25 at 15:12:20, fs wrote:
> Some additional context/background:
https://bugs.webkit.org/show_bug.cgi?id=14003

Yikes! I think your investigation may end up being a very nice perf win in
general.

[fs, 2016-04-26 08:15:07]

On 2016/04/26 at 00:07:42, pdr wrote:
> This is a little messy but I see the light at the end of the tunnel.

Yes... (I favored this over adding essentially dead code.)

The light should be just around the corner now. I'll be walking into it in a
moment I hope.

https://codereview.chromium.org/1920833002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceMarker.cpp
(right):

https://codereview.chromium.org/1920833002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceMarker.cpp:157: //
TODO(fs): Tweak this condition to be more precise and factor in actual
On 2016/04/26 at 00:07:42, pdr wrote:
> This is correct but was pretty confusing at first glance.

Yes, layout for markers seem a bit... opportunistic.

> Can you update the comment to be something like (here and below):
> // TODO(fs): Temporarily, needing a layout implies that the local transform
has
> // changed. This should be updated to be more precise and factor in the
> // actual (relevant) changes to the computed user-space transform.

Done (x2)

https://codereview.chromium.org/1920833002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceMarker.h
(right):

https://codereview.chromium.org/1920833002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceMarker.h:48: bool
calculateLocalTransform() override;
On 2016/04/26 at 00:07:42, pdr wrote:
> Should this be private?

Certain could be. Moved its closest friends too.

[fs, 2016-04-26 10:30:40]

The CQ bit was checked by fs@opera.com

[fs, 2016-04-26 10:30:41]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/1920833002/#ps20001
(title: "Clarify comments. Make some methods private.")

[commit-bot: I haz the power, 2016-04-26 10:30:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1920833002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1920833002/20001

[commit-bot: I haz the power, 2016-04-26 10:34:50]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-04-26 10:36:17]

Description was changed from

==========
Don't force layout of descendants of SVG containers needing self-layout

There isn't much "layout" required for a generic SVG container - it is
sensitive to changes to its descendants and any impact a filter it
itself references has. We would however always relayout the entire
subtree of a container that had been marked as needing layout.
Like he code removed by https://codereview.chromium.org/1907273002, this
remaining part of the condition is primarily a remnant of the old way of
performing paint invalidation.
This CL changes to not force layout unconditionally based on the
self-needs-layout flag, but instead relies on the scale-factor-changed
flag (and of course specific marking of descendants) to handle this job.
To achieve this, rudimentary scale-factor-changed "detection" is added
to LayoutSVGRoot, as well as code to propagate this state to
descendants. LayoutSVGResourceMarker is modified in a similar fashion.
This is needed to counter-act the removal of the forced layout, since
previously this would propagate to descendants (unconditionally) by
forcing layout in both the root and containers. This effect is now
achieved (for the root) by using the scale-factor-changed signal
instead. This signal will be improved in future CLs.

BUG=603956
==========

to

==========
Don't force layout of descendants of SVG containers needing self-layout

There isn't much "layout" required for a generic SVG container - it is
sensitive to changes to its descendants and any impact a filter it
itself references has. We would however always relayout the entire
subtree of a container that had been marked as needing layout.
Like he code removed by https://codereview.chromium.org/1907273002, this
remaining part of the condition is primarily a remnant of the old way of
performing paint invalidation.
This CL changes to not force layout unconditionally based on the
self-needs-layout flag, but instead relies on the scale-factor-changed
flag (and of course specific marking of descendants) to handle this job.
To achieve this, rudimentary scale-factor-changed "detection" is added
to LayoutSVGRoot, as well as code to propagate this state to
descendants. LayoutSVGResourceMarker is modified in a similar fashion.
This is needed to counter-act the removal of the forced layout, since
previously this would propagate to descendants (unconditionally) by
forcing layout in both the root and containers. This effect is now
achieved (for the root) by using the scale-factor-changed signal
instead. This signal will be improved in future CLs.

BUG=603956

Committed: https://crrev.com/6260c0fbdec366ad2c78c3ba5cb9be297859dbef
Cr-Commit-Position: refs/heads/master@{#389742}
==========

[commit-bot: I haz the power, 2016-04-26 10:36:18]

Patchset 2 (id:??) landed as
https://crrev.com/6260c0fbdec366ad2c78c3ba5cb9be297859dbef
Cr-Commit-Position: refs/heads/master@{#389742}