Last Comment Bug 715454 - Current group is switched when using switch-to-tab and opening new tab
: Current group is switched when using switch-to-tab and opening new tab
Status: RESOLVED FIXED
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: -- normal
: Firefox 14
Assigned To: Mario Alvarado [:marioalv]
:
Mentors:
: 653235 (view as bug list)
Depends on:
Blocks: 716880
  Show dependency treegraph
 
Reported: 2012-01-05 02:12 PST by Virgil Dicu [:virgil] [QA]
Modified: 2016-04-12 14:00 PDT (History)
7 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1 (25.70 KB, patch)
2012-03-01 14:51 PST, Mario Alvarado [:marioalv]
ttaubert: review-
Details | Diff | Splinter Review
Log after executing the mochi tests with the changes from comment 9 . (5.01 KB, text/plain)
2012-03-02 13:34 PST, Mario Alvarado [:marioalv]
no flags Details
v2 (2.81 KB, patch)
2012-03-05 16:16 PST, Mario Alvarado [:marioalv]
ttaubert: feedback+
Details | Diff | Splinter Review
Proposed test case for this bug. v1. (3.50 KB, patch)
2012-03-07 10:07 PST, Mario Alvarado [:marioalv]
no flags Details | Diff | Splinter Review
Proposed test case for this bug. v2. (3.29 KB, patch)
2012-03-08 20:25 PST, Mario Alvarado [:marioalv]
ttaubert: review-
Details | Diff | Splinter Review
v3 (3.43 KB, patch)
2012-03-08 20:35 PST, Mario Alvarado [:marioalv]
no flags Details | Diff | Splinter Review
v4 (10.30 KB, patch)
2012-03-12 12:02 PDT, Mario Alvarado [:marioalv]
ttaubert: feedback+
Details | Diff | Splinter Review
v5 (9.66 KB, patch)
2012-03-16 17:19 PDT, Mario Alvarado [:marioalv]
ttaubert: review+
Details | Diff | Splinter Review
Patch for checkin (2.17 KB, patch)
2012-03-19 21:07 PDT, Mario Alvarado [:marioalv]
no flags Details | Diff | Splinter Review
Patch for checkin (11.46 KB, patch)
2012-03-19 22:49 PDT, Mario Alvarado [:marioalv]
ttaubert: review+
Details | Diff | Splinter Review

Description Virgil Dicu [:virgil] [QA] 2012-01-05 02:12:23 PST
Mozilla/5.0 (X11; Linux x86_64; rv:12.0a1) Gecko/20120104 Firefox/12.0a1
Mozilla/5.0 (X11; Linux x86_64; rv:10.0) Gecko/20100101 Firefox/10.0 Beta 3

STR:

1. Start Firefox.
2. Create two groups of at least one tab each (e.g. group A and group B).
3. Open a new tab from one of the groups (e.g. Group A).
4. Type the name of a tab from Group B.
5. Switch to that tab using switch to tab from the dropdown options. (current group B)
6. Open a new tab from that group (Group B).

Actual result: the new tab is opened in group A.

Occurs on F4.0.1 as well, so no regression.

Thought I saw a related bug for this some time ago, but couldn't find it after searching.
Comment 1 Tim Taubert [:ttaubert] 2012-01-08 10:30:36 PST
FTR, this is caused by the call from https://mxr.mozilla.org/mozilla-central/source/browser/components/tabview/groupitems.js#1080

1) switchToTabHavingURI() is called
2) the selected tab is now the tab we switched to
3) the blank tab is closed
4) the old group gets activated again by groupitems.js:1080
Comment 2 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-01-09 15:45:03 PST
@Tim is this expected/designed behaviour or is it a bug in groupitems.js?
Comment 3 Tim Taubert [:ttaubert] 2012-01-09 15:46:32 PST
No, this is definitely a bug and should be fixed.
Comment 4 Marcos Aruj 2012-01-09 15:56:43 PST
I'm looking at it right now. I'll get back to you soon.
Comment 5 Tim Taubert [:ttaubert] 2012-01-10 07:39:04 PST
We should include a test for bug 716880 as well. That's the same issue with just a slightly different STR.
Comment 6 Marcos Aruj 2012-01-10 22:09:25 PST
Hi Tim,

Looking at the code and doing some testing, the problem might be fixed by checking if the current active group is the one where the empty tab is being closed. If I add a check for this before calling UI.setActive in groupitems.js, the problem is fixed.

