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

VERIFIED FIXED in Firefox 6

Status

()

Firefox
Bookmarks & History
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: Eugenio MORASSI, Assigned: David Shiga)

Tracking

(Blocks: 1 bug, {regression})

Trunk
Firefox 6
All
Mac OS X
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -
in-litmus ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [places-next-wanted][fixed-in-places][mozmill-test-needed])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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

6 years ago
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
Version: unspecified → Trunk
(Reporter)

Comment 2

6 years ago
Exactly : no new tabs open at all.
(Reporter)

Comment 3

6 years ago
(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.
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?
Status: UNCONFIRMED → NEW
Component: Tabbed Browser → Bookmarks & History
Ever confirmed: true
Keywords: regression, regressionwindow-wanted
QA Contact: tabbed.browser → bookmarks
Hardware: x86 → All
Summary: No Open in tabs if no window is open → "Open All in tabs" does not work from bookmarks menu if no window is open
I could suppose bug 562998
Whiteboard: [places-next-wanted]
Mh, so what about bug 629889? Why that hasn't been fixed it?
(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.
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.
Blocks: 562998
Flags: in-testsuite?
(Assignee)

Comment 9

6 years ago
Created attachment 529327 [details] [diff] [review]
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.
Attachment #529327 - Flags: review?(mak77)
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.
Attachment #529327 - Flags: review?(mak77) → review-
also "thus open a new one." rather than "so we open one."

Updated

6 years ago
Keywords: regressionwindow-wanted
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?
Flags: in-testsuite? → in-testsuite-
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. :)
(Assignee)

Comment 14

6 years ago
Created attachment 531835 [details] [diff] [review]
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.
Attachment #529327 - Attachment is obsolete: true
Attachment #531835 - Flags: review?(mak77)
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
Attachment #531835 - Flags: review?(mak77) → review+
(Assignee)

Comment 16

6 years ago
Created attachment 532439 [details] [diff] [review]
v3

Ok, here's the new version with those changes. Thanks for reviewing it.
Attachment #531835 - Attachment is obsolete: true

Updated

6 years ago
Assignee: nobody → dshigabz
Status: NEW → ASSIGNED
http://hg.mozilla.org/projects/places/rev/31a9c95c4b6d
Whiteboard: [places-next-wanted] → [places-next-wanted][fixed-in-places]
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 :)
Severity: major → normal
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 6
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.
Status: RESOLVED → VERIFIED
Flags: in-litmus?
Whiteboard: [places-next-wanted][fixed-in-places] → [places-next-wanted][fixed-in-places][mozmill-test-needed]
Blocks: 713551
You need to log in before you can comment on or make changes to this bug.