Closed Bug 1337959 Opened 7 years ago Closed 7 years ago

BrowserOpenAddonsMgr doesn't always resolve with a window

Categories

(Firefox :: General, defect)

defect
Not set
normal

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)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
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+
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.
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
https://hg.mozilla.org/mozilla-central/rev/ddf86248e301
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
(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.