SVG: Items inserted into list should be made a tear-off.

SVG: Items inserted into list should be made a tear-off.

Before this patch, static SVGPropertyTearOff items inserted into a SVGListTearOff were not marked as a tear-off.
For such items, |svgAttributeChanged| was not dispatched to its context element, and view was not updated when its value changed.

This patch fixes the issue by converting a static tear-off inserted into a list to a dynamic tear-off.

Also this patch fixes an issue that a tearoff returned from replaceItem/removeItem was still attached to the list.

BUG=380546, 380176
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176284

[kouhei (in TOK), 2014-06-11 07:12:23]

I will add a test shortly.

[kouhei (in TOK), 2014-06-11 07:43:23]

Added LayoutTest. PTAL

[kouhei (in TOK), 2014-06-11 07:44:07]

fixed cc list

[Erik Dahlström (inactive), 2014-06-11 08:22:03]

What if you have a static tearoff, insert it to make it dynamic, then take it
out again? Does it properly go back to being static tearoff, or is it still
hooked up to the old contextElement?

https://codereview.chromium.org/321403004/diff/20001/LayoutTests/svg/dom/modi...
File LayoutTests/svg/dom/modify-inserted-listitem.html (right):

https://codereview.chromium.org/321403004/diff/20001/LayoutTests/svg/dom/modi...
LayoutTests/svg/dom/modify-inserted-listitem.html:4: <rect id="move" x="10"
y="10" width="10" height="10" fill="red"></rect>
Wouldn't the test be better if the red rect indicated the proper position, and
the green rect would cover it when moved to the proper place?

Essentially:

<rect id="ref" x="20" y="30" width="10" height="10" fill="red"></rect>
<rect id="move" x="10" y="10" width="10" height="10" fill="green"></rect>

[Erik Dahlström (inactive), 2014-06-11 08:31:32]

https://codereview.chromium.org/321403004/diff/20001/Source/core/svg/SVGPathS...
File Source/core/svg/SVGPathSegListTearOff.h (right):

https://codereview.chromium.org/321403004/diff/20001/Source/core/svg/SVGPathS...
Source/core/svg/SVGPathSegListTearOff.h:51:
newItem->setContextElement(contextElement);
Doesn't this need to set attributeName too? Like if you move a pathseglist from
an animateMotion element to a path element for example ("path" to "d"
attribute). Please add a test like that. Does it also need to update the
PropertyIsAnimValType flag?

[fs, 2014-06-11 08:42:23]

https://codereview.chromium.org/321403004/diff/20001/LayoutTests/svg/dom/modi...
File LayoutTests/svg/dom/modify-inserted-listitem.html (right):

https://codereview.chromium.org/321403004/diff/20001/LayoutTests/svg/dom/modi...
LayoutTests/svg/dom/modify-inserted-listitem.html:8: enablePixelTesting = true;
Could you make this a repaint test instead? (I think that should cover the
invalidation case sufficiently?)

https://codereview.chromium.org/321403004/diff/20001/Source/core/svg/properti...
File Source/core/svg/properties/SVGPropertyTearOff.h (right):

https://codereview.chromium.org/321403004/diff/20001/Source/core/svg/properti...
Source/core/svg/properties/SVGPropertyTearOff.h:86: void makeTearOff(SVGElement*
contextElement, const QualifiedName& attributeName)
I can see where "makeTearOff" comes from, but it may not be the most obvious
name. What are your thoughts about calling it "connectTo..." or "attachTo..." or
something like that?

[kouhei (in TOK), 2014-06-12 04:25:28]

Thanks for your comments!

On 2014/06/11 08:22:03, Erik Dahlström wrote:
> What if you have a static tearoff, insert it to make it dynamic, then take it
> out again? Does it properly go back to being static tearoff, or is it still
> hooked up to the old contextElement?
Yes that would be still hooked up to the old contextElement, so changes to them
would still trigger extra repaints. However, the list would not have the tearoff
value as an element, so rendered result would be valid. (i.e. the value on the
detached tearoff is not used.)

