Tim Taubert [:ttaubert] (inactive) Reporter
	Description • 13 years ago

https://wiki.mozilla.org/Simplify_Panorama_UI

	Raymond Lee [:raymondlee] Assignee
	Comment 1 • 13 years ago

Please apply patch for bug 663612 before this one

	Raymond Lee [:raymondlee] Assignee
	Comment 2 • 13 years ago

(In reply to comment #1)
> Created attachment 539465 [details] [diff] [review] [review]
> v1
> 
> Please apply patch for bug 663612 before this one

Passed Try
http://tbpl.mozilla.org/?tree=Try&rev=b3c639db61ed

	Tim Taubert [:ttaubert] (inactive) Reporter
	Comment 3 • 13 years ago

Comment on attachment 539465 [details] [diff] [review]
v1

Review of attachment 539465 [details] [diff] [review]:
-----------------------------------------------------------------

Does that patch also remove the file "new-tab.png"?

::: browser/base/content/test/tabview/browser_tabview_bug588265.js
@@ +11,5 @@
> +    while (gBrowser.tabs[1])
> +      gBrowser.removeTab(gBrowser.tabs[1]);
> +    hideTabView(function() {});
> +  });
> +  gBrowser.loadOneTab("about:blank", { inBackground: false });

Please change inBackground to "true" so that we don't trigger a transition here just in case the tabview is already shown.

@@ +60,5 @@
>        is(groupItemOne.getChildren().length, 2, 
>           "The num of childen in group one is 2");
>  
>        // clean up and finish
>        groupItemTwo.addSubscriber(groupItemTwo, "close", function() {

You could use closeGroupItem(groupItemTwo, callback) here.

::: browser/base/content/test/tabview/browser_tabview_bug590606.js
@@ +36,5 @@
>         "The currently selected tab should be the first tab in the groupItemOne");
>  
>      // create another group with a tab.
> +    let groupItemTwo = createGroupItemWithBlankTabs(window, 300, 300, 40, 1);
> +    groupItemTwoId = groupItemTwoId;

groupItemTwoId = groupItemTwo.id;

::: browser/base/content/test/tabview/browser_tabview_bug595560.js
@@ +35,5 @@
>  }
>  
>  function testThree() {
> +  // create another group with a tab.
> +  let groupItem = createGroupItemWithBlankTabs(win, 300, 300, 40, 1);

Is there a reason why you changed the groupItem's padding?

@@ +37,5 @@
>  function testThree() {
> +  // create another group with a tab.
> +  let groupItem = createGroupItemWithBlankTabs(win, 300, 300, 40, 1);
> +  hideTabView(function() {
> +    showTabView(function() {

Is hiding and showing the tabview here part of the test? That seems superfluous.

@@ +67,3 @@
>  }
>  
>  function whenSearchEnabledAndDisabled(callback) {

Nit: That function name does not correctly describe what the function is doing.

::: browser/base/content/test/tabview/browser_tabview_group.js
@@ +14,3 @@
>    ok(TabView.isVisible(), "Tab View is visible");
>  
>    let contentWindow = document.getElementById("tab-view").contentWindow;

Please make that TabView.getContentWindow().

::: browser/base/content/test/tabview/browser_tabview_orphaned_tabs.js
@@ +18,3 @@
>    ok(newWin.TabView.isVisible(), "Tab View is visible");
>  
>    let contentWindow = newWin.document.getElementById("tab-view").contentWindow;

newWin.TabView.getContentWindow(), please :)

@@ +23,5 @@
>    ok(tabOne._tabViewTabItem.parent, "Tab one belongs to a group");
>    is(contentWindow.GroupItems.getOrphanedTabs().length, 0, "No orphaned tabs");
>  
> +  // 2) create a group, add a blank tab
> +  let groupItem = createGroupItemWithBlankTabs(newWin, 250, 250, 40, 1);

Is there a reason why you changed the groupItem's size and padding?

::: browser/base/content/test/tabview/browser_tabview_undo_group.js
@@ +10,5 @@
> +    while (gBrowser.tabs[1])
> +      gBrowser.removeTab(gBrowser.tabs[1]);
> +    hideTabView(function() {});
> +  });
> +  showTabView(onTabViewWindowLoaded)

Nit: missing semicolon

::: browser/base/content/test/tabview/head.js
@@ +33,5 @@
>      ++t;
>    });
> +  // to set one of tabItem to be active since we load tabs into a group 
> +  // in a non-standard flow.
> +  contentWindow.UI.setActive(groupItem);

Shouldn't that be done by https://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabview/groupitems.js#1023 - if not we should rather fix it there? The workflow doesn't seem so non-standard.

	Raymond Lee [:raymondlee] Assignee
	Comment 4 • 13 years ago

(In reply to comment #3)
> Comment on attachment 539465 [details] [diff] [review] [review]
> v1

Fixed everything except the following

> 
> ::: browser/base/content/test/tabview/head.js
> @@ +33,5 @@
> >      ++t;
> >    });
> > +  // to set one of tabItem to be active since we load tabs into a group 
> > +  // in a non-standard flow.
> > +  contentWindow.UI.setActive(groupItem);
> 
> Shouldn't that be done by
> https://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabview/
> groupitems.js#1023 - if not we should rather fix it there? The workflow
> doesn't seem so non-standard.

The workflow is a non-standard one.  User wouldn't be able to create an empty group item with a tab item in it, and get the tab item selected (with blue border) in Panorama without going through:
1) Drag and drop to create a group
2) Double click to create a tab item
3) Zoom into animation happens automatically and UI.setActive() would be called in TabItem_zoomIn()
4) UI.setActive() is also get called in UI_showTabView();

