Closed Bug 626500 Opened 14 years ago Closed 14 years ago

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

Categories

(Firefox :: Toolbars and Customization, defect)

Product:
Firefox
Component:
Toolbars and Customization
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 4.0b10
Tracking Flags:
Tracking Status
blocking2.0 --- final+

People

(Reporter: dao, Assigned: ehsan.akhgari)

References

Details

(Whiteboard: [hardblocker])

Attachments

(2 files, 4 obsolete files)

Part 1: Remove the panorama button from the default toolbar set
(deleted), patch
Details | Diff | Splinter Review
Part 3: test adjustments
(deleted), patch
Details | Diff | Splinter Review
blocking2.0: --- → ?
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.
blocking2.0: ? → final+
Whiteboard: [hardblocker]
(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.
Blocks: 626527
Attachment #504588 - Flags: review?(dao)
Assignee: nobody → ehsan
Attached patch Part 3: test adjustments (obsolete) (deleted) — — Splinter Review
These test adjustments are needed because the button doesn't exist by default any more.
Attachment #504594 - Flags: review?(ian)
Whiteboard: [hardblocker] → [hardblocker][has patch][needs review dao/ian]
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 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.
Attachment #504588 - Flags: review?(dao) → review-
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?
Attachment #504593 - Flags: review?(dao) → review-
Attachment #504588 - Attachment is obsolete: true
Attachment #504841 - Flags: review?(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".
Attachment #504841 - Flags: review?(dao) → review+
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.
Attachment #504593 - Attachment is obsolete: true
Attachment #504841 - Attachment is obsolete: true
Whiteboard: [hardblocker][has patch][needs review dao/ian] → [hardblocker][has patch][needs review ian]
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
Attachment #504594 - Flags: review?(ian) → review+
Attached patch Part 3: test adjustments (deleted) — — Splinter Review
(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.
Attachment #504594 - Attachment is obsolete: true
Whiteboard: [hardblocker][has patch][needs review ian] → [hardblocker][needs landing]
(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.
http://hg.mozilla.org/mozilla-central/rev/411c730f8617 http://hg.mozilla.org/mozilla-central/rev/64c98e4e1797
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [hardblocker][needs landing] → [hardblocker]
Target Milestone: --- → Firefox 4.0b10
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: