Don't dispatch '(SVG)load' to ancestors of <svg:image> after doc. 'load'

Don't dispatch '(SVG)load' to ancestors of <svg:image> after doc. 'load'

When an <svg:image> is inserted after document load has been dispatched,
we don't want to dispatch 'load' again to any ancestor 'svg' roots (inner
or outer). Hence only dispatch an event to the element itself in this
condition.

Take the opportunity to clean up SVGElement::sendSVGLoadEventIfPossible
a bit, by splitting it into send-to-self and send-to-self-and-ancestors
methods (most callers call the former). Also unravel the control-flow of
the ancestors-version a bit compared to the old version.

BUG=372946
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177814

[fs, 2014-07-08 18:11:00]

(Split+cleanup might hide the fix more than necessary - I can split those two
out if required.)

[pdr., 2014-07-08 18:57:34]

This is fantastic!

LGTM on the idea in general. Lets do one more round though.

https://codereview.chromium.org/375923002/diff/1/Source/core/svg/SVGElement.cpp
File Source/core/svg/SVGElement.cpp (right):

https://codereview.chromium.org/375923002/diff/1/Source/core/svg/SVGElement.c...
Source/core/svg/SVGElement.cpp:866: RefPtrWillBeRawPtr<Element> parent =
currentTarget->parentOrShadowHostElement();
I think we can use recursion to clean this up.

Instead of using a while loop, what if
sendSVGLoadEventToSelfAndAncestorChainIfPossible were to just call
sendSVGLoadEventIfPossible on itself and then call
sendSVGLoadEventToSelfAndAncestorChainIfPossible on it's parent if it's an
svgelement. This may fix a bug as well in the case where the load event removes
the element from the tree: the current code saves off the parent which may not
be the parent any longer after the load event is sent.

[fs, 2014-07-09 07:39:33]

https://codereview.chromium.org/375923002/diff/1/Source/core/svg/SVGElement.cpp
File Source/core/svg/SVGElement.cpp (right):

https://codereview.chromium.org/375923002/diff/1/Source/core/svg/SVGElement.c...
Source/core/svg/SVGElement.cpp:866: RefPtrWillBeRawPtr<Element> parent =
currentTarget->parentOrShadowHostElement();
On 2014/07/08 18:57:34, pdr wrote:
> I think we can use recursion to clean this up.
> 
> Instead of using a while loop, what if
> sendSVGLoadEventToSelfAndAncestorChainIfPossible were to just call
> sendSVGLoadEventIfPossible on itself and then call
> sendSVGLoadEventToSelfAndAncestorChainIfPossible on it's parent if it's an
> svgelement. This may fix a bug as well in the case where the load event
removes
> the element from the tree: the current code saves off the parent which may not
> be the parent any longer after the load event is sent.

Are you thinking of the case where the element is removed from the document, or
has it's parent changed? Given a structure like:

<svg>
  <svg></svg>
  <svg>
    <image onload="this.remove()" ...>
  </svg>
</svg>

It looks like 'load' ought to still be dispatched on the parent of <image>, and
if the remove() is replaced with a move to the prev. sibling of the parent, then
using the real parent would mean dispatching twice to the new parent, and never
to the old one? I'll see if I can make tests for those cases...

[fs, 2014-07-09 10:58:49]

https://codereview.chromium.org/375923002/diff/1/Source/core/svg/SVGElement.cpp
File Source/core/svg/SVGElement.cpp (right):

