817337 - Add a JS module to get the most recent browser window, with the option of restricting the search to include only private windows

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[(no longer active)]

I need this for my patches in bug 801232.

Comment 1

[(no longer active)]

Attachment: Patch (v1)

Comment 2

[Dão Gottwald [:dao]]

Do we need XPCOM here or would a JS-only API (e.g. as part of PrivateBrowsingUtils) be sufficient?

Comment 3

[(no longer active)]

We don't need XPCOM here strictly, but the code we need to implement this lives in nsBrowserGlue, and because of the z-order hacks that we have there, I'd really like us to not have to duplicate that logic to PrivateBrowsingUtils, even if it's at the expense of adding an XPCOM method... :/

Comment 4

[(no longer active)]

ping?

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 687454 [details] [diff] [review]
Patch (v1)

(In reply to Ehsan Akhgari [:ehsan] from comment #3)
> We don't need XPCOM here strictly, but the code we need to implement this
> lives in nsBrowserGlue, and because of the z-order hacks that we have there,
> I'd really like us to not have to duplicate that logic to
> PrivateBrowsingUtils, even if it's at the expense of adding an XPCOM
> method... :/

You can just move the implementation to a JS module and let nsBrowserGlue utilize that. This way we can also avoid the suboptimal "...WithPrivacyStatus" API style by letting the JS-only API accept an options object:
getMostRecentBrowserWindow({ private: true })

Comment 6

[(no longer active)]

Attachment: Patch (v2)

Comment 7

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

Comment on attachment 689044 [details] [diff] [review]
Patch (v2)

>diff --git a/browser/components/Makefile.in b/browser/components/Makefile.in

>+EXTRA_PP_JS_MODULES = RecentWindow.jsm

Should put this in browser/modules.

>diff --git a/browser/components/RecentWindow.jsm b/browser/components/RecentWindow.jsm

>+  getMostRecentBrowser: function RW_getMostRecentBrowser(aOptions) {
>+    let forcePrivate = typeof(aOptions) == "object" &&
>+                       "private" in aOptions &&
>+                       aOptions.private;

Don't need the "in" check.

>+    function matchesPrivacyStatus(win) {

>+      return PrivateBrowsingUtils.isWindowPrivate(win) == aIsPrivate;

Shouldn't that aIsPrivate be "forcePrivate"?

Rather than having both mathcesPrivacyStatus and isFullBrowserWindow, seems like it would be simpler to  just combine them and call it isSuitableBrowserWindow or something like that.

Comment 8

[(no longer active)]

Attachment: Patch (v3)

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 689075 [details] [diff] [review]
Patch (v3)

>+++ b/browser/modules/RecentWindow.jsm
>@@ -0,0 +1,69 @@
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+"use strict";
>+
>+this.EXPORTED_SYMBOLS = ["RecentWindow"];
>+
>+const Cu = Components.utils;
>+const Cc = Components.classes;
>+const Ci = Components.interfaces;

You need neither Cc nor Ci.

>+   * @param aOptions an object accepting the arguments for the search.
>+   *        Set the private property to true in order to restrict the
>+   *        search to private windows only.
>+   */
>+  getMostRecentBrowser: function RW_getMostRecentBrowser(aOptions) {
>+    let forcePrivate = typeof(aOptions) == "object" &&
>+                       aOptions.private;
>+
>+    function isSuitableBrowserWindow(win) {
>+      if (win.closed ||
>+          !win.toolbar.visible) {
>+        return false;
>+      }
>+      if (!forcePrivate) {
>+        return true;
>+      }
>+      return PrivateBrowsingUtils.isWindowPrivate(win) == forcePrivate;
>+    }

You need to distinguish between 'private' being omitted and being false. Otherwise you can't use this to get only a non-private window.

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 689075 [details] [diff] [review]
Patch (v3)

>+  getMostRecentBrowser: function RW_getMostRecentBrowser(aOptions) {

I'd prefer getMostRecentBrowserWindow for clarity and consistency with nsIBrowserGlue.

Comment 11

[(no longer active)]

Attachment: Patch (v4)

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 689180 [details] [diff] [review]
Patch (v4)

>+  getMostRecentBrowserWindow: function RW_getMostRecentBrowserWindow(aOptions) {
>+    let checkPrivacy = typeof(aOptions) == "object" &&

nit: typeof aOptions == "object"

>+    let forcePrivate = checkPrivacy && aOptions.private;

I don't really like forcePrivate since it may sound like you'd want to make a non-private window private. Maybe something like wantPrivate or needPrivate would be better. Also, I'd prefer a variable name that can be read both ways, i.e. when it's false it should be clear that the caller wants a non-private window rather than that it doesn't care about the privacy status.

>+    function isSuitableBrowserWindow(win) {
>+      if (win.closed ||
>+          !win.toolbar.visible) {
>+        return false;
>+      }
>+      if (!checkPrivacy) {
>+        return true;
>+      }
>+      return PrivateBrowsingUtils.isWindowPrivate(win) == forcePrivate;
>+    }

I think I'd prefer:

    function isSuitableBrowserWindow(win) {
      return (!win.closed &&
              win.toolbar.visible &&
              (!checkPrivacy ||
                 PrivateBrowsingUtils.isWindowPrivate(win) == forcePrivate));
    }

It's more compact and seems at least as readable.

Comment 13

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0112085d836

Comment 14

[(no longer active)]

Backed out <https://hg.mozilla.org/integration/mozilla-inbound/rev/cb7f10e936c0> for a mochitest orange which I'm not sure I understand:

https://tbpl.mozilla.org/php/getParsedLog.php?id=17671501&tree=Mozilla-Inbound

165790 INFO TEST-START | /tests/uriloader/exthandler/tests/mochitest/test_handlerApps.xhtml
165791 INFO TEST-PASS | /tests/uriloader/exthandler/tests/mochitest/test_handlerApps.xhtml | webHandler launchWithURI (existing window/tab) started
165792 ERROR TEST-UNEXPECTED-FAIL | /tests/uriloader/exthandler/tests/mochitest/test_handlerApps.xhtml | uncaught exception - NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: '[JavaScript Error: "RecentWindow is not defined" {file: "resource://gre/components/nsBrowserGlue.js" line: 1577}]' when calling method: [nsIBrowserGlue::getMostRecentBrowserWindow] at chrome://browser/content/browser.js:11482
165793 INFO TEST-END | /tests/uriloader/exthandler/tests/mochitest/test_handlerApps.xhtml | finished in 333ms

Comment 15

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #12)
> >+    let forcePrivate = checkPrivacy && aOptions.private;
> 
> I don't really like forcePrivate since it may sound like you'd want to make
> a non-private window private. Maybe something like wantPrivate or
> needPrivate would be better. Also, I'd prefer a variable name that can be
> read both ways, i.e. when it's false it should be clear that the caller
> wants a non-private window rather than that it doesn't care about the
> privacy status.

To address the second point, you could just get rid of the variable and use aOptions.private directly (still guarded by checkPrivacy, of course).

Comment 16

[(no longer active)]

(In reply to comment #15)
> (In reply to Dão Gottwald [:dao] from comment #12)
> > >+    let forcePrivate = checkPrivacy && aOptions.private;
> > 
> > I don't really like forcePrivate since it may sound like you'd want to make
> > a non-private window private. Maybe something like wantPrivate or
> > needPrivate would be better. Also, I'd prefer a variable name that can be
> > read both ways, i.e. when it's false it should be clear that the caller
> > wants a non-private window rather than that it doesn't care about the
> > privacy status.
> 
> To address the second point, you could just get rid of the variable and use
> aOptions.private directly (still guarded by checkPrivacy, of course).

Will do before relanding.

Comment 17

[Marco Bonardo [:mak]]

Comment on attachment 689180 [details] [diff] [review]
Patch (v4)

Review of attachment 689180 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/modules/Makefile.in
@@ +28,4 @@
>  	KeywordURLResetPrompter.jsm \
>  	$(NULL)
>  
> +EXTRA_PP_JS_MODULES = RecentWindow.jsm

on Windows this is being overwritten by:

EXTRA_PP_JS_MODULES = \
  WindowsJumpLists.jsm

Comment 18

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/80b3907e5c6a