839155 - Implement metro/immersive front-end for migration of data from other browsers

Status: RESOLVED INVALID

(Firefox for Metro Graveyard :: Browser, defect, P2)

Description

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

See user story in bug 831610.

A user of Firefox metro/immersive should be able to import data from other browsers.  This bug is about implementing a front-end that guides the user through the process.  The design for this front-end can be found in bug 839153.  The front-end implementation will likely rely on nsIBrowserProfileMigrator and MigrationUtils.

Comment 1

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v1 (WIP)

Minimal first pass. This patch just adds a section to the options flyout for importing data from other browsers.  It uses strings from migration.dtd, which is the file that contains strings for our import wizard.

Still TODO:
- Add individual options for each browser (Preferences, History, etc checkboxes)
- Actually hook this up to code that imports data

Comment 2

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Screenshot from patch v1

This is a screenshot of what the options flyout looks like with patch v1 applied.

Patch v1 uses strings from migration.dtd instead of creating new strings that match the mockup in bug 839153.  The advantage of using strings that are already present is that we're not creating new strings for our localizers to have to localize.  Even if we decide we absolutely need some new strings, I think we should use some strings from migration.dtd, such as "importFromIE.label" and "importFromChrome.label".  The strings available in migration.dtd can be found in [1].

Yuan: Which of these strings do you think we can reuse for our options flyout and what new strings do you think we need to create?

[1] https://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/migration/migration.dtd

Comment 3

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v2 (WIP)

Attached is a patch that correctly (mostly) populates the list of browsers to import from, and performs an import when the "import" button is pressed.  A lot of things don't work with this patch; the import that occurs has nothing to do with the checkboxes that were selected (the import is hardcoded to happen from IE), checking a browser's checkbox will cause its subitems to appear in a list next to the browser name instead of as separate checkboxes, and various other issues.

I'm handing the UI part of this bug off to someone on mbrubeck's team.  I'm not sure how useful the patch will be as a starting point, so I've summarized below what I've learned about what this code needs to do, in case it's easier to start from scratch.

We will start with a pre-defined list of browsers:

     ID              Label
  'ie'     : '&importFromIE.label;'
  'chrome' : '&importFromChrome.label;'
  'safari' : '&importFromSafari.label;'
  'firefox': '&importFromFirefox.label;'

Pass the ID of each browser to MigrationUtils.getMigrator() to see if we can import from that particular browser (either a migrator or null will be returned): Enable/disable (or maybe show/hide?) checkboxes for each of the browsers based on whether we have a migrator for that browser.

For each browser that has a migrator, call migrator.getMigrateData() to find out what items are available to import.  The value returned will be a bitwise-OR of values found in Ci.nsIBrowserProfileMigrator (e.g. BOOKMARKS, SETTINGS, etc) with 0 meaning ALL. Once you know what subitems are available, you can create checkboxes for each item that can be imported.  The label of each checkbox is created as: 2 ^ (subitem value) + '_' + ID.  See [1] for the label names that should be generated. See [2] for an example of how to generate the label names.

When the user clicks the "Import" button, for each migrator that was selected, we should call migrator.migrate(subitems, null, "") where subitems is a bitwise-OR of the options that the user selected for that browser (again, these are Ci.nsIBrowserProfileMigrator values like BOOKMARKS and SETTINGS).  All import-related checkboxes and the button should be disabled while the import is in progress.  We can listen for "Migration:Started" and "Migration:Ended" to decide when to disable/enable the UI elements.

Make it look like the picture [3] :)

[1] https://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/migration/migration.properties
[2] https://mxr.mozilla.org/mozilla-central/source/browser/components/migration/content/migration.js?rev=c43e96e4a41a#195
[3] bug 839153

Comment 4

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

I should also mention that none of this will work unless the patch in bug 848533 is applied

Comment 5

[Rodrigo Silveira [:rsilveira] [:rodms]]

Attachment: Implement metro front-end for importing data from other browsers

This can't land without the fix for bug 848533.

Comment 6

[Matt Brubeck (:mbrubeck)]

Comment on attachment 726958 [details] [diff] [review]
Implement metro front-end for importing data from other browsers

Review of attachment 726958 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/metro/base/content/browser.js
@@ +70,5 @@
>      ScrollwheelModule.init(Elements.browsers);
>      GestureModule.init();
>      BrowserTouchHandler.init();
>      PopupBlockerObserver.init();
> +    BrowserImport.init();

I'd like to avoid loading/running this code on every startup.  Can we delay it until the user the options flyout is first displayed?  See:

