Last Comment Bug 664669 - tab._tabViewTabIsRemovedAfterRestore should be attached to the tabItem rather than the xulTab
: tab._tabViewTabIsRemovedAfterRestore should be attached to the tabItem rather...
Status: RESOLVED FIXED
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: -- normal
: Firefox 7
Assigned To: Tim Taubert [:ttaubert]
:
Mentors:
Depends on: 663421
Blocks: 660175
  Show dependency treegraph
 
Reported: 2011-06-16 00:29 PDT by Dão Gottwald [:dao]
Modified: 2016-04-12 14:00 PDT (History)
1 user (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1 (2.94 KB, patch)
2011-06-17 04:40 PDT, Tim Taubert [:ttaubert]
dao+bmo: review+
Details | Diff | Splinter Review
patch v2 (1.96 KB, patch)
2011-06-17 05:29 PDT, Tim Taubert [:ttaubert]
dao+bmo: review+
Details | Diff | Splinter Review

Description Dão Gottwald [:dao] 2011-06-16 00:29:26 PDT
Since bug 664549 landed, I see this message a couple of times in a mochitest-browser-chrome log, e.g.:

TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_bug591706.js | Console message: TypeError: item.tab is null = {fileName: 'chrome://browser/content/tabview.js', lineNumber: 3298, }

That line appears to be in GroupItem_remove:

      // if a blank tab is selected while restoring a tab the blank tab gets
      // removed. we need to keep the group alive for the restored tab.
      if (item.tab._tabViewTabIsRemovedAfterRestore)
        options.dontClose = true;

Two thoughts:
- Apparently GroupItem_remove is called after TabItems_unlink -- is this expected?
- _tabViewTabIsRemovedAfterRestore really shouldn't be set on the tab, it should be a flag on the item.
Comment 1 Tim Taubert [:ttaubert] 2011-06-17 04:40:37 PDT
Created attachment 540035 [details] [diff] [review]
patch v1

(In reply to comment #0)
> - Apparently GroupItem_remove is called after TabItems_unlink -- is this
> expected?

It's expected because in GroupItem.destroy() we first try to close a tab and only if that succeeded we're removing it from the group.

> - _tabViewTabIsRemovedAfterRestore really shouldn't be set on the tab, it
> should be a flag on the item.

That is the easiest solution and what the patch does.
Comment 2 Tim Taubert [:ttaubert] 2011-06-17 05:16:11 PDT
This is fixed by bug 663421 because the failing LOC is removed. The only place where we access tab._tabViewTabIsRemovedAfterRestore is now in onTabClose(tab) where an xulTab is passed. So I think we should not change the current behavior and let it remain a flag on the xulTab.
Comment 3 Dão Gottwald [:dao] 2011-06-17 05:17:51 PDT
It doesn't belong on the tab, never did... This is all tabview code.
Comment 4 Tim Taubert [:ttaubert] 2011-06-17 05:29:32 PDT
Created attachment 540041 [details] [diff] [review]
patch v2

(In reply to comment #3)
> It doesn't belong on the tab, never did... This is all tabview code.

Yeah, you're right. Removed the groupitems.js changes based on bug 663421. Did you remove the bug dependencies intentionally?
Comment 5 Dão Gottwald [:dao] 2011-06-17 05:31:49 PDT
Comment on attachment 540041 [details] [diff] [review]
patch v2

(dependencies got cleared due to my comment colliding with your change to the bug)
Comment 7 Mounir Lamouri (:mounir) 2011-06-18 09:40:24 PDT
Pushed:
http://hg.mozilla.org/mozilla-central/rev/b3b509e9fbde

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