Bug 39026 - Recognize async attribute on script tags
: Recognize async attribute on script tags
Status: RESOLVED FIXED
: WebKit
WebCore JavaScript
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
: 20710
  Show dependency treegraph
 
Reported: 2010-05-12 14:08 PST by
Modified: 2010-06-20 18:05 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-05-12 14:08:19 PST
Recognize async attribute on script tags.
------- Comment #1 From 2010-05-12 14:11:16 PST -------
Created an attachment (id=55895) [details]
Patch
------- Comment #2 From 2010-05-12 18:31:36 PST -------
(From update of attachment 55895 [details])
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 From 2010-05-12 18:38:02 PST -------
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 From 2010-05-20 19:38:21 PST -------
(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 From 2010-06-01 18:35:20 PST -------
Uploading a new patch with a test.
------- Comment #6 From 2010-06-01 18:40:57 PST -------
Created an attachment (id=57612) [details]
Patch
------- Comment #7 From 2010-06-01 20:58:19 PST -------
(From update of attachment 57612 [details])
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 From 2010-06-01 23:20:26 PST -------
Created an attachment (id=57624) [details]
Patch
------- Comment #9 From 2010-06-01 23:30:22 PST -------
(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.
------- Comment #10 From 2010-06-02 00:26:25 PST -------
Created an attachment (id=57631) [details]
Patch
------- Comment #11 From 2010-06-02 10:14:28 PST -------
(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. 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 From 2010-06-02 14:22:12 PST -------
(In reply to comment #11)
> (In reply to comment #10)
> > Created an attachment (id=57631) [details] [details] [details]
> > Patch
> 
> (In reply to comment #9)
> > (From update of attachment 57624 [details] [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 From 2010-06-12 21:00:24 PST -------
(From update of attachment 57631 [details])
Lets do this after the HTML5 Parser is turned on (to minimize how much we're changing at once).  r- until then.
------- Comment #14 From 2010-06-18 10:48:30 PST -------
What do you think, is it okay to move forward with this now?
------- Comment #15 From 2010-06-18 10:50:08 PST -------
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 From 2010-06-19 16:58:48 PST -------
(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.
------- Comment #17 From 2010-06-19 17:09:18 PST -------
(In reply to comment #16)
> (From update of attachment 57631 [details] [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 From 2010-06-19 17:21:19 PST -------
(From update of attachment 57631 [details])
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 From 2010-06-20 10:43:31 PST -------
Created an attachment (id=59198) [details]
Patch
------- Comment #20 From 2010-06-20 10:45:57 PST -------
Synced up.
------- Comment #21 From 2010-06-20 10:51:07 PST -------
(From update of attachment 59198 [details])
There are a bunch of stray changes to WebKitTools/WinLauncher/WinLauncher.vcproj in your patch.
------- Comment #22 From 2010-06-20 10:57:56 PST -------
Created an attachment (id=59199) [details]
Sync up and remove a stray diff
------- Comment #23 From 2010-06-20 11:16:01 PST -------
(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()"

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 From 2010-06-20 12:01:36 PST -------
(In reply to comment #23)
> (From update of attachment 59198 [details] [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 From 2010-06-20 16:36:42 PST -------
(From update of attachment 59199 [details])
OK.
------- Comment #26 From 2010-06-20 18:05:27 PST -------
(From update of attachment 59199 [details])
Clearing flags on attachment: 59199

Committed r61518: <http://trac.webkit.org/changeset/61518>
------- Comment #27 From 2010-06-20 18:05:33 PST -------
All reviewed patches have been landed.  Closing bug.