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

RESOLVED FIXED

Status

()

RESOLVED FIXED
10 years ago
4 years ago

People

(Reporter: dao, Assigned: dao)

Tracking

({meta})

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(6 obsolete attachments)

Comment 1

10 years ago
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)
(Assignee)

Comment 2

10 years ago
Created attachment 336401 [details] [diff] [review]
patch
Assignee: nobody → dao
Status: NEW → ASSIGNED
Attachment #336401 - Flags: review?(gavin.sharp)
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.
(Assignee)

Comment 4

10 years ago
(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().
(Assignee)

Comment 5

10 years ago
Created attachment 336422 [details] [diff] [review]
patch v2
Attachment #336401 - Attachment is obsolete: true
Attachment #336401 - Flags: review?(gavin.sharp)
(Assignee)

Comment 6

10 years ago
Created attachment 337016 [details] [diff] [review]
patch v2, updated to trunk
Attachment #336422 - Attachment is obsolete: true
Attachment #337016 - Flags: review?(gavin.sharp)
(Assignee)

Comment 7

10 years ago
Created attachment 337017 [details] [diff] [review]
patch v2, updated to trunk
Attachment #337016 - Attachment is obsolete: true
Attachment #337017 - Flags: review?(gavin.sharp)
Attachment #337016 - Flags: review?(gavin.sharp)
(Assignee)

Comment 8

10 years ago
Created attachment 337174 [details] [diff] [review]
patch v2, updated to trunk
Attachment #337017 - Attachment is obsolete: true
Attachment #337174 - Flags: review?(gavin.sharp)
Attachment #337017 - Flags: review?(gavin.sharp)
(Assignee)

Comment 9

10 years ago
Created attachment 337415 [details] [diff] [review]
patch v2, updated to trunk
Attachment #337174 - Attachment is obsolete: true
Attachment #337415 - Flags: review?(gavin.sharp)
Attachment #337174 - Flags: review?(gavin.sharp)
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).
Attachment #337415 - Flags: review?(gavin.sharp)
(Assignee)

Updated

9 years ago
Depends on: 517708, 517706, 517704
(Assignee)

Updated

4 years ago
Depends on: 1110069
(Assignee)

Updated

4 years ago
No longer blocks: 448572
Depends on: 448572
(Assignee)

Updated

4 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Depends on: 544018
Keywords: meta
Resolution: --- → FIXED
(Assignee)

Updated

4 years ago
Attachment #337415 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.