There are at least two pages where zipcar.com breaks: 1. Go to zipcar.com. The Flash video on the homepage does not load properly. 2. You will need to have a zipcar membership to go to the second page. Log in, and go to reserve a car (for me, it is http://www.zipcar.com/sf/reservations/search-by-time). A calendar icon should appear next to the date fields, and when you click on the clock icon a drop-down list of times should appear. Neither of these occur. I traced the changelist that breaks this to 79114. The code that makes 2. work doesn't get called because of a bug in the jQuery plugin loading code. On a working browser I see that req.callReady (line 1602) in requireplugins-jquery-1.4.3.js gets called at least twice, in which the second time s.isDone is true. Right now on ToT req.callReady gets called only once, when s.isDone is false.
<rdar://problem/9482755>
Changelist listing: http://trac.webkit.org/changeset/79114/trunk
Frowns. Assigning to myself so this doesn't get lost. Feel free to steal.
This is an evangelism issue. requirejs is using a non-compliant (and obsolete) technique to force async script download, but in-order execution. Support for this was removed in change 79114. The HTML5-compliant technique was implemented in change 81198 to address bug 50115. The requirejs project needs to be updated to use async=false and zipcar.com needs to use the new version. Alternatively zipcar.com could use LABjs, which is where requirejs copied the old idea from. LABjs already supports the new technique. I can reach out to the requirejs project. I'm not sure what to do about zipcar.
Looking at the code in the requirejs project, it looks like they already support the new technique. We just need zipcar to update to a newer version of requirejs.
Is it possible to do site-specific hacks for sites that use the old version of the library?
(In reply to comment #6) > Is it possible to do site-specific hacks for sites that use the old version of the library? I'm not sure how the site-specific hacks work. Can you point me to an example?
Here's an example of removal of an obsolete site specific hack: http://trac.webkit.org/changeset/68683
Created attachment 94724 [details] Patch
Comment on attachment 94724 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94724&action=review > Source/WebCore/html/HTMLScriptElement.cpp:91 > + if (!settings->needsSiteSpecificQuirks()) > + return false; Do we have a sense for how many sites use an old version of this library? I wonder if we should restrict this behavior to only certain hosts. That's more likely to let us remove the quirk in the future because we can test the individual sites on the quirks list.
(In reply to comment #10) > (From update of attachment 94724 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=94724&action=review > > > Source/WebCore/html/HTMLScriptElement.cpp:91 > > + if (!settings->needsSiteSpecificQuirks()) > > + return false; > > Do we have a sense for how many sites use an old version of this library? I wonder if we should restrict this behavior to only certain hosts. That's more likely to let us remove the quirk in the future because we can test the individual sites on the quirks list. I'd prefer that as well. I'm not sure how to get a thorough list of users though. I scanned the Chrome bug reports and the only one that sounded like this issue was another report about Zipcar. I looked at the requirejs history. The now-broken behavior was introduced 6/29/10 in 0.12.0. It was fixed to be HTML5-compliant on 11/13/10 in 0.15.0. Zipcar is running 0.14.5. For the record, they're now on 0.24.0 and there have been 4 releases with the fix in it.
Is it somehow possible to inject a script into the main world that checks the version of requirejs?
(In reply to comment #12) > Is it somehow possible to inject a script into the main world that checks the version of requirejs? This might not be possible to do in a way that doesn't require a script to be injected into every document, come to think of it. Anyway, just a thought.
(In reply to comment #12) > Is it somehow possible to inject a script into the main world that checks the version of requirejs? Evaluating "window.require.version" will give the answer. Is that an acceptable thing to do when we spot a script/cache type?
Comment on attachment 94724 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94724&action=review >>> Source/WebCore/html/HTMLScriptElement.cpp:91 >>> + return false; >> >> Do we have a sense for how many sites use an old version of this library? I wonder if we should restrict this behavior to only certain hosts. That's more likely to let us remove the quirk in the future because we can test the individual sites on the quirks list. > > I'd prefer that as well. I'm not sure how to get a thorough list of users though. I scanned the Chrome bug reports and the only one that sounded like this issue was another report about Zipcar. > > I looked at the requirejs history. The now-broken behavior was introduced 6/29/10 in 0.12.0. It was fixed to be HTML5-compliant on 11/13/10 in 0.15.0. Zipcar is running 0.14.5. For the record, they're now on 0.24.0 and there have been 4 releases with the fix in it. I think it might be less risky to make this specific to just the one site for now. If other popular sites are affected, we will learn soon enough.
Comment on attachment 94724 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94724&action=review >>>> Source/WebCore/html/HTMLScriptElement.cpp:91 >>>> + return false; >>> >>> Do we have a sense for how many sites use an old version of this library? I wonder if we should restrict this behavior to only certain hosts. That's more likely to let us remove the quirk in the future because we can test the individual sites on the quirks list. >> >> I'd prefer that as well. I'm not sure how to get a thorough list of users though. I scanned the Chrome bug reports and the only one that sounded like this issue was another report about Zipcar. >> >> I looked at the requirejs history. The now-broken behavior was introduced 6/29/10 in 0.12.0. It was fixed to be HTML5-compliant on 11/13/10 in 0.15.0. Zipcar is running 0.14.5. For the record, they're now on 0.24.0 and there have been 4 releases with the fix in it. > > I think it might be less risky to make this specific to just the one site for now. If other popular sites are affected, we will learn soon enough. I’m not sure it really matters whether it’s site-specific. But we need to either review- or review+ to keep this moving along! > Source/WebCore/html/HTMLScriptElement.cpp:93 > + if (element->getAttribute(typeAttr) != "script/cache") Could use fastGetAttribute here.
I agree with Maciej - we can just do it for zipcar for now. We can add or remove domains in the future as we learn more, or sites get updated.
Created attachment 94831 [details] Patch
Comment on attachment 94831 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94831&action=review > Source/WebCore/html/HTMLScriptElement.cpp:90 > + if (url.host() != "www.zipcar.com") equalsIgnoringCase
Created attachment 94833 [details] Patch for landing
Comment on attachment 94833 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=94833&action=review > Source/WebCore/html/HTMLScriptElement.cpp:85 > +static bool needsOldRequirejsQuirk(HTMLScriptElement* element) One other tweak to consider for performance’s sake. We should have the most likely check to fail be the first one in the function. Thus I suggest checking "script/cache" first, and only then checking the host. Unless the host check is already super-fast.
Comment on attachment 94833 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=94833&action=review >> Source/WebCore/html/HTMLScriptElement.cpp:85 >> +static bool needsOldRequirejsQuirk(HTMLScriptElement* element) > > One other tweak to consider for performance’s sake. We should have the most likely check to fail be the first one in the function. Thus I suggest checking "script/cache" first, and only then checking the host. Unless the host check is already super-fast. Sorry, I mean the cheapest check that is will fail on almost every script element, *not* the most likely one to fail.
Created attachment 94859 [details] Patch for landing
Thanks, James!
Comment on attachment 94859 [details] Patch for landing Clearing flags on attachment: 94859 Committed r87361: <http://trac.webkit.org/changeset/87361>
All reviewed patches have been landed. Closing bug.