Propagate resource-triggered repaints to text node children of <text>

Propagate resource-triggered repaints to text node children of <text>

When a paint server is modified, it is marked for layout and its clients
are marked for repaint. In the case of <text> (and <tspan> and other
text content elements), the text nodes (for which the actual painting is
performed) uses their parent (LayoutObject) to get the fill/stroke. This
means that if a repaint is requested for all clients of a resource, the
text nodes will not be notified.
Add and use a new paint invalidation reason for this case:
PaintInvalidationSVGResourceChange
Handle this reason in LayoutSVGText and LayoutSVGInline.
Generalize the ancestorHadPaintInvalidationForLocationChange
functionality, renaming it to forcedSubtreeInvalidationWithinContainer.

BUG=528159
Committed: https://crrev.com/bb9fb099862cd8adc8b9d5b1702fc7b7d1cd4a37
git-svn-id: svn://svn.chromium.org/blink/trunk@202280 bbb929c8-8fbe-4397-9dbb-9b2b20218538

[fs, 2015-09-10 10:41:12]

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

[fs, 2015-09-10 10:41:13]

https://codereview.chromium.org/1331053002/diff/1/Source/core/layout/svg/Layo...
File Source/core/layout/svg/LayoutSVGResourceContainer.cpp (right):

https://codereview.chromium.org/1331053002/diff/1/Source/core/layout/svg/Layo...
Source/core/layout/svg/LayoutSVGResourceContainer.cpp:174: // TODO(fs): Allowing
LayoutSVGInlineTexts to have should allow them to
Unless you can tell me up-front that this won't work out, I'll see if I can make
it work this way. I think it'll be a more involved fix though (so may be worse
for backporting if needed).

[pdr., 2015-09-11 06:22:04]

https://codereview.chromium.org/1331053002/diff/1/Source/core/layout/svg/Layo...
File Source/core/layout/svg/LayoutSVGResourceContainer.cpp (right):

https://codereview.chromium.org/1331053002/diff/1/Source/core/layout/svg/Layo...
Source/core/layout/svg/LayoutSVGResourceContainer.cpp:174: // TODO(fs): Allowing
LayoutSVGInlineTexts to have should allow them to
On 2015/09/10 at 10:41:13, fs wrote:
> Unless you can tell me up-front that this won't work out, I'll see if I can
make it work this way. I think it'll be a more involved fix though (so may be
worse for backporting if needed).

This seems reasonable to me.

Ultra nit: missing a word in the comment.

https://codereview.chromium.org/1331053002/diff/1/Source/core/layout/svg/Layo...
Source/core/layout/svg/LayoutSVGResourceContainer.cpp:178: reason =
PaintInvalidationLocationChange;
This is pragmatic but I worry it could confuse us in the future. WDYT about
adding a new invalidation reason for this case and adding the over-invalidation
infrastructure, similar to PaintInvalidationLocationChange? This suggestion may
be as much work as your larger fix above :/

If this is just for merging into M45 and M46, I'd be okay with just making this
change (as-is) directly on the branches.

[fs, 2015-09-11 16:32:16]

https://codereview.chromium.org/1331053002/diff/1/Source/core/layout/svg/Layo...
File Source/core/layout/svg/LayoutSVGResourceContainer.cpp (right):

https://codereview.chromium.org/1331053002/diff/1/Source/core/layout/svg/Layo...
Source/core/layout/svg/LayoutSVGResourceContainer.cpp:174: // TODO(fs): Allowing
LayoutSVGInlineTexts to have should allow them to
On 2015/09/11 06:22:04, pdr wrote:
> On 2015/09/10 at 10:41:13, fs wrote:
> > Unless you can tell me up-front that this won't work out, I'll see if I can
> make it work this way. I think it'll be a more involved fix though (so may be
> worse for backporting if needed).
> 
> This seems reasonable to me.
> 
> Ultra nit: missing a word in the comment.

I accidentally an entire word! Will fix if we end up actually landing this part.