However, some tests fail, including the ones for this bug:

https://bugzilla.mozilla.org/show_bug.cgi?id=588265

Those tests check specifically if the tab group from which the tab was closed is now active. 

There's some discussions on that bug that I think are related to the behavior in our bug and how to fix it.

For example in comment # 2 from Aza:

"What this means is that if you create a new tab via command-t, it will appear in the last group you interacted with (i.e. B), but if you hit return it will activate the selected tab in A."

Wyt?

Cheers,

Marcos.
Comment 7 Mario Alvarado [:marioalv] 2012-02-25 00:23:19 PST
Hi everyone.
Fooling around with the code, I noticed that, if you comment this line https://mxr.mozilla.org/mozilla-central/source/browser/components/tabview/groupitems.js#1080 (the call to UI.setActive(this); ) and follow the STR from the bug's description, the new tab in opened in group B. This is the expected behavior.

I also tested the change with 3 tab groups, switching between groups and opening new tabs. Everything was working OK after the code change.

What do you think?
Comment 8 Raymond Lee [:raymondlee] 2012-02-25 07:38:37 PST
(In reply to Mario Alvarado [:marioalv] from comment #7)
> Hi everyone.
> Fooling around with the code, I noticed that, if you comment this line
> https://mxr.mozilla.org/mozilla-central/source/browser/components/tabview/
> groupitems.js#1080 (the call to UI.setActive(this); ) and follow the STR
> from the bug's description, the new tab in opened in group B. This is the
> expected behavior.
> 
> I also tested the change with 3 tab groups, switching between groups and
> opening new tabs. Everything was working OK after the code change.
> 
> What do you think?

We need to ensure that one of the tabs is highlighted/selected after user closes one in a tab group.  Therefore, removing UI.setActive(this) in GroupItem__onChildClose() doesn't fix the issue.
Comment 9 Mario Alvarado [:marioalv] 2012-02-28 15:05:36 PST
Right now I'm facing the same problem as Marcos.
Whenever I change this line: https://mxr.mozilla.org/mozilla-central/source/browser/components/tabview/groupitems.js#1079, some automated tests fail, specially the ones for this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=588265

I've tried this: 
if (this._children.length > 0 && this._activeTab && tabItem.closedManually) {
  UI.setActive(this);
}

which will actually provide the desired results in this bug's STR: new items will open in group B; and also will keep with what's specified in https://bugzilla.mozilla.org/show_bug.cgi?id=588265#c20 : the focus is placed at the group from where we have closed a tab.

So, visually speaking, everything works OK after the fix but some automated tests fail.
Should we review the tests for bug 588265 to see if they still fit with the proposed changes to fix this current bug or should we look for another way to fix this bug?
Comment 10 Tim Taubert [:ttaubert] 2012-02-29 01:36:25 PST
Hey Mario,

without looking at the tests I'd say they're doing the right thing and obviously catch any wrong changes we're applying :) I think the key to solving this bug is to set some kind of flag to remember if we're closing an empty tab after switching to an existing one. Here's the code that switches to the existing tab:

https://mxr.mozilla.org/mozilla-central/source/browser/base/content/urlbarBindings.xml#297

We already do something similar here:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#7747
Comment 11 Mario Alvarado [:marioalv] 2012-03-01 14:51:39 PST
Created attachment 602126 [details] [diff] [review]
v1
Comment 12 Mario Alvarado [:marioalv] 2012-03-01 14:54:07 PST
Hi Tim.
Thanks for your suggestions. I followed the code example and implemented a solution that passes all the tests.
As you mention, the tricky part for this bug was noticing when a "switch to tab" was executed in order to avoid giving focus to the group from where the "switch to tab" was executed (group A), after opening a new tab in the current group (group B).
Comment 13 Tim Taubert [:ttaubert] 2012-03-02 02:27:28 PST
Comment on attachment 602126 [details] [diff] [review]
v1

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

Thanks for your patch, Mario. First of all, you seem to have an editor that removes spaces at the end of lines and clears lines that consist solely of spaces. While that's definitely matching our coding guidelines it adds so much clutter to your patch that I'm having a hard time spotting the real changes. Additionally, those white space changes remove blame functionality which comes in quite handy sometimes - that's why we don't like them. Could you please adapt your editor settings?

::: browser/base/content/browser.js
@@ +8825,4 @@
>   * @return True if an existing tab was found, false otherwise
>   */
>  function switchToTabHavingURI(aURI, aOpenNew) {
> +

Nit: Please don't add a new lines.

@@ +8836,5 @@
>          aWindow.focus();
>          aWindow.gBrowser.tabContainer.selectedIndex = i;
> +        //for bug 715454 we need to know when we are executing a "switch to tab"
> +        //TabView.setIsTabSwitched(true);
> +        TabView.isTabSwitched = true;

That's not the right place to set this flag because we don't know if a tab will be removed after switchToTabHavingURI() was called.

This is the right place for this code:

https://mxr.mozilla.org/mozilla-central/source/browser/base/content/urlbarBindings.xml#297

::: browser/components/tabview/ui.js
@@ +68,5 @@
>    _closedSelectedTabInTabView: false,
>  
> +  // Variable: isTabSwitched
> +  // If true, the user has executed a "switch to tab" action.
> +  isTabSwitched: false,

We shouldn't track this here in UI but rather in TabView (browser-tabview.js) because Panorama might not be loaded yet at the time we switch the tab.

Please have a look at the code here:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#7747
Comment 14 Dão Gottwald [:dao] 2012-03-02 03:28:47 PST
Neither switchToTabHavingURI nor urlbarBindings.xml seem to do anything crazy but simply use basic tabbrowser APIs in a pretty straightforward way. tabview needs to be able to handle such cases without being notified individually.
Comment 15 Tim Taubert [:ttaubert] 2012-03-02 03:32:49 PST
Yeah, I actually don't want Panorama to be specially treated. We already do it that way when restoring a tab and the selected blank tab gets overridden because we have no better API. What would be a good API to do this? It's hard to detect that these otherwise atomic actions (add tab, remove tab | switch tab, remove tab) are a set of actions that need be handled differently.
Comment 16 Tim Taubert [:ttaubert] 2012-03-02 03:46:55 PST
Relying on the fact that these actions are executed without returning to the main loop we could do the following:

1) A tab gets restored.
2) We listened for SSTabRestored and track that a tab got restored, we use setTimout(0) to unset this flag again.
3) A tab gets removed.
4) We listened for TabClose and thus check if it was a blank tab. if the flag from (2) is still set we assume that this tab got overridden by restoring a tab.

