Closed Bug 705964 Opened 13 years ago Closed 8 years ago

TabItems remain in wrong group on session restore

Categories

(Firefox Graveyard :: Panorama, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: ttaubert, Unassigned)

References

Details

Attachments

(1 file, 3 obsolete files)

GroupItems.reconstitute() needs to prepare TabItems to be reconnected so that they can be re-assigned to new groups, if they changed groups compared to the new session that is restored.
Attached patch patch v1 (obsolete) — Splinter Review
Trivial patch with test case. Sets tabItem._reconnected to false, so that they're reconnected when TabItems.resumeReconnecting() gets called (after the session has been restored).
Attachment #577451 - Flags: review?(dietrich)
Depends on: 688695
I don't really understand the bug, so can't really evaluate the patch. Can you explain the STR?

Also, the way that groups code is reaching into tabItem "private" properties is a bit worrisome.
(In reply to Dietrich Ayala (:dietrich) from comment #2)
> I don't really understand the bug, so can't really evaluate the patch. Can
> you explain the STR?

Sorry... Here's a readable STR:

1) Create two groups with one tab per group.
2) Save the current session to some variable.
3) Let both tabs switch groups.
4) Restore the session from [2].

Actual:

Tabs are in the same groups as in [3].

Expected:

Session from [2] should be restored, so each tab should be back in its original group.

> Also, the way that groups code is reaching into tabItem "private" properties
> is a bit worrisome.

Mhh yeah :| How about something like tabItem.setReconnected(false)? tabItem.unsetReconnected()?
(In reply to Tim Taubert [:ttaubert] from comment #3)
> (In reply to Dietrich Ayala (:dietrich) from comment #2)
> > I don't really understand the bug, so can't really evaluate the patch. Can
> > you explain the STR?
> 
> Sorry... Here's a readable STR:

thanks!

> Mhh yeah :| How about something like tabItem.setReconnected(false)?
> tabItem.unsetReconnected()?

would it make sense to just do s/_reconnected/reconnected/g? might be simpler than adding new apis to manage this one property.
(In reply to Dietrich Ayala (:dietrich) from comment #4)
> would it make sense to just do s/_reconnected/reconnected/g? might be
> simpler than adding new apis to manage this one property.

Yeah, that's even easier and straight forward. Let's do that.
Attached patch patch v2 (obsolete) — Splinter Review
Replaced TabItem._reconnect() with TabItem.reconnect() and TabItem._reconnected with TabItem.reconnected.
Attachment #577695 - Flags: review?(dietrich)
Attachment #577451 - Attachment is obsolete: true
Attachment #577451 - Flags: review?(dietrich)
Comment on attachment 577695 [details] [diff] [review]
patch v2

Some tests are failing now after a pull from m-c. Cancelling review until those are fixed...
Attachment #577695 - Flags: review?(dietrich)
Attached patch patch v3 (obsolete) — Splinter Review
TabItem.reconnect() does not remove tabs from their groups anymore. This became unnecessary because we removed the orphan tab concept and groupItem.add() takes care of removing tabs from their old groups.

This fixed the test for bug 628270 which constantly failed.
Attachment #577695 - Attachment is obsolete: true
Attachment #578597 - Flags: review?(dietrich)
in GroupItems.reconstitute 
+ groupItem.getChildren().forEach(function (tabItem) {
+   tabItem.reconnected = false;
+ });

i don't think it is necessary to set all item to be reconnected only those that are not in the right place.
unless reconnect "cost" is nothing
Attached patch patch v4Splinter Review
(In reply to onemen.one from comment #9)
> i don't think it is necessary to set all item to be reconnected only those
> that are not in the right place.
> unless reconnect "cost" is nothing

Reconnecting is definitely costly, it might be better to not touch tabItems that haven't moved. Accessing the tab data storage is also not free but TabItem.reconnect() calls that anyway, so let's do that.
Attachment #578597 - Attachment is obsolete: true
Attachment #578597 - Flags: review?(dietrich)
Attachment #578606 - Flags: review?(dietrich)
i don't know if tabData can be null in this case, but it better be safe then...

-   if (tabData.groupID != data.id)
+   if (tabData && tabData.groupID != data.id)

or you can decide that if tabData is null we need to reconnect the tabItem to the active group.
(In reply to onemen.one from comment #11)
> i don't know if tabData can be null in this case, but it better be safe
> then...

tabData can't be null if this tab is contained in a group. It's ok to throw an error otherwise because that situation should never happen.
(In reply to Tim Taubert [:ttaubert] from comment #12)
> tabData can't be null if this tab is contained in a group. It's ok to throw
> an error otherwise because that situation should never happen.

I see your point, however in bug 706430, you did add new sanity check for the tab that are in toClose groups.

since all the item that we deal with in GroupItems_reconstitute are reused tabs we should treat all tabItems the same.

Also, when there is a mismatch between tabItem data and groupItemData you can not guess where is the error.

I think that in GroupItems_reconstitute we should only flag ALL reused tab with tabItem._reconnected = false; (also all tabs from groups that are not going to be close), then in TabItem__reconnect do all the sanity checks:
- if the tabItem already in the right group do nothing.
- if the tabItem belong to existing group connect it to the group
- if the tabItem don't have any data, add it to the current group
- if tab have a group id for non existing group - that's the tricky part - you can either add it to the current group or as i prefer create new group with that id , so all other tabItems with that same id will stay together, and set a flag to throw one error message for that session restored from UI_storageReady
Attachment #578606 - Flags: review?(dietrich)
Not going to work on this anytime soon.
Assignee: ttaubert → nobody
Status: ASSIGNED → NEW
Panorama has been removed from Firefox 45, currently in Beta and scheduled for release on March 7th. As such, I'm closing all existing Panorama bugs.

If you are still using Panorama, you will see a deprecation message in Firefox 44, and when 45 is released your tab group data will be migrated to bookmarks, with a folder for each group. There are also a few addons offering similar functionality.

See https://support.mozilla.org/en-US/kb/tab-groups-removal for more info.

We're removing Panorama because it has extremely low usage (about 0.01% of users), and has a large number of bugs and usability issues. The cost of fixing all those issues is far too high to justify, and so we'll instead be focusing our time and energy on improving other parts of Firefox.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: