Closed Bug 1221050 Opened 9 years ago Closed 8 years ago

Migrate existing tab groups

Categories

(Firefox Graveyard :: Panorama, defect)

defect
Not set
normal

Tracking

(firefox45 fixed, relnote-firefox 45+)

RESOLVED FIXED
Firefox 45
Tracking Status
firefox45 --- fixed
relnote-firefox --- 45+

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

Attachments

(10 files, 1 obsolete file)

213.98 KB, image/png
Details
1.09 MB, image/jpeg
Details
40 bytes, text/x-review-board-request
ttaubert
: review+
Details
40 bytes, text/x-review-board-request
ttaubert
: review+
Details
40 bytes, text/x-review-board-request
mak
: review+
ttaubert
: review+
Details
40 bytes, text/x-review-board-request
ttaubert
: review+
Details
40 bytes, text/x-review-board-request
ttaubert
: review+
Details
40 bytes, text/x-review-board-request
ttaubert
: review+
Details
40 bytes, text/x-review-board-request
ttaubert
: review+
Details
284.97 KB, image/png
verdi
: ui-review+
Details
We're removing panorama/tab groups/tabview, because it's mostly unused (estimates around 0.01% of users, see bug 1210773 comment 6), because its quality is not exactly great, and because it incurs comparably (very) high costs in terms of maintenance: just off the top of my head, fixes like bug 332195 and bug 967873 and bug 616853 and numerous other bugs all are/were more involved than they would have been without panorama, not to mention the intermittent test failures ( https://bugzilla.mozilla.org/buglist.cgi?quicksearch=comp%3Apanorama%20summary%3Aintermittent&list_id=12654413 ). We may eventually invest in this space (tab management) again but will not do so at the moment. If people want a solution to replace tab groups, we recommend looking at the existing offer of add-ons in this space.


We need a way to do this without causing dataloss. The two main suggestions I've heard so far are:

1) migrate all the non-active tab groups into bookmark folders
2) migrate all the non-active tab groups into individual windows

I prefer (1) because:
a) it'll scale a lot better (ie large number of tab groups per window -> large number of windows -> lots of jank on startup)
b) it behaves a lot more like panorama currently does in that non-active tab groups don't get their tabs loaded, whereas all the windows would actually cause network activity which may be unexpected
c) it doesn't require any immediate cleanup from the user. If I had >2 tab groups, I wouldn't want to deal with managing/closing/bookmarking all the resulting windows if we suddenly opened them as one window each
d) it doesn't heavily burden users who once made a tab group and then essentially forgot about it. Popping open that group again as a window would be highly surprising
e) you can easily escalate to new windows (open a new window, open bookmark folder, click 'open all in tabs'), whereas the reverse takes more operations (right click tab bar, 'bookmark all tabs', pick a name and place, click OK, close window, accept warning dialog) and more jank.

So I would prefer to go for bookmarks. Madhava/Philipp/Stephen, does that sound sensible or do you see more convincing arguments to open a bunch of new windows instead? 

I'd like to get this done ASAP so that we can do most if not all the cleanup (it got its tentacles almost everywhere) in the 45 (which will be ESR!) cycle so as to minimize the amount of backporting pain for frontend stuff after we do this.
Flags: needinfo?(shorlander)
Flags: needinfo?(philipp)
Flags: needinfo?(madhava)
(In reply to :Gijs Kruitbosch from comment #0)
> We need a way to do this without causing dataloss. The two main suggestions
> I've heard so far are:
> 
> 1) migrate all the non-active tab groups into bookmark folders

This could result in data loss. Non-active tabs are XUL tabs that are in the yet-to-be-restored (as in session restore) state.

This means that they can have things like half-written blog posts in them, and a host of other bits of session state.

> 2) migrate all the non-active tab groups into individual windows

This method could maintain the yet-to-be-restored tab state if you do the migration using the session restore API, resulting in no in-page data loss.

You would still lose the user's work to categorize their tabs. For example, after this migration I'd have 40 windows, without any way of find out which group they were without sifting through the tabs themselves.

It'll be sad to be a Panorama user either way.
(In reply to Dietrich Ayala (:dietrich) from comment #5)
> (In reply to :Gijs Kruitbosch from comment #0)
> > We need a way to do this without causing dataloss. The two main suggestions
> > I've heard so far are:
> > 
> > 1) migrate all the non-active tab groups into bookmark folders
> 
> This could result in data loss. Non-active tabs are XUL tabs that are in the
> yet-to-be-restored (as in session restore) state.
> 
> This means that they can have things like half-written blog posts in them,
> and a host of other bits of session state.
> 
> > 2) migrate all the non-active tab groups into individual windows
> 
> This method could maintain the yet-to-be-restored tab state if you do the
> migration using the session restore API, resulting in no in-page data loss.
> 
> You would still lose the user's work to categorize their tabs. For example,
> after this migration I'd have 40 windows, without any way of find out which
> group they were without sifting through the tabs themselves.
> 
> It'll be sad to be a Panorama user either way.

Could potentially do both: Open all of the tabs with saved state and create bookmarks that save the organizational structure.

I agree that there is going to some level of pain no matter what we do. More or less painful depending on how extensively you use tab groups.
(In reply to Dietrich Ayala (:dietrich) from comment #5)
> (In reply to :Gijs Kruitbosch from comment #0)
> > We need a way to do this without causing dataloss. The two main suggestions
> > I've heard so far are:
> > 
> > 1) migrate all the non-active tab groups into bookmark folders
> 
> This could result in data loss. Non-active tabs are XUL tabs that are in the
> yet-to-be-restored (as in session restore) state.
> 
> This means that they can have things like half-written blog posts in them,
> and a host of other bits of session state.

Yes. There aren't many ways left to fix this at this point. The only thing we could do is try to push really quickly to do a notice (l10n, so we'll need uplift approval) that this data will go away in 44. I am not even convinced that that's worth it considering the number of people affected.

> > 2) migrate all the non-active tab groups into individual windows
> 
> This method could maintain the yet-to-be-restored tab state if you do the
> migration using the session restore API, resulting in no in-page data loss.

Yes, but...

> You would still lose the user's work to categorize their tabs. For example,
> after this migration I'd have 40 windows,

... I think the 40 windows by itself is a serious reason not to do this. I would sooner stick the prettyprinted JSON in a tab for the user to sift through to rescue such blogposts (Many if not most of the users affected will be technical enough to be able to do this), than to just bombard the user with 40 physical windows. Many users' machines will just hang for minutes at a time trying to load such content, even if ours would deal in maybe 1 minute (before all the network requests have finished and so on). It'd be an incredibly crappy experience.

> without any way of find out which
> group they were without sifting through the tabs themselves.

Isn't "which group" just the title/name of the group? Would "which group was this" really not be obvious from the tabs in the group? On the upside, we could save that data if we used bookmarks rather than windows...
One other thing we could do is leveraging the "recently closed windows" feature to allow restoring the dead groups as windows on-demand. That would avoid most of the downsides in comment #0.

