Reland of "Remove redundant "layout size changed" state from LayoutSVGRoot"

Reland of "Remove redundant "layout size changed" state from LayoutSVGRoot"

In LayoutSVGRoot::layout, two slightly different "layout size changed"
values are computed - one which is used for propagation to children
via SVGLayoutSupport::layoutSizeOfNearestViewportChanged
(|m_isLayoutSizeChanged|), and one which is used to mark direct
descendant children (local |layoutSizeChanged|).
Ultimately their use is the same though, so only using the more narrow
predicate for both of these cases should yield the same result.
It also has the side-effect of making it more obvious that changes to
layout-size is only of interest when there exist clients of the SVG
root that have relative lengths.

BUG=603956
Committed: https://crrev.com/2fd756f78f5d05f69bd298515bb7eff07f500b6a
Cr-Commit-Position: refs/heads/master@{#400987}

[fs, 2016-06-14 15:11:03]

Description was changed from

==========
Remove redundant "layout size changed" signalling from LayoutSVGRoot

In LayoutSVGRoot::layout, two slightly different "layout size changed"
values are computed - one which is used for propagation to children
via SVGLayoutSupport::layoutSizeOfNearestViewportChanged
(|m_isLayoutSizeChanged|), and one which is used to mark direct
descendant children (local |layoutSizeChanged|).
Ultimately their use is the same though, so only using the more narrow
predicate for both of these cases should yield the same result.
It also has the side-effect of making it more obvious that changes to
layout-size is only of interest when there exist clients of the SVG
root that have relative lengths.

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

to

==========
Remove redundant "layout size changed" state from LayoutSVGRoot

In LayoutSVGRoot::layout, two slightly different "layout size changed"
values are computed - one which is used for propagation to children
via SVGLayoutSupport::layoutSizeOfNearestViewportChanged
(|m_isLayoutSizeChanged|), and one which is used to mark direct
descendant children (local |layoutSizeChanged|).
Ultimately their use is the same though, so only using the more narrow
predicate for both of these cases should yield the same result.
It also has the side-effect of making it more obvious that changes to
layout-size is only of interest when there exist clients of the SVG
root that have relative lengths.

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

[fs, 2016-06-14 18:40:38]

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

[Stephen Chennney, 2016-06-14 20:34:57]

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

[Stephen Chennney, 2016-06-14 20:34:58]

lgtm

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

https://codereview.chromium.org/2065093002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:158:
m_isLayoutSizeChanged = svg->hasRelativeLengths() && (selfNeedsLayout() ||
oldSize != size());
I built a truth table to verify, as this is messy logic. The only change I see
is that now m_isLayoutSizeChanged matches the old values for layoutSizeChanged,
while previously it was also true for (selfNeedsLayout && !hasRelativeLengths).

I agree that there's no point in doing anything if !hasRelativeLengths, so all
of this is a long way of saying I agree with you.

[fs, 2016-06-14 20:46:30]

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

https://codereview.chromium.org/2065093002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:158:
m_isLayoutSizeChanged = svg->hasRelativeLengths() && (selfNeedsLayout() ||
oldSize != size());
On 2016/06/14 at 20:34:58, Stephen Chennney wrote:
> I built a truth table to verify, as this is messy logic. The only change I see
is that now m_isLayoutSizeChanged matches the old values for layoutSizeChanged,
while previously it was also true for (selfNeedsLayout && !hasRelativeLengths).
> 
> I agree that there's no point in doing anything if !hasRelativeLengths, so all
of this is a long way of saying I agree with you.

I guess another tidbit of information that might worth mentioning is that the
"non-local" flag is not read if !hasRelativeLengths - because of how that
relation propagates to ancestors. (The same "guard" is also used in the other
place that sets a flag with the same purpose.)

[fs, 2016-06-14 20:47:13]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2016-06-14 20:47:41]

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

[commit-bot: I haz the power, 2016-06-14 21:48:33]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-06-14 21:48:55]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-14 21:50:04]

Description was changed from

==========
Remove redundant "layout size changed" state from LayoutSVGRoot

In LayoutSVGRoot::layout, two slightly different "layout size changed"
values are computed - one which is used for propagation to children
via SVGLayoutSupport::layoutSizeOfNearestViewportChanged
(|m_isLayoutSizeChanged|), and one which is used to mark direct
descendant children (local |layoutSizeChanged|).
Ultimately their use is the same though, so only using the more narrow
predicate for both of these cases should yield the same result.
It also has the side-effect of making it more obvious that changes to
layout-size is only of interest when there exist clients of the SVG
root that have relative lengths.

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

to

==========
Remove redundant "layout size changed" state from LayoutSVGRoot

