706973 - Support migration from Chromium and Chrome Canary

Status: VERIFIED FIXED

Description

[Matthew N. [:MattN] (PM if requests are blocking you)]

This should just be a matter of checking additional directories[1] and updating the Chrome migrator strings to be Chrome/Chromium.

[1] http://www.chromium.org/user-experience/user-data-directory

Comment 1

[Makoto Kato [:m_kato]]

Also, profile of Chrome Canary seems to be "Chrome SxS", not "Chrome"

Comment 2

[Matthew N. [:MattN] (PM if requests are blocking you)]

If we go with one common implementation for all of these (which I feel is probably best for maintainability) then we need to distinguish between profiles of the same name in different browsers.  For example, we need to be able to distinguish between the "Default" profile in all of these. 

I think fixing bug 705927 and appending prefixes/suffixes to the friendly names to distinguish the browser would be a straightforward solution.

Sample profile selection page:
* Chrome - Default
* Chrome - Development Profile
* Chromium - Default
* Canary - Default

Comment 3

[Matthew N. [:MattN] (PM if requests are blocking you)]

Not planning on working on this anytime soon.  It should wait for bug 718608 to avoid major bit-rot.

Comment 4

[Mike Connor [:mconnor]]

Yandex Browser is at 5% of market and growing in Russia, so I'd really like to see this addressed.  Especially with Opera also on Chromium, we've got enough mass to bump this back up, especially now that bug 718608 is fixed.

Comment 6

[Matthew N. [:MattN] (PM if requests are blocking you)]