This is an another bug and should be fixed, but I'd like to wait until the
oilpan transition. The fix would require tracking all attached tear-offs from
list tear-off, and it will complicate the property hierarchy life time
management even more.

https://codereview.chromium.org/321403004/diff/20001/LayoutTests/svg/dom/modi...
File LayoutTests/svg/dom/modify-inserted-listitem.html (right):

https://codereview.chromium.org/321403004/diff/20001/LayoutTests/svg/dom/modi...
LayoutTests/svg/dom/modify-inserted-listitem.html:4: <rect id="move" x="10"
y="10" width="10" height="10" fill="red"></rect>
On 2014/06/11 08:22:03, Erik Dahlström wrote:
> Wouldn't the test be better if the red rect indicated the proper position, and
> the green rect would cover it when moved to the proper place?
> 
> Essentially:
> 
> <rect id="ref" x="20" y="30" width="10" height="10" fill="red"></rect>
> <rect id="move" x="10" y="10" width="10" height="10" fill="green"></rect>

Done.

https://codereview.chromium.org/321403004/diff/20001/LayoutTests/svg/dom/modi...
LayoutTests/svg/dom/modify-inserted-listitem.html:8: enablePixelTesting = true;
On 2014/06/11 08:42:23, fs wrote:
> Could you make this a repaint test instead? (I think that should cover the
> invalidation case sufficiently?)

Done.

https://codereview.chromium.org/321403004/diff/20001/Source/core/svg/SVGPathS...
File Source/core/svg/SVGPathSegListTearOff.h (right):

https://codereview.chromium.org/321403004/diff/20001/Source/core/svg/SVGPathS...
Source/core/svg/SVGPathSegListTearOff.h:51:
newItem->setContextElement(contextElement);
On 2014/06/11 08:31:32, Erik Dahlström wrote:
> Doesn't this need to set attributeName too? Like if you move a pathseglist
from
> an animateMotion element to a path element for example ("path" to "d"
> attribute). Please add a test like that. Does it also need to update the
> PropertyIsAnimValType flag?

I think this is correct. SVGAnimateMotionElement do not expose DOM (both in
SVG{1.1,2} spec and in Blink impl), so the only attributeName is dAttr.

https://codereview.chromium.org/321403004/diff/20001/Source/core/svg/properti...
File Source/core/svg/properties/SVGPropertyTearOff.h (right):

https://codereview.chromium.org/321403004/diff/20001/Source/core/svg/properti...
Source/core/svg/properties/SVGPropertyTearOff.h:86: void makeTearOff(SVGElement*
contextElement, const QualifiedName& attributeName)
On 2014/06/11 08:42:23, fs wrote:
> I can see where "makeTearOff" comes from, but it may not be the most obvious
> name. What are your thoughts about calling it "connectTo..." or "attachTo..."
or
> something like that?
Yes this isn't a very good name. Trying "attachToSVGElementAttribute". Name
suggestions welcome :)

[fs, 2014-06-12 08:32:23]

LGTM, thanks.

https://codereview.chromium.org/321403004/diff/20001/Source/core/svg/properti...
File Source/core/svg/properties/SVGPropertyTearOff.h (right):

https://codereview.chromium.org/321403004/diff/20001/Source/core/svg/properti...
Source/core/svg/properties/SVGPropertyTearOff.h:86: void makeTearOff(SVGElement*
contextElement, const QualifiedName& attributeName)
On 2014/06/12 04:25:28, kouhei wrote:
> On 2014/06/11 08:42:23, fs wrote:
> > I can see where "makeTearOff" comes from, but it may not be the most obvious
> > name. What are your thoughts about calling it "connectTo..." or
"attachTo..."
> or
> > something like that?
> Yes this isn't a very good name. Trying "attachToSVGElementAttribute". Name
> suggestions welcome :)

Naming is hard, but this new shade is certainly an improvement =)

[Erik Dahlström (inactive), 2014-06-12 08:47:04]