https://codereview.chromium.org/1331053002/diff/1/Source/core/layout/svg/Layo...
Source/core/layout/svg/LayoutSVGResourceContainer.cpp:178: reason =
PaintInvalidationLocationChange;
On 2015/09/11 06:22:04, pdr wrote:
> This is pragmatic but I worry it could confuse us in the future. WDYT about
> adding a new invalidation reason for this case and adding the
over-invalidation
> infrastructure, similar to PaintInvalidationLocationChange? This suggestion
may
> be as much work as your larger fix above :/

Yes, I was pondering that, and decided against it because of the reason you
suggest below. It could be localized to LayoutSVGText/LayoutSVGInline, but yeah
quite possibly larger than the "real, proper fix"...
 
> If this is just for merging into M45 and M46, I'd be okay with just making
this
> change (as-is) directly on the branches.

TBH I'd feel a bit bad about backporting something "completely different", but
if you.think it's ok.

(Unfortunately I've been unable to get to this today =/, but I'll try to do it
on Monday...)

[pdr., 2015-09-11 18:03:06]

On 2015/09/11 at 16:32:16, fs wrote:
>
https://codereview.chromium.org/1331053002/diff/1/Source/core/layout/svg/Layo...
> File Source/core/layout/svg/LayoutSVGResourceContainer.cpp (right):
> 
>
https://codereview.chromium.org/1331053002/diff/1/Source/core/layout/svg/Layo...
> Source/core/layout/svg/LayoutSVGResourceContainer.cpp:174: // TODO(fs):
Allowing LayoutSVGInlineTexts to have should allow them to
> On 2015/09/11 06:22:04, pdr wrote:
> > On 2015/09/10 at 10:41:13, fs wrote:
> > > Unless you can tell me up-front that this won't work out, I'll see if I
can
> > make it work this way. I think it'll be a more involved fix though (so may
be
> > worse for backporting if needed).
> > 
> > This seems reasonable to me.
> > 
> > Ultra nit: missing a word in the comment.
> 
> I accidentally an entire word! Will fix if we end up actually landing this
part.
> 
>
https://codereview.chromium.org/1331053002/diff/1/Source/core/layout/svg/Layo...
> Source/core/layout/svg/LayoutSVGResourceContainer.cpp:178: reason =
PaintInvalidationLocationChange;
> On 2015/09/11 06:22:04, pdr wrote:
> > This is pragmatic but I worry it could confuse us in the future. WDYT about
> > adding a new invalidation reason for this case and adding the
over-invalidation
> > infrastructure, similar to PaintInvalidationLocationChange? This suggestion
may
> > be as much work as your larger fix above :/
> 
> Yes, I was pondering that, and decided against it because of the reason you
suggest below. It could be localized to LayoutSVGText/LayoutSVGInline, but yeah
quite possibly larger than the "real, proper fix"...
>  
> > If this is just for merging into M45 and M46, I'd be okay with just making
this
> > change (as-is) directly on the branches.
> 
> TBH I'd feel a bit bad about backporting something "completely different", but
if you.think it's ok.
> 
> (Unfortunately I've been unable to get to this today =/, but I'll try to do it
on Monday...)

Yeah, sorry about the slow reviews lately. Slimming paint regressions :/

Thinking about this more... I'm not sure this regression was bad enough to
necessitate a merge. Unless you think it needs one, lets just do the real fix
and have it filter out to stable in the regular release process.

[fs, 2015-09-14 13:38:09]

On 2015/09/11 18:03:06, pdr wrote:
> On 2015/09/11 at 16:32:16, fs wrote:
...
>
https://codereview.chromium.org/1331053002/diff/1/Source/core/layout/svg/Layo...
> > Source/core/layout/svg/LayoutSVGResourceContainer.cpp:178: reason =
> PaintInvalidationLocationChange;
> > On 2015/09/11 06:22:04, pdr wrote:
> > > This is pragmatic but I worry it could confuse us in the future. WDYT
about
> > > adding a new invalidation reason for this case and adding the
> over-invalidation
> > > infrastructure, similar to PaintInvalidationLocationChange? This
suggestion
> may
> > > be as much work as your larger fix above :/
> > 
> > Yes, I was pondering that, and decided against it because of the reason you
> suggest below. It could be localized to LayoutSVGText/LayoutSVGInline, but
yeah
> quite possibly larger than the "real, proper fix"...
> >  
> > > If this is just for merging into M45 and M46, I'd be okay with just making
> this
> > > change (as-is) directly on the branches.
> > 
> > TBH I'd feel a bit bad about backporting something "completely different",
but
> if you.think it's ok.
> > 
> > (Unfortunately I've been unable to get to this today =/, but I'll try to do
it
> on Monday...)
> 
> Yeah, sorry about the slow reviews lately. Slimming paint regressions :/
> 
> Thinking about this more... I'm not sure this regression was bad enough to
> necessitate a merge. Unless you think it needs one, lets just do the real fix
> and have it filter out to stable in the regular release process.

