Use smart getters in browser.js

RESOLVED FIXED in Firefox 3.1a2

Status

()

defect
P2
normal
RESOLVED FIXED
11 years ago
5 years ago

People

(Reporter: mano, Assigned: mano)

Tracking

(Blocks 1 bug, {dev-doc-complete})

Trunk
Firefox 3.1a2
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Posted patch patch (obsolete) — Splinter Review
Attachment #331770 - Flags: review?(mconnor)
Assignee: nobody → mano
Priority: -- → P2
Target Milestone: --- → Firefox 3.1a2
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: PC → All
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?
Attachment #331770 - Flags: review?(mconnor) → review+
Changeset a2e861f6443c.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 4

11 years ago
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();
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?
Looks like it was: there was a gNavigatorBundle assignment left over in web-panels.js.  Pushed changeset f9423035dffe to fix that.
(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

11 years ago
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"));
})();
(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.
Depends on: 448862
Depends on: 448885
Version: unspecified → Trunk
Depends on: 448936
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.
Keywords: dev-doc-needed
Docs updated.
Blocks: 404110
Could this land on 1.9.0 to fix bug 404110?
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x?
Blocks: 448936
No longer depends on: 448936
You need to log in before you can comment on or make changes to this bug.