https://codereview.chromium.org/321403004/diff/20001/Source/core/svg/SVGPathS...
File Source/core/svg/SVGPathSegListTearOff.h (right):

https://codereview.chromium.org/321403004/diff/20001/Source/core/svg/SVGPathS...
Source/core/svg/SVGPathSegListTearOff.h:51:
newItem->setContextElement(contextElement);
On 2014/06/12 04:25:28, kouhei wrote:
> On 2014/06/11 08:31:32, Erik Dahlström wrote:
> > Doesn't this need to set attributeName too? Like if you move a pathseglist
> from
> > an animateMotion element to a path element for example ("path" to "d"
> > attribute). Please add a test like that. Does it also need to update the
> > PropertyIsAnimValType flag?
> 
> I think this is correct. SVGAnimateMotionElement do not expose DOM (both in
> SVG{1.1,2} spec and in Blink impl), so the only attributeName is dAttr.

Fair enough for pathseglist then, but the other lists share the same issue.
Let's say you move an SVGLength from an x to an y attribute for example.

[kouhei (in TOK), 2014-06-16 00:23:45]

PTAL.

On 2014/06/12 08:47:04, Erik Dahlström wrote:
>
https://codereview.chromium.org/321403004/diff/20001/Source/core/svg/SVGPathS...
> File Source/core/svg/SVGPathSegListTearOff.h (right):
> 
>
https://codereview.chromium.org/321403004/diff/20001/Source/core/svg/SVGPathS...
> Source/core/svg/SVGPathSegListTearOff.h:51:
> newItem->setContextElement(contextElement);
> On 2014/06/12 04:25:28, kouhei wrote:
> > On 2014/06/11 08:31:32, Erik Dahlström wrote:
> > > Doesn't this need to set attributeName too? Like if you move a pathseglist
> > from
> > > an animateMotion element to a path element for example ("path" to "d"
> > > attribute). Please add a test like that. Does it also need to update the
> > > PropertyIsAnimValType flag?
> > 
> > I think this is correct. SVGAnimateMotionElement do not expose DOM (both in
> > SVG{1.1,2} spec and in Blink impl), so the only attributeName is dAttr.
> 
> Fair enough for pathseglist then, but the other lists share the same issue.
> Let's say you move an SVGLength from an x to an y attribute for example.

Added tests: modify-transferred-listitem{,-different-attr}.html

[Erik Dahlström (inactive), 2014-06-16 08:37:34]

On 2014/06/16 00:23:45, kouhei wrote:
> PTAL.
> 
> On 2014/06/12 08:47:04, Erik Dahlström wrote:
> >
>
https://codereview.chromium.org/321403004/diff/20001/Source/core/svg/SVGPathS...
> > File Source/core/svg/SVGPathSegListTearOff.h (right):
> > 
> >
>
https://codereview.chromium.org/321403004/diff/20001/Source/core/svg/SVGPathS...
> > Source/core/svg/SVGPathSegListTearOff.h:51:
> > newItem->setContextElement(contextElement);
> > On 2014/06/12 04:25:28, kouhei wrote:
> > > On 2014/06/11 08:31:32, Erik Dahlström wrote:
> > > > Doesn't this need to set attributeName too? Like if you move a
pathseglist
> > > from
> > > > an animateMotion element to a path element for example ("path" to "d"
> > > > attribute). Please add a test like that. Does it also need to update the
> > > > PropertyIsAnimValType flag?
> > > 
> > > I think this is correct. SVGAnimateMotionElement do not expose DOM (both
in
> > > SVG{1.1,2} spec and in Blink impl), so the only attributeName is dAttr.
> > 
> > Fair enough for pathseglist then, but the other lists share the same issue.
> > Let's say you move an SVGLength from an x to an y attribute for example.
> 
> Added tests: modify-transferred-listitem{,-different-attr}.html

LGTM with nit:

All the tests still seem to show red when passing. My expectation is that if the
test passes there's no red visible.

