code review -- startup script enhancement & fix

[Joel Webber]

Scott, Kelly:

Patch file: selectionScript-onload-basetag.patch (against r1198)

Modified files:

M      dev/core/src/com/google/gwt/dev/util/SelectionScriptTemplate.js

M      dev/core/src/com/google/gwt/dev/util/SelectionScriptTemplate-xs.js

I'd like you to have a look at these proposed startup script changes. The goals are as follows:

1. Fix a bug that was keeping IE from respecting rebasing the application via the <base> tag.

2. Have the module's entry point fire as soon as the DOM is fully loaded, but before window.onload() fires (so that images and iframes don't keep the app from starting).

---

First, rebasing. We currently determine the module's base url using a series of tests in the selection script:

- We try to find the path of the selection script itself.

- If that doesn't work, we use the path part of document.location.href.

- Finally, if the script path turns out to have been relative, we use the <img src='clear.cache.gif'> trick to get the browser to absolutify it.

This works on pretty much all browsers but IE, because most browsers canonicalize the src attribute of script tags. IE leaves it alone, however, causing us to (usually) fall back to document.location.href. The problem with this is that it doesn't take <base> tags into account. This patch addresses this issue by looking for the last <base> tag in existence as of the time the startup script is parsed. This is correct, because this will be the base tag in effect when the startup script was loaded. If another one is added later, the base can change, but that doesn't affect the startup script (I've confirmed this on all browsers).

---

Now, the onload problem. We all know about the onload issue -- namely, that it fires too bloody late, causing onModuleLoad() to be called only after all images and iframes are done loading. What we *really* want (IMHO) is to know when the *DOM* is ready, so that things like RootPanel.get('id') will work properly.

This is tricky in practice, as only Firefox and Opera9 have the DOMContentLoaded event. On other browsers, we fall back to a 50ms timer interval, checking document.readyState. Again, I've tested this code on all four browsers, and it does the right thing.

[Joel Webber]

I want to add that I just realized this addresses issue 1141 as well.

On 6/22/07, Joel Webber <j...@google.com> wrote:

Scott, Kelly:

 

Patch file: selectionScript-onload-basetag .patch (against r1198)

[Scott Blum]

- 58 (Nitpick): you can avoid a 'return' keyword by reversing the test
- 68,69,74: Use $doc instead of document (better obfuscation)
- 69: You need to unhook the onBodyDone method after it's fired; otherwise the browser won't be able to GC this scope
- 74: Clever!!  (Shame you have to do this, tho)

[Joel Webber]

Thanks, Scott. Committed as r1205.

[Scott Blum]

On 6/28/07, Joel Webber <j...@google.com> wrote:

Thanks, Scott. Committed as r1205.

On 6/26/07, Scott Blum < sco...@google.com> wrote:

- 69: You need to unhook the onBodyDone method after it's fired; otherwise the browser won't be able to GC this scope

I checked what you committed, and unless browsers clear the "DOMContentLoaded" listener after firing it, we're still trapping onBodyDone (and thus this whole inner scope).

On this note, I don't think the "onBodyDone = null" line does anything useful, since clearing the onBodyDoneTimerId interval should have effectively removed that system reference already.

Scott

[Scott Blum]

And if you do need to go back, here's one I missed.

This line:
var onBodyDone = function() {

can simply be:
function onBodyDone() {

[Joel Webber]

Let me make sure I understand what you mean by "unhook the onBodyDone method" -- you're talking about from the DOMContentLoaded event, right? I suppose I could do that inside onBodyDone() itself. Does this seem reasonable?

function onBodyDone() {

  if (!bodyDone) {

    bodyDone = true;

    maybeStartModule();

    if ($doc.removeEventListener) {

      $doc.removeEventListener("DOMContentLoaded", onBodyDone);

    }

  }

};

[Scott Blum]

Yep, that's exactly what I mean.  Otherwise the document will keep a permanent reference to onBodyDone, which will closure the whole module function, which we'd like to be GC'able.

Oh, it occurs to me that you could also move the timer-kill code inside of onBodyDone, so you don't have to wait the extra tick to stop the timer.

On 6/28/07, Joel Webber <j...@google.com> wrote:

[Joel Webber]

Take 2. I cleaned up the listener in onBodyDone(), but didn't move the clearInterval() into it, because I can't see how it would make any difference. Let me know if I misunderstood.

Thanks,

joel.

[Scott Blum]

I think you wanted a different patch.

The only difference is slight performance.  With the clearInt() outside onBodyDone, the order of events may often be:

DOMContentLoaded fires
onBodyDone (startModule, unhook event listener)
Timer fires
Check dom ready state
onBodyDone (no-op)
clearInterval

if you move it inside onBodyDone, it becomes in the common case:

DOMContentLoaded fires
onBodyDone (startModule, unhook event listener)
clearInterval

Plus it just seems nice to put the "cleaning up events" code for DOM/Timer together.

[Joel Webber]

I see where you're coming from. Whaddayathink, something like this?

 

  var onBodyDoneTimerId;

  function onBodyDone() {

    if (!bodyDone) {

      bodyDone = true;

      maybeStartModule();

      if ($doc.removeEventListener) {

        $doc.removeEventListener("DOMContentLoaded", onBodyDone);

      }

      if (onBodyDoneTimerId) {

      clearInterval(onBodyDoneTimerId);

      }

    }

  }

  // For everyone that supports DOMContentLoaded.

  if ($doc.addEventListener) {

    $doc.addEventListener("DOMContentLoaded", onBodyDone, false);

  }

  // Fallback. If onBodyDone() gets fired twice, it's not a big deal.

  var onBodyDoneTimerId = setInterval(function() {

    if (/interactive|loaded|complete/.test($doc.readyState)) {

      onBodyDone();

    }

  }, 50);

On 6/28/07, Scott Blum <sco...@google.com> wrote:

[Scott Blum]

LGTM.