https://codereview.chromium.org/375923002/diff/1/Source/core/svg/SVGElement.c...
Source/core/svg/SVGElement.cpp:866: RefPtrWillBeRawPtr<Element> parent =
currentTarget->parentOrShadowHostElement();
On 2014/07/09 07:39:32, fs wrote:
> On 2014/07/08 18:57:34, pdr wrote:
> > I think we can use recursion to clean this up.
> > 
> > Instead of using a while loop, what if
> > sendSVGLoadEventToSelfAndAncestorChainIfPossible were to just call
> > sendSVGLoadEventIfPossible on itself and then call
> > sendSVGLoadEventToSelfAndAncestorChainIfPossible on it's parent if it's an
> > svgelement. This may fix a bug as well in the case where the load event
> removes
> > the element from the tree: the current code saves off the parent which may
not
> > be the parent any longer after the load event is sent.
> 
> Are you thinking of the case where the element is removed from the document,
or
> has it's parent changed? Given a structure like:
> 
> <svg>
>   <svg></svg>
>   <svg>
>     <image onload="this.remove()" ...>
>   </svg>
> </svg>
> 
> It looks like 'load' ought to still be dispatched on the parent of <image>,
and
> if the remove() is replaced with a move to the prev. sibling of the parent,
then
> using the real parent would mean dispatching twice to the new parent, and
never
> to the old one? I'll see if I can make tests for those cases...

Uploaded two new TCs (load-image-re{mov,parent}ed-in-onload.html) to illustrate
the cases I were thinking of (in-case they matched the ones you're thinking of).
Appears to pass both before and after this CL (Gecko didn't seem to agree
though, but I couldn't quite grasp what was going on there...)

[pdr., 2014-07-10 03:48:15]

LGTM

After reading the spec on this I realized my questions about the edge cases of
onload are out of scope for this patch. I like the approach you took here.

https://codereview.chromium.org/375923002/diff/20001/LayoutTests/svg/custom/i...
File LayoutTests/svg/custom/image-svgload-after-docload.html (right):

https://codereview.chromium.org/375923002/diff/20001/LayoutTests/svg/custom/i...
LayoutTests/svg/custom/image-svgload-after-docload.html:18:
setTimeout(function() { t.done(); }, 0);
I don't think this setTimeout is needed, nor the assert_true(true).

https://codereview.chromium.org/375923002/diff/20001/Source/core/svg/SVGEleme...
File Source/core/svg/SVGElement.cpp (right):

https://codereview.chromium.org/375923002/diff/20001/Source/core/svg/SVGEleme...
Source/core/svg/SVGElement.cpp:867:
static_pointer_cast<SVGElement>(parent)->sendSVGLoadEventToSelfAndAncestorChainIfPossible();
Should this be
toSVGElement(parent)->sendSVGLoadEventToSelfAndAncestorChainIfPossible?

[fs, 2014-07-10 08:32:50]

https://codereview.chromium.org/375923002/diff/20001/LayoutTests/svg/custom/i...
File LayoutTests/svg/custom/image-svgload-after-docload.html (right):

https://codereview.chromium.org/375923002/diff/20001/LayoutTests/svg/custom/i...
LayoutTests/svg/custom/image-svgload-after-docload.html:18:
setTimeout(function() { t.done(); }, 0);
On 2014/07/10 03:48:15, pdr wrote:
> I don't think this setTimeout is needed, nor the assert_true(true).

The setTimeout is needed to provide a "window" for the <svg> load to be fired
(which will be after the <image> load, but still within the same "task"). (I.e.
without the setTimeout the test will pass without the fix.)

The assert_true can certainly be dropped.

https://codereview.chromium.org/375923002/diff/20001/Source/core/svg/SVGEleme...
File Source/core/svg/SVGElement.cpp (right):

https://codereview.chromium.org/375923002/diff/20001/Source/core/svg/SVGEleme...
Source/core/svg/SVGElement.cpp:867:
static_pointer_cast<SVGElement>(parent)->sendSVGLoadEventToSelfAndAncestorChainIfPossible();
On 2014/07/10 03:48:15, pdr wrote:
> Should this be
> toSVGElement(parent)->sendSVGLoadEventToSelfAndAncestorChainIfPossible?

Yeah, could be. Changed.

[fs, 2014-07-10 08:48:21]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2014-07-10 08:49:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/fs@opera.com/375923002/40001

[commit-bot: I haz the power, 2014-07-10 09:25:06]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)

[commit-bot: I haz the power, 2014-07-10 10:28:53]

Change committed as 177814