448572 - Use smart getters in browser.js

Status: RESOLVED FIXED

(Firefox :: General, defect, P2)

Description

[Asaf Romano (gone)]

Attachment: patch

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 331770 [details] [diff] [review]
patch

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>-function getNavToolbox()
>-{
>-  if (!gNavToolbox)
>-    gNavToolbox = document.getElementById("navigator-toolbox");
>-  return gNavToolbox;

I'm a bit nervous about removing this from an extension compat point of view, perhaps we should keep it in for now?

Comment 2

[Asaf Romano (gone)]

Attachment: for checkin

Comment 3

[Asaf Romano (gone)]

Changeset a2e861f6443c.

Comment 4

[Ed Lee :Mardak]

Will there be any issues of addons grabbing one of these globals too early, e.g., accessing gURLBar before prepareForStartup, and causing it to be stuck at null?

Example code: while (gURLBar == null) waitForBrowserToGetReady();

Comment 5

[Boris Zbarsky [:bzbarsky]]

Is it possible that this is causing the 

TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_bug409481.js | Timed out

on tinderbox?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Looks like it was: there was a gNavigatorBundle assignment left over in web-panels.js.  Pushed changeset f9423035dffe to fix that.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #4)
> Will there be any issues of addons grabbing one of these globals too early,
> e.g., accessing gURLBar before prepareForStartup, and causing it to be stuck at
> null?

It might, but is that actually a common pattern? Does it actually work? I would expect your example to iloop, and I'd like to think that people aren't going to be doing the equivalent with timers.

Comment 8

[Ed Lee :Mardak]

I don't think it's common, and if we do break something.. the addon owner should make it less hacky! ;)

(function showKeywords() {
  // Try to get the urlbar node
  if (gURLBar == null)
    return setTimeout(showKeywords, 100);

  // Change the search engine to include keywords
  gURLBar.setAttribute("autocompletesearch", "keyword " +
    gURLBar.getAttribute("autocompletesearch"));
})();

Comment 9

[Dão Gottwald [:dao]]

(In reply to comment #8)
> I don't think it's common, and if we do break something.. the addon owner
> should make it less hacky! ;)
> 
> (function showKeywords() {
>   // Try to get the urlbar node
>   if (gURLBar == null)
>     return setTimeout(showKeywords, 100);

The gURLBar getter will be executed once only, so this wouldn't have the expected effect. I guess the getter should only delete itself if the element was found.

Comment 10

[Dão Gottwald [:dao]]

So if getBrowser() is deprecated, that should probably be documented on http://developer.mozilla.org/en/docs/Code_snippets:Tabbed_browser and it should be removed from all other pages on devmo. See also bug 448936.

Comment 11

[Eric Shepherd [:sheppy]]

Docs updated.

Comment 12

[Serge Gautherie (:sgautherie)]

Could this land on 1.9.0 to fix bug 404110?