448936 - remove getBrowser() and getNavToolbox() usage from the tree

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Dão Gottwald [:dao]]

Bug 448572 deprecated getBrowser() and getNavToolbox(). However, we're still using them ourselves:
http://mxr.mozilla.org/mozilla-central/search?string=getBrowser\(|getNavToolbox&regexp=on&find=&findi=&filter=browser%2F|toolkit%2F&hitlimit=&tree=mozilla-central

Comment 1

[Simon Bünzli]

We might also want to remove those methods from the docs, e.g. http://developer.mozilla.org/en/docs/Special:Nutch?language=en&start=0&hitsPerPage=10&query=getbrowser&fulltext=Search ... (then again, AFAICT both methods seem to have been part of a blessed API for Mozilla browsers at one time, so maybe we shouldn't do that after all)

Comment 2

[Dão Gottwald [:dao]]

Attachment: patch

Comment 3

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

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

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

>-function getPPBrowser()
>-{
>-  return gBrowser;

>editor/ui/composer/content/ComposerCommands.js
>extensions/reporter/resources/content/reporter/reporterOverlay.js
>toolkit/components/printing/content/printUtils.js
>toolkit/content/viewZoomOverlay.js

SeaMonkey/Thunderbird (and perhaps others) also use these... doesn't seem like the simplification is worth the trouble of updating them too (could file bugs instead).

>diff --git a/browser/components/search/content/search.xml b/browser/components/search/content/search.xml

>-          var allbrowsers = getBrowser().mPanelContainer.childNodes;
>-          for (var tab = 0; tab < allbrowsers.length; tab++) {
>-            var browser = getBrowser().getBrowserAtIndex(tab);

Wow, this was pretty broken. Who reviewed this, anyways? :)

>diff --git a/embedding/browser/chrome/content/mini-nav.js b/embedding/browser/chrome/content/mini-nav.js

I have no idea whether anyone uses this, or whether it's maintained at all. I suppose these changes don't hurt, but it's possible that people are using it in places that don't have js1.7 features. I'd just avoid changing it (it's self contained anyways).

>diff --git a/testing/mochitest/tests/browser/browser_scope.js b/testing/mochitest/tests/browser/browser_scope.js

>-  is(gBrowser, getBrowser(), "both ways of getting tabbrowser work");
>+  is(getBrowser(), gBrowser, "getBrowser() returns gBrowser for backwards compatibility");

This kind of test probably belongs under /browser/base (testing/mochitest/tests/browser/ was meant to include tests that test the harness itself), but I guess it doesn't really matter.

>diff --git a/toolkit/components/help/content/help.js b/toolkit/components/help/content/help.js

I think SeaMonkey uses this too, but it's likely going to move out of toolkit and into SeaMonkey code (bug 425541), so just leave it for now.

>diff --git a/toolkit/components/viewsource/content/viewPartialSource.js b/toolkit/components/viewsource/content/viewPartialSource.js
>diff --git a/toolkit/components/viewsource/content/viewSource.js b/toolkit/components/viewsource/content/viewSource.js

These appear to be self contained, so I guess these changes are OK (apart from the printUtils.js-related changes).

>tools/test-harness/jssh-driver/jssh_driver.py

I don't know whether this script was meant to be compatible with SeaMonkey, I guess you'd have to ask bc.

Comment 4

[Dão Gottwald [:dao]]

(In reply to comment #3)
> >editor/ui/composer/content/ComposerCommands.js
> >extensions/reporter/resources/content/reporter/reporterOverlay.js
> >toolkit/components/printing/content/printUtils.js
> >toolkit/content/viewZoomOverlay.js
> 
> SeaMonkey/Thunderbird (and perhaps others) also use these... doesn't seem like
> the simplification is worth the trouble of updating them too (could file bugs
> instead).

File bugs on updating SeaMonkey and Thunderbird or on updating said files?

> >tools/test-harness/jssh-driver/jssh_driver.py
> 
> I don't know whether this script was meant to be compatible with SeaMonkey, I
> guess you'd have to ask bc.

I would expect it to work still as SeaMonkey sets gBrowser in Startup().

Comment 5

[Dão Gottwald [:dao]]

Attachment: patch v2

Comment 6

[Dão Gottwald [:dao]]

Attachment: patch v2, updated to trunk

Comment 7

[Dão Gottwald [:dao]]

Attachment: patch v2, updated to trunk

Comment 8

[Dão Gottwald [:dao]]

Attachment: patch v2, updated to trunk

Comment 9

[Dão Gottwald [:dao]]

Attachment: patch v2, updated to trunk

Comment 10

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

Comment on attachment 337415 [details] [diff] [review]
patch v2, updated to trunk

Most of these changes look OK, though the patch has bitrotted. Some that aren't OK:
-printUtils is used by SeaMonkey, so we should avoid the getPPBrowser etc. changes
-I think reporterOverlay is also used by seamonkey (which doesn't use a smart getter for gBrowser)
-toolkit/components/places/tests might be run by seamonkey?

It would probably be easier to review this if it was split into modules (or at least move the browser/ changes into their own patch).