Note: There are a few cases of duplicates in user autocompletion which are being worked on.

"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.