Closed
Bug 1337959
Opened 7 years ago
Closed 7 years ago
BrowserOpenAddonsMgr doesn't always resolve with a window
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: aswan, Assigned: Gijs)
Details
Attachments
(1 file)
BrowserOpenAddonsMgr() returns a promise that usually resolves with the window global from the about:addons window. But there is one branch where it resolves without a value, making it difficult for callers to rely on the return value: http://searchfox.org/mozilla-central/rev/672c83ed65da286b68be1d02799c35fdd14d0134/browser/base/content/browser.js#6478 (not sure about the component here, please feel free to re-file as necessary)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Reporter | ||
Comment 2•7 years ago
|
||
mozreview-review |
Comment on attachment 8835993 [details] Bug 1337959 - fix BrowserOpenAddonsMgr to always resolve the promise with a window, https://reviewboard.mozilla.org/r/111518/#review112890 nice, thank you! ::: browser/base/content/browser.js:6414 (Diff revision 1) > - switchToTabHavingURI("about:addons", true); > - > - if (aView) { > - // This must be a new load, else the ping/pong would have > + // This must be a new load, else the ping/pong would have > - // found the window above. > + // found the window above. > + switchToTabHavingURI("about:addons", true); Since we now know at this point that there isn't an existing tab, can you just use something like `openUILinkIn()` here instead? ::: browser/base/content/browser.js:6422 (Diff revision 1) > - Services.obs.removeObserver(observer, aTopic); > + Services.obs.removeObserver(observer, aTopic); > + if (aView) { > aSubject.loadView(aView); > + } > + aSubject.QueryInterface(Ci.nsIDOMWindow); > + aSubject.focus(); The comment for `switchToTabHavingURI` says that it focuses the returned window which I think makes this line unnecessary?
Attachment #8835993 -
Flags: review?(aswan) → review+
Assignee | ||
Comment 3•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8835993 [details] Bug 1337959 - fix BrowserOpenAddonsMgr to always resolve the promise with a window, https://reviewboard.mozilla.org/r/111518/#review112890 > The comment for `switchToTabHavingURI` says that it focuses the returned window which I think makes this line unnecessary? The comment is a lie for the case where it opens a new item, looking at the implementation (but not for the case where we select/focus an extant tab). Also, this is async after the load happens, so focus may have moved (either by the user or by a website) since.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•7 years ago
|
||
Was there a particular callsite that also wants updating now that it *can* rely on the return value?
Flags: needinfo?(aswan)
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/ddf86248e301 fix BrowserOpenAddonsMgr to always resolve the promise with a window, r=aswan
Comment 7•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ddf86248e301
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Reporter | ||
Comment 8•7 years ago
|
||
(In reply to :Gijs from comment #5) > Was there a particular callsite that also wants updating now that it *can* > rely on the return value? This came up while writing tests and the workaround wasn't horrendous so I'm not inclined to change them. Sorry if that gave a false sense of urgency for this one, but the changes are certainly an improvement.
Flags: needinfo?(aswan)
You need to log in
before you can comment on or make changes to this bug.
Description
•