Invalidate text metrics when the <text> subtree is mutated

Invalidate text metrics when the <text> subtree is mutated

When the content of a text node is modified, we would only invalidate
positioning values and not text metrics. This would lead to incorrect or
inconsistent text metrics/fonts being used, which would lead to repaint
bugs and similar issues.
Make sure all mutations to the <text> subtree trigger text metrics re-
computation.

Also take this opportunity to move the definition of the
willBeDestroyed() method for slightly better grouping.

BUG=299497, 594058
Committed: https://crrev.com/f19c8c6644e933e57a20d518b6ef62e2358ac1ad
Cr-Commit-Position: refs/heads/master@{#386300}

[fs, 2016-04-06 14:41:38]

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

[pdr., 2016-04-06 23:31:40]

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

https://codereview.chromium.org/1865923002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/LayoutSVGText.cpp:133:
invalidatePositioningValues(LayoutInvalidationReason::ChildChanged);
setNeedsTextMetricsUpdate() requires that we mark the object for needing layout
too. This works because layout is invalidated in invalidatePositioningValues but
it'll be easy to forget that in the future. WDYT about folding the
setNeedsTextMetricsUpdate call into invalidatePositioningValues or having
setNeedsTextMetricsUpdate invalidate layout?

https://codereview.chromium.org/1865923002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/LayoutSVGText.cpp:134:
setNeedsTextMetricsUpdate();
No tests seem fail without the change, but I don't understand why we don't need
this call in subtreeChildWillBeRemoved too.

[fs, 2016-04-07 09:00:00]

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

https://codereview.chromium.org/1865923002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/LayoutSVGText.cpp:133:
invalidatePositioningValues(LayoutInvalidationReason::ChildChanged);
On 2016/04/06 at 23:31:40, pdr wrote:
> setNeedsTextMetricsUpdate() requires that we mark the object for needing
layout too. This works because layout is invalidated in
invalidatePositioningValues but it'll be easy to forget that in the future. WDYT
about folding the setNeedsTextMetricsUpdate call into
invalidatePositioningValues or having setNeedsTextMetricsUpdate invalidate
layout?

I didn't want to fold this call into invalidatePositioningValues, because the
positioning values needing an update does not imply that the metrics value do
(but the reverse does hold I think...) I'm kind of hoping that eventually we
don't need *Added, *Removed et.c, but can just call (a tweaked version of)
invalidatePositioningValues directly instead.

https://codereview.chromium.org/1865923002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/LayoutSVGText.cpp:134:
setNeedsTextMetricsUpdate();
On 2016/04/06 at 23:31:40, pdr wrote:
> No tests seem fail without the change, but I don't understand why we don't
need this call in subtreeChildWillBeRemoved too.

I think we do need it in subtreeChildWillBeRemoved too - and I added it
https://codereview.chromium.org/1861013003 (which is where I first noticed that
I had fixed this issue "by accident"...) I've yet to come up with a test to show
that though, but I'll look into that (I think it needs involve whitespace
collapsing, so before the metric/attribute resolution split it probably wouldn't
reproduce because both were sort of computed regardless.)

[pdr., 2016-04-07 22:46:12]

Overcommunicating because I don't want to keep you waiting. When you said "I'll
look into that", is that something you plan to do in this review?

[fs, 2016-04-08 08:07:03]

On 2016/04/07 at 22:46:12, pdr wrote:
> Overcommunicating because I don't want to keep you waiting. When you said
"I'll look into that", is that something you plan to do in this review?

No. Primarily because I don't think I'll be able to observe a problem before
metrics and layout attributes are untangled
(https://codereview.chromium.org/1861013003 - in which I added the flag being
set where I think it'll be needed.) That being said, I don't really want to
block something because of a (so far only) theoretical problem.

[fs, 2016-04-08 13:23:20]

On 2016/04/08 at 08:07:03, fs wrote:
> On 2016/04/07 at 22:46:12, pdr wrote:
> > Overcommunicating because I don't want to keep you waiting. When you said
"I'll look into that", is that something you plan to do in this review?
> 
> No. Primarily because I don't think I'll be able to observe a problem before
metrics and layout attributes are untangled
(https://codereview.chromium.org/1861013003 - in which I added the flag being
set where I think it'll be needed.) That being said, I don't really want to
block something because of a (so far only) theoretical problem.

Test done and uploaded here: https://codereview.chromium.org/1872513005

It passes with and without this CL, but fails with
https://codereview.chromium.org/1861013003 with the call to
setNeedsTextMetricsUpdate() removed from
LayoutSVGText::subtreeChildWillBeRemoved().

[pdr., 2016-04-08 22:37:22]

On 2016/04/08 at 13:23:20, fs wrote:
> On 2016/04/08 at 08:07:03, fs wrote:
> > On 2016/04/07 at 22:46:12, pdr wrote:
> > > Overcommunicating because I don't want to keep you waiting. When you said
"I'll look into that", is that something you plan to do in this review?
> > 
> > No. Primarily because I don't think I'll be able to observe a problem before
metrics and layout attributes are untangled
(https://codereview.chromium.org/1861013003 - in which I added the flag being
set where I think it'll be needed.) That being said, I don't really want to
block something because of a (so far only) theoretical problem.
> 
> Test done and uploaded here: https://codereview.chromium.org/1872513005
> 
> It passes with and without this CL, but fails with
https://codereview.chromium.org/1861013003 with the call to
setNeedsTextMetricsUpdate() removed from
LayoutSVGText::subtreeChildWillBeRemoved().

LGTM

[fs, 2016-04-09 12:52:37]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2016-04-09 12:52:48]

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

[commit-bot: I haz the power, 2016-04-09 16:39:08]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-04-09 16:40:53]

Description was changed from

==========
Invalidate text metrics when the <text> subtree is mutated

When the content of a text node is modified, we would only invalidate
positioning values and not text metrics. This would lead to incorrect or
inconsistent text metrics/fonts being used, which would lead to repaint
bugs and similar issues.
Make sure all mutations to the <text> subtree trigger text metrics re-
computation.

Also take this opportunity to move the definition of the
willBeDestroyed() method for slightly better grouping.

BUG=299497, 594058
==========

to

==========
Invalidate text metrics when the <text> subtree is mutated

When the content of a text node is modified, we would only invalidate
positioning values and not text metrics. This would lead to incorrect or
inconsistent text metrics/fonts being used, which would lead to repaint
bugs and similar issues.
Make sure all mutations to the <text> subtree trigger text metrics re-
computation.

Also take this opportunity to move the definition of the
willBeDestroyed() method for slightly better grouping.

BUG=299497, 594058

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

[commit-bot: I haz the power, 2016-04-09 16:40:55]

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