Closed
Bug 664669
Opened 14 years ago
Closed 14 years ago
tab._tabViewTabIsRemovedAfterRestore should be attached to the tabItem rather than the xulTab
Categories
(Firefox Graveyard :: Panorama, defect)
Firefox Graveyard
Panorama
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 7
People
(Reporter: dao, Assigned: ttaubert)
References
Details
Attachments
(1 file, 1 obsolete file)
1.96 KB,
patch
|
dao
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•14 years ago
|
||
(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.
Reporter | ||
Updated•14 years ago
|
Attachment #540035 -
Flags: review?(dao) → review+
Assignee | ||
Comment 2•14 years ago
|
||
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.
Assignee | ||
Updated•14 years ago
|
Reporter | ||
Comment 3•14 years ago
|
||
It doesn't belong on the tab, never did... This is all tabview code.
Assignee | ||
Comment 4•14 years ago
|
||
(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?
Attachment #540035 -
Attachment is obsolete: true
Attachment #540041 -
Flags: review?(dao)
Reporter | ||
Comment 5•14 years ago
|
||
Comment on attachment 540041 [details] [diff] [review]
patch v2
(dependencies got cleared due to my comment colliding with your change to the bug)
Attachment #540041 -
Flags: review?(dao) → review+
Assignee | ||
Updated•14 years ago
|
Assignee | ||
Updated•14 years ago
|
Summary: TypeError: item.tab is null = {fileName: 'chrome://browser/content/tabview.js', lineNumber: 3298, } → tab._tabViewTabIsRemovedAfterRestore should be attached to the tabItem rather than the xulTab
Assignee | ||
Comment 6•14 years ago
|
||
Whiteboard: [inbound]
Comment 7•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 7
Updated•14 years ago
|
Whiteboard: [inbound]
Updated•9 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•