Kill the killswitch
kkaefer - April 24, 2009 - 21:27
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | javascript |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
| Issue tags: | Needs Documentation |
Jump to:
Description
The 'killswitch' in JavaScript is a pre-jQuery relic. If the browser doesn't support the features we query for, the jQuery will fail horribly anyway. The percentage of browsers not supporting jQuery (but supporting JavaScript) is too low to really be taken serious.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| killswitch-1.patch | 1.93 KB | Idle | Failed: Failed to apply patch. | View details | Re-test |
#1
#2
Looks good to me but it would be good to have a JavaScript wizard confirm this too.
#3
Looks good to me. I'm all for getting rid of the killswitch.
However, we'll need to be sure to add this removal to the module/theme upgrade documentation.
Also, kkaefer, what's going on with the document.cookie stuff in there? Are we moving from Drupal.settings over to a cookie-based system?
Other than the cookie question, this looks good to me.
#4
The cookie is created without explicit domain, that means the cookie will be created under the current domain in the URL. It looks good, but there's an issue here that I would like to mention now that this snippet is being review.
The cookie domain in settings.php can be set to a higher level of the domain being used for the site, and that allows sharing the cookies between subdomains of the same domain. But this cookie "has_js" cannot be shared because it will exist a different version for each subdomain.
If the global $cookie_domain was in Drupal.settings, then it could be used here to create this cookie under the same exact scope the rest of the cookies are created by Drupal.
#5
This patch is not about the cookie domain at all and doesn't change anything about that. It has been there before and it is still there.
#6
Code looks good and I tried it and everything works as expected
#7
Yep, it is time, sorry IE4. It's nice that the introduction of behaviors made this patch so much smaller that it would've been in D5 days. Second this RTBC.
#8
Great, thanks for all the reviews folks! I've committed this to HEAD. Marking needs work, as this needs to be reflected in the upgrade documentation, as I'm pretty sure checking for Drupal.jsEnabled is considered a best practice in contrib js files.
Also, markus_petrux's comment at #4 sounds like it's worthy of further discussion. I didn't block this patch for that since kkaefer is right, and it's just moving existing lines of code around, but Markus, could you start another issue to discuss moving this to the Drupal.settings array?
#9
#10
@webchick #8: #445098: Provide a consistent method to generate cookies
I hope that helps.
#11
I'm assigning this to myself to add it to the module/theme upgrade guides
#12
I've added information to:
* Module upgrade guide http://drupal.org/node/224333#no-jsenabled
* Theme upgrade guide (same text) http://drupal.org/update/theme/6/7#no-jsenabled
* Categorical module upgrade page http://drupal.org/node/394070 (I marked it as "unknown" as to Coder warning/upgrade status).
If someone could review the text there, that would probably be good... if the text on those pages is OK, it is probably time (I think) to mark this issue as fixed.
#13
#14
The last submitted patch failed testing.
#15
Maybe the bot will ignore it this time...
#16
Looks good
#17
Automatically closed -- issue fixed for 2 weeks with no activity.