Allow the 'javascript_always_use_jquery' variable to be configured via the Performance page

Thanks for getting this started - however I think the wording needs some more reviews and some work:

1. To be grammatically correct the checkbox should say "Load" rather than "Loading". And it shouldn't end in a period.
2. I think the "sites and module authors" wording doesn't fit here - this is just aimed at a single site owner, right?
3. This doesn't seem to be warning people of the risks in any way. I think we do need to do that, but in as compact a way as possible...

My tentative suggestion would be to put this under the existing "Bandwidth Optimization" fieldset (I think it's closely related) with something like this for the wording:

Checkbox label: "Skip loading jQuery when the system detects it will not be used"
Checkbox description: "Use with caution. The detection will be unreliable if the site includes JavaScript directly via <script> tags in the theme or in content."

However this needs some review.

Note that in the above I flipped the meaning of the checkbox also - because for all the other checkboxes on the page you click on them to improve performance, so I think this one should behave that way too.

Thanks for working on it.

The checkbox label was not changed to 'Skip loading jQuery when the system detects it will not be used', because flipping the meaning of the checkbox will force flip the variable value.

That's not true, though. There doesn't have to be a one-to-one mapping between checkbox values and variable values - there could just be a form submit handler that saves the opposite of the submitted checkbox value into the variable.

Thanks @yoroy. I wonder if you have any thoughts on my suggestion from #5 since I think it meets most of your criteria:

My tentative suggestion would be to put this under the existing "Bandwidth Optimization" fieldset (I think it's closely related) with something like this for the wording:

Checkbox label: "Skip loading jQuery when the system detects it will not be used"
Checkbox description: "Use with caution. The detection will be unreliable if the site includes JavaScript directly via <script> tags in the theme or in content."

(possibly with "The detection will be unreliable if the site includes..." changed to "The detection will not work if the site includes..." based on your feedback above, or even "This can break sites that include...")

No, the detection should actually work fine when individual JavaScript files are attached - see the tests: http://cgit.drupalcode.org/drupal/tree/modules/simpletest/tests/common.t...

The only scenario where it's not expected to work is if the Drupal API is bypassed entirely (for example, JavaScript added via direct <script> tags).

I think this could use work for a few small things though:

1. The <em> tags should be outside the translatable string rather than inside it. And they should probably get a dedicated class (similar to how the permission page warning has a class; that one is <em class="permission-warning">).
2. Rather than "‹script›" it should probably use the actual HTML angle brackets, i.e. <script>.
3. Although not absolutely critical, it would be nice to have a very simple automated test to ensure this can be set and unset correctly via the UI (given that we have special code to make the checkbox value the opposite of the variable value).

I think the new wording looks good by the way; I originally put "when the system detects" in there as a way of hinting that it's by definition not 100% reliable, but I suppose the new, louder warning in the description will be noticeable enough for that.

That looks good, but:

+    $this->assertFalse(variable_get('javascript_always_use_jquery', TRUE), 'When the checkbox is selected: 
.....
+    $this->assertTrue(variable_get('javascript_always_use_jquery', FALSE), 'When the checkbox is not selected: 

I see what you're trying to do here but I think it's confusing to use different default values in different places. It's probably better not to use default values at all; that way we can directly assert that the variable is actually set via the UI in the first place.

We could do it like the attached, where I also cleaned up a few minor code style and grammar/wording issues.

I tested this manually and it works great. The only changes I made were minor and to the tests + code comments, so I'm going to mark this RTBC.

One other thought I had:

+  $form['bandwidth_optimization']['javascript_always_use_jquery'] = array(
+    '#type' => 'checkbox',
+    '#title' => t('Skip loading jQuery when it will not be used'),
+    '#default_value' => !variable_get('javascript_always_use_jquery', TRUE),

It's a bit confusing to have the checkbox form element name be the exact same as the variable name (since they mean opposite things). However if we changed that we'd also have to add extra code to make sure system_settings_form_submit() doesn't create a variable in the database for the checkbox name. I think it's probably not a big deal the way it is so I left that the way it is for now.

I will assign this to @stefan.r (Drupal 7 Product Manager who did not work on this patch) for final review, since this is definitely a product-related issue. Hopefully it's possible to get this into 7.50, but we'll see. There are string changes here (which we generally like to do earlier in the release cycle) but they are only new strings, so it's not as bad, and I think there are other issues we're still considering for this release that add strings too.

If this gets in, I can create a quick change record pointing out that this is now configurable via the UI - for a description it will mostly just point to the older change record at https://www.drupal.org/node/2462717 though.

And now with periods at the ends of sentences....

Having discussed this a bit on IRC with @yoroy, this probably needs a bit more text, explaining when and why to enable this (not actual documentation, just enough information so that people can make a decision about checking the box or not). Perhaps we'll also want to clarify this is an advanced option (as in, non-expert users should leave it untouched).

Any suggestions on what to add? We could put something like "If you don't know whether your site includes JavaScript using those methods, don't enable this setting" at the end of the description (to encourage less advanced users from turning it on) but I'm not sure if it really adds much to what's already there.

Despite #11, I'd also argue for putting this in its own fieldset (or renaming the current one and rewriting the "external resources..." bit). It seems this is intended as an overall front-end performance optimization, rather than mainly being about "Bandwidth optimization".

Hm, really? To me this seems exactly like bandwidth optimization, nothing else. (We are automatically leaving out the jQuery library when it isn't used, which reduces the size and/or number of JavaScript file requests.)

---

Something else visible in the screenshot is that this new checkbox label doesn't have a period at the end, but the existing ones in the fieldset do. However, checkbox labels elsewhere on this page and elsewhere in Drupal tend not to end with a period (although some do; overall Drupal is pretty inconsistent here). I tend to think it's better without a period so it's worth leaving as is, but it could go either way given the general inconsistency here.

OK, I added the period by editing the patch file :)

There are several ideas for wording changes above; I think it would probably be a good idea to get more feedback from @yoroy here since he helped write/approve the earlier wording.

Regarding the "and related JavaScript libraries" part, I think the only others included in that are jquery.once and misc/drupal.js, which are pretty low-level and based heavily on jQuery itself. So I'm not sure it's worth mentioning that, at least not as part of the checkbox label (perhaps in a more detailed description though). However could do something more general like "Skip loading JavaScript libraries on pages where they won't be used" I suppose (since mentioning jQuery itself is somewhat of a technical detail).