Disable "move to group" and "next group" if Panorama hasn't been run

VERIFIED FIXED in Firefox 4.0b12

Status

defect
P2
normal
VERIFIED FIXED
9 years ago
3 years ago

People

(Reporter: iangilman, Assigned: raymondlee)

Tracking

(Blocks 1 bug)

unspecified
Firefox 4.0b12
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [softblocker][fx4-fixed-bugday])

Attachments

(1 attachment, 10 obsolete attachments)

Reporter

Description

9 years ago
As it is right now, right clicking on a tab in the tab bar spins up Panorama in the background, in order to support the "move to group" item on the context menu. Hitting the "next group" key combo does the same. 

In the spirit of bug 626500, attempting to make sure people's first Panorama experience is intentional (i.e. selected explicitly from the menu bar), I propose that we disable both of the above features until Panorama has been run once (as determined by the browser.panorama.experienced_firstrun pref).
Reporter

Updated

9 years ago
blocking2.0: --- → ?
Assignee

Updated

9 years ago
Assignee: nobody → raymond
Status: NEW → ASSIGNED
Can we make it so Panorama is only spun up when the Move to Group context item is hovered long enough for the submenu to pop open?
Assignee

Comment 2

9 years ago
Posted patch WIP (obsolete) — Splinter Review
This patch disables the "next group" key combination until Panorama has been run
once. 

Also, it makes Panorama is only spun up when the "Move to Group" menu
is hovered more than 2 secs for the submenu to pop open if Panorama hasn't been started before in the same browsing session.  Or we should just keep this behaviour only until Panorama has been run once in the app lifetime?
Why would we load the iframe at all when there aren't any hidden tabs?
Assignee

Comment 4