In LayoutSVGRoot::layout, two slightly different "layout size changed"
values are computed - one which is used for propagation to children
via SVGLayoutSupport::layoutSizeOfNearestViewportChanged
(|m_isLayoutSizeChanged|), and one which is used to mark direct
descendant children (local |layoutSizeChanged|).
Ultimately their use is the same though, so only using the more narrow
predicate for both of these cases should yield the same result.
It also has the side-effect of making it more obvious that changes to
layout-size is only of interest when there exist clients of the SVG
root that have relative lengths.

BUG=603956

Committed: https://crrev.com/30770a70834c73670884f0de91bb7624df0ba003
Cr-Commit-Position: refs/heads/master@{#399791}
==========

[commit-bot: I haz the power, 2016-06-14 21:50:05]

Patchset 1 (id:??) landed as
https://crrev.com/30770a70834c73670884f0de91bb7624df0ba003
Cr-Commit-Position: refs/heads/master@{#399791}

[fs, 2016-06-17 07:34:22]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/2071953004/ by fs@opera.com.

The reason for reverting is: Possible cause of crbug.com/620228.

[fs, 2016-06-20 11:16:39]

Description was changed from

==========
Remove redundant "layout size changed" state from LayoutSVGRoot

In LayoutSVGRoot::layout, two slightly different "layout size changed"
values are computed - one which is used for propagation to children
via SVGLayoutSupport::layoutSizeOfNearestViewportChanged
(|m_isLayoutSizeChanged|), and one which is used to mark direct
descendant children (local |layoutSizeChanged|).
Ultimately their use is the same though, so only using the more narrow
predicate for both of these cases should yield the same result.
It also has the side-effect of making it more obvious that changes to
layout-size is only of interest when there exist clients of the SVG
root that have relative lengths.

BUG=603956

Committed: https://crrev.com/30770a70834c73670884f0de91bb7624df0ba003
Cr-Commit-Position: refs/heads/master@{#399791}
==========

to

==========
Reland of "Remove redundant "layout size changed" state from LayoutSVGRoot"

In LayoutSVGRoot::layout, two slightly different "layout size changed"
values are computed - one which is used for propagation to children
via SVGLayoutSupport::layoutSizeOfNearestViewportChanged
(|m_isLayoutSizeChanged|), and one which is used to mark direct
descendant children (local |layoutSizeChanged|).
Ultimately their use is the same though, so only using the more narrow
predicate for both of these cases should yield the same result.
It also has the side-effect of making it more obvious that changes to
layout-size is only of interest when there exist clients of the SVG
root that have relative lengths.

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

[fs, 2016-06-20 11:17:04]

On 2016/06/17 at 07:34:22, fs wrote:
> A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/2071953004/ by fs@opera.com.
> 
> The reason for reverting is: Possible cause of crbug.com/620228.

Not the culprit. Relanding.

[fs, 2016-06-20 11:17:09]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2016-06-20 11:17:24]

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

[commit-bot: I haz the power, 2016-06-20 13:18:16]

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

[commit-bot: I haz the power, 2016-06-20 13:18:17]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[fs, 2016-06-20 13:23:24]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2016-06-20 13:23:35]

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

[commit-bot: I haz the power, 2016-06-20 14:54:01]

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

[commit-bot: I haz the power, 2016-06-20 14:54:03]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[fs, 2016-06-21 13:17:35]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2016-06-21 13:17:49]

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

[commit-bot: I haz the power, 2016-06-21 14:29:10]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-06-21 14:34:18]

Description was changed from

==========
Reland of "Remove redundant "layout size changed" state from LayoutSVGRoot"

In LayoutSVGRoot::layout, two slightly different "layout size changed"
values are computed - one which is used for propagation to children
via SVGLayoutSupport::layoutSizeOfNearestViewportChanged
(|m_isLayoutSizeChanged|), and one which is used to mark direct
descendant children (local |layoutSizeChanged|).
Ultimately their use is the same though, so only using the more narrow
predicate for both of these cases should yield the same result.
It also has the side-effect of making it more obvious that changes to
layout-size is only of interest when there exist clients of the SVG
root that have relative lengths.

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

to

==========
Reland of "Remove redundant "layout size changed" state from LayoutSVGRoot"

In LayoutSVGRoot::layout, two slightly different "layout size changed"
values are computed - one which is used for propagation to children
via SVGLayoutSupport::layoutSizeOfNearestViewportChanged
(|m_isLayoutSizeChanged|), and one which is used to mark direct
descendant children (local |layoutSizeChanged|).
Ultimately their use is the same though, so only using the more narrow
predicate for both of these cases should yield the same result.
It also has the side-effect of making it more obvious that changes to
layout-size is only of interest when there exist clients of the SVG
root that have relative lengths.

BUG=603956

Committed: https://crrev.com/2fd756f78f5d05f69bd298515bb7eff07f500b6a
Cr-Commit-Position: refs/heads/master@{#400987}
==========

[commit-bot: I haz the power, 2016-06-21 14:34:20]

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