Mark children of an SVG root as needing paint invalidation checking on resize.

Mark children of an SVG root as needing paint invalidation checking on resize.

BUG=649760
Committed: https://crrev.com/bd1529c0d0d4afe68e09b455203cc63d982e7b4d
Cr-Commit-Position: refs/heads/master@{#423638}

[chrishtr, 2016-10-05 22:29:05]

Description was changed from

==========
none


none

BUG=
==========

to

==========
Mark children of an SVG root as needing paint invalidation checking on resize.

BUG=649760
==========

[chrishtr, 2016-10-05 22:31:28]

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

[chrishtr, 2016-10-05 22:31:45]

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

[commit-bot: I haz the power, 2016-10-05 22:32:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chrishtr, 2016-10-05 22:40:40]

Also marked two tests as passing that were failing for the same reason as the
example I added.

[chrishtr, 2016-10-05 22:40:56]

chrishtr@chromium.org changed reviewers:
+ wkorman@chromium.org

[chrishtr, 2016-10-05 22:41:17]

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

[commit-bot: I haz the power, 2016-10-05 22:42:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[wkorman, 2016-10-05 23:00:36]

lgtm

Thanks, that's great if this fixes those other cases! Was hoping to find a more
focused solution than the additions I had to
LayoutBoxModelObject::invalidateTreeIfNeeded().

[commit-bot: I haz the power, 2016-10-06 00:00:48]

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

[commit-bot: I haz the power, 2016-10-06 00:00:49]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[fs, 2016-10-06 10:51:49]

lgtm

https://codereview.chromium.org/2397003002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/paint/invalidation/resources/text-based-repaint.js
(right):

https://codereview.chromium.org/2397003002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/paint/invalidation/resources/text-based-repaint.js:16:
window.expectedObjectInvalidations = [];
Maybe document that using this part of the framework requires testharness.js

Could also make sense to wrap the push onto this list in a function if you think
this is a mechanism that'll be used more in the future? Just a thought, feel
free to disregard.

https://codereview.chromium.org/2397003002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/paint/invalidation/resources/text-based-repaint.js:61:
assert_true(objectNameSet.has(objectName), "Expected object to be invalidated,
but was not: '" + objectName + "'");
Nit: ...but it was not

?

https://codereview.chromium.org/2397003002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/paint/invalidation/svg/resize-svg-invalidate-children.html
(right):

https://codereview.chromium.org/2397003002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/paint/invalidation/svg/resize-svg-invalidate-children.html:18:
<svg xmlns:xlink="http://www.w3.org/1999/xlink" version="1.1"  viewBox="0 0 200
200" xml:space="preserve">
Nit: xmlns:xlink, version and xml:space don't add anything, so can be removed.

https://codereview.chromium.org/2397003002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp (right):

https://codereview.chromium.org/2397003002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:160: if
(m_isLayoutSizeChanged)
I think this predicate may trigger over-invalidation when the SVG is fixed size,
but a descendant has relative lengths. Could you add a test for that case too?
(I've been trying to pick apart the selfNeedsLayout cases for this layout
object, so I'd like to keep dependencies spelled out if possible.)

[chrishtr, 2016-10-06 16:52:04]

https://codereview.chromium.org/2397003002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/paint/invalidation/resources/text-based-repaint.js
(right):

https://codereview.chromium.org/2397003002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/paint/invalidation/resources/text-based-repaint.js:16:
window.expectedObjectInvalidations = [];
On 2016/10/06 at 10:51:49, fs wrote:
> Maybe document that using this part of the framework requires testharness.js

Done.

> 
> Could also make sense to wrap the push onto this list in a function if you
think this is a mechanism that'll be used more in the future? Just a thought,
feel free to disregard.

Prefer to keep it simple for now.

https://codereview.chromium.org/2397003002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/paint/invalidation/resources/text-based-repaint.js:61:
assert_true(objectNameSet.has(objectName), "Expected object to be invalidated,
but was not: '" + objectName + "'");
On 2016/10/06 at 10:51:49, fs wrote:
> Nit: ...but it was not
> 
> ?

Done.

https://codereview.chromium.org/2397003002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/paint/invalidation/svg/resize-svg-invalidate-children.html
(right):

https://codereview.chromium.org/2397003002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/paint/invalidation/svg/resize-svg-invalidate-children.html:18:
<svg xmlns:xlink="http://www.w3.org/1999/xlink" version="1.1"  viewBox="0 0 200
200" xml:space="preserve">
On 2016/10/06 at 10:51:49, fs wrote:
> Nit: xmlns:xlink, version and xml:space don't add anything, so can be removed.

Done.

[chrishtr, 2016-10-06 16:52:07]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2016-10-06 16:52:07]

The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com,
wkorman@chromium.org
Link to the patchset: https://codereview.chromium.org/2397003002/#ps80001
(title: "none")

[commit-bot: I haz the power, 2016-10-06 16:52:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chrishtr, 2016-10-06 18:16:06]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2016-10-06 18:16:06]

The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com,
wkorman@chromium.org
Link to the patchset: https://codereview.chromium.org/2397003002/#ps100001
(title: "none")

[commit-bot: I haz the power, 2016-10-06 18:16:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-06 19:58:25]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-10-06 20:01:46]

Description was changed from

==========
Mark children of an SVG root as needing paint invalidation checking on resize.

BUG=649760
==========

to

==========
Mark children of an SVG root as needing paint invalidation checking on resize.

BUG=649760

Committed: https://crrev.com/bd1529c0d0d4afe68e09b455203cc63d982e7b4d
Cr-Commit-Position: refs/heads/master@{#423638}
==========

[commit-bot: I haz the power, 2016-10-06 20:01:47]

Patchset 6 (id:??) landed as
https://crrev.com/bd1529c0d0d4afe68e09b455203cc63d982e7b4d
Cr-Commit-Position: refs/heads/master@{#423638}

[chrishtr, 2016-10-06 20:08:37]

https://codereview.chromium.org/2397003002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp (right):

https://codereview.chromium.org/2397003002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:160: if
(m_isLayoutSizeChanged)
On 2016/10/06 at 10:51:49, fs wrote:
> I think this predicate may trigger over-invalidation when the SVG is fixed
size, but a descendant has relative lengths. Could you add a test for that case
too? (I've been trying to pick apart the selfNeedsLayout cases for this layout
object, so I'd like to keep dependencies spelled out if possible.)

Sorry, I accidentally missed this comment.

Can you paste an example in a reply to this comment? I will turn it into a
layout test.

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

https://codereview.chromium.org/2397003002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp (right):

https://codereview.chromium.org/2397003002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:160: if
(m_isLayoutSizeChanged)
On 2016/10/06 at 20:08:37, chrishtr wrote:
> On 2016/10/06 at 10:51:49, fs wrote:
> > I think this predicate may trigger over-invalidation when the SVG is fixed
size, but a descendant has relative lengths. Could you add a test for that case
too? (I've been trying to pick apart the selfNeedsLayout cases for this layout
object, so I'd like to keep dependencies spelled out if possible.)
> 
> Sorry, I accidentally missed this comment.
> 
> Can you paste an example in a reply to this comment? I will turn it into a
layout test.

Something like resize-svg-invalidate-children.html, but with
width="<somenumber>" and height="<somenumber>" (maybe <somenumber>=100) on the
<svg>, and then width=100% or so on the <rect> ought to do it:

<svg viewBox="0 0 200 200" width="100" height="100">
  <rect width="100%" height="100"/>
</svg>

(and the rest the same.)