http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/preferences.js

(When changing this, we should be make sure it doesn't noticeably slow down the options flyout or cause things to flicker/change after the popup appears.)

::: browser/metro/base/content/browser.xul
@@ +451,5 @@
> +        <!-- The browserName attribute is used by browserImport.js to grab the right migrator. -->
> +        <!-- If we ever want to import from additonal browsers, we'll have to manually -->
> +        <!-- add a checkbox.  There isn't a way around this since the value of the label -->
> +        <!-- (e.g. '&importFromIE.label', '&importFromChrome.label', etc) and the browser name -->
> +        <!-- are important pieces of information that we can't discover programmatically -->

If you wanted to make this more dynamic, you could switch from an DTD file to a .properties file for the labels and browser names.  However, I think the current approach is fine (especially since it is lets us reuse Firefox code with minimal changes).

::: browser/metro/base/content/browserImport.js
@@ +17,5 @@
> +      }
> +
> +      let migrator = MigrationUtils.getMigrator(browserName);
> +      if (!migrator) {
> +        browserCheckbox.setAttribute("hidden", "true");

As a shorthand you can use "browserCheckbox.hidden = true":

https://developer.mozilla.org/en-US/docs/XUL/Property/hidden

@@ +37,5 @@
> +
> +      // Create checkboxes for each type of data we can import from this source
> +      for (let i = 0; i < 16; ++i) {
> +        let itemID = (subItems >> i) & 0x1 ? Math.pow(2, i) : 0;
> +        if (itemID > 0) {

Minor suggestion: I would find this slightly more readable as:

  let itemID = 1 << i;
  if (subItems & itemID) {
    // ...

@@ +65,5 @@
> +    let importButton = document.getElementById("prefs-import-import");
> +    let allCheckboxes = document.querySelectorAll("#prefs-import checkbox");
> +
> +    // Show importing notification
> +    importNotificationDeck.selectedIndex = 1;

Could you use selectedPanel instead of selectedIndex for the deck items?  I find it more readable, and less likely to break when the content changes.

@@ +82,5 @@
> +      let migrator = MigrationUtils.getMigrator(browserName);
> +
> +      if (!migrator) {
> +        continue;
> +      }

This should never happen, right?  If so, maybe we should just remove this check and let an exception be thrown.

@@ +87,5 @@
> +
> +      let subItemWrapper = document.getElementById("prefs-import-subitems-" + browserName);
> +      if (!subItemWrapper) {
> +        continue;
> +      }

Same here.

@@ +107,5 @@
> +
> +      if (!hasSubItem) {
> +        dump('Browser ' + browserName + ' had no subitems selected.');
> +        continue;
> +      }

Should we prevent this situation, since the user probably didn't mean for this to happen?

Maybe we shouldn't enable the the import button unless at least one parent item and one sub-item is checked.  Also we could automatically un-check a parent item if all its the sub-items are unchecked.

Or we could just alert the user and bail out here.

::: browser/metro/theme/browser.css
@@ +730,5 @@
>  
> +/* Import browser data */
> +.import-browser[hidden] {
> +  display: none;
> +}

Is this needed?  Hidden XUL elements should disappear automatically:
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/xul.css#33

::: toolkit/locales/en-US/chrome/migration/migration.dtd
@@ +37,5 @@
>  
> +<!ENTITY importTitle.label              "Import From Other Browsers">
> +<!ENTITY importTitle.description        "Choose browsers and types of data to import">
> +
> +<!ENTITY migrate.label                  "Import">

Note that we will need a toolkit peer to sign off on changes to this file.  (Or if the toolkit folks prefer, we could put Metro-only strings in /browser/metro/locales instead.)

Comment 7

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 726958 [details] [diff] [review]
Implement metro front-end for importing data from other browsers

This seems good to me; I'll leave the more detailed review to mbrubeck.

Rodrigo; there's a lot of churn going on in bug 848533.  It might be wise to pause work on this until there is a clear path forward (and hopefully a patch to work with) in the other bug.

Comment 8

[Rodrigo Silveira [:rsilveira] [:rodms]]

Thanks Tim, I was thinking the same thing. I'll attach the latest patch I have and resume once bug 848533 is at a better state.

Comment 9

[Rodrigo Silveira [:rsilveira] [:rodms]]

Attachment: Latest patch upate

Will get back to this patch once bug 848533 has less churn.

Comment 10

[Rodrigo Silveira [:rsilveira] [:rodms]]

Unassigning myself since I'm not working on this.