The createGroupItemWithTabs in head.js doesn't do step 3 or 4 so the tab item doesn't get selected.  Since that is a test helper function and not doing a standard workflow, IMO we shouldn't do anything in the Panorama code.

	Tim Taubert [:ttaubert] (inactive) Reporter
	Comment 5 • 13 years ago

(In reply to comment #4)
> The workflow is a non-standard one.  User wouldn't be able to create an
> empty group item with a tab item in it, and get the tab item selected (with
> blue border) in Panorama without going through:
> 1) Drag and drop to create a group
> 2) Double click to create a tab item
> 3) Zoom into animation happens automatically and UI.setActive() would be
> called in TabItem_zoomIn()
> 4) UI.setActive() is also get called in UI_showTabView();
> 
> The createGroupItemWithTabs in head.js doesn't do step 3 or 4 so the tab
> item doesn't get selected.  Since that is a test helper function and not
> doing a standard workflow, IMO we shouldn't do anything in the Panorama code.

Convinced :)

	Ian Gilman [:iangilman]
	Comment 6 • 13 years ago

Comment on attachment 540968 [details] [diff] [review]
v2

Review of attachment 540968 [details] [diff] [review]:
-----------------------------------------------------------------

Everything else looks good.

::: browser/base/content/test/tabview/browser_tabview_bug588265.js
@@ +11,5 @@
> +    while (gBrowser.tabs[1])
> +      gBrowser.removeTab(gBrowser.tabs[1]);
> +    hideTabView(function() {});
> +  });
> +  gBrowser.loadOneTab("about:blank", { inBackground: true });

Why are we loading a tab here? The rest of the changes to this test seem natural enough, but this looks like an addition (unless I'm missing something).

	Raymond Lee [:raymondlee] Assignee
	Comment 7 • 13 years ago

(In reply to comment #6)
> Comment on attachment 540968 [details] [diff] [review] [review]
> v2
> 
> Review of attachment 540968 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> Everything else looks good.
> 
> ::: browser/base/content/test/tabview/browser_tabview_bug588265.js
> @@ +11,5 @@
> > +    while (gBrowser.tabs[1])
> > +      gBrowser.removeTab(gBrowser.tabs[1]);
> > +    hideTabView(function() {});
> > +  });
> > +  gBrowser.loadOneTab("about:blank", { inBackground: true });
> 
> Why are we loading a tab here? The rest of the changes to this test seem
> natural enough, but this looks like an addition (unless I'm missing
> something).

Since the test needs to have two tab items in a group, the original code shows tabView, adds a tab item, hides tabView, etc. I think it would be much simple to add a new tab so when tabView is shown, two tab items would be in a group item.

	Raymond Lee [:raymondlee] Assignee
	Comment 8 • 13 years ago

Please apply patch for bug 663612 before this.

Passed try.  There are two intermittents which aren't related to this.
http://tbpl.mozilla.org/?tree=Try&rev=fbfba4260605

	Ian Gilman [:iangilman]
	Comment 9 • 13 years ago

(In reply to comment #7)
> Since the test needs to have two tab items in a group, the original code
> shows tabView, adds a tab item, hides tabView, etc. I think it would be much
> simple to add a new tab so when tabView is shown, two tab items would be in
> a group item.

Thank you for the explanation!

	Tim Taubert [:ttaubert] (inactive) Reporter
	Comment 10 • 13 years ago

Comment on attachment 540968 [details] [diff] [review]
v2

This patch landed on ux-branch yesterday.

	Alex Limi (:limi) — Firefox UX Team
	Comment 11 • 13 years ago

Comment on attachment 540968 [details] [diff] [review]
v2

Works as intended!

	Tim Taubert [:ttaubert] (inactive) Reporter
	Comment 12 • 13 years ago

http://hg.mozilla.org/integration/fx-team/rev/54ed6181b38c

	Siddhartha Dugar [:sdrocking]
	Comment 13 • 13 years ago

The button has been removed (in latest UX builds) but still tab thumbnails are not allowed in the bottom area of the tab groups. Please verify and fix this.

	Raymond Lee [:raymondlee] Assignee
	Comment 14 • 13 years ago

(In reply to comment #13)
> The button has been removed (in latest UX builds) but still tab thumbnails
> are not allowed in the bottom area of the tab groups. Please verify and fix
> this.

That is related to bug 595224

	Tim Taubert [:ttaubert] (inactive) Reporter
	Comment 15 • 13 years ago

http://hg.mozilla.org/mozilla-central/rev/54ed6181b38c

	Andreas Jung
	Comment 16 • 13 years ago

(In reply to comment #14)
> (In reply to comment #13)
> > The button has been removed (in latest UX builds) but still tab thumbnails
> > are not allowed in the bottom area of the tab groups. Please verify and fix
> > this.
> 
> That is related to bug 595224

Well, the bottom 33px seem to be never used because of groupitems.js:
509     box.height -= 33; // For new tab button

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabview/groupitems.js#509

Now that the new tab button is removed this line should probably go away as well.

	Tim Taubert [:ttaubert] (inactive) Reporter
	Comment 17 • 13 years ago

(In reply to comment #16)
> Now that the new tab button is removed this line should probably go away as
> well.

Thanks, filed follow-up bug 673825.

	Virgil Dicu [:virgil] [QA]
	Comment 18 • 13 years ago

Mozilla/5.0 (X11; Linux x86_64; rv:9.0a1) Gecko/20110907 Firefox/9.0a1

The "new tab" button has been removed. Setting to Verified Fixed.