I suppose the "most severe" effects are limited to <pattern>s - and that's more
for cases like Noel's than the original bug (at least I'd think that mutating
paint servers isn't _that_ common).

Looking at doing it "The Right Way(TM)", I fairly quickly came to the conclusion
that that would be awfully complex what it was trying to achieve
(Element/SVGElement -> Node in many places with the problems that in turn
gives...), so I instead went with the "add a new invalidation reason"-version -
more code-dupe than I'd like, but... (I suppose I could've revised the TODO
instead of dropping it - we could/should handling this case as a style-change
since the properties in question inherit - that's the same type of fix that'd
allow us to get rid of the filter invalidation hack though.)

[pdr., 2015-09-15 02:33:38]

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

[pdr., 2015-09-15 02:33:39]

LGTM

https://codereview.chromium.org/1331053002/diff/40001/Source/core/layout/Pain...
File Source/core/layout/PaintInvalidationState.cpp (right):

https://codereview.chromium.org/1331053002/diff/40001/Source/core/layout/Pain...
Source/core/layout/PaintInvalidationState.cpp:56: // from our parents will just
move the whole invalidation container.
Can you update this comment?

https://codereview.chromium.org/1331053002/diff/40001/Source/core/layout/Pain...
File Source/core/layout/PaintInvalidationState.h (right):

https://codereview.chromium.org/1331053002/diff/40001/Source/core/layout/Pain...
Source/core/layout/PaintInvalidationState.h:39: bool forcedPaintInvalidation()
const { return m_forcedPaintInvalidation; }
Bikeshed: wdyt about changing this to reflect that it's for the subtree only?

Maybe something like forcedSubtreeInvalidation or
forcedSubtreeInvalidationWithinContainer?

[fs, 2015-09-15 08:44:35]

https://codereview.chromium.org/1331053002/diff/40001/Source/core/layout/Pain...
File Source/core/layout/PaintInvalidationState.cpp (right):

https://codereview.chromium.org/1331053002/diff/40001/Source/core/layout/Pain...
Source/core/layout/PaintInvalidationState.cpp:56: // from our parents will just
move the whole invalidation container.
On 2015/09/15 02:33:39, pdr wrote:
> Can you update this comment?

Done.

https://codereview.chromium.org/1331053002/diff/40001/Source/core/layout/Pain...
File Source/core/layout/PaintInvalidationState.h (right):

https://codereview.chromium.org/1331053002/diff/40001/Source/core/layout/Pain...
Source/core/layout/PaintInvalidationState.h:39: bool forcedPaintInvalidation()
const { return m_forcedPaintInvalidation; }
On 2015/09/15 02:33:39, pdr wrote:
> Bikeshed: wdyt about changing this to reflect that it's for the subtree only?
> 
> Maybe something like forcedSubtreeInvalidation or
> forcedSubtreeInvalidationWithinContainer?

Renamed to the latter.

[pdr., 2015-09-15 18:19:55]

The CQ bit was checked by pdr@chromium.org

[pdr., 2015-09-15 18:19:56]

lgtm

[commit-bot: I haz the power, 2015-09-15 18:20:14]

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

[commit-bot: I haz the power, 2015-09-15 18:28:14]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=202280

[commit-bot: I haz the power, 2015-09-23 12:47:13]

Patchset 4 (id:??) landed as
https://crrev.com/bb9fb099862cd8adc8b9d5b1702fc7b7d1cd4a37