626500 - Don't put the Panorama button on the tab bar by default

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Dão Gottwald [:dao]]

See bug 596017 comment 69

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

Further, I'd support it if the Panorama button was added to the toolbar the first time it was activated, since that indicates a user who has decided they want to use the feature.

Comment 2

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #1)
> Further, I'd support it if the Panorama button was added to the toolbar the
> first time it was activated, since that indicates a user who has decided they
> want to use the feature.

Such handling could be done in Panorama itself, during the "firstrun" behavior (displaying the welcome tab), which is tracked in the browser.panorama.experienced_firstrun pref.

Comment 3

[Ian Gilman [:iangilman]]

Related: bug 626525

Comment 4

[(no longer active)]

Attachment: Part 1: Remove the panorama button from the default toolbar set

Comment 5

[(no longer active)]

Attachment: Part 2: Add the Panorama button to the toolbar after the first time that the user enters Panorama

Comment 6

[(no longer active)]

Attachment: Part 3: test adjustments

These test adjustments are needed because the button doesn't exist by default any more.

Comment 7

[Raymond Lee [:raymondlee]]

Comment on attachment 504593 [details] [diff] [review]
Part 2: Add the Panorama button to the toolbar after the first time that the user enters Panorama

If user has multiple browser windows opened, then enters Panorama for the first time.  Does this patch add the tabview-button to all existing browser windows?

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 504588 [details] [diff] [review]
Part 1: Remove the panorama button from the default toolbar set

The browserShared.inc change is wrong, #tabview-button needs to be part of that list just like the other optional buttons.

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 504593 [details] [diff] [review]
Part 2: Add the Panorama button to the toolbar after the first time that the user enters Panorama

>+      if (firstTime) {
>+        gWindow.document.getElementById("TabsToolbar").insertItem("tabview-button",
>+          gWindow.document.getElementById("alltabs-button"));

This doesn't persist the currentset, does it?

What if alltabs-button has been removed? Seems like you'd append after tabs-closebutton then.

Also, can we move this to a separate bug?

Comment 10

[(no longer active)]

Attachment: Part 1: Remove the panorama button from the default toolbar set

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 504841 [details] [diff] [review]
Part 1: Remove the panorama button from the default toolbar set

>+      <toolbarbutton id="tabview-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
>+                     label="&tabGroupsButton.label;"
>+                     command="Browser:ToggleTabView"
>+                     tooltiptext="&tabGroupsButton.tooltip;"
>+                     removable="true"
>+                     observes="tabviewGroupsNumber"/>

Remove removable="true".

Comment 12

[(no longer active)]

Comment on attachment 504593 [details] [diff] [review]
Part 2: Add the Panorama button to the toolbar after the first time that the user enters Panorama

(In reply to comment #9)
> Comment on attachment 504593 [details] [diff] [review]
> Part 2: Add the Panorama button to the toolbar after the first time that the
> user enters Panorama
> 
> >+      if (firstTime) {
> >+        gWindow.document.getElementById("TabsToolbar").insertItem("tabview-button",
> >+          gWindow.document.getElementById("alltabs-button"));
> 
> This doesn't persist the currentset, does it?
> 
> What if alltabs-button has been removed? Seems like you'd append after
> tabs-closebutton then.
> 
> Also, can we move this to a separate bug?

I filed bug 626791 for that.  I'm not going to work on it though, since I seem to not be familiar enough with the toolbar stuff.

Comment 13

[(no longer active)]

Attachment: Part 1: Remove the panorama button from the default toolbar set

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 504594 [details] [diff] [review]
Part 3: test adjustments

>--- a/browser/base/content/test/tabview/browser_tabview_launch.js
>+++ b/browser/base/content/test/tabview/browser_tabview_launch.js
>@@ -42,18 +42,20 @@ function test() {
>   waitForExplicitFinish();
> 
>   // verify initial state
>   ok(!TabView.isVisible(), "Tab View starts hidden");
> 
>   // use the Tab View button to launch it for the first time
>   window.addEventListener("tabviewshown", onTabViewLoadedAndShown, false);
>   let button = document.getElementById("tabview-button");
>-  ok(button, "Tab View button exists");
>-  button.doCommand();
>+  ok(!button, "Tab View button not exist by default");
>+  let buttonCommand = document.getElementById("Browser:ToggleTabView");
>+  ok(buttonCommand, "The button command should exist, however");
>+  buttonCommand.doCommand();

s/buttonCommand/tabViewCommand/

The button isn't of interest here, I don't think there's a point in verifying that it doesn't exist.

Also, buttonCommand.doCommand() makes ok(buttonCommand, ...) redundant.

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

> function toggleTabView() {
>   let button = document.getElementById("tabview-button");
>-  ok(button, "Tab View button exists");
>-  button.doCommand();
>+  ok(!button, "Tab View button not exist by default");
>+  let buttonCommand = document.getElementById("Browser:ToggleTabView");
>+  ok(buttonCommand, "The button command should exist, however");
>+  buttonCommand.doCommand();
> }

ditto

Comment 15

[(no longer active)]

Attachment: Part 3: test adjustments

(In reply to comment #14)
> The button isn't of interest here, I don't think there's a point in verifying
> that it doesn't exist.

That's the whole point behind this bug, so I think I'm going to keep that check around.

Comment 16

[Dão Gottwald [:dao]]

(In reply to comment #15)
> Created attachment 505095 [details] [diff] [review]
> Part 3: test adjustments
> 
> (In reply to comment #14)
> > The button isn't of interest here, I don't think there's a point in verifying
> > that it doesn't exist.
> 
> That's the whole point behind this bug, so I think I'm going to keep that check
> around.

It's not the point of the test. All the test wants is to open panorama.

Comment 17

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/411c730f8617
http://hg.mozilla.org/mozilla-central/rev/64c98e4e1797