[kouhei (in TOK), 2014-06-16 23:24:47]

On 2014/06/16 08:37:34, Erik Dahlström wrote:
> On 2014/06/16 00:23:45, kouhei wrote:
> > PTAL.
> > 
> > On 2014/06/12 08:47:04, Erik Dahlström wrote:
> > >
> >
>
https://codereview.chromium.org/321403004/diff/20001/Source/core/svg/SVGPathS...
> > > File Source/core/svg/SVGPathSegListTearOff.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/321403004/diff/20001/Source/core/svg/SVGPathS...
> > > Source/core/svg/SVGPathSegListTearOff.h:51:
> > > newItem->setContextElement(contextElement);
> > > On 2014/06/12 04:25:28, kouhei wrote:
> > > > On 2014/06/11 08:31:32, Erik Dahlström wrote:
> > > > > Doesn't this need to set attributeName too? Like if you move a
> pathseglist
> > > > from
> > > > > an animateMotion element to a path element for example ("path" to "d"
> > > > > attribute). Please add a test like that. Does it also need to update
the
> > > > > PropertyIsAnimValType flag?
> > > > 
> > > > I think this is correct. SVGAnimateMotionElement do not expose DOM (both
> in
> > > > SVG{1.1,2} spec and in Blink impl), so the only attributeName is dAttr.
> > > 
> > > Fair enough for pathseglist then, but the other lists share the same
issue.
> > > Let's say you move an SVGLength from an x to an y attribute for example.
> > 
> > Added tests: modify-transferred-listitem{,-different-attr}.html
> 
> LGTM with nit:
> 
> All the tests still seem to show red when passing. My expectation is that if
the
> test passes there's no red visible.
Done.
This was because I have set expectation shape to green and result shape to red.
I swapped their colors.

[kouhei (in TOK), 2014-06-16 23:24:58]

The CQ bit was checked by kouhei@chromium.org

[kouhei (in TOK), 2014-06-16 23:32:55]

Thanks for review!

[kouhei (in TOK), 2014-06-16 23:33:00]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2014-06-16 23:33:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kouhei@chromium.org/321403004/140001

[commit-bot: I haz the power, 2014-06-17 01:36:33]

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

[commit-bot: I haz the power, 2014-06-17 01:36:34]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/12199)

[kouhei (in TOK), 2014-06-17 01:48:37]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2014-06-17 01:49:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kouhei@chromium.org/321403004/160001

[commit-bot: I haz the power, 2014-06-17 01:49:13]

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

[kouhei (in TOK), 2014-06-17 01:49:14]

On 2014/06/17 01:36:34, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   win_blink_rel on tryserver.blink
>
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/12199)
Win/Mac failed from different fonts. Trying NeedsRebaseline.

[commit-bot: I haz the power, 2014-06-17 01:49:14]

Failed to apply patch for LayoutTests/TestExpectations:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file LayoutTests/TestExpectations
  Hunk #1 FAILED at 1124.
  1 out of 1 hunk FAILED -- saving rejects to file
LayoutTests/TestExpectations.rej

