Clean Up Code

subscribe

Subscribe.

Thanks @mfer. I've added your CSS and generated the markup via a theme() function. I'd like to be able define the markup once, rather than having it in two places (module and js). The original IE6 no more banner code is designed to be placed directly in the page, so it would be good to have a non-js option. Ideas welcome.

Why wouldn't the browser cache the js after the first request?

subscribe

subscribe

(subscribe).

FYI l'm using this module simply because it looks neat, and offers good browser variants instead of just newer IE versions, unlike http://drupal.org/project/ie6update.

My 2 cents:

Having the js be a drupal callback is a terrible idea. Normally, when a js file is loaded the webserver gets it from the file system and delivers it. This is fairly fast. The setup here where the js is defined as a menu callback means that the webserver calls back to Drupal, Drupal does a complete bootstrap, and then Drupal generates the js. Even with agressive caching (not available to authenticated users) this is much much much slower and much more resource intensive.

I totally agree. Except that in this case the additional pageload is not really affecting users other than the IE6-masochists (since the script load is in the conditional statement), and thus has no effect for "normal" users. Still, it's wasting server resources.

I'd like to be able define the markup once, rather than having it in two places (module and js).

Seriously? This callback is just .. uhm.. not cool. Even if the performance impact is relativily limited, performance should always be prioritized, when possible.
The 2 minutes that it would take to change the code to point the JS to a normal file to avoid completely useless bootstrapping - would definitely be time well spent.

On an unrelated note (I'm too lazy to open another issue), I noticed a minor bug in this code:

/**
 * Make the upgrade banner markup available in the page template.
 */
function ie6nomore_preprocess_page(&$vars) {
  $vars['ie6nomore'] = '<!--[if lt IE 7]>'. theme('ie6nomore_banner') .'<![endif]-->';

  $vars['scripts'] .= '<!--[if lt IE 7]>';
  $vars['scripts'] .= '<script type="text/javascript" src="/ie6nomore.js"></script>';
  $vars['scripts'] .= '<![endif]-->'."\n";
}

To be more specific:
$vars['scripts'] .= '<script type="text/javascript" src="/ie6nomore.js"></script>';

The "/" in src="/ie6nomore.js" should be replaced by base_path(). Otherwise this whole thing wont work if Drupal was installed in a subdirectory (instead of apache document root).

Also, instead of using ie6nomore_preprocess_page(), the conditional script could also be set via drupal_set_html_head() ( http://api.drupal.org/api/function/drupal_set_html_head/6 ). I guess the current version works fine, but this might have some impact on the theme used.

I don't want to sound like a smartass, but I think adding the JS in this way would work better:

/**
 * Make the upgrade banner markup available in the page template.
 */
function ie6nomore_init() {
  $script .= '<!--[if lt IE 7]>';
  $script .= '<script type="text/javascript" src=" ' . base_path(). drupal_get_path('module', 'ie6nomore.js'). '/ie6nomore.js"></script>';
  $script .= '<![endif]-->'."\n";

  drupal_set_html_head($script);

}

I haven't even tested this code, but it should work in any normal theme, subdirectory install or not and without unnecessary bootstrapping. The JS would still not get aggregated, since it would require drupal_add_js(), and also serverside client recognizition (not really an issue, but not necessary either).

... (oh and $vars['ie6nomore'] = '<!--[if lt IE 7]>'. theme('ie6nomore_banner') .'<![endif]-->'), would still belong in the preprocess-function.

Thanks. Per @mfer the menu callback has been abandoned and js is now injected via hook_init() (refer alpha3 release).