[webkit-reviews] review denied: [Bug 20372] HTML/SVGScriptElement need to share code : [Attachment 23454] Patch 2/2 - v1: Let SVGScriptElement use the new ScriptElement base class

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 20372: HTML/SVGScriptElement need to share code
https://bugs.webkit.org/show_bug.cgi?id=20372

Attachment 23454: Patch 2/2 - v1: Let SVGScriptElement use the new
ScriptElement base class
https://bugs.webkit.org/attachment.cgi?id=23454&action=edit

------- Additional Comments from Eric Seidel <eric at webkit.org>
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.