Patch:       LayoutTests/TestExpectations
Index: LayoutTests/TestExpectations
diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
index
dae9ce4aa7d5bb436a31fa675a87c15da8aa3aa9..c038495000308a2541a5a30c50a11638951f5029
100644
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -1124,3 +1124,5 @@ crbug.com/384396 [ Win ]
battery-status/page-visibility.html [ Pass Failure Time
 
 # Made substantially flakier in r176213
 crbug.com/354447 inspector-protocol/stylesheet-tracking-restart.html [ Pass
Failure Timeout ]
+
+crbug.com/380176 [ Win Mac ]
svg/repaint/modify-transferred-listitem-different-attr.html [ NeedsRebaseline ]

[kouhei (in TOK), 2014-06-17 02:00:05]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2014-06-17 02:00:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kouhei@chromium.org/321403004/180001

[kouhei (in TOK), 2014-06-17 02:00:26]

The CQ bit was unchecked by kouhei@chromium.org

[commit-bot: I haz the power, 2014-06-17 02:00:39]

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

[commit-bot: I haz the power, 2014-06-17 02:00:40]

Failed to apply patch for LayoutTests/TestExpectations:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file LayoutTests/TestExpectations
  Hunk #1 FAILED at 261.
  Hunk #2 FAILED at 1124.
  2 out of 2 hunks FAILED -- saving rejects to file
LayoutTests/TestExpectations.rej

Patch:       LayoutTests/TestExpectations
Index: LayoutTests/TestExpectations
diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
index
dae9ce4aa7d5bb436a31fa675a87c15da8aa3aa9..650ea9d01dcf79fc9a0749d84fea9613c714b94f
100644
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -261,8 +261,8 @@ crbug.com/378919 [ Linux ]
fast/forms/calendar-picker/calendar-picker-appearance
 crbug.com/378919 [ Linux ]
fast/forms/calendar-picker/calendar-picker-appearance-required-ar.html [
NeedsManualRebaseline ]
 crbug.com/378919 [ Linux Android ] fast/forms/date/date-appearance-l10n.html [
NeedsManualRebaseline ]
 crbug.com/378919 [ Linux Android ]
fast/forms/datetimelocal/datetimelocal-appearance-basic.html [
NeedsManualRebaseline ]
-crbug.com/378919 [ Linux ]
fast/forms/datetimelocal/datetimelocal-appearance-l10n.html [
NeedsManualRebaseline ]
-crbug.com/378919 [ Linux ] fast/forms/month/month-appearance-l10n.html [
NeedsManualRebaseline ]
+crbug.com/378919 fast/forms/datetimelocal/datetimelocal-appearance-l10n.html [
NeedsManualRebaseline ]
+crbug.com/378919 fast/forms/month/month-appearance-l10n.html [
NeedsManualRebaseline ]
 crbug.com/378919 [ Linux ]
fast/forms/suggestion-picker/datetimelocal-suggestion-picker-appearance-locale-hebrew.html
[ NeedsManualRebaseline ]
 crbug.com/378919 [ Linux Android ]
fast/forms/suggestion-picker/datetimelocal-suggestion-picker-appearance-rtl.html
[ NeedsManualRebaseline ]
 crbug.com/378919 [ Linux Android ]
fast/forms/suggestion-picker/datetimelocal-suggestion-picker-appearance-with-scroll-bar.html
[ NeedsManualRebaseline ]
@@ -1124,3 +1124,10 @@ crbug.com/384396 [ Win ]
battery-status/page-visibility.html [ Pass Failure Time
 
 # Made substantially flakier in r176213
 crbug.com/354447 inspector-protocol/stylesheet-tracking-restart.html [ Pass
Failure Timeout ]
+
+Bug(eae) fast/text/justify-ideograph-complex.html [ NeedsRebaseline ]
+Bug(eae) fast/text/justify-ideograph-simple.html [ NeedsRebaseline ]
+Bug(eae) media/track/track-cue-rendering-vertical.html [ NeedsRebaseline ]
+Bug(eae) virtual/antialiasedtext/fast/text/justify-ideograph-complex.html [
NeedsRebaseline ]
+Bug(eae) virtual/antialiasedtext/fast/text/justify-ideograph-simple.html [
NeedsRebaseline ]
+

[kouhei (in TOK), 2014-06-17 02:01:32]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2014-06-17 02:02:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kouhei@chromium.org/321403004/200001

[commit-bot: I haz the power, 2014-06-17 03:51:57]

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

[commit-bot: I haz the power, 2014-06-17 03:51:59]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/12227)

[kouhei (in TOK), 2014-06-17 04:06:44]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2014-06-17 04:07:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kouhei@chromium.org/321403004/220001

[kouhei (in TOK), 2014-06-17 04:58:43]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2014-06-17 04:58:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kouhei@chromium.org/321403004/240001

[commit-bot: I haz the power, 2014-06-17 06:05:21]

Change committed as 176284