Bug 20372 - HTML/SVGScriptElement need to share code
Summary: HTML/SVGScriptElement need to share code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
: 15363 19391 (view as bug list)
Depends on:
Blocks:
 
Reported: 2008-08-13 12:00 PDT by Nikolas Zimmermann
Modified: 2008-10-03 01:06 PDT (History)
2 users (show)

See Also:


Attachments
Patch 1/2 - v1: Refactor HTMLScriptElement code in common base class (38.35 KB, patch)
2008-08-13 12:08 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch 1/2 - v2: Refactor HTMLScriptElement code in common base class (53.28 KB, patch)
2008-08-13 18:42 PDT, Nikolas Zimmermann
eric: review+
Details | Formatted Diff | Diff
Patch 2/2 - v1: Let SVGScriptElement use the new ScriptElement base class (39.50 KB, patch)
2008-09-15 18:07 PDT, Nikolas Zimmermann
eric: review-
Details | Formatted Diff | Diff
Patch 2/2 - v2: Let SVGScriptElement use the new ScriptElement base class (49.47 KB, patch)
2008-09-19 20:00 PDT, Nikolas Zimmermann
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2008-08-13 12:00:08 PDT
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.
Comment 1 Nikolas Zimmermann 2008-08-13 12:03:04 PDT
*** Bug 19391 has been marked as a duplicate of this bug. ***
Comment 2 Nikolas Zimmermann 2008-08-13 12:03:07 PDT
*** Bug 15363 has been marked as a duplicate of this bug. ***
Comment 3 Nikolas Zimmermann 2008-08-13 12:08:31 PDT
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.
Comment 4 Nikolas Zimmermann 2008-08-13 18:42:43 PDT
Created attachment 22784 [details]
Patch 1/2 - v2: Refactor HTMLScriptElement code in common base class

Updated version, resolving the MI issues Eric & Maciej noted.
Comment 5 Eric Seidel (no email) 2008-08-13 19:00:42 PDT
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!
Comment 6 Nikolas Zimmermann 2008-08-13 19:23:12 PDT
Patch 1/2 landed in r35744.
Comment 7 Nikolas Zimmermann 2008-09-15 18:07:56 PDT
Created attachment 23454 [details]
Patch 2/2 - v1: Let SVGScriptElement use the new ScriptElement base class
Comment 8 Eric Seidel (no email) 2008-09-15 18:30:37 PDT
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.
Comment 9 Nikolas Zimmermann 2008-09-15 18:50:48 PDT
(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 :-)
Comment 10 Nikolas Zimmermann 2008-09-19 20:00:31 PDT
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.
Comment 11 Antti Koivisto 2008-09-19 20:30:54 PDT
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.
Comment 12 Nikolas Zimmermann 2008-09-19 20:36:30 PDT
Thanks for the review! Landed in r36699.
Comment 13 Darin Fisher (:fishd, Google) 2008-10-03 01:06:23 PDT
FYI, I'm pretty sure this caused a serious crash.  See bug 21329.