I don't know how easily we can temporarily change the limit of how many restored windows we offer to restore, though, which I imagine is going to be <40. Tim?
Flags: needinfo?(ttaubert)
(In reply to :Gijs Kruitbosch from comment #7)
> > without any way of find out which
> > group they were without sifting through the tabs themselves.
> 
> Isn't "which group" just the title/name of the group? Would "which group was
> this" really not be obvious from the tabs in the group?

That's like renaming all the bookmark folders to "". I mean, isn't it obvious from the pages bookmarked? ;)
Seriously though, option #1 sounds like the way to go. Would be great if those folders were identifiable somehow, for importing into the new Panorama add-on.
(In reply to :Gijs Kruitbosch from comment #7)
> ... I think the 40 windows by itself is a serious reason not to do this. I
> would sooner stick the prettyprinted JSON in a tab for the user to sift
> through to rescue such blogposts (Many if not most of the users affected
> will be technical enough to be able to do this), than to just bombard the
> user with 40 physical windows. Many users' machines will just hang for
> minutes at a time trying to load such content, even if ours would deal in
> maybe 1 minute (before all the network requests have finished and so on).
> It'd be an incredibly crappy experience.

Most of the tabs should be restored lazily. It still might not be a great experience on older machines, but few of them should hang for minutes.

Pretty printing session data from windows that have it, or allowing users to choose which windows to restore, does sound like a better option, though.

(In reply to Dietrich Ayala (:dietrich) from comment #10)
> Seriously though, option #1 sounds like the way to go. Would be great if
> those folders were identifiable somehow, for importing into the new Panorama
> add-on.

Making them all sub-folders of "Tab Groups" should solve that problem.
(In reply to Kris Maglione [:kmag] from comment #11)
> Most of the tabs should be restored lazily. It still might not be a great
> experience on older machines, but few of them should hang for minutes.

I'm extrapolating from how my surface pro 3 (8gb RAM model) deals with my attempting to use similar multitasking to what I do on my desktop machines and my mbp. It's not pretty.

> Pretty printing session data from windows that have it, or allowing users to
> choose which windows to restore, does sound like a better option, though.

Right. The other thing we could do, somewhat similar to the "restore recently closed windows" thing, is making another about:sessionrestore/about:welcomeback clone for this, though I'm not sure offhand how easy it is to cherrypick what goes in there. Probably not trivial. :-\

It seems the recently closed windows are in browser.sessionstore.max_windows_undo . I can play around with a way to increase that maximum specifically for this migration path, if that's what we want to do.

At this point I'd really like UX to make a decision here on which approach they'd like us to move forward to so I can start implementing it. Madhava/Stephen/Philipp?
Flags: needinfo?(ttaubert)
(In reply to Kris Maglione [:kmag] from comment #11)
> (In reply to Dietrich Ayala (:dietrich) from comment #10)
> > Seriously though, option #1 sounds like the way to go. Would be great if
> > those folders were identifiable somehow, for importing into the new Panorama
> > add-on.
> 
> Making them all sub-folders of "Tab Groups" should solve that problem.

That would solve the problem and create another one: localization. How do you identify a bookmark folder when its name varies by locale, and could very well have been created in a locale that is not the current one (although the likeliness of the latter is very low)?
(In reply to Mike Hommey [:glandium] from comment #13)
> That would solve the problem and create another one: localization. How do
> you identify a bookmark folder when its name varies by locale, and could
> very well have been created in a locale that is not the current one
> (although the likeliness of the latter is very low)?

Well, if we create the folder with a locale string, any add-on that wants to find it can use the same locale string. Until we remove the migration code, anyway. In the worst case, they could ask the user to choose the folder, which may be desirable anyway.
(In reply to Mike Hommey [:glandium] from comment #13)
> (In reply to Kris Maglione [:kmag] from comment #11)
> > (In reply to Dietrich Ayala (:dietrich) from comment #10)
> > > Seriously though, option #1 sounds like the way to go. Would be great if
> > > those folders were identifiable somehow, for importing into the new Panorama
> > > add-on.
> > 
> > Making them all sub-folders of "Tab Groups" should solve that problem.
> 
> That would solve the problem and create another one: localization. How do
> you identify a bookmark folder when its name varies by locale, and could
> very well have been created in a locale that is not the current one
> (although the likeliness of the latter is very low)?

This is overthinking a problem that affects very, very, very few people (and not really an issue we should be optimizing a solution for). The addon could just ask the user to select what folder to import, or just check for all the localized folder names.
Since we are removing tab groups without replacement, we should at least give people a heads up and point them to alternatives for dealing with many tabs. This means a few things:

- Show a desktop notification in the version *prior* to the one that removes tab groups saying something like »Tab groups will be removed from Firefox in a few weeks. Click to learn more.«

- Have that notification point to a support page that lists add-ons like side tabs, tree style tabs and tab mix plus. We should also make sure that those are add-ons that won't be deprecated through our new add-ons strategy in the next few months.

- That support page should also open in a background tab after the user updates to the version where groups are actually removed.

- By doing all this, we should limit the surprise when tab groups are actually removed, meaning that the solution that converts tab groups into bookmarks is more acceptable. I'd still prefer a solution where we convert to windows if the user has fewer than, say 5 groups.

- As for naming of the bookmark folders, here's a proposal for the structure:

Bookmarks menu
- Tab Groups
-- Work
-- Personal
-- Tab Group 3 (If they don't have a name, just number them like this. If they do have a name, use the name)
-- Tab Group 4
Flags: needinfo?(shorlander)
Flags: needinfo?(philipp)
Flags: needinfo?(madhava)
Depends on: 1221497
(In reply to Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from comment #16)
> - Show a desktop notification in the version *prior* to the one that removes
> tab groups saying something like »Tab groups will be removed from Firefox in
> a few weeks. Click to learn more.«

Why a desktop notification? I believe our standard UI for this kind of warning would a more permanent notification bar.
Depends on: 1221500
(In reply to Dão Gottwald [:dao] from comment #17)
> (In reply to Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from
> comment #16)
> > - Show a desktop notification in the version *prior* to the one that removes
> > tab groups saying something like »Tab groups will be removed from Firefox in
> > a few weeks. Click to learn more.«
> 
> Why a desktop notification? I believe our standard UI for this kind of
> warning would a more permanent notification bar.
Flags: needinfo?(philipp)
Attached image tab groups warning.png
(In reply to Dão Gottwald [:dao] from comment #17)
> (In reply to Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from
> comment #16)
> > - Show a desktop notification in the version *prior* to the one that removes
> > tab groups saying something like »Tab groups will be removed from Firefox in
> > a few weeks. Click to learn more.«
> 
> Why a desktop notification? I believe our standard UI for this kind of
> warning would a more permanent notification bar.

We should consider desktop notifications for these kinds of prompts in the future. They have different properties in terms of information density and persistence.

That said, upon further reflection, they are probably not ready to perform that job well in Firefox 44 yet. Given that, I agree with you that a notificaiton bar would be the best option. Let's make it permanent (not tab specific), placed at the bottom of the window and use the yellow alert color:

Heads up! Tab groups will be removed from Firefox soon. [Learn more]

Matej, what do you think of that copy? It would only be shown to users who have more than one tab group open.

If possible, we could also show a warning inside the tab groups UI like in the attachment.
Flags: needinfo?(philipp)
I've moved the discussion about the notification method for older builds to bug 1221500 where I already had a patch for the desktop notifications. I will adapt that patch to use a notification bar and the updated copy.
Attached image tab_group_recovery.jpg
Stephen and I talked about this a bit this morning. Gijs -- we'll check in with Philipp about this tomorrow so we can get to some resolution and un-needinfo ourselves for real :)

I like the thinking around giving panorama users a heads up for a release cycle, or some amount of time, even if that just serves to give them a chance to get angry early and get used to the idea before it actually happens. Something within the tab groups view seems like a good placement -- in context.

Bookmarking everything seems like a good idea, no matter what we do. People may only realize later that that thing they wanted was in one of those tab groups vs. knowing how to make a good decision in the moment.

Regarding data-loss from in-progress work getting swept away, I don't know that I'm so worried about that happening for things sitting around in background tab-groups (not saying it won't happen there), but it _does_ seem like an odd experience if, on upgrade, you'd lose even what you'd been right in the middle of - we certainly ought to default to restoring whatever was in the active tab group (probably this is implicit in all the thinking above).

This all led me to what's in the attached sketch -- could we (relatively) easily repurpose the about:sessionrestore UI to show people, on upgrade to a post-tabgroups world, a list of what they had in their groups? I don't think we'd offer them a choice of delete/restore/bookmark -- it's too much to do at this point; better to just bookmark it all for safekeeping anwyay. But this could give people the opportunity to recover in windows what they want, and feel like it was an actually addressed migration experience.
You know, it might be better, rather than showing the above screen on upgrade, when it's in the user's way, to show it instead when the user clicks the Tab Groups button (which we'd have to keep for a release or so).

Alternatively, depending on how bad it is to store session info, we could keep the session around as a list, so the user could open things from it whenever they want?
(In reply to Madhava Enros [:madhava] from comment #22)
> You know, it might be better, rather than showing the above screen on
> upgrade, when it's in the user's way, to show it instead when the user
> clicks the Tab Groups button (which we'd have to keep for a release or so).
> 
> Alternatively, depending on how bad it is to store session info, we could
> keep the session around as a list, so the user could open things from it
> whenever they want?

It's bad enough (especially when stored in magical places for that button to recall) that we definitely don't want to keep lugging it around.

Which then means your suggestion delays the problem and creates a new one: when are we getting rid of that list / data, and how do we warn the user when that's going to happen and the data is *really* gone? Another notification bar to indicate the data that we made accessible under that button is now really going to go away the next version doesn't seem like a great idea. :-\
(In reply to Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from comment #16)
> - Have that notification point to a support page that lists add-ons like
> side tabs, tree style tabs and tab mix plus. We should also make sure that
> those are add-ons that won't be deprecated through our new add-ons strategy
> in the next few months.

I think this is very important but comments on #836758 suggest that all current add-ons rely on Panorama code, so they'd pretty much all be worthless once panorama is out of the picture. I can't verify this but as an add-on developer myself I doubt that anyone would write their own tab-handling code if they could have used the existing, tested and official "API" for that.

I'm also surprised that no one mentions the idea of gathering all extra tab groups into the main window ("first tab group") instead of using bookmarks or extra windows as the upgrade path. Perhaps it's not been mentioned because it's obviously a bad idea but I don't see it. This way you could keep the unrestored data (no data loss) and I think the background tabs would not load unless you select them, which means there is no possibly-terrible load that leaves FF unresponsive for minutes. I know the "don't load until selected" is an option but perhaps it can be forced per-tab? Anyway, I think this would be most intuitive way to handle things, speaking as a user - the tab groups are gone but the tabs are still around, just not organized in groups anymore.
(In reply to tukkek from comment #27)
> I'm also surprised that no one mentions the idea of gathering all extra tab
> groups into the main window ("first tab group") instead of using bookmarks
> or extra windows as the upgrade path. Perhaps it's not been mentioned
> because it's obviously a bad idea but I don't see it. This way you could
> keep the unrestored data (no data loss) and I think the background tabs
> would not load unless you select them, which means there is no
> possibly-terrible load that leaves FF unresponsive for minutes.

This loses the grouping of the non-selected tab groups, and it clutters up the main/selected/visible window(s). The result would be "messier", as it were. I'm/We're trying to keep the groups created by the user as best we can.
I see no point in keeping the groups if the group functionality is going to be removed. Sounds very counter-intuitive to me. If the users are going to receive warning beforehand then they should be able to organize themselves.

Don't get me wrong, I can understand the reasoning of not losing any sort of information but as a user my biggest fear is losing tabs entirely. If a tab is still there, it wouldn't be hard for me to figure out which group it belonged to so I could reorganize it somehow.

Anyways this ends my participation here, I'm not a FF developer so I won't be bothering the team anymore. I just think this is a cleaner way to implement this change and would be more intuitive too. I, for one, never use the bookmark menu or extra windows (except incognito) so this would be the preferred way AFAIC and I can see it being the same for other panorama users.

Anyway thanks for being the best browser out there and keep up the good work!
(In reply to tukkek from comment #27)
> I doubt that anyone would write their own tab-handling code if they could
> have used the existing, tested and official "API" for that.

Panorama never had an official API. Extensions that made use of Panorama code did so by loading its iframe and poking around in its private UI code. There really isn't any way of preserving that interface without keeping all of Panorama around. They're one and the same.
(In reply to Madhava Enros [:madhava] from comment #21)
> Regarding data-loss from in-progress work getting swept away, I don't know
> that I'm so worried about that happening for things sitting around in
> background tab-groups (not saying it won't happen there), but it _does_ seem
> like an odd experience if, on upgrade, you'd lose even what you'd been right
> in the middle of - we certainly ought to default to restoring whatever was
> in the active tab group (probably this is implicit in all the thinking
> above).

Panorama groups arent static bookmarks, they are active tabs temporarily hidden in the background when I dont need them so they dont clutter up the tab bar. They have in-progress work like all other tabs that I am planning to return to. I'd imagine this is the use case for other Panorama users as well and data loss in background groups would be a problem.

(In reply to tukkek from comment #30)
> I, for one, never use the bookmark menu or extra windows (except incognito) so this would be the
> preferred way AFAIC and I can see it being the same for other panorama users.

Same here. The point of panorama to me is that my other tabs, that are unneeded for the moment, dont have to clutter up the tab bar and dont need separate windows, which is the point of tabs in the first place. They arent bookmarks, I'd wager data loss and disappearing tabs results in some quite angry users.
>> You would still lose the user's work to categorize their tabs. For example,
>> after this migration I'd have 40 windows,

>... I think the 40 windows by itself is a serious reason not to do this.

Right, so HOW can it be done?
Why not do it better than any add-on has ever done it?

I once thought about having similarly-themed tabs grouped in 'real' groups, such as task managers do it (like lxpanel here on LXDE). That is, if you have 4 FF windows, you get a Firefox (1), Firefox (2), Firefox (3) and Firefox (4).
BUT - and this is the difference - the windows will be grouped _vertically_.

And this would be a great idea to do in FF: having similarly-themed (resp. identical) tabs grouped WITH a single tab, i. e. had you activated that option, you would have a, let's say, small arrow per tab -- clicking on that arrow will display a drop-down menu where the similar tabs are listed.

That would no longer mean 25 e-bay tabs to be open next to each other, making it an utter nightmare to select a particular one. But instead, they would all be grouped under a special tab (maybe lay-outed a particular way, I'm going to rely on your creativity)
Are APIs introduced for Panorama also removed? For example: gBrowser.visibleTabs. It will break some legacy style addons. Or, such addons are also killed together with those APIs, by migration from XUL-overlay to WebExtensions?
I'm not a developer but long time Firefox user who uses Panorama hugely (I'm the one from that 0.001% list). I want the rock solid Firefox to be blazing fast! If removing Panorama makes the job done, then just do it. But I need something instead for grouping tabs. Because I switch "groups" or "themes" like work, fun, reading, learning, browsing and other things, all the time.

I like the idea of moving "tab groups" into bookmarks. I do not have a solution for it right now. But I have some ideas that maybe useful.

There is currently available add-on that can get the similar effect like Panorama and help migrate from it to bookrmarks.

OneTab https://addons.mozilla.org/en-US/firefox/addon/onetab/ - looks like you described in about:sessionrestore but with additional features for grouping tabs.

So, basically you can get a working prototype from this add-on and make it possible working with bookmarks manager or whatever you think is possible.

OneTab shows all the tabs in resource://extension-at-one-tab-dot-com/onetab/data/onetab.html and new implementation could do the same in about:tabs (for example). Show the same list as it is available in OneTab but use Firefox bookmarks database.

And next - to get back the Panorama grouping feature (usability) - add a nice grid with tab groups. That grid would have a tab title and screenshot of first website. It can be a simple HTML block grid that fits into full page. Depending on how many tab groups a user has. In each group show only the first page screenshot or favicon and group title or first page title. That's it.

The main point is to access all tabs fast. Like button or pinned tab. And of course - show it in full page, so the user can see all the "themes" or "groups" she can open by clicking on it.

Add a view switching button - show list (current OneTab implementation) or grid view (Panorama). Probably there are users who like to see a long list.

Add a new category to bookmarks manager - Tab groups. Currently we have there History, Downloads, tags, bookmarks.

As a result you would get:

- natural migration from something that showed the path of what needs to be made from usability point of view (Panorama) to where it belongs naturally (bookmarks manager)
- integrate or remake another nice add-on OneTab into core of Firefox
- new feature that can be advertised to users as a new way on working better with information and tabs (improved Panorama)
- new possibilities on continuing to improve search, history and bookmarks and all that stuff that is in bookmarks manager
- and this time it will be done for good ;)
Btw, design of that tab group grid already exists - it is current about:newtab window. It just needs some visual improvement so that the user understands that under this icon there are many websites that are going to be opened. 

And again - it feels as a very natural Tab experience improvement or next step - Tail Groups. https://www.mozilla.org/en-US/firefox/tiles/
Many good points in many comments here.

I just want to summarize the position of UX here after having talked with Madhava offline, since his and my comments were slightly contradictory.

- We should still do the notification bar with a link to the SUMO article.
- As for suggested add-ons, there are some that do similar things like tab groups (e.g. OneTab) and some that just help with managing many tabs in general (like Tree Style Tabs).
- [UX Todo] We need to design a page that will show up when the actual migration happens. I like Madhavas direction on this.
- The idea of keeping all tabs open but in one window has its merit. Could we keep the active group in one window and combine everything else into a second window? That we we don't clutter up the main workspace and also won't open 40 windows.
- Regardless of the above, we should save all groups as bookmarks as described in comment 16

Re. comment 35: OneTab is definitely one of the closest things in the add-ons space and we should promote it. I would caution against integrating any of it into Firefox directly at this point, since that would be another rushed re-implementation of tab groups.
(In reply to YUKI "Piro" Hiroshi from comment #34)
> Are APIs introduced for Panorama also removed? For example:
> gBrowser.visibleTabs. It will break some legacy style addons. Or, such
> addons are also killed together with those APIs, by migration from
> XUL-overlay to WebExtensions?

We're not currently planning on removing gBrowser.visibleTabs.

(In reply to coolynx from comment #35)
> And next - to get back the Panorama grouping feature (usability) - add a
> nice grid with tab groups. That grid would have a tab title and screenshot
> of first website. It can be a simple HTML block grid that fits into full
> page. Depending on how many tab groups a user has. In each group show only
> the first page screenshot or favicon and group title or first page title.
> That's it.

We won't be reimplementing something "like" panorama for the moment. That does not preclude add-ons doing so, or us doing something like this in the future. But not as part of this bug.

The bookmarks manager and sidebar already let you do some of what you suggest.

(In reply to Andreas Eibach from comment #33)
> Right, so HOW can it be done?
> Why not do it better than any add-on has ever done it?

For the same reason we're removing things in the first place: we don't have the time to do this (at a level of quality that we're happy with).
Blocks: 1222490
Keywords: addon-compat
Summary: Remove panorama and migrate existing tab groups somewhere → Migrate existing tab groups
(In reply to Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from comment #37)
> Re. comment 35: OneTab is definitely one of the closest things in the
> add-ons space and we should promote it.

I'd considered recommending OneTab, but it has a lot of reviews with people complaining about it losing their data. If that's still a common problem, I think we should definitely not promote it until it's fixed.
(In reply to Kris Maglione [:kmag] from comment #39)
> (In reply to Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from
> comment #37)
> > Re. comment 35: OneTab is definitely one of the closest things in the
> > add-ons space and we should promote it.
> 
> I'd considered recommending OneTab, but it has a lot of reviews with people
> complaining about it losing their data. If that's still a common problem, I
> think we should definitely not promote it until it's fixed.

Their webpage says they've updated it to store data in more places, and they're working on cloud sync (whatever that effectively boils down to... not sure how many people would be comfortable sharing their tabs, but I digress). An actual look at their code would probably be a more effective way of seeing why data is getting lost and/or what could be done to fix this...

Anywho, I've pushed the notification to nightly, still waiting for aurora approval for the strings etc. If/when that arrives I will push the strings, but I'll likely wait with the actual notification code until the migration strategy is more solidified and we have a better idea of which add-ons will keep working and are a reasonable recommendation.

I will look at the migration of the data at the beginning of next week. Per discussion on IRC, I'll try to use about:sessionrestore as a template that lets people restore selected tabs in some way. It's hard to say right now exactly how far that'll go.

If people want to make other suggestions for the "here's what you can do now" page, it's a SUMO page so you can suggest edits, plus there's bug 1221497, which also links to a google doc if you don't want to create a SUMO account.
(In reply to Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from comment #37)

> - The idea of keeping all tabs open but in one window has its merit. Could
> we keep the active group in one window and combine everything else into a
> second window? That we we don't clutter up the main workspace and also won't
> open 40 windows.

Worth thinking about... A different view of the telemetry data from bug 1210773 comment 6 (http://mzl.la/1iI5rkj) shows that 90% of users have < 6 groups, so we wouldn't be jumbling together too many groups for most people.

Although the telemetry for PANORAMA_MEDIAN_TABS_IN_GROUPS_COUNT (http://mzl.la/1PfoQrc) suggests that the median tabs-per group is <=40 for 90% of users, which seems to imply that users who used Panorama put lots of tabs in groups. (And so smooshing them all together isn't going to be great; it would be less of a deal if we found most people only had a handful of tabs per group.)

I'm also a little concerned that AFAIK we don't have any data on how many users have tab-group data, but don't actually use Panorama now. For example, if you experimented with Panorama that one time in college in the 60s (BUT TOTALLY DIDN'T INHALE), you might be surprised to have a bunch of tabs suddenly opened up for all your browsing of psychedelic rock, Beatles fanfic, and Nixon's appearance on Rowan & Martin’s Laugh-In.


> - Regardless of the above, we should save all groups as bookmarks as
> described in comment 16

Oh, and random comment on the potential data-loss issue (comment 5)... I suspect this is a bit of an edge case, and so not worth over-rotating on. But one mitigation we could consider is saving an extra backup of session-store.json (say, last-tab-groups.json), so that in the event someone really really needs to get something back, there's a way to temporarily downgrade/ESR and get your old tab groups and session data. Not a great experience, but seems fair to consider as a tradeoff between number of users affected, and cost to implement. I'd expect bookmarks to be fine for the overwhelming majority, and the json backup could take care of the rest.
FWIW, I like the idea to present something like about:sessionrestore, which also would prevent any data loss.
(In reply to :Gijs Kruitbosch from comment #40)
> Their webpage says they've updated it to store data in more places, and
> they're working on cloud sync (whatever that effectively boils down to...
> not sure how many people would be comfortable sharing their tabs, but I
> digress). An actual look at their code would probably be a more effective
> way of seeing why data is getting lost and/or what could be done to fix
> this...

Hm. Well, their latest update on AMO is older than most of the complaints about lost data, so it doesn't seem promising. I don't think I'm going to have a chance to look through their code any time soon, but I do have sources if anyone with an NDA wants to look at them.
Release Note Request (optional, but appreciated)
[Why is this notable]: Removal of a feature
[Suggested wording]: Panorama feature removed (do we want to list panorama/tab groups/tabview explicitly ?)
[Links (documentation, blog post, etc)]: I guess we will have one.
relnote-firefox: --- → ?
Just spoke to Madhava. We'd like to put in a link to a survey that asks users about their experience with tab groups. This data may help inform Tab management in the future. @verdi - put some time on my calendar.
(In reply to Matt Grimes [:Matt_G] from comment #47)
> Just spoke to Madhava. We'd like to put in a link to a survey that asks
> users about their experience with tab groups. This data may help inform Tab
> management in the future. @verdi - put some time on my calendar.

If we're increasing the scope here still further, can we please do it in separate follow-up bugs? A survey is something we can add after we land this on Nightly, not required for the initial landing, right?
Flags: needinfo?(mgrimes)
Absolutely. I'll open a new bug for that. Thanks!
Flags: needinfo?(mgrimes)
Bug 1221050 - part 0: remove notification for tab groups removal, r?ttaubert
Attachment #8687212 - Flags: review?(ttaubert)
Bug 1221050 - part 1: set up migration infrastructure, r?ttaubert
Attachment #8687213 - Flags: review?(ttaubert)
Bug 1221050 - part 2: migrate data into bookmarks, r?ttaubert,mak
Attachment #8687214 - Flags: review?(ttaubert)
Attachment #8687214 - Flags: review?(mak77)
Bug 1221050 - part 3: strip hidden tab groups out of 'real' session store data, r?ttaubert
Attachment #8687215 - Flags: review?(ttaubert)
Bug 1221050 - part 4: write a backup to desktop, r?ttaubert
Attachment #8687216 - Flags: review?(ttaubert)
Bug 1221050 - part 5: create page for use with tabview migration, r?ttaubert
Attachment #8687217 - Flags: review?(ttaubert)
Bug 1221050 - part z: rudimentary tests to ensure the individual bits work as advertised, r?ttaubert
Attachment #8687218 - Flags: review?(ttaubert)
So, this patch does a number of things with the data that doesn't get restored as the main tab group:

0) restore active groups on the startup where it hits (no change, essentially)

1) create a plain exact backup copy of the session info on the desktop when we first read the session info.
   (I picked the desktop because then it's
    a) easy to find
    b) you can/will delete it if you don't need it - there is privacy-sensitive
       information in session storage files, and leaving it in the profile directory
       (likely forevermore) seemed like a bad idea from that perspective.
   )
2) strip out all the tabview data
3) create a set of bookmarks for *all* the groups, including the currently-visible ones, in the bookmarks manager
   (I kept the visible ones, too, because it seemed weird to exclude them
    if you like organizing stuff like this, and because they're easily deleted)
4) show you an about:sessionstore-like page that lets you restore the *hidden* groups.
   This page also has a button that brings up the library with the bookmarks from step 3.
   I used copy from attachment 8683100 [details] as much as possible, and improvised some of the rest.

I added small amounts of xpcshell tests to essentially develop these individual "bits" without having to constantly go and create groups and then check the different parts of the behaviour, and I left them in for good measure.

I tried to keep the different bits logically separate for ease of review, and so we can omit things if people are now surprised at this set of actions or really hate one of them or whatever.

For add-ons that re-instate panorama-like functionality, I expect that you should be able to use an "override" directive in a chrome manifest file in order to replace the TabGroupsMigrator.jsm, and then you can just no-op the migration behaviour.

Migration only happens once, based on migration UI version. I considered always running the code, but that seemed like it would impact startup times, and I didn't think it was worth doing.

I'll attach a screenshot of the migration page in a bit and ask for ui-review.

Tim, per earlier IRC discussion I asked you for review - please feel free to delegate to others if you're too busy or think others should look at different parts of this. Quite a lot of it deals with sessionstore-type things, though. I would particularly appreciate if you check that I'm not making assumptions that would not hold for some sessionstore files (ie assume some bit of data is always there when that is not in fact guaranteed).
Attached image Migration page (obsolete) —
Attachment #8687232 - Flags: ui-review?(philipp)
Comment on attachment 8687212 [details]
MozReview Request: Bug 1221050 - part 0: remove notification for tab groups removal, r?ttaubert

https://reviewboard.mozilla.org/r/25133/#review22931
Attachment #8687212 - Flags: review?(ttaubert) → review+
(In reply to :Gijs Kruitbosch from comment #58)
> Created attachment 8687232 [details]
> Migration page

I think we should include a link to the support doc here. I may have missed (or not read the earlier notification) and I wouldn't know what to do with a session backup without some instructions.

What does clicking "Show Bookmarked Tab Groups" do?
(In reply to Verdi [:verdi] from comment #60)
> (In reply to :Gijs Kruitbosch from comment #58)
> > Created attachment 8687232 [details]
> > Migration page
> 
> I think we should include a link to the support doc here.

That's doable. It would be good if UX and/or you could suggest copy and/or where on the page you think that should go.

> I may have missed
> (or not read the earlier notification) and I wouldn't know what to do with a
> session backup without some instructions.

It's really mostly for use by extensions (to re-import tab group information) and/or if you find you lost something SUPER IMPORTANT because you closed this tab or whatever, and want to go spelunking through the JSON to find it back. See tail end of comment #41 . I think we won't need it in 90-99% of cases. Maybe UX doesn't want to do it at all. It is technically much easier than everything else we do, and it seemed like it could potentially be useful. I considered doing it but not mentioning it on the support page, but I imagine that in that case people will wonder what this newly-created file on their desktop is (or just miss it entirely and then assume there isn't a backup).

> What does clicking "Show Bookmarked Tab Groups" do?

It opens the bookmarks manager with the newly-created "Migrated Tab Groups" folder selected.
(In reply to :Gijs Kruitbosch from comment #61)
> (In reply to Verdi [:verdi] from comment #60)
> > (In reply to :Gijs Kruitbosch from comment #58)
> > > Created attachment 8687232 [details]
> > > Migration page
> > 
> > I think we should include a link to the support doc here.
> 
> That's doable. It would be good if UX and/or you could suggest copy and/or
> where on the page you think that should go.
> 

We could do this:
* Add a "learn more" link at the end of the first line. It should take you to the support doc that we link to in the message about tab group removal in Fx 44 and that doc should include instructions for what to do with the backup if you need it.
* Then we could remove the second line about the back up.
* Which then requires a change to the wording of the last line. Maybe something like, "You can also choose to restore some or all background groups into windows now:"
Here's a quick mockup: https://www.dropbox.com/s/0h8m2nszmge23fb/restore-w-support-link.png?dl=0

> 
> > What does clicking "Show Bookmarked Tab Groups" do?
> 
> It opens the bookmarks manager with the newly-created "Migrated Tab Groups"
> folder selected.

Ok. If that looks like what I think it looks like, it might make the bookmarks hard to find later. Here's an idea:https://www.dropbox.com/s/0hod5c2hf6c12im/highlight-bookmarks.jpg?dl=0
(In reply to Verdi [:verdi] from comment #62)
> (In reply to :Gijs Kruitbosch from comment #61)
> > (In reply to Verdi [:verdi] from comment #60)
> > > What does clicking "Show Bookmarked Tab Groups" do?
> > 
> > It opens the bookmarks manager with the newly-created "Migrated Tab Groups"
> > folder selected.
> 
> Ok. If that looks like what I think it looks like, it might make the
> bookmarks hard to find later. Here's an
> idea:https://www.dropbox.com/s/0hod5c2hf6c12im/highlight-bookmarks.jpg?dl=0

I don't understand why it would be better to show UITour in the bookmarks menu than to just open the library.

The fact that searching through bookmarks doesn't find folders is bug 406157.

I'm not really convinced that the users who have so many bookmarks to trigger overflow in that menu won't understand that the folder might be at the bottom. That would be a problem for the folders they create themselves, too, right? So I don't know that we should worry too much about it.

That said, we could easily change the insertion point to be near the top, which seems to be your main request. I'd like to wait for UI feedback from Philipp, who's not around today, and review feedback from Tim, before going ahead with that change. In any case, it should (fingers crossed) be as simple as setting {index: 0} in the bookmark object when inserting the bookmarks folder into the "bookmarks menu" bookmark folder. Is that the main point of what you're saying here, or do I misunderstand your point?
Flags: needinfo?(mverdi)
(In reply to :Gijs Kruitbosch from comment #63)
> 
> I don't understand why it would be better to show UITour in the bookmarks
> menu than to just open the library.

Because even if we can highlight the new folder in the Library when someone clicks the button on the restore page, if they try to come back to it later, the Library window defaults to not showing any bookmarks and is kind of difficult to navigate (I have lots of user videos I can show you but usually looks like this https://www.dropbox.com/s/5zyfghmk7ww9f1b/bookmarks.m4v?dl=0 ). If we put them at the top of the bookmarks menu and show them that's where they are, it will be easier to find them again later. And since we're talking about tab groups that were hidden, it might be a reasonable bet that they'd want to open them up later.

> That said, we could easily change the insertion point to be near the top,
> which seems to be your main request. I'd like to wait for UI feedback from
> Philipp, who's not around today, and review feedback from Tim, before going
> ahead with that change. In any case, it should (fingers crossed) be as
> simple as setting {index: 0} in the bookmark object when inserting the
> bookmarks folder into the "bookmarks menu" bookmark folder. Is that the main
> point of what you're saying here, or do I misunderstand your point?

That's half of it (other half above).
Flags: needinfo?(mverdi)
(In reply to Verdi [:verdi] from comment #64)
> (In reply to :Gijs Kruitbosch from comment #63)
> > 
> > I don't understand why it would be better to show UITour in the bookmarks
> > menu than to just open the library.
> 
> Because even if we can highlight the new folder in the Library when someone
> clicks the button on the restore page, if they try to come back to it later,
> the Library window defaults to not showing any bookmarks and is kind of
> difficult to navigate (I have lots of user videos I can show you but usually
> looks like this https://www.dropbox.com/s/5zyfghmk7ww9f1b/bookmarks.m4v?dl=0
> ). 

I don't understand what this video is showing. Is it trying to show that some people do not understand they could hit enter or doubleclick or use the tree on the left hand side to navigate, and find the Library completely useless?
TBH, if people managed to use tab groups, despite how hard it is to find, I don't think that they fall into the user group who are not able to navigate the library.

> If we put them at the top of the bookmarks menu and show them that's
> where they are, it will be easier to find them again later. And since we're
> talking about tab groups that were hidden, it might be a reasonable bet that
> they'd want to open them up later.

If we put it at the top of the bookmarks menu, I don't understand why that would be difficult to find back in the bookmarks menu or library.

Reasons to show the library rather than the menu include:
- I'm fairly sure we can't easily force-show the main menu on mac, and it's hidden by default on Windows > XP.
- the bookmarks menu button may or may not be customized away.
- users will likely want to delete active group bookmarks if those are the only ones they're really interested in (and had forgotten about old inactive groups), or maybe even all of them. This is much easier in the library.
- users who want their groups probably want to rename the subfolders we create if their groups didn't have names in tab groups itself

Either way, if it's right at the top of the (bookmarks portion of the) bookmarks menu, I don't buy that it is "hard to find" irrespective of whether we show the library or the bookmarks menu first, especially since tab groups users are much more likely to be heavy and/or tech-literate users.

Finally, adding a UI Tour location and allowing this particular page to invoke UI Tour highlights would be significantly more complicated than the current 1-liner to show the library, and given that and all the other reasons to show the Library, I don't think changing the action of that button is warranted. Philipp can let us know if he disagrees on this.
(In reply to :Gijs Kruitbosch from comment #65)
Ok, I take your points about the difficulties in showing the bookmarks menu. The library is the only thing we can count on. So it would be great if we can go ahead with inserting the folder at the top of the menu folder and I'd like us to change the name of the folder from "Migrated Tab Groups" to "Bookmarked Tab Groups" to match the button on the restore page. This way the page says we've bookmarked your tab groups and has a button that says show bookmarked tab groups which then opens the library window with a folder named bookmarked tab groups which is selected.
> It will land when it's ready. We're aiming for Firefox 45. You would be better off using an add-on to replace it, as Firefox 44 won't get security updates, and 38 ESR will stop getting updates a few months after 45 is released, too, as 45 itself is scheduled to be the next ESR release.

Are there any addons to replace Panorama? I don't see any proposed replacements. So far I've only found some extensions that allow changing groups that are defined in Panorama. Is it even possible to make something similar? From what I've seen tabs that are grouped and not in view are not closed until you close the window... So not sure if extensions are allowed to hide tabs like that.

Correct me if I'm wrong but this bug will break:
* Tabs Manager: http://custombuttons.sourceforge.net/forum/viewtopic.php?f=4&t=3629
* https://addons.mozilla.org/en-us/firefox/addon/tab-groups-helper/
* https://addons.mozilla.org/en-us/firefox/addon/tabgroups-menu/

Let me ask something different... Would this be too late in the process to change your mind about all this mayhem ;-) if you get someone to contribute in the development of Panorama and possibly problems that arise around it?

Take this question seriously. I'm a professional programmer. I will obviously need some time to get my head around it and I understand I need to gain some trust before making contributions directly... But there are at least 10 000 users of Panorama (judging just by extension usage) and maybe there would also be someone else that would like to give it a shoot.
(In reply to Maciej Jaros from comment #71)

> Are there any addons to replace Panorama?

There are two that I'm aware of:

https://github.com/Quicksaver/Tab-Groups
https://addons.mozilla.org/en-US/firefox/addon/tab-groups/
(In reply to Dmitry Gutov from comment #72)
> There are two that I'm aware of:
> 
> https://github.com/Quicksaver/Tab-Groups
> https://addons.mozilla.org/en-US/firefox/addon/tab-groups/

Thanks for info! I tested the last one, since it is published (but not signed) on addons page. And looks like it is going to have the minimum necessary features for fully replacing Panorama. https://github.com/denschub/firefox-tabgroups/issues

Currently it works excellent!
Attachment #8687213 - Flags: review?(ttaubert) → review+
Comment on attachment 8687213 [details]
MozReview Request: Bug 1221050 - part 1: set up migration infrastructure, r=ttaubert

https://reviewboard.mozilla.org/r/25135/#review22933

::: browser/modules/TabGroupsMigrator.jsm:55
(Diff revision 1)
> +      // We create the backup with the original string, from before all our
> +      // changes:
> +      this._createBackup(stateStr);

Alternatively, you could also just copy $profile/sessionstore-backups/previous.js to the Desktop.

::: browser/modules/TabGroupsMigrator.jsm:65
(Diff revision 1)
> +      this._bookmarkAllGroupsFromState(groupData);

This returns a promise, I think it's important that this blocks shutdown too. Otherwise people would just lose that data if they hit Cmd+Q right after startup for whatever reason.
Attachment #8687214 - Flags: review?(ttaubert)
Comment on attachment 8687214 [details]
MozReview Request: Bug 1221050 - part 2: migrate data into bookmarks, r?ttaubert,mak

https://reviewboard.mozilla.org/r/25137/#review22939

::: browser/modules/TabGroupsMigrator.jsm:141
(Diff revision 1)
> +      let windowGroups = [... windowGroupMap.values()].sort((a, b) => a.anonGroupID < b.anonGroupID);

Shouldn't it be:

.sort((a, b) => a.anonGroupID - b.anonGroupID); ?

What about groups with titles, that have no .anonGroupID property?

::: browser/modules/TabGroupsMigrator.jsm:143
(Diff revision 1)
> +        return Task.spawn(function() {

return Task.spawn(function*() {
Attachment #8687215 - Flags: review?(ttaubert) → review+
Comment on attachment 8687215 [details]
MozReview Request: Bug 1221050 - part 3: strip hidden tab groups out of 'real' session store data, r=ttaubert

https://reviewboard.mozilla.org/r/25139/#review23347

::: browser/modules/TabGroupsMigrator.jsm:201
(Diff revision 1)
> +            group.tabGroupsMigrationTitle = group.title;
> +            delete group.title;

Could we just use |title| as the attribute? Or does that have a special meaning here?
Comment on attachment 8687216 [details]
MozReview Request: Bug 1221050 - part 4: write a backup to desktop, r=ttaubert

https://reviewboard.mozilla.org/r/25141/#review22935

::: browser/modules/TabGroupsMigrator.jsm:127
(Diff revision 1)
> -    // TODO
> +    let dest = Services.dirsvc.get("Desk", Ci.nsIFile);
> +    dest.append("Firefox-tabgroups-backup.json");
> +    let promise = OS.File.writeAtomic(dest.path, stateStr, {encoding: "utf-8"});
> +    AsyncShutdown.webWorkersShutdown.addBlocker("TabGroupsMigrator", promise);
> +    return promise;

As said in part 1, maybe just copy previous.js? This is fine too though.
Attachment #8687216 - Flags: review?(ttaubert) → review+
Comment on attachment 8687217 [details]
MozReview Request: Bug 1221050 - part 5: create page for use with tabview migration, r?ttaubert

https://reviewboard.mozilla.org/r/25143/#review22937

The page looks great, IMO. Should be good to go with ui-r+.

::: browser/base/content/aboutTabGroupsMigration.js:12
(Diff revision 1)
> +var loadPromise = new Promise(resolve => {
> +  let loadHandler = e => {

Use |let| everywhere?

::: browser/modules/TabGroupsMigrator.jsm:230
(Diff revision 1)
> +    let url = "chrome://browser/content/aboutTabGroupsMigration.xhtml";

Should be a const at the top.

::: browser/modules/TabGroupsMigrator.jsm:233
(Diff revision 1)
> +    win.tabs.push(newTab);
> +    win.selected = win.tabs.length;

win.selected = win.tabs.push(newTab);

If you want to be super fancy ;)
Attachment #8687217 - Flags: review?(ttaubert) → review+
Attachment #8687218 - Flags: review?(ttaubert) → review+
Comment on attachment 8687218 [details]
MozReview Request: Bug 1221050 - part z: rudimentary tests to ensure the individual bits work as advertised, r=ttaubert

https://reviewboard.mozilla.org/r/25145/#review23349

::: browser/modules/test/xpcshell/test_TabGroupsMigrator.js:60
(Diff revision 1)
> +add_task(function*() {

The tasks in this file should ideally have descriptive names.

::: browser/modules/test/xpcshell/test_TabGroupsMigrator.js:80
(Diff revision 1)
> +add_task(function*() {
> +  let groupInfo = TabGroupsMigrator._gatherGroupData(TEST_STATES.TWO_GROUPS);
> +  yield TabGroupsMigrator._bookmarkAllGroupsFromState(groupInfo);

Should we remove the bookmarks afterwards or are xpcshell tests separated enough so that this isn't an issue?
to comment 69: (OT)
>BTW. Grouping tabs was the main reason I migrated from Opera 12 
>(which had a great grouping UI). Also note that almost all power-user features are >hardly ever used. I would argue though that power-users are good for open source communities (...)

I think the main reason for removal of Panorama was that it (apparently?) had too many issues, possibly due to a sort of 'lackluster' implementation.
So personally I reckon, everything is not lost after all: let the add-on(s) become as perfect and rock-solid as they can be, and eventually the Mozilla core team *might* even consider re-implementing a similar feature, this time appropved by their QA.
But speaking of the latter: this was the main reason for removal: insufficient quality (and hence lack of solidity)
Comment on attachment 8687214 [details]
MozReview Request: Bug 1221050 - part 2: migrate data into bookmarks, r?ttaubert,mak

https://reviewboard.mozilla.org/r/25137/#review23503

It looks good generally, but I don't understand why you are trying to use Array.map and Promise.all at any cost.

::: browser/modules/TabGroupsMigrator.jsm:135
(Diff revision 1)
> +    let tabgroupsBM = yield this.bookmarkedGroupsPromise;

nit: s/tabgroupsBM/tabGroupsFolder/ or even just rootFolder.
in general I'd like to keep the Folder suffix for bookmark folders.

::: browser/modules/TabGroupsMigrator.jsm:143
(Diff revision 1)
> +        return Task.spawn(function() {

.map(Task.async(function* (group) {
  ...

::: browser/modules/TabGroupsMigrator.jsm:144
(Diff revision 1)
> +          let groupBM = yield PlacesUtils.bookmarks.insert({

nit: groupFolder

::: browser/modules/TabGroupsMigrator.jsm:151
(Diff revision 1)
> +          yield Promise.all(group.tabs.map(tab => {

The use of Promise.all (in both loops) means that as soon as something rejects, we might reject the whole method.
For example if inserting one of the first folders fails, _bookmarkAllGroupsFromState will immediately reject.
I don't know if you rely on the promise to know if the work is complete, currently it doesn't  do that.
The only reason to use Promise.all seems to be the will to use .map, but I actually think it makes the code even less readable than simple for..of loops, also cause we're already in a Task so yielding is easy.
In a simple for..of loop you could try/catch every insertion and then guarantee the promise will notify the real end of the work.
Moreover you could reportError every single error that may be useful to debug issues in the migration.
If you try to rewrite this without Promise.all, Array-map and Task.spawn, I'm sure it will be far easier to follow.
Attachment #8687214 - Flags: review?(mak77) → review+
(In reply to Marco Bonardo [::mak] from comment #84)
> The use of Promise.all (in both loops) means that as soon as something
> rejects, we might reject the whole method.
> For example if inserting one of the first folders fails,
> _bookmarkAllGroupsFromState will immediately reject.
> I don't know if you rely on the promise to know if the work is complete,
> currently it doesn't  do that.

I'm very confused by this. The promise is resolved with the new guid. How does it have the new guid if the work is not complete when the promise is resolved?

> The only reason to use Promise.all seems to be the will to use .map, but I
> actually think it makes the code even less readable than simple for..of
> loops, also cause we're already in a Task so yielding is easy.

No - I'm using Promise.all rather than yielding in the loop because yielding in the loop would mean we wait for every single bookmark to be created. It would make the process a lot slower for large numbers of bookmarks (people report in various bugs that they use panorama with upwards of 500 tabs, so I think optimizing this makes sense). There is even a comment above this code that points out that I'm trying to do the inserts for each group in parallel... Please let me know how that comment could be made clearer.

The rejection outcome is unfortunate, and one I hadn't considered. Can you suggest a way to achieve the same thing without yielding for every single insert we do? I could change the promise to include a .catch() that reportError'd the relevant error, but that feels ugly... is there a better way?

> In a simple for..of loop you could try/catch every insertion and then
> guarantee the promise will notify the real end of the work.

It is not clear to me from your comment here why Promise.all() wouldn't guarantee the end of the work if all of the inserts succeed. Are you only worried about the case where an insert fails?

> If you try to rewrite this without Promise.all, Array-map and Task.spawn,
> I'm sure it will be far easier to follow.

Maybe - but right now I just want a way to make this not be slow. yielding for every insert into the sqlite db seems like a bad idea.
Flags: needinfo?(mak77)
to comment 84:
>If you try to rewrite this without Promise.all, Array-map and Task.spawn, 
>I'm sure it will be far easier to follow.

What's the problem with Array.map() ? 
Personally I too love this feature, as it will make lots of silly for-loops superfluous. Plus, I don't think that .map() would generate so much extra overhead that it is justified to do without it.
(In reply to :Gijs Kruitbosch from comment #85)
> I'm very confused by this. The promise is resolved with the new guid. How
> does it have the new guid if the work is not complete when the promise is
> resolved?

Which promise?
From the main Task.async call (_bookmarkAllGroupsFromState) you are not returning anything specific, so ti will be resolved when all the work is done... provided nothing rejects, otherwise it will just reject as soon as one of the Promise.all rejects.

> No - I'm using Promise.all rather than yielding in the loop because yielding
> in the loop would mean we wait for every single bookmark to be created.

Did you measure that with 500 bookmarks inserts? All bookmarks operations are serialized regardless by Sqlite, so I'd not expect a so big win. If there is a big win, you can clearly ignore me.

> The rejection outcome is unfortunate, and one I hadn't considered. Can you
> suggest a way to achieve the same thing without yielding for every single
> insert we do? I could change the promise to include a .catch() that
> reportError'd the relevant error, but that feels ugly... is there a better
> way?

I don't think so, to retain the Promise.all() behavior I'd also think about adding .catch(Cu.reportError) to each promise...

> It is not clear to me from your comment here why Promise.all() wouldn't
> guarantee the end of the work if all of the inserts succeed. Are you only
> worried about the case where an insert fails?

Right. if all the inserts succeed, there would be no problem. but just one rejection will make you main method reject immediately. That may or may not be an issue, it depends on how you plan to use the returned promise. It's worth documenting that "issue" above the method if you don't fix it.
(In reply to Andreas Eibach from comment #86)
> What's the problem with Array.map() ? 

No problem with it, I also love it. I was commenting on this specific usage.
Flags: needinfo?(mak77)
https://reviewboard.mozilla.org/r/25131/#review23517

::: browser/modules/TabGroupsMigrator.jsm:91
(Diff revision 1)
> +          // This is annoying, but we'll try to deal with this.

This made me curious, what do you mean by this? Is it just a failsafe that came to your mind or did you actually run into this specific issue? And if so, is it somehow corrupted data that wouldn't even be valid for TabView itself, or valid stored data in string form that somehow isn't translating correctly to an object?
https://reviewboard.mozilla.org/r/25135/#review23519

::: browser/modules/TabGroupsMigrator.jsm:106
(Diff revision 1)
> +              title,

Should this be "title: title" or is it some form of super-duper-futuristic-ECMA18.45 JS I don't yet know about? (Sometimes I find bits of code around here that takes me hours of research in the dark corners of the internet to understand.) :)
(In reply to Luís Miguel [:quicksaver] from comment #90)
> > +              title,
> 
> Should this be "title: title" or is it some form of
> super-duper-futuristic-ECMA18.45 JS I don't yet know about?

ES6 Shorthand property names:
https://developer.mozilla.org/it/docs/Web/JavaScript/Reference/Operators/Object_initializer
(In reply to Luís Miguel [:quicksaver] from comment #89)
> https://reviewboard.mozilla.org/r/25131/#review23517
> 
> ::: browser/modules/TabGroupsMigrator.jsm:91
> (Diff revision 1)
> > +          // This is annoying, but we'll try to deal with this.
> 
> This made me curious, what do you mean by this? Is it just a failsafe that
> came to your mind or did you actually run into this specific issue? And if
> so, is it somehow corrupted data that wouldn't even be valid for TabView
> itself, or valid stored data in string form that somehow isn't translating
> correctly to an object?

I'm just being cautious, though IME which data tabview regenerates if you kill part of it and which it updates / fails to update is a bit hit and miss. IIRC it only regenerates window-level grouping information if you move groups around in the UI, while keeping the tab information. So you can add a new tab to the group and the window wills not regain its group info. I have no guarantees that add-ons or other things don't mess with the data, so I chose to be cautious. Note also that I really don't want the entire migration to fail if some data is corrupt.
(In reply to :Gijs Kruitbosch from comment #58)
> Created attachment 8687232 [details]
> Migration page

Philipp is out all week - so doing ui review in his place.

This looks great. Let's go with that along with a few text changes:
* Add a "learn more" link at the end of the first line. It should take you to the support doc.
* Remove the second line about the back up.
* Change to the wording of the last line to, "You can also choose to restore some or all background groups into windows now:"
Here's a quick mockup: https://www.dropbox.com/s/0h8m2nszmge23fb/restore-w-support-link.png?dl=0

In addition, we should:
* Insert the folder of bookmarked tab groups at the top of the bookmarks menu folder.
* Change the containing folder name from "Migrated Tab Groups" to "Bookmarked Tab Groups" to match the button on the restore page.
Will there be an option to open the tabs on the same window?
(In reply to jjdelc@gmail.com from comment #94)
> Will there be an option to open the tabs on the same window?

No. You could move the tabs back to the main window yourself, of course.
https://reviewboard.mozilla.org/r/25135/#review22933

> This returns a promise, I think it's important that this blocks shutdown too. Otherwise people would just lose that data if they hit Cmd+Q right after startup for whatever reason.

Yes, I'll adjust this.
https://reviewboard.mozilla.org/r/25139/#review23347

> Could we just use |title| as the attribute? Or does that have a special meaning here?

I avoided doing that because we need to use this in the sessionrestore restore code, and I didn't know if we keep a window title in session restore. Right now windows are always restored as "Window 1", "Window 2" etc. in about:sessionrestore and about:welcomeback, and I didn't want to inadvertently change the behaviour there. Using a unique name seemed less likely to cause issues there, and to make it clear that the change there is specific to the tab groups migration code, and can be removed again when we remove the migration infrastructure again in the future (which I believe we should do at some point, e.g. around the next (52) ESR release).

Given this, would you prefer I use "title", or not? :-)
https://reviewboard.mozilla.org/r/25145/#review23349

> Should we remove the bookmarks afterwards or are xpcshell tests separated enough so that this isn't an issue?

We create and tear down all of XPCOM and use a separate profile for every instance, and we run-by-dir, so I don't think this is an issue.
(In reply to :Gijs Kruitbosch from comment #99)
> Given this, would you prefer I use "title", or not? :-)

Sounds reasonable, esp. wanting clarity what it's used for and that it's gone soon-ish anyway. Let's leave it as is :)
Comment on attachment 8687212 [details]
MozReview Request: Bug 1221050 - part 0: remove notification for tab groups removal, r?ttaubert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25133/diff/1-2/
Comment on attachment 8687213 [details]
MozReview Request: Bug 1221050 - part 1: set up migration infrastructure, r=ttaubert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25135/diff/1-2/
Attachment #8687213 - Attachment description: MozReview Request: Bug 1221050 - part 1: set up migration infrastructure, r?ttaubert → MozReview Request: Bug 1221050 - part 1: set up migration infrastructure, r=ttaubert
Comment on attachment 8687214 [details]
MozReview Request: Bug 1221050 - part 2: migrate data into bookmarks, r?ttaubert,mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25137/diff/1-2/
Attachment #8687214 - Flags: review?(ttaubert)
Comment on attachment 8687215 [details]
MozReview Request: Bug 1221050 - part 3: strip hidden tab groups out of 'real' session store data, r=ttaubert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25139/diff/1-2/
Attachment #8687215 - Attachment description: MozReview Request: Bug 1221050 - part 3: strip hidden tab groups out of 'real' session store data, r?ttaubert → MozReview Request: Bug 1221050 - part 3: strip hidden tab groups out of 'real' session store data, r=ttaubert
Comment on attachment 8687216 [details]
MozReview Request: Bug 1221050 - part 4: write a backup to desktop, r=ttaubert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25141/diff/1-2/
Attachment #8687216 - Attachment description: MozReview Request: Bug 1221050 - part 4: write a backup to desktop, r?ttaubert → MozReview Request: Bug 1221050 - part 4: write a backup to desktop, r=ttaubert
Comment on attachment 8687217 [details]
MozReview Request: Bug 1221050 - part 5: create page for use with tabview migration, r?ttaubert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25143/diff/1-2/
Comment on attachment 8687218 [details]
MozReview Request: Bug 1221050 - part z: rudimentary tests to ensure the individual bits work as advertised, r=ttaubert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25145/diff/1-2/
Attachment #8687218 - Attachment description: MozReview Request: Bug 1221050 - part z: rudimentary tests to ensure the individual bits work as advertised, r?ttaubert → MozReview Request: Bug 1221050 - part z: rudimentary tests to ensure the individual bits work as advertised, r=ttaubert
Attached image Migration page v2
Verdi, can you give ui-r+ here, then? I've attached the updated version of the page (the link is purple because I visited it - it's the usual color if it's unvisited...). The link opens in a new tab (well, _blank, so a new window if you've configured that) because replacing that restore tab seemed like a Really Bad Idea. I believe I addressed all your points, but let me know if I missed something. :-)

There are also builds available on https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1f5fd91a69f&selectedJob=14114999 (win64 opt and linux and linux64 opt have finished, you probably need to wait a bit longer still for osx and win32 opt). Those all have patches from both here and the actual removal, so tab groups will not be usable in them anymore.

One last thing: I've implemented this migration as a "one-off" thing the same way we do most of our migrations. This means that if your profile is used by a profile that has this migration implemented, group info is removed and you will not see the migration a second time on that profile, even if you then use the same profile with an older build that still has tab groups. It's difficult to do much about this - the build that's running can't know if you plan to use the same profile on an older build, of course. We discussed this in the last fx quality meeting, but I'm just repeating it here because it's important.
Attachment #8687232 - Attachment is obsolete: true
Attachment #8687232 - Flags: ui-review?(philipp)
Attachment #8692132 - Flags: ui-review?(mverdi)
Comment on attachment 8687217 [details]
MozReview Request: Bug 1221050 - part 5: create page for use with tabview migration, r?ttaubert

Tim, can you doublecheck my l10n fu here? Because of the link, there's a slight change. Here's the interdiff:

https://reviewboard.mozilla.org/r/25143/diff/1-2/
Attachment #8687217 - Flags: review+ → review?(ttaubert)
(In reply to Marco Bonardo [::mak] from comment #87)
> > No - I'm using Promise.all rather than yielding in the loop because yielding
> > in the loop would mean we wait for every single bookmark to be created.
> 
> Did you measure that with 500 bookmarks inserts? All bookmarks operations
> are serialized regardless by Sqlite, so I'd not expect a so big win. If
> there is a big win, you can clearly ignore me.

You were right - I couldn't see a big win (though I did notice that it became a lot slower if you had the bookmarks manager open at the same time, and even on my fast & ssd-using mbp, it took ~7.5 seconds without the library open, and 13 (!) seconds when it was open). I changed the code to just use for...of loops and individual yields, though I also did add .catch()s so that the entirety of the loop doesn't go bust immediately (yields in Task.jsm will throw if the promise rejects, and that would also break the rest of the backup). Tim's reviewing that again, but feel free to take another pass if you want. :-)
(In reply to :Gijs Kruitbosch from comment #112)
> You were right - I couldn't see a big win (though I did notice that it
> became a lot slower if you had the bookmarks manager open at the same time,
> and even on my fast & ssd-using mbp, it took ~7.5 seconds without the
> library open, and 13 (!) seconds when it was open).

That's because the Library left pane is listening and reacting to bookmark changes. There is surely stuff we can do there to improve perf, sometimes in the future.
Attachment #8687217 - Flags: review?(ttaubert) → review+
Comment on attachment 8687217 [details]
MozReview Request: Bug 1221050 - part 5: create page for use with tabview migration, r?ttaubert

https://reviewboard.mozilla.org/r/25143/#review23691

Quite fancy but seems okay to me.
Comment on attachment 8687214 [details]
MozReview Request: Bug 1221050 - part 2: migrate data into bookmarks, r?ttaubert,mak

https://reviewboard.mozilla.org/r/25137/#review23693

LGTM
Attachment #8687214 - Flags: review?(ttaubert) → review+
Comment on attachment 8692132 [details]
Migration page v2

This looks great. Thanks Gijs.
Attachment #8692132 - Flags: ui-review+
Attachment #8692132 - Flags: ui-review?(mverdi)
This is looking great! Are we doing another round of copy-revision on the page text? "Tab Groups is No More. Sorry!" is obviously not something should say to our users (I know this came from my original sketch).
Flags: needinfo?(matej)
(In reply to Madhava Enros [:madhava] from comment #119)
> This is looking great! Are we doing another round of copy-revision on the
> page text? "Tab Groups is No More. Sorry!" is obviously not something should
> say to our users (I know this came from my original sketch).

Err, I already landed this, and Verdi ui-reviewed it. If we want to make adjustments, please can you file a different bug? Thanks!
Flags: needinfo?(matej) → needinfo?(madhava)
How about:

"You broke Tab Groups. Way to go."


Though you probably wanted a real answer, in which case, here are some options:

"Tab Groups are gone, but not your tabs"

"We've removed Tab Groups, but saved your tabs"

"Sorry, Tab Groups have been discontinued"
Depends on: 1229695
Depends on: 1229772
Depends on: 1229831
Gijs,

Tabmix and Session manager extensions both let user save sessions.

What do you think we need to do when a user wants to restore saved session that contains tab group data?
(In reply to tabmix.onemen from comment #123)
> Gijs,
> 
> Tabmix and Session manager extensions both let user save sessions.
> 
> What do you think we need to do when a user wants to restore saved session
> that contains tab group data?

Well, tab groups data doesn't necessarily indicate there are background groups to restore, so you'd have to explicitly check that.

Then you'd probably just want to warn the user that you can't restore those unless they have one of the add-ons installed that deal with tab groups, and/or point to the SUMO page, and/or just mark all the tabs visible and ignore/strip the extra data. In principle you could also yourself invoke the restore page and/or use the migration module to parse out the removed bits (probably from a clone of the object, maybe?). But I don't know how long we'll keep that around and if it would be futureproof to rely on it and/or its private API. I don't intend to make many changes there but they're still private APIs and so they could potentially change.

In fact, I guess you could in principle just call the public method with the new data passed in the right way, and it should do the Right Thing, except that it'll make all those extra bookmarks and a file copy that you likely don't want/need, maybe?

I don't really want to make a public API here because ultimately the migration itself is going to go away too, and there isn't much point in keeping all that code around otherwise. The most straightforward solution might therefore be to check and/or recommend the relevant add-ons.

In the end, how far you want to go in supporting this is up to you, and I don't have strong opinions on it either way. Does that help, and/or were you looking for something specific? It might make more sense to just email me privately than to discuss it on this bug, where it's not super strictly on topic.
Flags: needinfo?(madhava)
Depends on: 1230831
Depends on: 1231112
Depends on: 1231114
Depends on: 1231629
Depends on: 1232677
Depends on: 1231135
No longer blocks: 1238549
Depends on: 1238549
Added to the release notes with "Panorama feature removed" as wording (better wording is welcome)
I just noticed Firefox creates a backup of the current session into profileDir/sessionstore-backups every time it upgrades. And it only keeps 3 backups by default, so they eventually rotate out.

I wonder if that kinda makes creating a specific (identical?) backup here superfluous, not to mention this one isn't sanitized until the user deletes it manually (which I'm guessing the vast-vast majority of the users don't know how to or won't bother to do it) or until someone writes its removal code (which could be never if no one ever remembers to do it).

BTW, not trying to make any point, it's just a curiosity I noticed.
(In reply to Luís Miguel [:quicksaver] from comment #126)
> I just noticed Firefox creates a backup of the current session into
> profileDir/sessionstore-backups every time it upgrades. And it only keeps 3
> backups by default, so they eventually rotate out.
> 
> I wonder if that kinda makes creating a specific (identical?) backup here
> superfluous, not to mention this one isn't sanitized until the user deletes
> it manually (which I'm guessing the vast-vast majority of the users don't
> know how to or won't bother to do it) or until someone writes its removal
> code (which could be never if no one ever remembers to do it).
> 
> BTW, not trying to make any point, it's just a curiosity I noticed.

Please file a separate bug.
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.