644161 - "Open All in tabs" does not work from bookmarks menu if no window is open

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Eugenio MORASSI]

User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0) Gecko/20100101 Firefox/4.0
Build Identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0) Gecko/20100101 Firefox/4.0

Please excuse my poor English.

* * *

In previous versions of Firefox 3.x for Mac (PPC|Intel) if no window were open and one selects

Bookmarks menu -> a folder -> Open all in tabs (sort of, mine is in Italian...)

a new window is open AND all folder's bookmarks were open, each in a tab.

This is NOT working in FF4 unless you manually open a new window

Reproducible: Always

Steps to Reproduce:
1.Choose Bookmarks menu
2.Choose a folder
3.Select Open All in tabs (sort of)
Actual Results:  
Nothing happens if no window is open

Expected Results:  
A number o tabs in a new window

Comment 1

[Ed Morley [:emorley]]

By "nothing happens" do you mean that no new tabs open at all? Or that they all open in the current window? I believe the expected behaviour is the latter.

Failing that, could you see if the issue occurs if using Firefox in safe mode:
http://support.mozilla.com/kb/Safe+Mode

How about with a new, empty testing profile? (Don't install any addons into it)
http://support.mozilla.com/kb/Basic+Troubleshooting#w_8-make-a-new-profile

Comment 2

[Eugenio MORASSI]

Exactly : no new tabs open at all.

Comment 3

[Eugenio MORASSI]

(In reply to comment #1)
> By "nothing happens" do you mean that no new tabs open at all? Or that they all
> open in the current window? I believe the expected behaviour is the latter.

Exactly: no new tabs open at all.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Clearly a regression in Places. I see the same and get the following to errors in the error console:

Error: The api has changed. A places view                                     should be passed to PUIU_openContainerInTabs.                                      Not passing a view will throw in a future                                     release.
Source File: resource:///modules/PlacesUIUtils.jsm
Line: 820

Error: aWindow is null
Source File: resource:///modules/PlacesUIUtils.jsm
Line: 778


Marco, do you know which check-in could have caused this?

Comment 5

[Marco Bonardo [:mak]]

I could suppose bug 562998

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Mh, so what about bug 629889? Why that hasn't been fixed it?

Comment 7

[Marco Bonardo [:mak]]

(In reply to comment #6)
> Mh, so what about bug 629889? Why that hasn't been fixed it?

most likely some view is fixed and some is not, the library uses trees, this is a menu.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Ok, so lets assume bug 562998 as the one which regressed this feature. I would really like to see tests once we have checked in a fix.

Comment 9

[David Shiga]

Attachment: patch

When there's no window, aWindow is null in _openTabset function of PlacesUIUtils.jsm. It tries to open a window with call to aWindow.openDialog, which fails because aWindow is null.

Changed _openTabset it so it handles case where aWindow is null and opens window in more robust way.

Also made a small change to browserPlacesViews.js so that getWindow method in PlacesUIUtils.jsm is happy and doesn't give the "the api has changed" error message.

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 529327 [details] [diff] [review]
patch

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

I still want to test the final patch locally, thus minusing for now, but the approach seems ok.

::: browser/components/places/content/browserPlacesViews.js
@@ +748,5 @@
>        // at least two menuitems with places result nodes.
>        aPopup._endOptOpenAllInTabs = document.createElement("menuitem");
>        aPopup._endOptOpenAllInTabs.className = "openintabs-menuitem";
>        aPopup._endOptOpenAllInTabs.setAttribute("oncommand",
> +        "PlacesUIUtils.openContainerNodeInTabs(this.parentNode._placesNode, event, PlacesUIUtils.getViewForNode(this));");

split this proper in 2 lines
"PlacesUIUtils.openContainerNodeInTabs(this.parentNode._placesNode, event, " +
                                       "PlacesUIUtils.getViewForNode(this));");

::: browser/components/places/src/PlacesUIUtils.jsm
@@ +778,5 @@
> +    if (aWindow) {
> +      browserWindow =
> +        aWindow.document.documentElement.getAttribute("windowtype") == "navigator:browser" ?
> +        aWindow : this._getTopBrowserWin();
> +    }

You could reduce changes here by doing
browserWindow =
  aWindow && aWindow.document ... ?
  aWindow : this._getTopBrowserWin();

@@ +785,5 @@
>      // open (Bug 630255).
>      var where = browserWindow ?
>                  browserWindow.whereToOpenLink(aEvent, false, true) : "window";
>      if (where == "window") {
> +      //There is no browser window open, so we open one.

Missing whitespace in "//There"

@@ +787,5 @@
>                  browserWindow.whereToOpenLink(aEvent, false, true) : "window";
>      if (where == "window") {
> +      //There is no browser window open, so we open one.
> +      var sa = Cc["@mozilla.org/supports-array;1"].
> +                  createInstance(Ci.nsISupportsArray);

rename to "args" and move definition just before calling .AppendElement

@@ +790,5 @@
> +      var sa = Cc["@mozilla.org/supports-array;1"].
> +                  createInstance(Ci.nsISupportsArray);
> +      var wuri = Cc["@mozilla.org/supports-string;1"].
> +                  createInstance(Ci.nsISupportsString);
> +      wuri.data = urls.join("|");

rename to "uriList"

@@ +794,5 @@
> +      wuri.data = urls.join("|");
> +      sa.AppendElement(wuri);      
> +      browserWindow = Services.ww.openWindow(null,
> +                                            "chrome://browser/content/browser.xul",
> +                                            null, "chrome,dialog=no,all", sa);

It's possible we have aWindow but it's not a browser window, to properly set the parent you should pass aWindow as the first param.

Comment 11

[Marco Bonardo [:mak]]

also "thus open a new one." rather than "so we open one."

Comment 12

[Marco Bonardo [:mak]]

Henrik, I don't think we can make an automated test for this since it should close all browser windows, included the testing one. Would be scary for random failures. Any idea?

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Marco, in such a case we could automate that with a Mozmill test, where we don't have problems in closing the one and only browser window. Just waiting for a fix. :)

Comment 14

[David Shiga]

Attachment: v2

Ok, I this addresses all your comments. Some very long expressions were a bit awkward to split into multiple lines, have done my best to make it readable.

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 531835 [details] [diff] [review]
v2

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

Sorry if it took some more time, I had to clobber my mac build to test it.
It works pretty well, thank you very much for your contribution!

Once you fix the following two small things and attach a patch I'll take care of pushing it, to attach a ready-to-push patch you can follow some of the suggestions here: http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed

::: browser/components/places/src/PlacesUIUtils.jsm
@@ +780,1 @@
>        aWindow.document.documentElement.getAttribute("windowtype") == "navigator:browser" ?

just put aWindow && aWindows... on the same line.
Even if we usually require not going over 80 chars, we accept exceptions where splitting causes readability problems.

@@ +793,5 @@
> +                  createInstance(Ci.nsISupportsArray);
> +      args.AppendElement(uriList);      
> +      browserWindow = Services.ww.openWindow(aWindow,
> +                                            "chrome://browser/content/browser.xul",
> +                                            null, "chrome,dialog=no,all", args);

these 2 lines should be indented still 1 space more, they should be in line with the beginning of aWindow

Comment 16

[David Shiga]

Attachment: v3

Ok, here's the new version with those changes. Thanks for reviewing it.

Comment 17

[Marco Bonardo [:mak]]

http://hg.mozilla.org/projects/places/rev/31a9c95c4b6d

Comment 18

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/31a9c95c4b6d

Thank you for your contribution, and congratulations for your first code in the tree (supposed it is).
Feel free to steal other bugs around :)

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0a1) Gecko/20110518 Firefox/6.0a1

This needs a Litmus test after the next merge with Aurora has happened.