[webkit-reviews] review denied: [Bug 54676] Make ScriptElement match the HTML5 spec : [Attachment 82876] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied James Simonsen
<simonjam at chromium.org>'s request for review:
Bug 54676: Make ScriptElement match the HTML5 spec
https://bugs.webkit.org/show_bug.cgi?id=54676

Attachment 82876: Patch
https://bugs.webkit.org/attachment.cgi?id=82876&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=82876&action=review

I don't think your changes to the layout tests are correct.  You're essentially
disabling these tests.

> LayoutTests/ChangeLog:22
> +	   * fast/dom/HTMLScriptElement/move-in-beforeload.html: Original
author says test was only meant to check for crashes. Beforeload is not
specified by HTML5. Test was modified to assume moved script wouldn't execute
because wasAlreadyStarted is set.

You should make sure that we don't execute the script.	Just removing the check
isn't sufficient.

> LayoutTests/ChangeLog:23
> +	   * fast/dom/script-clone-rerun-src.xhtml: According to HTML5 spec,
load should only fire after a script executes. Test was modified to match that.
The old broken behavior was that load fired on the cloned element, even though
it didn't execute.

What are you talking about?  This test was testing that load() is called
exactly once.

> LayoutTests/fast/dom/HTMLScriptElement/move-in-beforeload.html:21
> -    }, false);
> -    s.addEventListener("error", function() {
> -	   testFailed("error event should not fire.")
> -    }, false);
> -    s.addEventListener("load", function() {
>	   testPassed("");
> -	   if (window.layoutTestController)
> -	       layoutTestController.notifyDone();
>      }, false);

I don't think removing all these checks is great.  If we think the script
shouldn't execute, then we should verify that it doesn't.

> LayoutTests/fast/dom/script-clone-rerun-src.xhtml:43
> +loaded();
> +

Why do we need to call loaded here?  It seems that we're breaking the test.  r-
for this.