The same could apply for the issue being fixed by this bug. I'm not sure if that approach is safe to take but without knowing *why* a tab got closed it's hard to tell from the outside.

OTOH, we could introduce a .closedReason property or the like that might exist on the tab itself. Or maybe it's the TabClose event that provides more information.
Comment 17 Dão Gottwald [:dao] 2012-03-02 03:54:07 PST
I don't understand why tabview thinks it should switch groups when some random tab is removed from an inactive group. Comment 9 seems on the right track with tabItem.closedManually.
Comment 18 Tim Taubert [:ttaubert] 2012-03-02 05:45:58 PST
(I was trying to find a more generic solution to Panorama's special treatment in some cases)

Hmm. Now that I re-read comment #9 this is probably the best way to fix this, sorry Mario. You were right about fixing the test for bug 588265, it should be adapted to the new behavior.

That's as easy as changing this line:

http://mxr.mozilla.org/mozilla-central/source/browser/components/tabview/test/browser_tabview_bug588265.js#75

to:

EventUtils.synthesizeMouseAtCenter(tabItem.$close[0], {}, contentWindow);

so that the tabItem has .closedManually=true. There might be changes to some other tests needed that rely on the current behavior.
Comment 19 Mario Alvarado [:marioalv] 2012-03-02 13:34:14 PST
Created attachment 602470 [details]
Log after executing the mochi tests with the changes from comment 9 .

Hi.
Thanks for your suggestions. 
Now, I think we have 2 scenarios: 

1) Going by comment #9 . 

if (this._children.length > 0 && this._activeTab && tabItem.closedManually) {
  UI.setActive(this);
}

The cons about this change is that a lot of automated test fail:
browser_tabview_bug588265
browser_tabview_bug628270
browser_tabview_bug728887
browser_tabview_exit_button
browser_tabview_group
browser_tabview_privatebrowsing

I've attached a text file with the log after the mochi tests execution.

2) Implementing a flag in TabView indicating when we are executing a "switch to tab" action.

In TabView (browser-tabview.js): 

  // ----------
  get isTabSwitched() {
    return this._isTabSwitched;
  },

  // ----------
  set isTabSwitched(val) {
    this._isTabSwitched = val;
  },