(In reply to Matthew N. [:MattN] from comment #2)
> If we go with one common implementation for all of these (which I feel is
> probably best for maintainability) then we need to distinguish between
> profiles of the same name in different browsers.

Now that bug 718608 landed, I think we should just share a common prototype based on the current Chrome one but have each override the directory for profile folders. I no longer think we should combine all Chromium-based browsers on one profile selection page as Opera and Yandex would feel a bit weird on the same page as Chrome/Chromium/Canary.

Comment 7

[Matthew N. [:MattN] (PM if requests are blocking you)]

Created attachment 8432192 [details] [diff] [review]
WIP 1 - Use the Chrome prototype for Chromium

Mano, what you do you think of this approach (e.g. compared to comment 2)? The most unfortunate thing is the duplication in migration.properties but I can personally live with it for now. I was thinking about have Chrome, Chromium, and Canary in the existing file and have separate files still using ChromeProfileMigrator.prototype (possibly with a rename of it and this._chromeUserDataFolder) for Opera and Yandex as it's less obvious they are related and I think they are more likely to diverge from the Google ones e.g. implementing their own bookmark/history backend.

Canary paths are documented at http://www.chromium.org/chrome-release-channels#TOC-Back-up-your-data- which I'll include once there is agreement on the implementation approach.

Comment 8

[Matthew N. [:MattN] (PM if requests are blocking you)]

firefox-backlog+ per comment 4 as it starts the foundation for Opera and Yandex too.

Comment 9

[Matthew N. [:MattN] (PM if requests are blocking you)]

(In reply to Matthew N. [:MattN] from comment #7)
> The most unfortunate thing is the duplication in migration.properties but I
> can personally live with it for now.

A simple hack to avoid this would be to use a regex replace in getLocalizedString (where we already do overrides) to replace "chromium" and "canary" with "chrome" in the right context of the string name.

Comment 10

[Mano (::mano, needinfo? for any questions; not reading general bugmail)]

Comment on attachment 8432192 [details] [diff] [review]
WIP 1 - Use the Chrome prototype for Chromium

As I said over IRC, it's just a shame I never got to deCOM the migrators. Too bad, but it's out of the scope of this bug. I'll prioritize the deCOM bug once we have all of this chrome-in-a-costume migrators in place.

However, the migration.properties duplication is only tolerable as long as there's just one duplicate. Since we know that is not to be the case, I'm going to consider this a temporary solution. This must be fixed before the next duplicate lands. I suggested a solution over IRC that should be easy to implement.

Comment 11

[Matthew N. [:MattN] (PM if requests are blocking you)]

Created attachment 8436402 [details] [diff] [review]
v.1 Chromium and Canary

Notes:
* Canary isn't available for Linux
* Chrome and Chrome Canary cannot be distinguished by the default browser check but I don't think that's a big deal since Canary will still be listed.
* I just did the simple fix to avoid duplicating strings for now as it felt like the long-term fix should be in a separate bug as this patch was involved enough.

Comment 12

[Mano (::mano, needinfo? for any questions; not reading general bugmail)]

Comment on attachment 8436402 [details] [diff] [review]
v.1 Chromium and Canary

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

My r+ depends on manual testing across all three platforms ("default browser" situation included).

::: browser/components/migration/src/BrowserProfileMigrators.manifest
@@ +1,4 @@
>  component {6F8BB968-C14F-4D6F-9733-6C6737B35DCE} ProfileMigrator.js
>  contract @mozilla.org/toolkit/profile-migrator;1 {6F8BB968-C14F-4D6F-9733-6C6737B35DCE}
> +
> +#if defined(XP_WIN) || defined(XP_MACOSX)

Hrm? In ChromeProfileMigrator it looks like you support all platforms.

::: browser/components/migration/src/MigrationUtils.jsm
@@ +51,5 @@
>    // for each Firefox-update channel.
>    const APP_DESC_TO_KEY = {
>      "Internet Explorer": "ie",
>      "Safari":            "safari",
> +    "Google Chrome":     "chrome",  // Windows, Linux - Canary too but we don't handle that now.

The comment is unclear. Please improve the wording, file a bug, and reference it here.

@@ +410,5 @@
>     *
>     * @see nsIStringBundle
>     */
>    getLocalizedString: function MU_getLocalizedString(aKey, aReplacements) {
> +    aKey = aKey.replace(/_(canary|chromium)$/, "_chrome");

File a bug for the more general fix we discussed and reference it here.

::: browser/locales/en-US/chrome/browser/migration/migration.properties
@@ +6,5 @@
>  
>  # Browser Specific
>  sourceNameIE=Internet Explorer
>  sourceNameSafari=Safari
> +sourceNameCanary=Google Chrome Canary

By the way, note that my review is purely technical, you should ensure that migration from Canary isn't a wontfix.

Comment 13

[[e-mail]]

Is there a release goal?

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/e00e8b9e3b35

Comment 15

[Matthew N. [:MattN] (PM if requests are blocking you)]

(In reply to johnsc301 from comment #13)
> Is there a release goal?

Firefox 42 if things go well. If you could test the Nightly in 2 days it would be great: https://nightly.mozilla.org/

Comment 16

[Matthew N. [:MattN] (PM if requests are blocking you)]

I updated https://wiki.mozilla.org/QA/Firefox_migrators

QA should mostly confirm that the wizard doesn't break. Chromium/Canary don't need tier 1 testing as they currently just work the same as Chrome but look in different directories.

Comment 17

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/e00e8b9e3b35

Comment 18

[Vasilica Mihasca, QA [away for an extended period of time - please needinfo [e-mail]]]

Verified fixed on Firefox 42 beta 7 (20151015151621) under Ubuntu 14.04 32-bit and Windows 7 64-bit while importing from Chromium, Chrome and Canary.

I’ve noticed a potential issue: Chromium and Canary bookmarks are imported in a “From Google Chrome” folder. 
I think ”From Chromium” would be a better name for bookmarks folder when importing from Chromium and “From Google Chrome Canary” for bookmark folder when importing from Canary.

Matthew, any thoughts about this? Should I file a new bug for the mentioned issue?

Comment 19

[Matthew N. [:MattN] (PM if requests are blocking you)]

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #18)
> Matthew, any thoughts about this? Should I file a new bug for the mentioned
> issue?

Thanks! I agree it should be fixed but I don't think it's a hard-blocker since we only create that folder when importing from within Firefox, not during the more common firstrun import. I filed this as bug 1216186 with pointers to the code.