9 years ago
(In reply to comment #3)
> Why would we load the iframe at all when there aren't any hidden tabs?

Actually we don't need to load when there aren't any hidden tabs.  We can do these
* If there aren't any hidden tabs, we only load the iframe if user clicks on "Move To Group" > "New Group"
* If there are more than one hidden tab and the "Move to Group" menu
is hovered more than 2 secs, the iframe would then be loaded and the submenu is shown.

WYT?
(In reply to comment #4)
> * If there are more than one hidden tab and the "Move to Group" menu
> is hovered more than 2 secs, the iframe would then be loaded and the submenu is
> shown.

We don't need a delay in that case.
Comment on attachment 504657 [details] [diff] [review]
WIP

>     window.addEventListener("keypress", function(event) {
>-      if (self.isVisible())
>+      if (self.isVisible() ||
>+          Services.prefs.getBoolPref("browser.panorama.experienced_first_run"))
>         return;

Wait, I'm confused. Doesn't this mean we ignore the keypress if Panorama is visible *or* if we're a Panorama user?

Don't you want to check for !Services.prefs.getBoolPref... ?
Assignee

Comment 7

9 years ago
(In reply to comment #6)
> Comment on attachment 504657 [details] [diff] [review]
> WIP
> 
> >     window.addEventListener("keypress", function(event) {
> >-      if (self.isVisible())
> >+      if (self.isVisible() ||
> >+          Services.prefs.getBoolPref("browser.panorama.experienced_first_run"))
> >         return;
> 
> Wait, I'm confused. Doesn't this mean we ignore the keypress if Panorama is
> visible *or* if we're a Panorama user?
> 
> Don't you want to check for !Services.prefs.getBoolPref... ?

Yes, we should check for !Services.prefs.getBoolPref...  This patch is WIP so I haven't tested that yet.
I think we may want to disable this entirely until panorama has a primary UI control by default.  There isn't necessarily a good correlation between if a Firefox profile has triggered panorama once, and if the user is aware of the feature (old computers, shared computers, etc.)  Also, when we turn this back on I think it should only be for named groups, so we can make a cleaner distinction between bookmarks and the user's session (more discussion about that here: https://bugzilla.mozilla.org/show_bug.cgi?id=622452#c8 )
Summary: Disable "move to group" and "next group" if Panorama hasn't been run → Disable "move to group" and "next group" (if Panorama hasn't been run?)
No longer blocks: 625423
Assignee

Comment 9

9 years ago
(In reply to comment #8)
> I think we may want to disable this entirely until panorama has a primary UI
> control by default.  There isn't necessarily a good correlation between if a
> Firefox profile has triggered panorama once, and if the user is aware of the
> feature (old computers, shared computers, etc.)  Also, when we turn this back
> on I think it should only be for named groups, so we can make a cleaner
> distinction between bookmarks and the user's session (more discussion about
> that here: https://bugzilla.mozilla.org/show_bug.cgi?id=622452#c8 )

For now, do you mean we should disable the following items until user places the Panorama button to the tabstrip manually?
* tab context menu - "Move To Tab"
* key combination to next group
* View menu > Tab Groups

Comment 10

9 years ago
(In reply to comment #8)
> I think we may want to disable this entirely until panorama has a primary UI
> control by default.  There isn't necessarily a good correlation between if a
> Firefox profile has triggered panorama once, and if the user is aware of the
> feature (old computers, shared computers, etc.)  Also, when we turn this back
> on I think it should only be for named groups, so we can make a cleaner
> distinction between bookmarks and the user's session (more discussion about
> that here: https://bugzilla.mozilla.org/show_bug.cgi?id=622452#c8 )

Do you mean that this menu item should not allow a user to move a tab to a new group?

I agree that move-to-group shouldn't list anything but named groups, but I think it's important that the user be able to move a tab to a new group from the tabbed browser.
Reporter

Comment 11

9 years ago
I propose this spec:

* "Move to group" is disabled entirely if Panorama's "first run" flag is off. Otherwise it doesn't spin up Panorama unless there are hidden tabs or you select "new group".

* "Next group" is disabled if there are not hidden tabs.
Assignee

Comment 12

9 years ago
Posted patch WIP 2 (obsolete) — Splinter Review
Will add test shortly
Attachment #504657 - Attachment is obsolete: true

Comment 13

9 years ago
bugspam. Moving b10 to b11
Blocks: 627096

Comment 14

9 years ago
bugspam. Removing b10
No longer blocks: 608028
Duplicate of this bug: 628167
Adding keywords to reflect the following user experience:

1. Open two tabs.
2. Right-click the second tab, and select "Move to" -> "New Group".
Only the first tab is left, with no indication of where the other went.
To a user, who has never used Panorama, this would be perceived as dataloss.
Meanwhile, the tab is still there, so Firefox would continue to use just as much memory.
Assignee

Comment 17

9 years ago
Posted patch v1 (obsolete) — Splinter Review
Attachment #505747 - Attachment is obsolete: true
Attachment #506328 - Flags: review?(ian)
Assignee

Comment 18

9 years ago
(In reply to comment #16)
> Adding keywords to reflect the following user experience:
> 
> 1. Open two tabs.
> 2. Right-click the second tab, and select "Move to" -> "New Group".
> Only the first tab is left, with no indication of where the other went.
> To a user, who has never used Panorama, this would be perceived as dataloss.
> Meanwhile, the tab is still there, so Firefox would continue to use just as
> much memory.

Hi Frank, could you file another for this please? This bug is about disabling the "move to group" and "next group".
(In reply to comment #18)
Filed: bug 628235
Comment on attachment 506328 [details] [diff] [review]
v1

>     window.addEventListener("keypress", function(event) {
>-      if (self.isVisible())
>+      if (self.isVisible() ||
>+          (gBrowser.tabs.length - gBrowser.visibleTabs.length) == 0)
>         return;

It looks like this will prevent use of the tab view shortcut when all tabs are in one group, even if a user wants to use the spatial interface to separate tabs into multiple groups.

Also, why isn't panorama using a <key/> in browser.xul for its keyboard shortcut?
AIUI, it would be superior in terms of performance.
Attaching event handlers to |window| via JS generally is avoided in browser window code.
Assignee

Comment 21

9 years ago
(In reply to comment #20)
> Comment on attachment 506328 [details] [diff] [review]
> v1
> 
> >     window.addEventListener("keypress", function(event) {
> >-      if (self.isVisible())
> >+      if (self.isVisible() ||
> >+          (gBrowser.tabs.length - gBrowser.visibleTabs.length) == 0)
> >         return;
> 
> It looks like this will prevent use of the tab view shortcut when all tabs are
> in one group, even if a user wants to use the spatial interface to separate
> tabs into multiple groups.
> 

Please have a look at this.  The keypress is for next/previous group key combination.
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-tabview.js#224

> Also, why isn't panorama using a <key/> in browser.xul for its keyboard
> shortcut?
> AIUI, it would be superior in terms of performance.
> Attaching event handlers to |window| via JS generally is avoided in browser
> window code.

I remember that  Control (+ Shift) + ` doesn't work if it's set in a <key/>.  I might be wrong, please find a new bug if you want.
Comment on attachment 506328 [details] [diff] [review]
v1

>     let data = this._sessionstore.getWindowValue(window, this.VISIBILITY_IDENTIFIER);
...

Move the data = this._sessionstore... down here, before this line:

>     if (data && data == "true") {

>+  // ----------
>+  // On move to group pop showing.
>+  moveToGroupPopupShowing: function Tabview_moveToGroupPopupShowing(event) {
>+    // there are hidden tabs so initialize the iframe and update the context menu
>+    if (gBrowser.tabs.length - gBrowser.visibleTabs.length > 0) {
>+      this.updateContextMenu(TabContextMenu.contextTab, event.target);

Another reason to show the menu would be if there are no hidden tabs, but there are other (empty) groups. Obviously, we can't currently get this information before we init the frame, but we should file a followup for the future (post-bug 588217).

Looks great!
Attachment #506328 - Flags: feedback+
If we disable this command (grayed out and inactive), then I think it is important that it has the panorama icon next to it, so that users have a way of figuring out what it is, and how they might go about making it functional.

Otherwise, I think we should opt to remove the item entirely, and have it appear once panorama is active.
Summary: Disable "move to group" and "next group" (if Panorama hasn't been run?) → Disable "move to group" and "next group" if Panorama hasn't been run
Assignee

Updated

9 years ago
Blocks: 628594
Assignee

Comment 24

9 years ago
Posted patch v2 (obsolete) — Splinter Review
#comment 22: Updated based on the comment and filed bug 628594
#comment 23: Added the panorama icon next to the "Move to group" menu
Attachment #506328 - Attachment is obsolete: true
Attachment #506684 - Flags: review?(ian)
Attachment #506328 - Flags: review?(ian)
blocking2.0: ? → betaN+
Whiteboard: [softblocker]
Reporter

Comment 25

9 years ago
Comment on attachment 506684 [details] [diff] [review]
v2

Looks good to me, but the browser stuff is outside my domain, so sending to Dao for review.
Attachment #506684 - Flags: review?(ian)
Attachment #506684 - Flags: review?(dao)
Attachment #506684 - Flags: feedback+
Comment on attachment 506684 [details] [diff] [review]
v2

>+    if (!Services.prefs.prefHasUserValue("browser.panorama.experienced_first_run") ||
>+        !Services.prefs.getBoolPref("browser.panorama.experienced_first_run")) {
>+      let prefsObserver = function() {
>+        Services.prefs.removeObserver(
>+          "browser.panorama.experienced_first_run", prefsObserver);
>+        document.getElementById("context_tabViewMenu").disabled = false;
>+      };
>+      Services.prefs.addObserver(
>+        "browser.panorama.experienced_first_run", prefsObserver, false);
>+
>+      document.getElementById("context_tabViewMenu").disabled = true;

Should this be hidden rather than disabled? An item that's universally disabled until some magic steps are executed doesn't seem very useful.

>   // ----------
>   // Adds new key commands to the browser, for invoking the Tab Candy UI
>   // and for switching between groups of tabs when outside of the Tab Candy UI.
>-  _setBrowserKeyHandlers : function() {
>+  _setBrowserKeyHandlers: function() {
>     let self = this;
> 
>     window.addEventListener("keypress", function(event) {
>-      if (self.isVisible())
>+      if (self.isVisible() ||
>+          (gBrowser.tabs.length - gBrowser.visibleTabs.length) == 0)
>         return;

This code shouldn't really run in a window without groups... I don't understand why it still exists at all, the key combo seems quite undiscoverable.
Reporter

Comment 27

9 years ago
(In reply to comment #26)
> Should this be hidden rather than disabled? An item that's universally disabled
> until some magic steps are executed doesn't seem very useful.

In comment 23, Faaborg suggested grayed with an icon, so we went with that.

> This code shouldn't really run in a window without groups... I don't understand
> why it still exists at all, the key combo seems quite undiscoverable.

We definitely want the key combo; it's quite useful for people who use Panorama. I agree that making it more discoverable would be lovely; perhaps we could add a menu item for it?

As for running in a window without groups, we need some way to detect if the window has groups. I suppose we could install the key handler at start up if there are hidden tabs, and if there weren't tabs hidden to begin with, listen for hide events. Does that sound better?
(In reply to comment #27)
> (In reply to comment #26)
> > Should this be hidden rather than disabled? An item that's universally disabled
> > until some magic steps are executed doesn't seem very useful.
> 
> In comment 23, Faaborg suggested grayed with an icon, so we went with that.

Ugh, I hadn't noticed the icon part yet, because I just looked at the patch without actually applying it... Context menus aren't really supposed to have icons. Hiding the item (also mentioned in comment 23) seems much cleaner.

> > This code shouldn't really run in a window without groups... I don't understand
> > why it still exists at all, the key combo seems quite undiscoverable.
> 
> We definitely want the key combo; it's quite useful for people who use
> Panorama. I agree that making it more discoverable would be lovely; perhaps we
> could add a menu item for it?

We could, but not for Firefox 4, since we don't have strings for it afaik. I don't think there's a point in keeping the key combo, especially if it adds overhead to key events processing for all users, as long as it's not discoverable. People just aren't going to know about it.

> As for running in a window without groups, we need some way to detect if the
> window has groups. I suppose we could install the key handler at start up if
> there are hidden tabs, and if there weren't tabs hidden to begin with, listen
> for hide events. Does that sound better?

That's not sufficient, since tabs can be hidden outside of panorama land. See e.g. http://hg.mozilla.org/mozilla-central/annotate/e98b94aa64fa/browser/base/content/browser.js#l8343
Reporter

Comment 29

9 years ago
(In reply to comment #28)
> Ugh, I hadn't noticed the icon part yet, because I just looked at the patch
> without actually applying it... Context menus aren't really supposed to have
> icons. Hiding the item (also mentioned in comment 23) seems much cleaner.

Fair enough; that's fine with me.

> We could, but not for Firefox 4, since we don't have strings for it afaik. I
> don't think there's a point in keeping the key combo, especially if it adds
> overhead to key events processing for all users, as long as it's not
> discoverable. People just aren't going to know about it.

It's a shame it's not discoverable, but it's an important key-combo... it's a very popular request. We'll just have to settle for spreading the word via Help and word of mouth.

> > As for running in a window without groups, we need some way to detect if the
> > window has groups. I suppose we could install the key handler at start up if
> > there are hidden tabs, and if there weren't tabs hidden to begin with, listen
> > for hide events. Does that sound better?
> 
> That's not sufficient, since tabs can be hidden outside of panorama land. See
> e.g.
> http://hg.mozilla.org/mozilla-central/annotate/e98b94aa64fa/browser/base/content/browser.js#l8343

I was referring to the hide event: 

http://hg.mozilla.org/mozilla-central/annotate/e98b94aa64fa/browser/base/content/tabbrowser.xml#l1947

That should cover it, right?
>Context menus aren't really supposed to have
>icons. Hiding the item (also mentioned in comment 23) seems much cleaner.

We're going to give significant context menu items icons in the next release, so we might as well start now.  Icons in context menus is native on Vista and 7 (but only for some items in each menu).  We need to provide the user with some form of clue as to how they could get this command enabled, and the icon does that.

>I don't understand why it still exists at all, the key combo seems quite
>undiscoverable.

I agree that the key combo is undiscoverable in the product itself, but it's a useful accelerator for advanced users, and there is little risk of users hitting it by accident.  It's a bit of a stretch (literally) and it only works if you've created multiple groups.
(In reply to comment #30)
> >Context menus aren't really supposed to have
> >icons. Hiding the item (also mentioned in comment 23) seems much cleaner.
> 
> We're going to give significant context menu items icons in the next release,
> so we might as well start now.  Icons in context menus is native on Vista and 7
> (but only for some items in each menu).  We need to provide the user with some
> form of clue as to how they could get this command enabled, and the icon does
> that.

The icon actually does not give a clue, as the panorama button isn't on the toolbar by default.

(In reply to comment #29)
> > > As for running in a window without groups, we need some way to detect if the
> > > window has groups. I suppose we could install the key handler at start up if
> > > there are hidden tabs, and if there weren't tabs hidden to begin with, listen
> > > for hide events. Does that sound better?
> > 
> > That's not sufficient, since tabs can be hidden outside of panorama land. See
> > e.g.
> > http://hg.mozilla.org/mozilla-central/annotate/e98b94aa64fa/browser/base/content/browser.js#l8343
> 
> I was referring to the hide event: 
> 
> http://hg.mozilla.org/mozilla-central/annotate/e98b94aa64fa/browser/base/content/tabbrowser.xml#l1947
> 
> That should cover it, right?

The code I linked to triggers the event.
Attachment #506684 - Flags: review?(dao) → review-
Reporter

Comment 32

9 years ago
(In reply to comment #31)
> > We're going to give significant context menu items icons in the next release,
> > so we might as well start now.  Icons in context menus is native on Vista and 7
> > (but only for some items in each menu).  We need to provide the user with some
> > form of clue as to how they could get this command enabled, and the icon does
> > that.
> 
> The icon actually does not give a clue, as the panorama button isn't on the
> toolbar by default.

That icon is also being used for the Panorama menu item (that's being moved to the "list all tabs" menu), see bug 625320; we need it on both of them to tie it together.

> > > > As for running in a window without groups, we need some way to detect if the
> > > > window has groups. I suppose we could install the key handler at start up if
> > > > there are hidden tabs, and if there weren't tabs hidden to begin with, listen
> > > > for hide events. Does that sound better?
> > > 
> > > That's not sufficient, since tabs can be hidden outside of panorama land. See
> > > e.g.
> > > http://hg.mozilla.org/mozilla-central/annotate/e98b94aa64fa/browser/base/content/browser.js#l8343
> > 
> > I was referring to the hide event: 
> > 
> > http://hg.mozilla.org/mozilla-central/annotate/e98b94aa64fa/browser/base/content/tabbrowser.xml#l1947
> > 
> > That should cover it, right?
> 
> The code I linked to triggers the event.

Ah, you mean it'll be a false positive? Sorry, I didn't catch that. Good point. 

I think tabs being in a hidden state on startup is a pretty good indication, though. Let's check for that at startup, and attach the handler then if there are hidden tabs. Otherwise we should attach the handler when Panorama is first started during the session.
(In reply to comment #32)
> (In reply to comment #31)
> > > We're going to give significant context menu items icons in the next release,
> > > so we might as well start now.  Icons in context menus is native on Vista and 7
> > > (but only for some items in each menu).  We need to provide the user with some
> > > form of clue as to how they could get this command enabled, and the icon does
> > > that.
> > 
> > The icon actually does not give a clue, as the panorama button isn't on the
> > toolbar by default.
> 
> That icon is also being used for the Panorama menu item (that's being moved to
> the "list all tabs" menu), see bug 625320; we need it on both of them to tie it
> together.

Can we please fix this bug in the most straightforward way and move on? An always-disabled item just isn't helpful, bug 625320 doesn't change this.
>The icon actually does not give a clue, as the panorama button isn't on the
>toolbar by default.

>An always-disabled item just isn't helpful, bug 625320 doesn't change this.

Here's the sequence of actions I'm hoping for (although not necessarily in this order)

1) the user encounters "move to group" with a unique symbol (4 interlocking squares) and thinks "hm, seems interesting but isn't enabled, I wonder what the deal with that is"

2) user clicks on list all tabs, and perhaps they have a tremendously large number of tabs open (which is entirely possible if they are taking advantage of the list all tabs menu).  At the top is the solution to the problem that they didn't even know they had: "Group your tabs."  again the user sees a unique symbol of 4 interlocking squares

[2.5 - optional - the user reads a review of Firefox 4 that covers panorama and tab groups, and shows them the unique symbol for the feature, introducing what it does]

3) now a toolbar button is available with that same unique symbol, and (if they have created another group) the context menu action is turned on.  Even if the user forgets about one of the entry points (or a new user sits down at the machine) the three interfaces visually reference each other.
2) and 3) make sense, I just don't see the critical role of 1) in there. 2) might happen weeks after 1), or never. Why would we intentionally puzzle the user like this?
Assignee

Comment 36

9 years ago
Posted patch v3 (obsolete) — Splinter Review
Hide the "Move to group" menu item if the experienced_first_run pref hasn't been set.  Keeping the icon for the "Move to group" menu item.
Attachment #506684 - Attachment is obsolete: true
Attachment #508324 - Flags: review?(ian)
Comment on attachment 508324 [details] [diff] [review]
v3

r- on the browser/themes/ part, menu icons need to be 16*16px.
Attachment #508324 - Flags: review-
Assignee

Comment 38

9 years ago
Posted patch v4 (obsolete) — Splinter Review
Checked and updated the CSS for 16x16 icons.  They look fine on different platforms
Attachment #508324 - Attachment is obsolete: true
Attachment #508331 - Flags: review?(dao)
Attachment #508324 - Flags: review?(ian)
Comment on attachment 508331 [details] [diff] [review]
v4

>--- a/browser/themes/pinstripe/browser/browser.css
>+++ b/browser/themes/pinstripe/browser/browser.css
>@@ -582,16 +582,21 @@ toolbar[iconsize="small"][mode="icons"] 
> #tabview-button[groups="2"] {
>   -moz-image-region: rect(0, 60px, 20px, 40px);
> }
> 
> #tabview-button[groups="3"] {
>   -moz-image-region: rect(0, 80px, 20px, 60px);
> }
> 
>+#context_tabViewMenu {
>+  list-style-image: url(chrome://browser/skin/tabview/tabview.png);
>+  -moz-image-region: rect(2px, 18px, 18px, 2px);
>+}

I think this might be a problem on OS X (cf. bug 603605), but I have no way of testing this.
Can you move the iconification to a separate bug? I don't think it belongs in here.
Attachment #508331 - Flags: review?(dao) → review-
Assignee

Comment 40

9 years ago
Posted patch v5 (obsolete) — Splinter Review
Removed the icons and filed bug 630128
Attachment #508331 - Attachment is obsolete: true
Attachment #508334 - Flags: review?(dao)
Assignee

Updated

9 years ago
Attachment #508334 - Attachment is patch: true
Attachment #508334 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 508334 [details] [diff] [review]
v5

>   init: function TabView_init() {
>-    // ___ keys    
>-    this._setBrowserKeyHandlers();
>+    let self = this;
>+
>+    // ___ keys
>+    if ((gBrowser.tabs.length - gBrowser.visibleTabs.length) > 0)
>+      this._setBrowserKeyHandlers();

Will session restore already have hidden the tabs at this point?

>     // ___ visibility
>     this._sessionstore =
>       Cc["@mozilla.org/browser/sessionstore;1"].
>         getService(Ci.nsISessionStore);

Btw, why this._sessionstore rather than a local variable?

>+    if (!Services.prefs.prefHasUserValue("browser.panorama.experienced_first_run") ||
>+        !Services.prefs.getBoolPref("browser.panorama.experienced_first_run")) {
>+      let prefsObserver = function() {
>+        Services.prefs.removeObserver(
>+          "browser.panorama.experienced_first_run", prefsObserver);
>+        self._firstRunExperienced = true;
>+      };
>+      Services.prefs.addObserver(
>+        "browser.panorama.experienced_first_run", prefsObserver, false);
>+      this._firstRunExperienced = false;

nit: last line is redundant

>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js

>+    document.getElementById("context_tabViewMenu").hidden = 

nit: trailing space
Assignee

Comment 42

9 years ago
Posted patch v5 (obsolete) — Splinter Review
(In reply to comment #41)
> Comment on attachment 508334 [details] [diff] [review]
> v5
> 
> >   init: function TabView_init() {
> >-    // ___ keys    
> >-    this._setBrowserKeyHandlers();
> >+    let self = this;
> >+
> >+    // ___ keys
> >+    if ((gBrowser.tabs.length - gBrowser.visibleTabs.length) > 0)
> >+      this._setBrowserKeyHandlers();
> 
> Will session restore already have hidden the tabs at this point?

Yes, I've checked that the session restore already loads and hides the tabs before this point.

> 
> >     // ___ visibility
> >     this._sessionstore =
> >       Cc["@mozilla.org/browser/sessionstore;1"].
> >         getService(Ci.nsISessionStore);
> 
> Btw, why this._sessionstore rather than a local variable?

Fixed

> 
> >+    if (!Services.prefs.prefHasUserValue("browser.panorama.experienced_first_run") ||
> >+        !Services.prefs.getBoolPref("browser.panorama.experienced_first_run")) {
> >+      let prefsObserver = function() {
> >+        Services.prefs.removeObserver(
> >+          "browser.panorama.experienced_first_run", prefsObserver);
> >+        self._firstRunExperienced = true;
> >+      };
> >+      Services.prefs.addObserver(
> >+        "browser.panorama.experienced_first_run", prefsObserver, false);
> >+      this._firstRunExperienced = false;
> 
> nit: last line is redundant

Removed.

> 
> >--- a/browser/base/content/browser.js
> >+++ b/browser/base/content/browser.js
> 
> >+    document.getElementById("context_tabViewMenu").hidden = 
> 
> nit: trailing space

Fixed.
Attachment #508334 - Attachment is obsolete: true
Attachment #508338 - Flags: review?(dao)
Attachment #508334 - Flags: review?(dao)
Comment on attachment 508338 [details] [diff] [review]
v5

>--- a/browser/base/content/browser-tabview.js
>+++ b/browser/base/content/browser-tabview.js

>   _sessionstore: null,

This is now unused.

>   init: function TabView_init() {
>-    // ___ keys    
>-    this._setBrowserKeyHandlers();
>+    let self = this;
>+
>+    // ___ keys
>+    if ((gBrowser.tabs.length - gBrowser.visibleTabs.length) > 0)
>+      this._setBrowserKeyHandlers();
>+
>+    if (!Services.prefs.prefHasUserValue("browser.panorama.experienced_first_run") ||
>+        !Services.prefs.getBoolPref("browser.panorama.experienced_first_run")) {
>+      let prefsObserver = function() {
>+        Services.prefs.removeObserver(
>+          "browser.panorama.experienced_first_run", prefsObserver);
>+        self._firstRunExperienced = true;
>+      };
>+      Services.prefs.addObserver(
>+        "browser.panorama.experienced_first_run", prefsObserver, false);
>+    } else {
>+      this._firstRunExperienced = true;
>+    }
> 
>     // ___ visibility
>-    this._sessionstore =
>-      Cc["@mozilla.org/browser/sessionstore;1"].
>-        getService(Ci.nsISessionStore);
>-
>-    let data = this._sessionstore.getWindowValue(window, this.VISIBILITY_IDENTIFIER);
>+    let sessionstore =
>+      Cc["@mozilla.org/browser/sessionstore;1"].getService(Ci.nsISessionStore);
>+    let data = sessionstore.getWindowValue(window, this.VISIBILITY_IDENTIFIER);
> 
>     if (data && data == "true") {
>       this.show();
>     } else {

Can you avoid some work for browser.panorama.experienced_first_run = false, like checking ((gBrowser.tabs.length - gBrowser.visibleTabs.length) > 0) and getting the sessionstore service?
Assignee

Comment 44

9 years ago
Posted patch v6 (obsolete) — Splinter Review
Removed some work for browser.panorama.experienced_first_run = false i.e.
checking ((gBrowser.tabs.length - gBrowser.visibleTabs.length) > 0) and
getting the sessionstore service.
Attachment #508338 - Attachment is obsolete: true
Attachment #508670 - Flags: review?(dao)
Attachment #508338 - Flags: review?(dao)
Comment on attachment 508670 [details] [diff] [review]
v6

>   init: function TabView_init() {
>-    // ___ keys    
>-    this._setBrowserKeyHandlers();
>+    let self = this;

Please move this down to the branches it's needed in.

>+  _setBrowserKeyHandlers: function() {
>     let self = this;
> 
>+    if (this._browserKeyHandlerInitialized)
>+      return;
>+
>+    this._browserKeyHandlerInitialized = true;
>+
>     window.addEventListener("keypress", function(event) {
>-      if (self.isVisible())
>+      if (self.isVisible() ||
>+          (gBrowser.tabs.length - gBrowser.visibleTabs.length) == 0)
>         return;

let self = this; should be right before window.addEventListener.

The test is using 'win.window', which I suspect is going to be identical to 'win', in which case you should write just that.
Attachment #508670 - Flags: review?(dao) → review+
Comment on attachment 508670 [details] [diff] [review]
v6

>+    if (!Services.prefs.prefHasUserValue("browser.panorama.experienced_first_run") ||
>+        !Services.prefs.getBoolPref("browser.panorama.experienced_first_run")) {
>+      let prefsObserver = function() {
>+        Services.prefs.removeObserver(
>+          "browser.panorama.experienced_first_run", prefsObserver);
>+        self._firstRunExperienced = true;
>+      };
>+      Services.prefs.addObserver(
>+        "browser.panorama.experienced_first_run", prefsObserver, false);

Actually, I think you need to remove the observer when the window closes. To that end, can you make the TabView object implement an 'observe' method and directly add/remove 'this' as the observer?
Attachment #508670 - Flags: review+ → review-
>2) and 3) make sense, I just don't see the critical role of 1)

Critical role is probably overselling it, but I do think that users who currently rely on the command "bookmark all tabs" in that menu are going to become curious about the idea of tab groups, since it is pretty much built exactly for them.

Also, the existence of the icon isn't meant to make it super special (as in the only context menu item in all of Firefox that has an icon), but rather just special (one of the top 2 or 3 items in that particular menu that has an icon.
Related comment on this not being the only icon in the menu: https://bugzilla.mozilla.org/show_bug.cgi?id=630128#c15
>uiwanted
ignore, just testing out search > whine event > email > sms
Keywords: uiwanted
Assignee

Comment 50

9 years ago
Posted patch v7 (obsolete) — Splinter Review
Attachment #508670 - Attachment is obsolete: true
Attachment #509056 - Flags: review?(dao)
(In reply to comment #47)
> >2) and 3) make sense, I just don't see the critical role of 1)
> 
> Critical role is probably overselling it, but I do think that users who
> currently rely on the command "bookmark all tabs" in that menu are going to
> become curious about the idea of tab groups, since it is pretty much built
> exactly for them.

Unfortunately we can't detect these users. (Well, technically we could, but we don't do it right now, and the idea seems a bit strange.) I've never seen anyone bookmark all tabs. If it's as low-usage as I suspect, it shouldn't really be considered relevant here.
Comment on attachment 509056 [details] [diff] [review]
v7

>+  uninit: function TabView_uninit() {

>+    if (this._tabShowEventListener) {
>+      gBrowser.tabContainer.removeEventListener(
>         "TabShow", this._tabShowEventListener, true);
>     }

This should be needed, as this event listener shouldn't keep the window alive.
Attachment #509056 - Flags: review?(dao) → review+
http://hg.mozilla.org/mozilla-central/rev/f2a6a78478dc
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b12
verified with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre
Status: RESOLVED → VERIFIED
Whiteboard: [softblocker] → [softblocker][fx4-fixed-bugday]

Comment 56

9 years ago
In my nightly "move to group" is disabled even if I have run Panorama. Which I use a lot, BTW, and this really annoys me, considering the fact that the feature has become part of my regular usage pattern.

I does not appear to be bug 628594 because I do have other named groups. (I have 7 groups, all named.)

Should I file a bug?
(In reply to comment #56)
> In my nightly "move to group" is disabled even if I have run Panorama. Which I
> use a lot, BTW, and this really annoys me, considering the fact that the
> feature has become part of my regular usage pattern.

This will be fixed by bug 635051, sorry for the inconvenience.

> I does not appear to be bug 628594 because I do have other named groups. (I
> have 7 groups, all named.)

For a temporary workaround you can just create a new group and close it. After that the menu items should be available again.
I'm on the 2/22 nightly, am a regular panorama users, and don't see this command at all (completely gone, as opposed to being present and disabled).  Is there another bug still open, or should re re-open this one.
(In reply to comment #58)
> I'm on the 2/22 nightly, am a regular panorama users, and don't see this
> command at all (completely gone, as opposed to being present and disabled).  Is
> there another bug still open, or should re re-open this one.

It's bug 635051 that is fixed now but unfortunately not in b12. You can set the name of some group in panorama and the menu items are permanently enabled again.
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.