Bug 1231114 - fix labels of unnamed groups as well as group ordering on the tab migration page, r=mconley
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Mon, 14 Dec 2015 20:46:00 +0000
changeset 315842 f8840e2aec5afdebf63216dc219e103b5d4c391a
parent 315841 b5336b2a4e6ebb6fd73a6b19052ac8eaa75e121a
child 315843 dd4bb6fad8dafc2557bd7835d2a26028b7d21e7a
push id8469
push userbgrinstead@mozilla.com
push dateWed, 16 Dec 2015 19:53:11 +0000
reviewersmconley
bugs1231114
milestone46.0a1
Bug 1231114 - fix labels of unnamed groups as well as group ordering on the tab migration page, r=mconley
browser/modules/TabGroupsMigrator.jsm
browser/modules/test/xpcshell/test_TabGroupsMigrator.js
--- a/browser/modules/TabGroupsMigrator.jsm
+++ b/browser/modules/TabGroupsMigrator.jsm
@@ -139,16 +139,19 @@ this.TabGroupsMigrator = {
           if (!groupData) {
             let title = (groupInfo[group] && groupInfo[group].title) || "";
             groupData = {
               tabs: [],
               tabGroupsMigrationTitle: title,
             };
             if (!title) {
               groupData.anonGroupID = ++globalAnonGroupID;
+              groupData.tabGroupsMigrationTitle =
+                gBrowserBundle.formatStringFromName("tabgroups.migration.anonGroup",
+                                                    [groupData.anonGroupID], 1);
             }
             // If this is the active group, set the active group ID and add
             // all the already-known tabs (that didn't list a group ID), if any.
             if (!activeGroupID && !tab.hidden) {
               activeGroupID = group;
               groupData.tabs = tabsWithoutGroup;
               tabsWithoutGroup = null;
             }
@@ -174,42 +177,43 @@ this.TabGroupsMigrator = {
   _createBackup(stateStr) {
     let dest = Services.dirsvc.get("ProfD", Ci.nsIFile);
     dest.append("tabgroups-session-backup.json");
     let promise = OS.File.writeAtomic(dest.path, stateStr, {encoding: "utf-8"});
     AsyncShutdown.webWorkersShutdown.addBlocker("TabGroupsMigrator", promise);
     return promise;
   },
 
+  _groupSorter(a, b) {
+    if (!a.anonGroupID) {
+      return -1;
+    }
+    if (!b.anonGroupID) {
+      return 1;
+    }
+    return a.anonGroupID - b.anonGroupID;
+  },
+
   _bookmarkAllGroupsFromState: Task.async(function*(groupData) {
     // First create a folder in which to put all these bookmarks:
     this.bookmarkedGroupsPromise = PlacesUtils.bookmarks.insert({
       parentGuid: PlacesUtils.bookmarks.menuGuid,
       type: PlacesUtils.bookmarks.TYPE_FOLDER,
       index: 0,
       title: gBrowserBundle.GetStringFromName("tabgroups.migration.tabGroupBookmarkFolderName"),
     }).catch(Cu.reportError);
     let tabgroupsFolder = yield this.bookmarkedGroupsPromise;
 
     for (let [, windowGroupMap] of groupData) {
-      let windowGroups = [... windowGroupMap.values()].sort((a, b) => {
-        if (!a.anonGroupID) {
-          return -1;
-        }
-        if (!b.anonGroupID) {
-          return 1;
-        }
-        return a.anonGroupID - b.anonGroupID;
-      });
+      let windowGroups = [... windowGroupMap.values()].sort(this._groupSorter);
       for (let group of windowGroups) {
         let groupFolder = yield PlacesUtils.bookmarks.insert({
           parentGuid: tabgroupsFolder.guid,
           type: PlacesUtils.bookmarks.TYPE_FOLDER,
-          title: group.tabGroupsMigrationTitle ||
-            gBrowserBundle.formatStringFromName("tabgroups.migration.anonGroup", [group.anonGroupID], 1),
+          title: group.tabGroupsMigrationTitle
         }).catch(Cu.reportError);
 
         for (let tab of group.tabs) {
           let entry = tab.entries[tab.index - 1];
           yield PlacesUtils.bookmarks.insert({
             parentGuid: groupFolder.guid,
             title: tab.title || entry.title,
             url: entry.url,
@@ -248,22 +252,25 @@ this.TabGroupsMigrator = {
           // Make sure we unhide it, or it won't get restored correctly
           tab.hidden = false;
         }
       }
 
       // We then convert any hidden groups into windows for the state object
       // we show in about:tabgroupsdata
       if (groupInfoForWindow) {
+        let windowsToReturn = [];
         for (let groupID of hiddenGroupIDs) {
           let group = groupInfoForWindow.get("" + groupID);
           if (group) {
-            stateToReturn.windows.push(group);
+            windowsToReturn.push(group);
           }
         }
+        windowsToReturn.sort(this._groupSorter);
+        stateToReturn.windows = stateToReturn.windows.concat(windowsToReturn);
       }
 
       // Finally we remove tab groups data from the window:
       if (win.extData) {
         delete win.extData["tabview-group"];
         delete win.extData["tabview-groups"];
         delete win.extData["tabview-ui"];
         delete win.extData["tabview-visibility"];
--- a/browser/modules/test/xpcshell/test_TabGroupsMigrator.js
+++ b/browser/modules/test/xpcshell/test_TabGroupsMigrator.js
@@ -160,27 +160,109 @@ const TEST_STATES = {
           }
         ],
         extData: {
           "tabview-group": "{\"2\":{}}",
         },
       }
     ],
   },
+  SORTING_NAMING_RESTORE_PAGE: {
+    windows: [
+      {
+        tabs: [
+          {
+            entries: [{
+              url: "about:robots",
+              title: "Robots 1",
+            }],
+            index: 1,
+            hidden: false,
+            extData: {
+              "tabview-tab": "{\"groupID\":2,\"active\":true}",
+            },
+          },
+          {
+            entries: [{
+              url: "about:robots",
+              title: "Robots 2",
+            }],
+            index: 1,
+            hidden: false,
+            extData: {
+              "tabview-tab": "{\"groupID\":2}",
+            },
+          },
+          {
+            entries: [{
+              url: "about:robots",
+              title: "Robots 3",
+            }],
+            index: 1,
+            hidden: true,
+            extData: {
+              "tabview-tab": "{\"groupID\":13}",
+            },
+          },
+          {
+            entries: [{
+              url: "about:robots",
+              title: "Robots 4",
+            }],
+            index: 1,
+            hidden: true,
+            extData: {
+              "tabview-tab": "{\"groupID\":15}",
+            },
+          },
+          {
+            entries: [{
+              url: "about:robots",
+              title: "Robots 5",
+            }],
+            index: 1,
+            hidden: true,
+            extData: {
+              "tabview-tab": "{\"groupID\":16}",
+            },
+          },
+          {
+            entries: [{
+              url: "about:robots",
+              title: "Robots 6",
+            }],
+            index: 1,
+            hidden: true,
+            extData: {
+              "tabview-tab": "{\"groupID\":17}",
+            },
+          }
+        ],
+        extData: {
+          "tabview-group": "{\"2\":{},\"13\":{\"title\":\"Foopy\"}, \"15\":{\"title\":\"Barry\"}, \"16\":{}, \"17\":{}}",
+          "tabview-groups": "{\"nextID\":20,\"activeGroupId\":2,\"totalNumber\":5}",
+          "tabview-visibility": "false"
+        },
+      },
+    ]
+  },
 };
 
 add_task(function* gatherGroupDataTest() {
   let groupInfo = TabGroupsMigrator._gatherGroupData(TEST_STATES.TWO_GROUPS);
   Assert.equal(groupInfo.size, 1, "Information about 1 window");
   let singleWinGroups = [... groupInfo.values()][0];
   Assert.equal(singleWinGroups.size, 2, "2 groups");
   let group2 = singleWinGroups.get("2");
   Assert.ok(!!group2, "group 2 should exist");
   Assert.equal(group2.tabs.length, 2, "2 tabs in group 2");
-  Assert.equal(group2.tabGroupsMigrationTitle, "", "We don't assign titles to untitled groups");
+  // Note that this has groupID 2 in the internal representation of tab groups,
+  // but because it was the first group we encountered when migrating, it was
+  // labeled "group 1" for the user
+  Assert.equal(group2.tabGroupsMigrationTitle, "Group 1", "We assign a numeric title to untitled groups");
   Assert.equal(group2.anonGroupID, "1", "We mark an untitled group with an anonymous id");
   let group13 = singleWinGroups.get("13");
   Assert.ok(!!group13, "group 13 should exist");
   Assert.equal(group13.tabs.length, 1, "1 tabs in group 13");
   Assert.equal(group13.tabGroupsMigrationTitle, "Foopy", "Group with title has correct title");
   Assert.ok(!("anonGroupID" in group13), "We don't mark a titled group with an anonymous id");
 });
 
@@ -303,8 +385,22 @@ add_task(function* dealWithNoGroupInfo()
   Assert.equal(groupInfo.size, 1, "Should have 1 window");
   let windowGroups = [...groupInfo][0][1];
   Assert.equal(windowGroups.size, 1, "Window should have 1 group");
   let fallbackActiveGroup = windowGroups.get("active group");
   Assert.ok(fallbackActiveGroup, "Fallback group should exist");
   Assert.equal(fallbackActiveGroup.tabs.length, 3, "There should be 3 tabs in the fallback group");
 });
 
+add_task(function* groupSortingInRemovedDataUsedForRestorePage() {
+  let stateClone = JSON.parse(JSON.stringify(TEST_STATES.SORTING_NAMING_RESTORE_PAGE));
+  let groupInfo = TabGroupsMigrator._gatherGroupData(stateClone);
+  let removedGroups = TabGroupsMigrator._removeHiddenTabGroupsFromState(stateClone, groupInfo);
+  Assert.equal(stateClone.windows.length, 1, "Should still only have 1 window");
+  Assert.equal(stateClone.windows[0].tabs.length, 2, "Should now have 2 tabs");
+
+  let restoredWindowTitles = removedGroups.windows.map(win => win.tabGroupsMigrationTitle);
+  // Note that group 1 is the active group and as such it won't appear in the list of
+  // things the user can restore:
+  Assert.deepEqual(restoredWindowTitles,
+    ["Barry", "Foopy", "Group 2", "Group 3"]);
+});
+