Toolbar module's setting of padding-top css on html body is non-responsive

.

Well that works very well.

There has been so much frustration with this, yet the patch is that simple :p

The binding isn't done in the right place though. We'll end up binding it a lot of times for nothing when ajax is involved. please move that code to Drupal.toolbar.init

Looked a bit more into it, because resize is used a lot it's worth spending some time on optimizations.

1. Can you add some lines to make sure we change the body padding value when that value actually changes? Right now we're trashing things and it makes the browser recalculate a lot of stuff it shouldn't have to.
2. Can you cache $('body') in a variable outside the resize callback? Don't need to query it at every resize.
3. And use 'padding-top' instead of 'paddingTop', saves jQuery from doing some work as well.

After that we're good to go. Ideal would be to add debouncing but it's a bit out of scope I think.

wow long overdue review, sorry.

1. +++ b/modules/toolbar/toolbar.js
   @@ -2,6 +2,8 @@
   +Drupal.toolbar.body = $('body');
   

   You can make it a local $body variable. There is no need to expose this in the Drupal object.

2. +++ b/modules/toolbar/toolbar.js
   @@ -35,6 +37,13 @@ Drupal.toolbar.init = function() {
   +    if (parseInt(Drupal.toolbar.body.css('padding-top')) !== Drupal.toolbar.height()) {
   

   The issue with this is that we're still hitting the DOM to get the size of body padding. We can know this value. I would rather create a new local variable and cache that padding value, updating it and the padding-top value only when it changes.

   // in toolbar.init().
   var paddingTop = parseInt($body.css('padding-top'), 10);
   
   // Inside the resize handler.
   var height = Drupal.toolbar.height()
   if (paddingTop !== height) {
     paddingTop = height;
     $body.css('padding-top', paddingTop);
   }
   

   That way we limit our interactions with the DOM to the minimum.

It goes a little bit beyond a simple change but it makes a lot of sense.

It'll need to be profiled to check whether the additional changes have some visible (to the profiler, not user) perf improvements compared to the naive approach. I'm pretty sure it'll be the case because we get rid of a call to .css() in all browsers.

Provided the profiling shows improvement big +1 from me.

few nitpicks:

1. +++ b/modules/toolbar/toolbar.js
   @@ -2,6 +2,11 @@
   +//since some selectors are being used in a resize handler lets cache them.
   

   space and capital here: // Since…

2. +++ b/modules/toolbar/toolbar.js
   @@ -2,6 +2,11 @@
   +var $body = $('body');
   +var $toolbar = $('#toolbar');
   +var toolbarHeightModifier = 0;
   

   This should be initialized in Drupal.behavior, we can't be sure the DOM is loaded outside a behavior. Edgecase-y but still.

+++ b/modules/toolbar/toolbar.js
@@ -1,6 +1,12 @@
+var $body
+var $toolbar

It's missing ";" on each line. Otherwise it's RTBC.

Profiling shows that Drupal.toolbar.height cost less than 1ms on a recent browser, I'm comfortable with that. Some of the slowness comes from the old version of jQuery, can't do much about it. I guess the only thing left if we want to be extra sure is to fire up IE6 and see how it goes. I don't have it though.

Looks like a testbot fluke.

Testbot fluke.

For the record, #20 is indeed RTBC.

 (function ($) {

 Drupal.toolbar = Drupal.toolbar || {};
+// Since some selectors are being used in a resize handler lets cache them.
+// Need them available to all functions in the file so define here but for
+// edge-case of DOM not being loaded yet, set the selectors in Drupal.behaviors.
+var $body;
+var $toolbar;
+var toolbarHeightModifier = 0;

This seems really non-standard. Won't it break other code outside the toolbar module that is trying to use the Drupal.toolbar object?

For example, if you call Drupal.toolbar.height() from another module's behaviors, it seemed to work before but breaks after this patch...

A couple more minor notes:

1. The above code comment really needs some improvement for grammar, etc. I tried to rewrite it a bit while trying to understand what it does and came up with this potential replacement:

   +// Since some selectors are used in a resize handler, they should be cached.
   +// Define them here since they need to be available to all functions in the
   +// file, but handle the edge case of the DOM not being loaded yet by waiting
   +// until Drupal.behaviors.toolbar.attach() is called to set their values.
   

2. +  var bodyPaddingTop = parseInt($body.css('padding-top'),10);
   

   Should have a space between the comma and the "10".