In urlbarBindings.xml

if (switchToTabHavingURI(url) &&
   isTabEmpty(prevTab)) {
  TabView.isTabSwitched(true);
  gBrowser.removeTab(prevTab);
}


In GroupItem (groupitems.js):

    if (this._children.length > 0 && this._activeTab && !gTabView.isTabSwitched)
      UI.setActive(this);

    if (gTabView.isTabSwitched)
      gTabView.isTabSwitched(false);

I think it would be better to go by Option 2 because we would not change the automated tests.

What do you think?
Comment 20 Tim Taubert [:ttaubert] 2012-03-02 13:38:45 PST
I think we should go with option 1 and fix the failing tests. browser_tabview_bug588265.js shouldn't be failing with the fix from comment #18. I think that not all of these tests you listed are failing but rather these failures are caused by previous tests that didn't clean up correctly.
Comment 21 Mario Alvarado [:marioalv] 2012-03-02 15:54:50 PST
Changing tabItem.close(); for EventUtils.synthesizeMouseAtCenter(tabItem.$close[0], {}, contentWindow); in browser_tabview_588265.js does fix the errors for that specific test.

Now, for browser_tabview_628270.js I tried changing gBrowser.removeTab(tabItem.tab); for EventUtils.synthesizeMouseAtCenter(tabItem.$close[0], {}, TabView.getContentWindow()); (and some more combinations for this call) but this statment not work. 

