Last Comment Bug 644161 - "Open All in tabs" does not work from bookmarks menu if no window is open
: "Open All in tabs" does not work from bookmarks menu if no window is open
: regression
Product: Firefox
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: Trunk
: All Mac OS X
: -- normal (vote)
: Firefox 6
Assigned To: David Shiga
: Marco Bonardo [::mak]
Depends on:
Blocks: 713551 562998
  Show dependency treegraph
Reported: 2011-03-23 08:38 PDT by Eugenio MORASSI
Modified: 2011-12-26 12:34 PST (History)
7 users (show)
mak77: in‑testsuite-
hskupin: in‑litmus?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (3.22 KB, patch)
2011-04-30 19:30 PDT, David Shiga
mak77: review-
Details | Diff | Splinter Review
v2 (3.11 KB, patch)
2011-05-11 20:05 PDT, David Shiga
mak77: review+
Details | Diff | Splinter Review
v3 (3.39 KB, patch)
2011-05-14 08:25 PDT, David Shiga
no flags Details | Diff | Splinter Review

Description Eugenio MORASSI 2011-03-23 08:38:50 PDT
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] 2011-03-23 08:45:04 PDT
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:

How about with a new, empty testing profile? (Don't install any addons into it)
Comment 2 Eugenio MORASSI 2011-03-23 08:46:41 PDT
Exactly : no new tabs open at all.
Comment 3 Eugenio MORASSI 2011-03-23 08:47:30 PDT
(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) 2011-03-23 08:53:04 PDT
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] 2011-03-23 10:59:52 PDT
I could suppose bug 562998
Comment 6 Henrik Skupin (:whimboo) 2011-03-23 11:16:30 PDT
Mh, so what about bug 629889? Why that hasn't been fixed it?
Comment 7 Marco Bonardo [::mak] 2011-03-23 11:34:42 PDT
(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) 2011-03-23 11:48:01 PDT
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 2011-04-30 19:30:32 PDT
Created attachment 529327 [details] [diff] [review]

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] 2011-05-10 18:42:18 PDT
Comment on attachment 529327 [details] [diff] [review]

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, " +

::: 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[";1"].
> +                  createInstance(Ci.nsISupportsArray);

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

@@ +790,5 @@
> +      var sa = Cc[";1"].
> +                  createInstance(Ci.nsISupportsArray);
> +      var wuri = Cc[";1"].
> +                  createInstance(Ci.nsISupportsString);
> + = urls.join("|");

rename to "uriList"

@@ +794,5 @@
> + = 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] 2011-05-10 18:43:25 PDT
also "thus open a new one." rather than "so we open one."
Comment 12 Marco Bonardo [::mak] 2011-05-10 18:44:42 PDT
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) 2011-05-11 01:36:08 PDT
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 2011-05-11 20:05:21 PDT
Created attachment 531835 [details] [diff] [review]

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] 2011-05-13 06:10:09 PDT
Comment on attachment 531835 [details] [diff] [review]

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:

::: 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 2011-05-14 08:25:08 PDT
Created attachment 532439 [details] [diff] [review]

Ok, here's the new version with those changes. Thanks for reviewing it.
Comment 17 Marco Bonardo [::mak] 2011-05-16 03:48:32 PDT
Comment 18 Marco Bonardo [::mak] 2011-05-17 08:11:47 PDT

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) 2011-05-19 03:55:46 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.