Bug 39026 – Recognize async attribute on script tags

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Tony Gentilcore]

Recognize async attribute on script tags.

[Tony Gentilcore]

Created attachment 55895 [details]
Patch

[Sam Weinig]

Comment on attachment 55895 [details]
Patch

This doesn't actually implement async behavior. Do you plan on adding that? It is also not true that this is not testable as the async property is now detectable.

[Tony Gentilcore]

Yes. I have a follow-up patch almost ready that implements defer and async. But I thought it would be good to break this out into a separate patch.

But you are right that in the interest of detectability, they should be landed at about the same time. Unfortunately detectability was already broken for defer.

I'll ping this review once the actual implementation is up for review.

[Tony Gentilcore]

(In reply to comment #3)
> Yes. I have a follow-up patch almost ready that implements defer and async. But I thought it would be good to break this out into a separate patch.
> 
> But you are right that in the interest of detectability, they should be landed at about the same time. Unfortunately detectability was already broken for defer.
> 
> I'll ping this review once the actual implementation is up for review.

I've added the full patch at https://bugs.webkit.org/show_bug.cgi?id=20710

It is a little over 20k. Let me know if you think it is reasonable to resurrect this patch to split it up or if you think it needs to stay together.

[Tony Gentilcore]

Uploading a new patch with a test.

[Tony Gentilcore]

Created attachment 57612 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 57612 [details]
Patch

Seems at least the SVG part of this change is untested.

This test could be much simpler using a little document.write().  Since then you could have a function which writes a script tag with a specific ID and then returns the .async value.

testAsyncValue("ASYNC")

function testAsyncValue(async_value)
{
    var scriptId = "test" + testNumber;
    document.write("<script id='" + scriptId + "' src='ignored.js' async='" + async_value + "'></scr" + "ipt>");
    scriptElement = document.getElementById(scriptId);
    testNumber++;
    return scriptElement.async;
}

or something like that.  You can document.write to an iframe if that works nicer.

r- for lack of SVG testing.

[Tony Gentilcore]

Created attachment 57624 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 57624 [details]
Patch

We haven't yet shown you the wonderful world of script-tests it seems.

Your long mass of:
+<script>
+if (isParserInsertedScriptAsync("async") === true)
+    writePass("p1");
+</script>

Could be fixed by making isParserInsertedScriptAsync work on an iframe (newly created or otherwise), and then using:

shouldBeTrue("isParserInsertedScriptAsync('async')");

That will do all the beautiful "PASS", "FAIL" stuff for you.  script-tests are javascript files only.

You can see lots of examples in LayoutTests looking in the script-tests directory.  Sadly they only work (easily) for .html files, however you can use the scirpt-test scripts (fast/js/resources/js-test-pre.js and -post.js) in any file, including a .svg file.

[Tony Gentilcore]

Created attachment 57631 [details]
Patch

[Tony Gentilcore]

(In reply to comment #10)
> Created an attachment (id=57631) [details]
> Patch

(In reply to comment #9)
> (From update of attachment 57624 [details])
> We haven't yet shown you the wonderful world of script-tests it seems.
> 
> Your long mass of:
> +<script>
> +if (isParserInsertedScriptAsync("async") === true)
> +    writePass("p1");
> +</script>
> 
> Could be fixed by making isParserInsertedScriptAsync work on an iframe (newly created or otherwise), and then using:
> 
> shouldBeTrue("isParserInsertedScriptAsync('async')");
> 
> That will do all the beautiful "PASS", "FAIL" stuff for you.  script-tests are javascript files only.
> 
> You can see lots of examples in LayoutTests looking in the script-tests directory.  Sadly they only work (easily) for .html files, however you can use the scirpt-test scripts (fast/js/resources/js-test-pre.js and -post.js) in any file, including a .svg file.

Thanks for showing me the light. That is so much cleaner!

However, for the HTMLScriptElement test, I went back to actually writing the <script> tags in the source HTML rather than document.writing them. This was for two reasons:
 1 There seems to be a bug with:
    <script>
        document.write('<script id=foo></scri'+'pt>');
        document.getElementById('foo');
        document.write('<script id=bar></scri'+'pt>');
        document.getElementById('bar');
    </script>
    The first getElementById('foo') will succeed but the second getElementById('bar') will return NULL. I plan to file a bug and look into that separately. But it is orthogonal to this patch and there is no reason to complicate this LayoutTest with that issue.
 2 It turns out to be fewer lines overall and, IMHO, easier to read.

For the SVGScriptElement test, I just left it as-is because the test case is so simple. Let me know if you think I should try to use script-tests there.

[Tony Gentilcore]

(In reply to comment #11)
> (In reply to comment #10)
> > Created an attachment (id=57631) [details] [details]
> > Patch
> 
> (In reply to comment #9)
> > (From update of attachment 57624 [details] [details])
> > We haven't yet shown you the wonderful world of script-tests it seems.
> > 
> > Your long mass of:
> > +<script>
> > +if (isParserInsertedScriptAsync("async") === true)
> > +    writePass("p1");
> > +</script>
> > 
> > Could be fixed by making isParserInsertedScriptAsync work on an iframe (newly created or otherwise), and then using:
> > 
> > shouldBeTrue("isParserInsertedScriptAsync('async')");
> > 
> > That will do all the beautiful "PASS", "FAIL" stuff for you.  script-tests are javascript files only.
> > 
> > You can see lots of examples in LayoutTests looking in the script-tests directory.  Sadly they only work (easily) for .html files, however you can use the scirpt-test scripts (fast/js/resources/js-test-pre.js and -post.js) in any file, including a .svg file.
> 
> Thanks for showing me the light. That is so much cleaner!
> 
> However, for the HTMLScriptElement test, I went back to actually writing the <script> tags in the source HTML rather than document.writing them. This was for two reasons:
>  1 There seems to be a bug with:
>     <script>
>         document.write('<script id=foo></scri'+'pt>');
>         document.getElementById('foo');
>         document.write('<script id=bar></scri'+'pt>');
>         document.getElementById('bar');
>     </script>
>     The first getElementById('foo') will succeed but the second getElementById('bar') will return NULL. I plan to file a bug and look into that separately.

Since it doesn't block this bug, for posterity sake, I'm adding a link to the bug:
https://bugs.webkit.org/show_bug.cgi?id=40068

 But it is orthogonal to this patch and there is no reason to complicate this LayoutTest with that issue.
>  2 It turns out to be fewer lines overall and, IMHO, easier to read.
> 
> For the SVGScriptElement test, I just left it as-is because the test case is so simple. Let me know if you think I should try to use script-tests there.

[Eric Seidel (no email)]

Comment on attachment 57631 [details]
Patch

Lets do this after the HTML5 Parser is turned on (to minimize how much we're changing at once).  r- until then.

[Tony Gentilcore]

What do you think, is it okay to move forward with this now?

[Eric Seidel (no email)]

Yes.  https://bugs.webkit.org/show_bug.cgi?id=39891 is one tricky case that we may need to logic out first though.  (The test case inline has a couple errors, but I can post a fixed copy which reveals some of the problems in our current impl.)

[Adam Barth]

Comment on attachment 57631 [details]
Patch

Looks good.  We should land support for executing these soon too, otherwise people's feature detection scripts might get confused.

[Tony Gentilcore]

(In reply to comment #16)
> (From update of attachment 57631 [details])
> Looks good.  We should land support for executing these soon too, otherwise people's feature detection scripts might get confused.

Agreed.

I'll post separate patches for async and defer on Monday or Tuesday. Depending on whether Eric's fixes block them, we can probably get them landed this week.

[WebKit Commit Bot]

Comment on attachment 57631 [details]
Patch

Rejecting patch 57631 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Adam Barth', u'--force']" exit_code: 1
Last 500 characters of output:
om/ScriptElement.cpp
patching file WebCore/dom/ScriptElement.h
patching file WebCore/html/HTMLAttributeNames.in
Hunk #1 succeeded at 43 (offset 1 line).
patching file WebCore/html/HTMLScriptElement.cpp
patching file WebCore/html/HTMLScriptElement.h
patching file WebCore/html/HTMLScriptElement.idl
Hunk #1 FAILED at 24.
1 out of 1 hunk FAILED -- saving rejects to file WebCore/html/HTMLScriptElement.idl.rej
patching file WebCore/svg/SVGScriptElement.cpp
patching file WebCore/svg/SVGScriptElement.h

Full output: http://webkit-commit-queue.appspot.com/results/3336425

[Tony Gentilcore]

Created attachment 59198 [details]
Patch

[Tony Gentilcore]

Synced up.

[Adam Barth]

Comment on attachment 59198 [details]
Patch

There are a bunch of stray changes to WebKitTools/WinLauncher/WinLauncher.vcproj in your patch.

[Tony Gentilcore]

Created attachment 59199 [details]
Sync up and remove a stray diff

[Eric Seidel (no email)]

Comment on attachment 59198 [details]
Patch

WebCore/dom/ScriptElement.cpp:312
 +      return !m_scriptElement->sourceAttributeValue().isEmpty() && m_scriptElement->asyncAttributeValue();
I suspect that AtomicString has a bool operator, which probably uses isEmpty() under the covers, so you could probably just write this as "m_scriptElement->sourceAttributeValue()"

WebCore/html/HTMLScriptElement.cpp:159
 +      setAttribute(asyncAttr, async ? "" : 0);
The two sides of the ternary are different types.  Intentional?

WebKitTools/WinLauncher/WinLauncher.vcproj:215
 +  				AdditionalDependencies="comctl32.lib shlwapi.lib user32.lib ole32.lib oleaut32.lib WebKitGUID$(WebKitConfigSuffix).lib WebKit$(WebKitDLLConfigSuffix).lib"
Confused by the project changes?

[Tony Gentilcore]

(In reply to comment #23)
> (From update of attachment 59198 [details])
> WebCore/dom/ScriptElement.cpp:312
>  +      return !m_scriptElement->sourceAttributeValue().isEmpty() && m_scriptElement->asyncAttributeValue();
> I suspect that AtomicString has a bool operator, which probably uses isEmpty() under the covers, so you could probably just write this as "m_scriptElement->sourceAttributeValue()"

The ! operator checks isNull() rather than isEmpty().

> 
> WebCore/html/HTMLScriptElement.cpp:159
>  +      setAttribute(asyncAttr, async ? "" : 0);
> The two sides of the ternary are different types.  Intentional?
> 

This was just copied from setDefer(). Should I update them both to something else?

> WebKitTools/WinLauncher/WinLauncher.vcproj:215
>  +                  AdditionalDependencies="comctl32.lib shlwapi.lib user32.lib ole32.lib oleaut32.lib WebKitGUID$(WebKitConfigSuffix).lib WebKit$(WebKitDLLConfigSuffix).lib"
> Confused by the project changes?

Sorry for the noise. That was a stray diff that accidentally crept in during my sync. It is removed in the latest patch.

[Eric Seidel (no email)]

Comment on attachment 59199 [details]
Sync up and remove a stray diff

OK.

[WebKit Commit Bot]

Comment on attachment 59199 [details]
Sync up and remove a stray diff

Clearing flags on attachment: 59199

Committed r61518: <http://trac.webkit.org/changeset/61518>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.