How should I change the test browser_tabview_628270.js ?
Comment 22 Raymond Lee [:raymondlee] 2012-03-02 20:35:42 PST
(In reply to Mario Alvarado [:marioalv] from comment #21)
> Changing tabItem.close(); for
> EventUtils.synthesizeMouseAtCenter(tabItem.$close[0], {}, contentWindow); in
> browser_tabview_588265.js does fix the errors for that specific test.
> 
> Now, for browser_tabview_628270.js I tried changing
> gBrowser.removeTab(tabItem.tab); for
> EventUtils.synthesizeMouseAtCenter(tabItem.$close[0], {},
> TabView.getContentWindow()); (and some more combinations for this call) but
> this statment not work. 
> 
> How should I change the test browser_tabview_628270.js ?

In this test, I see that gBrowser.removeTab(tabItem.tab) is called when we are not in Panorama so I don't think we should replace it with EventUtils.synthesizeMouseAtCenter(tabItem.$close[0], {}, TabView.getContentWindow());.  Do you know why the gBrowser.removeTab() doesn't work as it is a common statement which would be called when not in Panorama?
Comment 23 Mario Alvarado [:marioalv] 2012-03-02 23:25:52 PST
This is the log for test that does not pass:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/tabview/test/browser_tabview_bug628270.js | restore: there are 2 tabs in the group - Got 1, expected 2
Stack trace:
    JS frame :: chrome://mochikit/content/browser-test.js :: test_is :: line 446
    JS frame :: chrome://mochitests/content/browser/browser/components/tabview/test/browser_tabview_bug628270.js :: <TOP_LEVEL> :: line 50
    JS frame :: chrome://mochitests/content/browser/browser/components/tabview/test/browser_tabview_bug628270.js :: <TOP_LEVEL> :: line 80
    JS frame :: chrome://mochitests/content/browser/browser/components/tabview/test/head.js :: <TOP_LEVEL> :: line 357
    JS frame :: chrome://mochikit/content/browser-test.js :: <TOP_LEVEL> :: line 473


Actually, all these data is on the attachment on comment 19.

If I do this, the test does pass:

    gBrowser.removeTab(tabItem.tab);
    let activeTabItem = getGroupItem(1).getChild(0);
    cw.GroupItems.updateActiveGroupItemAndTabBar(activeTabItem);

    restoreTab(function () {
      assertNumberOfTabsInGroup(groupItem, 2);

I don't know if this is the way to go or we should change something else. What should we do?

Now, assuming that test browser_tabview_bug628270 passes (after the code change I made), the next failing tests are:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/tabview/test/browser_tabview_bug728887.js | one groupItem after double clicking - Got 2, expected 1
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/tabview/test/browser_tabview_exit_button.js | we start with one group (the default) - Got 2, expected 1

There are just the two tests that fail next.  There are 21 failing tests after browser_tabview_bug628270 passes. The log information for the non-passing tests is on the attachment from comment 19 .

On the other hand, if I only execute browser_tabview_bug728887, the test passes:
TEST-START | chrome://mochitests/content/browser/browser/components/tabview/test/browser_tabview_bug728887.js
TEST-PASS | chrome://mochitests/content/browser/browser/components/tabview/test/browser_tabview_bug728887.js | one groupItem after double clicking

I'm not sure why browser_tabview_bug728887 does not pass when executing the whole test set (when browser_tabview_bug628270 passes) and it passes when being executed alone.

What should we do?
Comment 24 Raymond Lee [:raymondlee] 2012-03-03 01:28:33 PST
(In reply to Mario Alvarado [:marioalv] from comment #23)
> This is the log for test that does not pass:
> 
> If I do this, the test does pass:
> 
>     gBrowser.removeTab(tabItem.tab);
>     let activeTabItem = getGroupItem(1).getChild(0);
>     cw.GroupItems.updateActiveGroupItemAndTabBar(activeTabItem);
> 
>     restoreTab(function () {
>       assertNumberOfTabsInGroup(groupItem, 2);
> 
> I don't know if this is the way to go or we should change something else.
> What should we do?

I don't think that adding those lines is right because you have to update active group item and tab bar manually in the test.
Have you tried the STR in bug 628270 after applying your patch?  If it's not working, I believe you have to fix the panorama code, rather than the test.

> 
> I'm not sure why browser_tabview_bug728887 does not pass when executing the
> whole test set (when browser_tabview_bug628270 passes) and it passes when
> being executed alone.
> 
> What should we do?

it may be a test which runs before browser_tabview_bug728887 doesn't clear everything properly.  You can add some assertions to test when everything is right before browser_tabview_bug728887 starts.
Comment 25 Tim Taubert [:ttaubert] 2012-03-05 07:12:06 PST
So we have three failing tests here:

bug 588265 (has a simple fix already)
bug 628720 (no idea from a quick look)
bug 685692 (haven't looked, yet)

If you remove the last two, the whole suite runs successfully.
Comment 26 Tim Taubert [:ttaubert] 2012-03-05 07:12:56 PST
(In reply to Tim Taubert [:ttaubert] from comment #25)
> bug 628720 (no idea from a quick look)

That should've been bug 628270, of course.
Comment 27 Tim Taubert [:ttaubert] 2012-03-05 07:42:12 PST
WRT bug 628270:

This whole bug is actually a session store only change with a Panorama test to cover the STR we found. Turns out, the test I wrote long time ago doesn't do the correct thing. It expects the closed tab to be restored in its original group (which actually we don't want). So there are two options:

1) Let browser_bug_628270.js close the tab via EventUtils.synthesizeMouse*() while Panorama is shown. That way the tab is restored in its original group because the group gets activated when closing.

2) Change the tab count checks to assert we have two tabs in the first group and one in the second group.

The second option is easier but I'd rather have a test implementing the correct STR so we should go with (1).
Comment 28 Tim Taubert [:ttaubert] 2012-03-05 08:10:35 PST
WRT bug 685692:

The test doesn't clean up correctly, so there's an additional tab group that isn't removed and makes following tests fail. A simple fix is to replace this line:

http://mxr.mozilla.org/mozilla-central/source/browser/components/tabview/test/browser_tabview_bug685692.js#31

with |groupItemOne.close();|.

With this last fix all tests finish successfully for me.
Comment 29 Mario Alvarado [:marioalv] 2012-03-05 14:20:56 PST
Thanks to your suggestions, I think I fixed all the failing tests:

bug 588265: 
changed tabItem.close(); to EventUtils.synthesizeMouseAtCenter(tabItem.$close[0], {}, contentWindow);
-> Worked OK

bug 685692: 
changed cw.UI.setActive(groupItemOne); to groupItemOne.close();
-> Worked OK

bug 628720:
changes: 
A) comment out the call to hideTabView
  showTabView(function () {
    //hideTabView(function () {

B) change gBrowser.removeTab(tabItem.tab); to EventUtils.synthesizeMouseAtCenter(tabItem.$close[0], {}, TabView.getContentWindow());

C) change finishTest(); to hideTabView(finishTest);
-> Worked OK

For this test case I tried adding TabView.hide() everywhere, but that call didn't work :(. So, I used the call to hideTavView(finishTest); .  As I understood, we should've kept the TabView shown while the test was being executed and close the TabView before the test finishes.

Is this OK?
Comment 30 Tim Taubert [:ttaubert] 2012-03-05 15:12:42 PST
(In reply to Mario Alvarado [:marioalv] from comment #29)
> For this test case I tried adding TabView.hide() everywhere, but that call

What exactly do you mean by "everywhere"?

> didn't work :(. So, I used the call to hideTavView(finishTest); .  As I

That's because hiding Panorama is a transition. TabView.hide() kicks off the transition but doesn't wait until it's finished. hideTabView() is a test-only helper function that calls TabView.hide() and waits until the transition has completed.

> understood, we should've kept the TabView shown while the test was being
> executed and close the TabView before the test finishes.
> 
> Is this OK?

I *think* this is okay when all tests are green but I'm not totally sure we're on the same page. This sounds about ready, why not just upload the patch and ask me for review? Then it's clear which changes you made :)
Comment 31 Mario Alvarado [:marioalv] 2012-03-05 16:16:22 PST
Created attachment 603097 [details] [diff] [review]
v2

By "everywhere" I meant everywhere in the code, in order to close the TabView.

Well, the patch has been submitted. I hope I got it right this time.

Thanks.
Comment 32 Tim Taubert [:ttaubert] 2012-03-05 21:42:18 PST
Comment on attachment 603097 [details] [diff] [review]
v2

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

Awesome, thank you, that looks pretty good to me. Now we need to make sure we don't regress this functionality and thus write a test for this and bug 716880. The latter is exactly the same issue but has a slightly different STR.
Comment 33 Mario Alvarado [:marioalv] 2012-03-06 08:41:44 PST
Good :) . Thanks for your review.

I'll write the tests and include them on a V3 of the patch. Is this the right way to proceed?
Comment 34 Tim Taubert [:ttaubert] 2012-03-06 08:50:40 PST
Yeah, exactly. Just ask me for review again when you uploaded the patch.
Comment 35 Mario Alvarado [:marioalv] 2012-03-07 10:07:29 PST
Created attachment 603772 [details] [diff] [review]
Proposed test case for this bug. v1.

Hi.
Please find attached the test case for this bug.

I have a couple of questions for this test case code.
I tried to close the second tab in group one with groupItemOne.getChild(1).close(); and with gBrowser.removeTab(groupItemOne.getChild(1)); but these calls did not work. Here are the errors I got:


For groupItemOne.getChild(1).close();

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/tabview/test/browser_tabview_bug715454.js | an unexpected uncaught JS exception reported through window.onerror - uncaught exception: [Exception... "'Illegal value' when calling method: [nsISessionStore::setWindowValue]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: chrome://browser/content/tabview.js :: Storage_saveVisibilityData :: line 1096"  data: no] at :0
Stack trace:
    JS frame :: chrome://mochikit/content/tests/SimpleTest/SimpleTest.js :: simpletestOnerror :: line 969
    native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0

_____________________________

For gBrowser.removeTab(groupItemOne.getChild(1));

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/tabview/test/browser_tabview_bug715454.js | an unexpected uncaught JS exception reported through window.onerror - browser is undefined at chrome://browser/content/tabbrowser.xml:1558
Stack trace:
    JS frame :: chrome://mochikit/content/tests/SimpleTest/SimpleTest.js :: simpletestOnerror :: line 969
    native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0

TEST-INFO | chrome://mochitests/content/browser/browser/components/tabview/test/browser_tabview_bug715454.js | Console message: [JavaScript Error: "browser is undefined" {file: "chrome://browser/content/tabbrowser.xml" line: 1558}]


The call to gBrowser.removeTab(newTab); does work.

I'm curious about why calls to roupItemOne.getChild(1).close(); and gBrowser.removeTab(groupItemOne.getChild(1)) fail and the call to gBrowser.removeTab(newTab); works.
Comment 36 Mario Alvarado [:marioalv] 2012-03-08 20:25:03 PST
Created attachment 604293 [details] [diff] [review]
Proposed test case for this bug. v2.

Hi.
I found out there were some errors with my test case, specially by not closing the second tab group due to an undefined variable: groupItemTwo.id . I have fixed this error and uploaded a new test case for this bug.

By the way, I changed the way tabs were opened from let newTab = gBrowser.loadOneTab("about:blank", { }); to groupItemOne.newTab("about:blank", { }); . This solves my questions in comment 35.
Comment 37 Mario Alvarado [:marioalv] 2012-03-08 20:35:08 PST
Created attachment 604295 [details] [diff] [review]
v3

Added the changes to Makefile.in to include the execution of the test cases for bugs 715454 and 716880.
Comment 38 Tim Taubert [:ttaubert] 2012-03-12 03:52:31 PDT
Mario, there are no new tests in your patch. Did you forget to "hg add" them before refreshing your patch?
Comment 39 Tim Taubert [:ttaubert] 2012-03-12 03:53:47 PDT
Oops, that's just a plain file, not a diff - that's why splinter review didn't show any files. Can you please put this all together in one patch and ask me for review?
Comment 40 Tim Taubert [:ttaubert] 2012-03-12 03:54:36 PDT
Comment on attachment 604293 [details] [diff] [review]
Proposed test case for this bug. v2.

Please put the fix and this test case together in one patch.
Comment 41 Mario Alvarado [:marioalv] 2012-03-12 12:02:01 PDT
Created attachment 605039 [details] [diff] [review]
v4

Please find attached the patch for this bug. The patch also includes the test cases for bug 715454 and bug 716880 .
Comment 42 Tim Taubert [:ttaubert] 2012-03-13 04:42:10 PDT
(In reply to Mario Alvarado [:marioalv] from comment #35)
> I'm curious about why calls to roupItemOne.getChild(1).close(); and
> gBrowser.removeTab(groupItemOne.getChild(1)) fail and the call to
> gBrowser.removeTab(newTab); works.

group.getChild() returns a TabItem not a <xul:tab> - that's why it doesn't work. gBrowser.removeTab(groupItemOne.getChild(1).tab) would have worked.

groupItemOne.getChild(1).close() should work too, not sure what's going wrong here.
Comment 43 Tim Taubert [:ttaubert] 2012-03-13 05:06:49 PDT
Comment on attachment 605039 [details] [diff] [review]
v4

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

This looks good be we're not quite there, yet. First patches are always the hardest :)


1) As a general rule, when writing comments, instead of this:

//remove the tab from where the switchToTabHavingURI() was called

please write them like this:

// Remove the tab from where the switchToTabHavingURI() was called.

(With proper capitalization and punctuation.)


2) The two tests seem to do lots of similar stuff. Could we maybe combine those into one test case?

::: browser/components/tabview/test/browser_tabview_bug715454.js
@@ +1,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +let contentWindow;
> +let groupItemTwoId;

This should be a local variable of setup().

@@ +4,5 @@
> +let contentWindow;
> +let groupItemTwoId;
> +
> +const DUMMY_PAGE_URL =
> +  'http://mochi.test:8888/browser/browser/components/tabview/test/dummy_page.html';

We don't really need a special url here. This make it looks as if the test relies on a html page with specific code in it. Just using 'about:mozilla' is fine for this case.

@@ +78,5 @@
> +   * contained in group two and then open a new tab in group two after the
> +   * switch. The tab should be opened in group two and not in group one.
> +   */
> +  let result = window.switchToTabHavingURI(DUMMY_PAGE_URL);
> +  is(result, true, "Switch successfully executed");

You can do:

> ok(result, "Switch successfully executed");

when checking boolean values.

@@ +81,5 @@
> +  let result = window.switchToTabHavingURI(DUMMY_PAGE_URL);
> +  is(result, true, "Switch successfully executed");
> +
> +  //remove the tab from where the switchToTabHavingURI() was called
> +  groupItemOne.getChild(1).close();

This part is wrong. We shouldn't close the tab ourselves. We should have a blank tab, use the url bar to switch to a tab with a specific URI and then check everything. You can do it like this:

>  // Set the urlbar to include the moz-action
>  gURLBar.value = "moz-action:switchtab," + DUMMY_PAGE_URL;
>  // Focus the urlbar so we can press enter
>  gURLBar.focus();
>  // Press enter!
>  EventUtils.synthesizeKey("VK_RETURN", {});

::: browser/components/tabview/test/browser_tabview_bug716880.js
@@ +2,5 @@
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +let contentWindow;
> +let pinnedTab;
> +let groupItemTwoId;

This should be a local variable of setup().

@@ +8,5 @@
> +const DUMMY_PAGE_URL_1 =
> +  'about:blank';
> +
> +const DUMMY_PAGE_URL_2 =
> +  'http://mochi.test:8888/browser/browser/components/tabview/test/dummy_page.html';

Same here, just use 'about:mozilla'.

@@ +24,5 @@
> +  gBrowser.pinTab(pinnedTab);
> +
> +  ok(pinnedTab.pinned, "Tab 1 is pinned");
> +
> +  var tab2 = gBrowser.addTab(DUMMY_PAGE_URL_2);

"let is the new var" - and we don't really need a variable here at all.

@@ +79,5 @@
> +  let result = window.switchToTabHavingURI(DUMMY_PAGE_URL_2);
> +  is(result, true, "Switch successfully executed");
> +
> +  //remove the tab from where the switchToTabHavingURI() was called
> +  groupItemTwo.getChild(1).close();

Please use the url bar here, too.
Comment 44 Dão Gottwald [:dao] 2012-03-14 09:13:49 PDT
*** Bug 653235 has been marked as a duplicate of this bug. ***
Comment 45 Mario Alvarado [:marioalv] 2012-03-16 17:19:49 PDT
Created attachment 606797 [details] [diff] [review]
v5

Hi.
Thanks for all your suggestions. 
I corrected the test cases based on your comments.
Please let me know if I need to correct something else.

Thanks.
Comment 46 Tim Taubert [:ttaubert] 2012-03-19 07:00:29 PDT
Comment on attachment 606797 [details] [diff] [review]
v5

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

Thanks for doing this, looks good!

r=me with the nits below fixed.

Please attach the fixed patch, prepared for checkin. mak wrote a nice blog post about how to do this: http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed

::: browser/components/tabview/test/browser_tabview_bug715454.js
@@ +2,5 @@
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +let contentWindow;
> +
> +const DUMMY_PAGE_URL = "about:mozilla";

Nit: we don't need this const. You can use "about:mozilla" directly.

@@ +37,5 @@
> +
> +  is(groupItemOne.getChildren().length, 2,
> +    prefix + "The number of tabs in group one is 2");
> +
> +  //Create a second group with a dummy page.

Nit: "// Create..." (space after //). Please correct all the other comments in this test as well.

::: browser/components/tabview/test/browser_tabview_bug716880.js
@@ +3,5 @@
> +
> +let contentWindow;
> +let pinnedTab;
> +
> +const DUMMY_PAGE_URL = "about:mozilla";

Nit: we don't need this const. You can use "about:mozilla" directly.

@@ +40,5 @@
> +
> +  is(groupItemOne.getChildren().length, 2,
> +    prefix + "The number of tabs in group one is 2");
> +
> +  //Create a second group with a dummy page.

Nit: space after //. Please fix all comments in this test as well.
Comment 47 Mario Alvarado [:marioalv] 2012-03-19 21:07:34 PDT
Created attachment 607440 [details] [diff] [review]
Patch for checkin

Hi.
Here is the latest version of the patch.
I've added the author's username and a title to the patch, and I also corrected the test cases based on Tim's suggestions.

Please let me know if everything's OK now.

Thanks.
Comment 48 Mozilla RelEng Bot 2012-03-19 21:47:34 PDT
Autoland Patchset:
	Patches: 607440
	Branch: mozilla-central => try
Insufficient permissions to push to try.
Comment 49 Mario Alvarado [:marioalv] 2012-03-19 22:49:36 PDT
Created attachment 607462 [details] [diff] [review]
Patch for checkin

There was an error with the previous patch, so I uploaded v6 again.
Comment 50 Raymond Lee [:raymondlee] 2012-03-19 22:59:04 PDT
(In reply to Mario Alvarado [:marioalv] from comment #49)
> Created attachment 607462 [details] [diff] [review]
> Patch for checkin
> 
> There was an error with the previous patch, so I uploaded v6 again.

Pushed to try and waiting for results
https://tbpl.mozilla.org/?tree=Try&rev=b65a6347b737
Comment 51 Tim Taubert [:ttaubert] 2012-03-20 03:50:37 PDT
Comment on attachment 607462 [details] [diff] [review]
Patch for checkin

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

Thanks, looks good! I'll push it to the fx-team branch in a sec and then it should be merged into m-c today or tomorrow.
Comment 52 Tim Taubert [:ttaubert] 2012-03-20 03:51:43 PDT
https://hg.mozilla.org/integration/fx-team/rev/40e83506f87b
Comment 53 Marco Bonardo [::mak] 2012-03-21 15:54:20 PDT
https://hg.mozilla.org/mozilla-central/rev/40e83506f87b

Note You need to log in before you can comment on or make changes to this bug.