Bug 39026 - Recognize async attribute on script tags
Summary: Recognize async attribute on script tags
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 20710
  Show dependency treegraph
 
Reported: 2010-05-12 14:08 PDT by Tony Gentilcore
Modified: 2010-06-20 18:05 PDT (History)
5 users (show)

See Also:


Attachments
Patch (2.96 KB, patch)
2010-05-12 14:11 PDT, Tony Gentilcore
no flags Details | Formatted Diff | Diff
Patch (11.50 KB, patch)
2010-06-01 18:40 PDT, Tony Gentilcore
no flags Details | Formatted Diff | Diff
Patch (13.21 KB, patch)
2010-06-01 23:20 PDT, Tony Gentilcore
no flags Details | Formatted Diff | Diff
Patch (12.71 KB, patch)
2010-06-02 00:26 PDT, Tony Gentilcore
no flags Details | Formatted Diff | Diff
Patch (15.38 KB, patch)
2010-06-20 10:43 PDT, Tony Gentilcore
no flags Details | Formatted Diff | Diff
Sync up and remove a stray diff (12.68 KB, patch)
2010-06-20 10:57 PDT, Tony Gentilcore
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Gentilcore 2010-05-12 14:08:19 PDT
Recognize async attribute on script tags.
Comment 1 Tony Gentilcore 2010-05-12 14:11:16 PDT
Created attachment 55895 [details]
Patch
Comment 2 Sam Weinig 2010-05-12 18:31:36 PDT
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.
Comment 3 Tony Gentilcore 2010-05-12 18:38:02 PDT
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.
Comment 4 Tony Gentilcore 2010-05-20 19:38:21 PDT
(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.
Comment 5 Tony Gentilcore 2010-06-01 18:35:20 PDT
Uploading a new patch with a test.
Comment 6 Tony Gentilcore 2010-06-01 18:40:57 PDT
Created attachment 57612 [details]
Patch
Comment 7 Eric Seidel (no email) 2010-06-01 20:58:19 PDT
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.
Comment 8 Tony Gentilcore 2010-06-01 23:20:26 PDT
Created attachment 57624 [details]
Patch
Comment 9 Eric Seidel (no email) 2010-06-01 23:30:22 PDT
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.
Comment 10 Tony Gentilcore 2010-06-02 00:26:25 PDT
Created attachment 57631 [details]
Patch
Comment 11 Tony Gentilcore 2010-06-02 10:14:28 PDT
(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.
Comment 12 Tony Gentilcore 2010-06-02 14:22:12 PDT
(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.
Comment 13 Eric Seidel (no email) 2010-06-12 21:00:24 PDT
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.
Comment 14 Tony Gentilcore 2010-06-18 10:48:30 PDT
What do you think, is it okay to move forward with this now?
Comment 15 Eric Seidel (no email) 2010-06-18 10:50:08 PDT
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.)
Comment 16 Adam Barth 2010-06-19 16:58:48 PDT
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.
Comment 17 Tony Gentilcore 2010-06-19 17:09:18 PDT
(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.
Comment 18 WebKit Commit Bot 2010-06-19 17:21:19 PDT
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
Comment 19 Tony Gentilcore 2010-06-20 10:43:31 PDT
Created attachment 59198 [details]
Patch
Comment 20 Tony Gentilcore 2010-06-20 10:45:57 PDT
Synced up.
Comment 21 Adam Barth 2010-06-20 10:51:07 PDT
Comment on attachment 59198 [details]
Patch

There are a bunch of stray changes to WebKitTools/WinLauncher/WinLauncher.vcproj in your patch.
Comment 22 Tony Gentilcore 2010-06-20 10:57:56 PDT
Created attachment 59199 [details]
Sync up and remove a stray diff
Comment 23 Eric Seidel (no email) 2010-06-20 11:16:01 PDT
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?
Comment 24 Tony Gentilcore 2010-06-20 12:01:36 PDT
(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.
Comment 25 Eric Seidel (no email) 2010-06-20 16:36:42 PDT
Comment on attachment 59199 [details]
Sync up and remove a stray diff

OK.
Comment 26 WebKit Commit Bot 2010-06-20 18:05:27 PDT
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>
Comment 27 WebKit Commit Bot 2010-06-20 18:05:33 PDT
All reviewed patches have been landed.  Closing bug.