Bug 20372 – HTML/SVGScriptElement need to share code

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Nikolas Zimmermann]

Solution: Create common base class ScriptElement (just like there's StyleElement for HTML/SVGStyleElement), refactor all code from HTMLScriptElement.

After that's landed, make SVGScriptElement use the new base class, as SVGScriptElement is really incomplete and buggy.

[Nikolas Zimmermann]

*** Bug 19391 has been marked as a duplicate of this bug. ***

[Nikolas Zimmermann]

*** Bug 15363 has been marked as a duplicate of this bug. ***

[Nikolas Zimmermann]

Created attachment 22778 [details]
Patch 1/2 - v1: Refactor HTMLScriptElement code in common base class

Refactor HTMLScriptElement code in ScriptElement. SVGScriptElement left nearly as is.

[Nikolas Zimmermann]

Created attachment 22784 [details]
Patch 1/2 - v2: Refactor HTMLScriptElement code in common base class

Updated version, resolving the MI issues Eric & Maciej noted.

[Eric Seidel (no email)]

Comment on attachment 22784 [details]
Patch 1/2 - v2: Refactor HTMLScriptElement code in common base class

We talked a bit over IRC.

I don't really like the name "ScriptElementData", and I'm not sure that "ScriptElement" shoudl be called an Element since it's not...

But I don't really have better suggestions for either name, so r=me.

Looking forward to part 2!

[Nikolas Zimmermann]

Patch 1/2 landed in r35744.

[Nikolas Zimmermann]

Created attachment 23454 [details]
Patch 2/2 - v1: Let SVGScriptElement use the new ScriptElement base class

[Eric Seidel (no email)]

Comment on attachment 23454 [details]
Patch 2/2 - v1: Let SVGScriptElement use the new ScriptElement base class

I'm not sure what "resied" means:
+    // m_createdByParser is never reset - always resied at the initial value set while parsing.

Maybe I'm missing something.  It's not clear to me that you're testing both cases of externalResourcesRequired with your test cases (and what about the case where exernalResourcesRequired is set from true to false while a load is still in progress?)

This seems a little confusing:
+bool SVGScriptElement::haveLoadedRequiredResources()
+{
+    return !externalResourcesRequiredBaseValue() || m_data.haveFiredLoadEvent();
+}

It returns "true" if it's sent a load event.  Is the "sentLoadEvent" reset if the src changes?  I don't see it ever reset.  Seems a second load event should be sent if the src for a script element is changed, no?

Seems that "haveFiredLoadEvent" should be set after the event was actually sent, no?

+    // Eventually send SVGLoad event now for the dynamically inserted script element
+    if (!externalResourcesRequiredBaseValue()) {
+        m_data.setHaveFiredLoadEvent(true);
+        sendSVGLoadEventIfPossible();
+    }

Or maybe it just shouldn't call the "Send if possible" method in this case, since it's always going to send, no?

This is a good patch, I'm just not fully convinced it's 100% correct.  Perhaps you can convince me via IRC, or by replying to my comments above.

[Nikolas Zimmermann]

(In reply to comment #8)
> (From update of attachment 23454 [details] [edit])
> I'm not sure what "resied" means:
> +    // m_createdByParser is never reset - always resied at the initial value
> set while parsing.
Should be "resides".
 
> Maybe I'm missing something.  It's not clear to me that you're testing both
> cases of externalResourcesRequired with your test cases (and what about the
> case where exernalResourcesRequired is set from true to false while a load is
> still in progress?)
Changing this attribute while a load is still in progress would cause the SVGLoad event to be not send at all. If externalResourcesRequired is set to false, the SVGLoad event is fired as soon as </script> is reached, or as soon as the element is inserted into the document. If it's set to true, we're supposed to wait until the script is loaded. So if you change this attribute while loading from true to false, the time where the SVGLoad event should have been sent, has passed. So we won't actually send it. Not sure if it's okay, the specification doesn't say a word about this.

Though this is really the most rare corner case I've ever thought about ;-)

> This seems a little confusing:
> +bool SVGScriptElement::haveLoadedRequiredResources()
> +{
> +    return !externalResourcesRequiredBaseValue() ||
> m_data.haveFiredLoadEvent();
> +}
> 
> It returns "true" if it's sent a load event.  Is the "sentLoadEvent" reset if
> the src changes?  I don't see it ever reset.  Seems a second load event should
> be sent if the src for a script element is changed, no?
It's just like the same check in SVGImageElement, which is your work ;-)
The firedLoad event is reset by ScriptElement::requestScript, which is called when src changes and the <script> element was not created by the parser (<script> elements created by the parser are not allowed to react to src attribute changes, only dynamically created ones are, that is well tested for both HTML/SVG).
 
> Seems that "haveFiredLoadEvent" should be set after the event was actually
> sent, no?
No, as sendSVGLoadEventIfPossible() checks haveLoadedRequiredResources() which in turn asks ScriptElement::haveFiredLoadEvent(). A bit confusing, but modelled just like the existing concept in HTMLImageLoader.

> 
> +    // Eventually send SVGLoad event now for the dynamically inserted script
> element
> +    if (!externalResourcesRequiredBaseValue()) {
> +        m_data.setHaveFiredLoadEvent(true);
> +        sendSVGLoadEventIfPossible();
> +    }
> 
> Or maybe it just shouldn't call the "Send if possible" method in this case,
> since it's always going to send, no?
No, the line above reads:

    if (m_data.createdByParser())
        return;

That's only called for dynamic script elements, as the comment says.

> 
> This is a good patch, I'm just not fully convinced it's 100% correct.  Perhaps
> you can convince me via IRC, or by replying to my comments above.

Thanks, I hope this is convincing enough :-)

[Nikolas Zimmermann]

Created attachment 23595 [details]
Patch 2/2 - v2: Let SVGScriptElement use the new ScriptElement base class

The patch hasn't changed, a new layout test has been added, covering the modification of 'externalResourcesRequired' of dynamic SVG script elements.

[Antti Koivisto]

Comment on attachment 23595 [details]
Patch 2/2 - v2: Let SVGScriptElement use the new ScriptElement base class

Looks good to me and I hear Eric greenlighted this in irc as well.

[Nikolas Zimmermann]

Thanks for the review! Landed in r36699.

[Darin Fisher (:fishd, Google)]

FYI, I'm pretty sure this caused a serious crash.  See bug 21329.