847952 - getTopWin should limit its search to windows of the same privacy status

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Josh Matthews [:jdm]]

It looks like all users would benefit from restricted searches.

Comment 1

[Josh Matthews [:jdm]]

Attachment: Make utility function to return the topmost window discriminate between public and private windows.

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 722270 [details] [diff] [review]
Make utility function to return the topmost window discriminate between public and private windows.

>   if (skipPopups) {
>-    return Components.classes["@mozilla.org/browser/browserglue;1"]
>-                     .getService(Components.interfaces.nsIBrowserGlue)
>-                     .getMostRecentBrowserWindow();
>+    let isPrivate = PrivateBrowsingUtils.isWindowPrivate(window);
>+    return RecentWindow.getMostRecentBrowserWindow({private: isPrivate});
>   }
>-  return Services.wm.getMostRecentWindow("navigator:browser");
>+  return RecentWindow.getMostRecentWindow({type: "navigator:browser",
>+                                           private: isPrivate});

This breaks the skipPopups distinction. You're actually searching for the same kind of window in both cases.

Comment 3

[Josh Matthews [:jdm]]

Attachment: Make utility function to return the topmost window discriminate between public and private windows.

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 725116 [details] [diff] [review]
Make utility function to return the topmost window discriminate between public and private windows.

>@@ -54,21 +55,21 @@ function getTopWin(skipPopups) {
>   // If this is called in a browser window, use that window regardless of
>   // whether it's the frontmost window, since commands can be executed in
>   // background windows (bug 626148).
>   if (top.document.documentElement.getAttribute("windowtype") == "navigator:browser" &&
>       (!skipPopups || top.toolbar.visible))
>     return top;
> 
>   if (skipPopups) {
>-    return Components.classes["@mozilla.org/browser/browserglue;1"]
>-                     .getService(Components.interfaces.nsIBrowserGlue)
>-                     .getMostRecentBrowserWindow();
>+    let isPrivate = PrivateBrowsingUtils.isWindowPrivate(window);
>+    return RecentWindow.getMostRecentBrowserWindow({private: isPrivate});
>   }
>-  return Services.wm.getMostRecentWindow("navigator:browser");
>+  return RecentWindow.getMostRecentWindow({type: "navigator:browser",
>+                                           private: isPrivate});
> }

isPrivate isn't declared outside of the |if| block.

Your introduction of RecentWindow.getMostRecentWindow is still pointless, because you're looking for a navigator:browser window in both cases.

Comment 5

[Josh Matthews [:jdm]]

(In reply to Dão Gottwald [:dao] from comment #4)
> Comment on attachment 725116 [details] [diff] [review]
> Make utility function to return the topmost window discriminate between
> public and private windows.
> 
> >@@ -54,21 +55,21 @@ function getTopWin(skipPopups) {
> >   // If this is called in a browser window, use that window regardless of
> >   // whether it's the frontmost window, since commands can be executed in
> >   // background windows (bug 626148).
> >   if (top.document.documentElement.getAttribute("windowtype") == "navigator:browser" &&
> >       (!skipPopups || top.toolbar.visible))
> >     return top;
> > 
> >   if (skipPopups) {
> >-    return Components.classes["@mozilla.org/browser/browserglue;1"]
> >-                     .getService(Components.interfaces.nsIBrowserGlue)
> >-                     .getMostRecentBrowserWindow();
> >+    let isPrivate = PrivateBrowsingUtils.isWindowPrivate(window);
> >+    return RecentWindow.getMostRecentBrowserWindow({private: isPrivate});
> >   }
> >-  return Services.wm.getMostRecentWindow("navigator:browser");
> >+  return RecentWindow.getMostRecentWindow({type: "navigator:browser",
> >+                                           private: isPrivate});
> > }
> 
> isPrivate isn't declared outside of the |if| block.

You're right.

> Your introduction of RecentWindow.getMostRecentWindow is still pointless,
> because you're looking for a navigator:browser window in both cases.

You're correct that it's looking for navigator:browser, but I added the check for skipping popups in this version of the patch. I claim that's not pointless.

Comment 6

[Dão Gottwald [:dao]]

(In reply to Josh Matthews [:jdm] from comment #5)
> > Your introduction of RecentWindow.getMostRecentWindow is still pointless,
> > because you're looking for a navigator:browser window in both cases.
> 
> You're correct that it's looking for navigator:browser, but I added the
> check for skipping popups in this version of the patch. I claim that's not
> pointless.

You don't need RecentWindow.getMostRecentWindow for that. You should just call RecentWindow.getMostRecentBrowserWindow with a parameter added to allow popups.

Comment 7

[Josh Matthews [:jdm]]

Attachment: Make utility function to return the topmost window discriminate between public and private windows.

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 725508 [details] [diff] [review]
Make utility function to return the topmost window discriminate between public and private windows.

>+  let isPrivate = PrivateBrowsingUtils.isWindowPrivate(window);
>   if (skipPopups) {
>-    return Components.classes["@mozilla.org/browser/browserglue;1"]
>-                     .getService(Components.interfaces.nsIBrowserGlue)
>-                     .getMostRecentBrowserWindow();
>+    return RecentWindow.getMostRecentBrowserWindow({private: isPrivate});
>   }
>-  return Services.wm.getMostRecentWindow("navigator:browser");
>+  return RecentWindow.getMostRecentBrowserWindow({private: isPrivate,
>+                                                  skipPopups: false});

You can consolidate these lines:

  let isPrivate = PrivateBrowsingUtils.isWindowPrivate(window);
  return RecentWindow.getMostRecentBrowserWindow({private: isPrivate,
                                                  skipPopups: skipPopups});

Also, the double negation in skipPopups:false seems unfortunate. Can you flip this around and call the parameter allowPopups, includePopups or something like that?

Comment 9

[Josh Matthews [:jdm]]

Attachment: Make utility function to return the topmost window discriminate between public and private windows.

Comment 10

[Josh Matthews [:jdm]]

Attachment: Make utility function to return the topmost window discriminate between public and private windows.

Apologies, I forgot to qref.

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 725512 [details] [diff] [review]
Make utility function to return the topmost window discriminate between public and private windows.

>+   *        * allowPopups: false to skip any popups encountered.

That's the default behavior. Please document what 'true' is doing.

>   getMostRecentBrowserWindow: function RW_getMostRecentBrowserWindow(aOptions) {
>     let checkPrivacy = typeof aOptions == "object" &&
>                        "private" in aOptions;
>+    let allowPopups = false;
>+    if (typeof aOptions == "object" && "allowPopups" in aOptions) {
>+      allowPopups = aOptions.allowPopups;
>+    }

This can be simplified now that 'false' is the default state:

let allowPopups = typeof aOptions == "object" &&
                  !!aOptions.allowPopups;

>     function isSuitableBrowserWindow(win) {
>       return (!win.closed &&
>-              win.toolbar.visible &&
>+              (allowPopups ? true : win.toolbar.visible) &&

(allowPopups || win.toolbar.visible) &&

r=me with these changes

Comment 12

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f534921cf61

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6396d766a611