1410839 - Implement the helper function to scan and list all extensions installed in users' Chrome browser

Status: RESOLVED FIXED

(Firefox :: Migration, enhancement, P3)

Description

[Evan Tseng [:evanxd]]

Since in Bug 1398298 we would like to do thing like "offer to install UBlock or AdBlock if user had it installed in Chrome", we need helper function to scan and list all extensions install in users Chrome browser.

Comment 1

[Evan Tseng [:evanxd]]

Attachment: Bug 1410839 - Add the ChromeMigrationUtils.jsm module and implement the getExtensionList and related functions to export all extensions information installed in a specific profile.

Review commit: https://reviewboard.mozilla.org/r/195630/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195630/

Comment 2

[Evan Tseng [:evanxd]]

Uploaded the WIP patch which can check a specific extension is installed or not in Chrome.

Comment 3

[:Gijs (he/him)]

Please use OS.File and async IO instead of the synchronous nsIFile create/append/exists() methods.

Comment 4

[Evan Tseng [:evanxd]]

(In reply to :Gijs (no reviews; PTO recovery mode) from comment #3)
> Please use OS.File and async IO instead of the synchronous nsIFile
> create/append/exists() methods.

Sure, let's do it.

Comment 5

[Evan Tseng [:evanxd]]

Discussed what helper function we need here with Evelyn, she suggested we should list all Chrome extensions with necessary information to support potential future work (dealing with Chrome extensions).

And I think we could export the extension ID, name, and description information. For example, we will export the below JSON data for AdBlock.
```
{
  "id": "gighmmpiobklfepjocnamgkkbiglidom",
  "name": "AdBlock",
  "description": "The most popular Chrome extension, with over 40 million users! Blocks ads all over the web."
}
```

Comment 6

[Evan Tseng [:evanxd]]

Attachment: adblock-manifest.json

Comment 7

[Evan Tseng [:evanxd]]

Hi Gijs,

Based on Comment 5, I think we could implement a helper function maybe call "getExtensionList" and export the "extension id", "extension name", and "extension description" information for all installed Chrome extensions.

So the function interface might be looked like:
```
/**
 * Get all extensions installed in a specific profile.
 * @param {String} profileId - A Chrome user profile ID. For example, "Profile 1".
 * @returns {Array} All installed Chrome extensions information.
 */
function getExtensionList(profileId) {
  return [];
}
```

And we also have a helper function "getLastActiveProfileId" to get the last active profile's ID.
```
/**
 * Get the last active profile's ID.
 * @returns {String} The last active profile's ID.
 */
function getLastActiveProfileId() {
  return "lastActiveProfileId";
}
```

What do you think?

Comment 8

[Evan Tseng [:evanxd]]

And you could check the full AdBlock's manifest file in the attachment 8924896 [details].

Comment 9

[Evan Tseng [:evanxd]]

Since we should use async IO, the above "getExtensionList" and "getLastActiveProfileId" functions should return Promise with the data.

Comment 10

[:Gijs (he/him)]

(In reply to Evan Tseng [:evanxd] from comment #9)
> Since we should use async IO, the above "getExtensionList" and
> "getLastActiveProfileId" functions should return Promise with the data.

Or you could just declare them as async functions, which will make it easier. :-)

Otherwise the API in comment #7 seems sane to me.

Comment 11

[Evan Tseng [:evanxd]]

Comment on attachment 8924372 [details]
Bug 1410839 - Add the ChromeMigrationUtils.jsm module and implement the getExtensionList and related functions to export all extensions information installed in a specific profile.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195630/diff/1-2/

Comment 12

[Evan Tseng [:evanxd]]

(In reply to :Gijs (slow, PTO recovery mode) from comment #10)
> Or you could just declare them as async functions, which will make it
> easier. :-)
> 
> Otherwise the API in comment #7 seems sane to me.

Sure, let's declare them as async functions and follow the comment #7 API interface.

Comment 13

[Evan Tseng [:evanxd]]

Updated the patch to build the APIs as async functions.

Comment 14

[Evan Tseng [:evanxd]]

Comment on attachment 8924372 [details]
Bug 1410839 - Add the ChromeMigrationUtils.jsm module and implement the getExtensionList and related functions to export all extensions information installed in a specific profile.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195630/diff/2-3/

Comment 15

[Evan Tseng [:evanxd]]

Comment on attachment 8924372 [details]
Bug 1410839 - Add the ChromeMigrationUtils.jsm module and implement the getExtensionList and related functions to export all extensions information installed in a specific profile.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195630/diff/3-4/

Comment 16

[Evan Tseng [:evanxd]]

The `getExtensionList` helper function is working in progress. And some tests are added to protect `getLastUsedProfileId`, `isExtensionInstalled`, and `getExtensionList` functions.

Comment 17

[Evan Tseng [:evanxd]]

Continue to fix the localized string issues.

Comment 18

[Evan Tseng [:evanxd]]

Comment on attachment 8924372 [details]
Bug 1410839 - Add the ChromeMigrationUtils.jsm module and implement the getExtensionList and related functions to export all extensions information installed in a specific profile.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195630/diff/4-5/

Comment 19

[Evan Tseng [:evanxd]]

Comment on attachment 8924372 [details]
Bug 1410839 - Add the ChromeMigrationUtils.jsm module and implement the getExtensionList and related functions to export all extensions information installed in a specific profile.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195630/diff/5-6/

Comment 20

[Evan Tseng [:evanxd]]

Comment on attachment 8924372 [details]
Bug 1410839 - Add the ChromeMigrationUtils.jsm module and implement the getExtensionList and related functions to export all extensions information installed in a specific profile.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195630/diff/6-7/

Comment 21

[Evan Tseng [:evanxd]]

Comment on attachment 8924372 [details]
Bug 1410839 - Add the ChromeMigrationUtils.jsm module and implement the getExtensionList and related functions to export all extensions information installed in a specific profile.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195630/diff/7-8/

Comment 22

[Evan Tseng [:evanxd]]

Comment on attachment 8924372 [details]
Bug 1410839 - Add the ChromeMigrationUtils.jsm module and implement the getExtensionList and related functions to export all extensions information installed in a specific profile.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195630/diff/8-9/

Comment 23

[Evan Tseng [:evanxd]]

Comment on attachment 8924372 [details]
Bug 1410839 - Add the ChromeMigrationUtils.jsm module and implement the getExtensionList and related functions to export all extensions information installed in a specific profile.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195630/diff/9-10/

Comment 24

[Evan Tseng [:evanxd]]

Comment on attachment 8924372 [details]
Bug 1410839 - Add the ChromeMigrationUtils.jsm module and implement the getExtensionList and related functions to export all extensions information installed in a specific profile.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195630/diff/10-11/

Comment 25

[Evan Tseng [:evanxd]]

Comment on attachment 8924372 [details]
Bug 1410839 - Add the ChromeMigrationUtils.jsm module and implement the getExtensionList and related functions to export all extensions information installed in a specific profile.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195630/diff/11-12/

Comment 26

[Evan Tseng [:evanxd]]

Comment on attachment 8924372 [details]
Bug 1410839 - Add the ChromeMigrationUtils.jsm module and implement the getExtensionList and related functions to export all extensions information installed in a specific profile.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195630/diff/12-13/

Comment 27

[Evan Tseng [:evanxd]]

Comment on attachment 8924372 [details]
Bug 1410839 - Add the ChromeMigrationUtils.jsm module and implement the getExtensionList and related functions to export all extensions information installed in a specific profile.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195630/diff/13-14/

Comment 28

[Evan Tseng [:evanxd]]

Hi Gijs,

Could you help review the patch?

Thank you very much. :)

Comment 29

[:Gijs (he/him)]

Comment on attachment 8924372 [details]
Bug 1410839 - Add the ChromeMigrationUtils.jsm module and implement the getExtensionList and related functions to export all extensions information installed in a specific profile.

https://reviewboard.mozilla.org/r/195630/#review204860

I'm a bit confused... do we already have a usecase for getting the extension names etc.? Because for bug 1398298 we only need to be able to answer "is extension with ID <whatever> installed", for which we don't need most of this code.

Perhaps it would make sense to split this patch so that we land the 'is this add-on installed' bits first, and can still build something to recommend users install corresponding add-ons for any on our 'high profile' list, and then we can do the 'get a list of nice strings for each installed add-on' bit later.

::: browser/components/migration/ChromeMigrationUtils.jsm:9
(Diff revision 14)
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/AppConstants.jsm");
> +Cu.import("resource://gre/modules/osfile.jsm");

Nit: please sort alphabetically.

::: browser/components/migration/ChromeMigrationUtils.jsm:13
(Diff revision 14)
> +XPCOMUtils.defineLazyServiceGetter(this, "gDirService",
> +                                   "@mozilla.org/file/directory_service;1",
> +                                   "nsIProperties");

As noted below, you don't need this, so you can omit it. :-)

::: browser/components/migration/ChromeMigrationUtils.jsm:30
(Diff revision 14)
> +      try {
> +        if (entry.isDir) {
> +          let extensionInformation = await this.getExtensionInformation(entry.name, profileId);
> +          extensionInformation && extensionList.push(extensionInformation);
> +        }
> +      } catch (ex) {
> +        Cu.reportError(ex);
> +      }

I think you're overusing try...catch in this file. Every method has most of its code within a try...catch.

What would throw here? Can `entry.isDir` throw? Can `entry` be null? `getExtensionInformation` already has a try...catch itself, so I don't think that can throw either.

I would prefer to limit the amount of error-catching because it makes the code less performant, and can actually hide errors and/or make them harder to track down, because tests won't fail but will report errors to the console. We should only use try...catch where it is necessary to do so.

::: browser/components/migration/ChromeMigrationUtils.jsm:50
(Diff revision 14)
> +   * @param {String} profileId - The user profile's ID.
> +   * @retruns {Object} The Chrome extension information.
> +   */
> +  async getExtensionInformation(extensionId, profileId = "Default") {
> +    let extensionInformation = null;
> +    if (await this.isExtensionInstalled(extensionId, profileId)) {

This is unfortunate because it will basically run a stat check on the files you're iterating over already, when called from `getExtensionList`.

The manifest path iterator will just fail if the directory doesn't exist, right? So instead of checking first whether the extension exists, just omit that check, but specifically catch "file not found" errors for `_sortFilesByLastModificatioNDate` and just early-return `null` in that case.

::: browser/components/migration/ChromeMigrationUtils.jsm:93
(Diff revision 14)
> +   * @param {String} extensionId - The extension ID.
> +   * @param {String} profileId - The user profile's ID.
> +   * @retruns {String} The locale string.
> +   */
> +  async getLocaleString(key, locale, extensionId, profileId = "Default") {
> +    let localeString = null;

You're using this like this:

```js
let foo = foo && this._isLocaleKey(foo) ? this.getLocaleString(foo) : foo;
```

but then here, you check `_isLocaleKey` again.

It would make the code easier to read if you didn't check `_isLocaleKey` at the callsite, and made this method return the argument (instead of null) if the argument isn't a locale key.

::: browser/components/migration/ChromeMigrationUtils.jsm:94
(Diff revision 14)
> +   * @param {String} profileId - The user profile's ID.
> +   * @retruns {String} The locale string.
> +   */
> +  async getLocaleString(key, locale, extensionId, profileId = "Default") {
> +    let localeString = null;
> +    if (this._isLocaleKey(key) && await this.isExtensionInstalled(extensionId, profileId)) {

Here, too, it seems checking if the add-on is installed is unnecessary.

::: browser/components/migration/ChromeMigrationUtils.jsm:103
(Diff revision 14)
> +        localeFile = JSON.parse(localeFile);
> +        const PREFIX_LENGTH = 6;
> +        const SUFFIX_LENGTH = 2;
> +        // Get the locale key from the string with locale prefix and suffix.
> +        // For example, it will get the "name" sub-string from the "__MSG_name__" string.
> +        key = key.substring(PREFIX_LENGTH, key.length - SUFFIX_LENGTH);

Is there a spec of how these keys work? The hardcoded prefix and suffix lengths seem error-prone to me.

::: browser/components/migration/ChromeMigrationUtils.jsm:140
(Diff revision 14)
> +
> +  /**
> +   * Get the last used user profile's ID.
> +   * @returns {String} The last used user profile's ID.
> +   */
> +  async getLastUsedProfileId() {

This too is, as far as I know, already implemented in ChromeProfileMigrator. Please ensure we aren't duplicating code.

::: browser/components/migration/ChromeMigrationUtils.jsm:177
(Diff revision 14)
> +   * @param {Array} subDirectoriesWin - The sub-directories on Windows.
> +   * @param {Array} subDirectoriesOSX - The sub-directories on masOS.
> +   * @param {Array} subDirectoriesUnix - The sub-directories on Unix/Linux.
> +   * @returns {String} The path of application data directory.
> +   */
> +  getDataPath(subDirectoriesWin, subDirectoriesOSX, subDirectoriesUnix) {

This is copied from the ChromeProfileMigrator.js . Can we make sure only 1 copy of this code exists? 

Also, can we push the `["Google", "Chrome"]` subdirectory stuff into this method instead of having the 3 arguments?

::: browser/components/migration/ChromeMigrationUtils.jsm:194
(Diff revision 14)
> +      subfolders = ["Application Support"].concat(subDirectoriesOSX);
> +    } else {
> +      dirServiceID = "Home";
> +      subfolders = [".config"].concat(subDirectoriesUnix);
> +    }
> +    subfolders.unshift(gDirService.get(dirServiceID, Ci.nsIFile).path);

Instead of using `gDirService`, you can use OS.Constants.Path: https://developer.mozilla.org/en-US/docs/JavaScript_OS.Constants#OS.Constants.Path .

::: browser/components/migration/ChromeMigrationUtils.jsm:215
(Diff revision 14)
> +   * @param {String} path - The path of a directory.
> +   * @param {Boolean} filterDir - Only return directory objects if true,
> +   * otherwise return all file/directory object.
> +   * @returns {Array} The file/directory object array.
> +   */
> +  async _sortFilesByLastModificationDate(path, filterDir = false) {

This is kinda unfortunate. When does this happen, ie when do we need to sort the subdirectories? Do we do this for every add-on?

It also seems this gets called once to find the add-on strings, and then *again* for *every* locale string we look up. That's really IO-intensive, we might `stat` every subdirectory 5 times or something, and all to get exactly the same output.

Please find a way of caching the right subdir to use so that we don't have to do this repeatedly.

Also, is there no better way to tell which is the most recent version? For me, it looks like the directory names are just version numbers. Perhaps we can use the version comparator (`Services.vc`) to compare strings (chopping off the `_0` bit at the end of the directory name)? That would avoid stat'ing all the dirs.

::: browser/components/migration/ChromeMigrationUtils.jsm:220
(Diff revision 14)
> +      if (filterDir && info.isDir) {
> +        entries.push({ entry, lastModificationDate: info.lastModificationDate });
> +      } else if (!filterDir) {

Combine these 2 if conditions, so:

```js
if (!filterDir || info.isDir) {
  ...
}
```

::: browser/components/migration/ChromeMigrationUtils.jsm:229
(Diff revision 14)
> +    entries = entries.map(element => {
> +      return element.entry;
> +    });
> +    return entries;

Just return the result of the .map() immediately.

::: browser/components/migration/tests/unit/test_ChromeMigrationUtils.js:7
(Diff revision 14)
> +
> +var { classes: Cc, interfaces: Ci, results: Cr, utils: Cu } = Components;
> +
> +Cu.import("resource:///modules/ChromeMigrationUtils.jsm");
> +
> +registerFakePath("ULibDir", do_get_file("Library/"));

You should be able to just use the same code as in the other test to ensure we register the right path on all OSes. Alternatively, given that you test the dir stuff elsewhere, you could modify the getter that's fetching the base path from within the test so that the output is always the same root dir, which would also make this work x-platform.

::: browser/components/migration/tests/unit/test_ChromeMigrationUtils_path.js:21
(Diff revision 14)
> +  pathId = "Home";
> +}
> +registerFakePath(pathId, rootDir);
> +
> +add_task(async function test_getDataPath_function() {
> +  let chromeUserDataPath = ChromeMigrationUtils.getDataPath(["test-app"], ["test-app"], ["test-app"]);

When pushing the 'google chrome' bits into the utility function, we can just remove this testcase. We still test the right stuff by virtue of the other 2 tasks here.

::: browser/components/migration/tests/unit/xpcshell.ini:18
(Diff revision 14)
>  [test_Chrome_cookies.js]
>  skip-if = os != "mac" # Relies on ULibDir
>  [test_Chrome_passwords.js]
>  skip-if = os != "win"
> +[test_ChromeMigrationUtils.js]
> +skip-if = os != "mac" # Relies on ULibDir

Why is this new test disabled on non-mac? It looks off-hand like the test should also work on linux and windows.

Comment 30

[Evan Tseng [:evanxd]]

Comment on attachment 8924372 [details]
Bug 1410839 - Add the ChromeMigrationUtils.jsm module and implement the getExtensionList and related functions to export all extensions information installed in a specific profile.

https://reviewboard.mozilla.org/r/195630/#review205786

::: browser/components/migration/ChromeMigrationUtils.jsm:9
(Diff revision 14)
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/AppConstants.jsm");
> +Cu.import("resource://gre/modules/osfile.jsm");

Sure.

::: browser/components/migration/ChromeMigrationUtils.jsm:13
(Diff revision 14)
> +XPCOMUtils.defineLazyServiceGetter(this, "gDirService",
> +                                   "@mozilla.org/file/directory_service;1",
> +                                   "nsIProperties");

OK, thanks.

::: browser/components/migration/ChromeMigrationUtils.jsm:30
(Diff revision 14)
> +      try {
> +        if (entry.isDir) {
> +          let extensionInformation = await this.getExtensionInformation(entry.name, profileId);
> +          extensionInformation && extensionList.push(extensionInformation);
> +        }
> +      } catch (ex) {
> +        Cu.reportError(ex);
> +      }

Thanks, let's remove the try catch.

::: browser/components/migration/ChromeMigrationUtils.jsm:103
(Diff revision 14)
> +        localeFile = JSON.parse(localeFile);
> +        const PREFIX_LENGTH = 6;
> +        const SUFFIX_LENGTH = 2;
> +        // Get the locale key from the string with locale prefix and suffix.
> +        // For example, it will get the "name" sub-string from the "__MSG_name__" string.
> +        key = key.substring(PREFIX_LENGTH, key.length - SUFFIX_LENGTH);

This is the spec[1]. And let's add the URL into the above comment to help people understand the rule.

[1]: https://developer.chrome.com/apps/i18n

::: browser/components/migration/ChromeMigrationUtils.jsm:140
(Diff revision 14)
> +
> +  /**
> +   * Get the last used user profile's ID.
> +   * @returns {String} The last used user profile's ID.
> +   */
> +  async getLastUsedProfileId() {

Let's move the code of getting Local State file from ChromeProfileMigrator into here.

::: browser/components/migration/tests/unit/test_ChromeMigrationUtils.js:7
(Diff revision 14)
> +
> +var { classes: Cc, interfaces: Ci, results: Cr, utils: Cu } = Components;
> +
> +Cu.import("resource:///modules/ChromeMigrationUtils.jsm");
> +
> +registerFakePath("ULibDir", do_get_file("Library/"));

Good point! Let's do it.

::: browser/components/migration/tests/unit/xpcshell.ini:18
(Diff revision 14)
>  [test_Chrome_cookies.js]
>  skip-if = os != "mac" # Relies on ULibDir
>  [test_Chrome_passwords.js]
>  skip-if = os != "win"
> +[test_ChromeMigrationUtils.js]
> +skip-if = os != "mac" # Relies on ULibDir

Yes, let's run the test on all platforms.

Comment 31

[Evan Tseng [:evanxd]]

Comment on attachment 8924372 [details]
Bug 1410839 - Add the ChromeMigrationUtils.jsm module and implement the getExtensionList and related functions to export all extensions information installed in a specific profile.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195630/diff/14-15/

Comment 32

[Evan Tseng [:evanxd]]

Some issues are not fixed yet. Will continue to fix it next week.

Comment 33

[Evan Tseng [:evanxd]]

Comment on attachment 8924372 [details]
Bug 1410839 - Add the ChromeMigrationUtils.jsm module and implement the getExtensionList and related functions to export all extensions information installed in a specific profile.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195630/diff/15-16/

Comment 34

[Evan Tseng [:evanxd]]

Comment on attachment 8924372 [details]
Bug 1410839 - Add the ChromeMigrationUtils.jsm module and implement the getExtensionList and related functions to export all extensions information installed in a specific profile.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195630/diff/16-17/

Comment 35

[Evan Tseng [:evanxd]]

Comment on attachment 8924372 [details]
Bug 1410839 - Add the ChromeMigrationUtils.jsm module and implement the getExtensionList and related functions to export all extensions information installed in a specific profile.

https://reviewboard.mozilla.org/r/195630/#review204860

> This is unfortunate because it will basically run a stat check on the files you're iterating over already, when called from `getExtensionList`.
> 
> The manifest path iterator will just fail if the directory doesn't exist, right? So instead of checking first whether the extension exists, just omit that check, but specifically catch "file not found" errors for `_sortFilesByLastModificatioNDate` and just early-return `null` in that case.

Sure, let's do it.

> This is copied from the ChromeProfileMigrator.js . Can we make sure only 1 copy of this code exists? 
> 
> Also, can we push the `["Google", "Chrome"]` subdirectory stuff into this method instead of having the 3 arguments?

Yes, we can re-use the `getDataPath` method in the `ChromeProfileMigrator.js` and remove the duplicate code.

But it seems like we should not remove the 3 (`subDirectoriesWin`, `subDirectoriesOSX`, `subDirectoriesUnix`) arguments, or how do we deal with the below two situations existed in the `ChromeProfileMigrator.js` since they need thearbuments to get the Chromium's and Chrome Canary's data path.

1. The ChromiumProfileMigrator[1]:
```
getDataFolder(["Chromium"], ["Chromium"], ["chromium"]);
```
2. The CanaryProfileMigrator[2]:
```
getDataFolder(["Google", "Chrome SxS"], ["Google", "Chrome Canary"]);
```

I think we should keep the arguments. Does it make sense? Or I misunderstood somthing here?

[1]: https://searchfox.org/mozilla-central/source/browser/components/migration/ChromeProfileMigrator.js#506
[2]: https://searchfox.org/mozilla-central/source/browser/components/migration/ChromeProfileMigrator.js#522

> Instead of using `gDirService`, you can use OS.Constants.Path: https://developer.mozilla.org/en-US/docs/JavaScript_OS.Constants#OS.Constants.Path .

We currently might not have the ability to instead of using `gDirService` by `OS.Constants.Path` because we could not get the local app data directory path on Windows with `winAppDataDir`(it returns `C:\\Users\\XXX\\AppData\\Roaming` not `C:\\Users\\XXX\\AppData\\Local`). Or do you think use `OS.Path.join(OS.Constants.Path.homeDir, "AppData", "Local")` could be a good idea?

P.S. We could use `macUserLibDir` and `homeDir` to get the app data directory path on macOS and Linux.

> This is kinda unfortunate. When does this happen, ie when do we need to sort the subdirectories? Do we do this for every add-on?
> 
> It also seems this gets called once to find the add-on strings, and then *again* for *every* locale string we look up. That's really IO-intensive, we might `stat` every subdirectory 5 times or something, and all to get exactly the same output.
> 
> Please find a way of caching the right subdir to use so that we don't have to do this repeatedly.
> 
> Also, is there no better way to tell which is the most recent version? For me, it looks like the directory names are just version numbers. Perhaps we can use the version comparator (`Services.vc`) to compare strings (chopping off the `_0` bit at the end of the directory name)? That would avoid stat'ing all the dirs.

Yes, it does for every add-on.

Sure, let's do the caching thing and use the `Services.vc` module to get the last version's directory.

> When pushing the 'google chrome' bits into the utility function, we can just remove this testcase. We still test the right stuff by virtue of the other 2 tasks here.

Since we might still keep the 3 arguments (see the above comment), do we still need to revmoe the testcase?

Comment 36

[Evan Tseng [:evanxd]]

Comment on attachment 8924372 [details]
Bug 1410839 - Add the ChromeMigrationUtils.jsm module and implement the getExtensionList and related functions to export all extensions information installed in a specific profile.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195630/diff/17-18/

Comment 37

[Evan Tseng [:evanxd]]

Comment on attachment 8924372 [details]
Bug 1410839 - Add the ChromeMigrationUtils.jsm module and implement the getExtensionList and related functions to export all extensions information installed in a specific profile.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195630/diff/18-19/

Comment 38

[Evan Tseng [:evanxd]]

Comment on attachment 8924372 [details]
Bug 1410839 - Add the ChromeMigrationUtils.jsm module and implement the getExtensionList and related functions to export all extensions information installed in a specific profile.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195630/diff/19-20/

Comment 39

[Evan Tseng [:evanxd]]

Comment on attachment 8924372 [details]
Bug 1410839 - Add the ChromeMigrationUtils.jsm module and implement the getExtensionList and related functions to export all extensions information installed in a specific profile.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195630/diff/20-21/

Comment 40

[Evan Tseng [:evanxd]]

Comment on attachment 8924372 [details]
Bug 1410839 - Add the ChromeMigrationUtils.jsm module and implement the getExtensionList and related functions to export all extensions information installed in a specific profile.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195630/diff/21-22/

Comment 41

[Evan Tseng [:evanxd]]

Comment on attachment 8924372 [details]
Bug 1410839 - Add the ChromeMigrationUtils.jsm module and implement the getExtensionList and related functions to export all extensions information installed in a specific profile.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195630/diff/22-23/

Comment 42

[Evan Tseng [:evanxd]]

Comment on attachment 8924372 [details]
Bug 1410839 - Add the ChromeMigrationUtils.jsm module and implement the getExtensionList and related functions to export all extensions information installed in a specific profile.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195630/diff/23-24/

Comment 43

[Evan Tseng [:evanxd]]

Comment on attachment 8924372 [details]
Bug 1410839 - Add the ChromeMigrationUtils.jsm module and implement the getExtensionList and related functions to export all extensions information installed in a specific profile.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195630/diff/24-25/

Comment 44

[Evan Tseng [:evanxd]]

Hi Gijs,

I updated the patch for the review comments. And we still have 3 issue to discuss for the patch, please check the reviewboard issues[1] (Comment 35). Could you help review it again? Thank you very much.

(In reply to :Gijs (away until Nov 20) from comment #29)
> Comment on attachment 8924372 [details]
> Bug 1410839 - Add the ChromeMigrationUtils.jsm module and implement the
> getExtensionList and related functions to export all extensions information
> installed in a specific profile.
> 
> https://reviewboard.mozilla.org/r/195630/#review204860
> 
> I'm a bit confused... do we already have a usecase for getting the extension
> names etc.? Because for bug 1398298 we only need to be able to answer "is
> extension with ID <whatever> installed", for which we don't need most of
> this code.
Yes, we have a usecase for getting the extension information, which is the Chrome extension migration feature. It will recommend corresponding Firefox add-on to users and they could have a better Firefox experience. And our product manager is experimenting with the feature. The Taipei engineering team is hoping that I could finish this helper functions (isExtensionInstalled and getExtensionList) in this Q4 and it is also my Q4 goal since the helper function work is the fundamental thing for these related features.

> Perhaps it would make sense to split this patch so that we land the 'is this
> add-on installed' bits first, and can still build something to recommend
> users install corresponding add-ons for any on our 'high profile' list, and
> then we can do the 'get a list of nice strings for each installed add-on'
> bit later.
According to the above thought, I think we could land these code at once (if it also works for you) since I've already finished updated the patch for your previous review and we would like to finish this work in Q4. And it is OK to me if you strongly recommend we should split this patch and it is more efficient to us. 

[1]: https://reviewboard.mozilla.org/r/195630/#issue-summary

Comment 45

[:Gijs (he/him)]

(In reply to Evan Tseng [:evanxd] from comment #35)
> > This is copied from the ChromeProfileMigrator.js . Can we make sure only 1 copy of this code exists? 
> > 
> > Also, can we push the `["Google", "Chrome"]` subdirectory stuff into this method instead of having the 3 arguments?
> 
> Yes, we can re-use the `getDataPath` method in the
> `ChromeProfileMigrator.js` and remove the duplicate code.
> 
> But it seems like we should not remove the 3 (`subDirectoriesWin`,
> `subDirectoriesOSX`, `subDirectoriesUnix`) arguments, or how do we deal with
> the below two situations existed in the `ChromeProfileMigrator.js` since
> they need thearbuments to get the Chromium's and Chrome Canary's data path.
> 
> 1. The ChromiumProfileMigrator[1]:
> ```
> getDataFolder(["Chromium"], ["Chromium"], ["chromium"]);
> ```
> 2. The CanaryProfileMigrator[2]:
> ```
> getDataFolder(["Google", "Chrome SxS"], ["Google", "Chrome Canary"]);
> ```
> 
> I think we should keep the arguments. Does it make sense? Or I misunderstood
> somthing here?

I would prefer that the method take only 1 argument, which is either "Chrome", "Chromium" or "Canary", and that the code inside the method pick the right paths per-OS. That avoids duplicating all the path logic when there are multiple callsites.


> [1]:
> https://searchfox.org/mozilla-central/source/browser/components/migration/
> ChromeProfileMigrator.js#506
> [2]:
> https://searchfox.org/mozilla-central/source/browser/components/migration/
> ChromeProfileMigrator.js#522
> 
> > Instead of using `gDirService`, you can use OS.Constants.Path: https://developer.mozilla.org/en-US/docs/JavaScript_OS.Constants#OS.Constants.Path .
> 
> We currently might not have the ability to instead of using `gDirService` by
> `OS.Constants.Path` because we could not get the local app data directory
> path on Windows with `winAppDataDir`(it returns
> `C:\\Users\\XXX\\AppData\\Roaming` not `C:\\Users\\XXX\\AppData\\Local`). Or
> do you think use `OS.Path.join(OS.Constants.Path.homeDir, "AppData",
> "Local")` could be a good idea?
> 
> P.S. We could use `macUserLibDir` and `homeDir` to get the app data
> directory path on macOS and Linux.

Oh, ugh, sorry. I looked at this - it seems the directory service uses some Windows APIs to get this. However, it also seems like the implementation in the OS.Constants side is very simple. You could probably add `winLocalAppDataDir` and forward it appropriately here: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/osfile/modules/osfile_async_front.jsm#86-103 ?

> > When pushing the 'google chrome' bits into the utility function, we can just remove this testcase. We still test the right stuff by virtue of the other 2 tasks here.
> 
> Since we might still keep the 3 arguments (see the above comment), do we
> still need to revmoe the testcase?

No, we could keep the testcase.

Comment 46

[Evan Tseng [:evanxd]]

Comment on attachment 8924372 [details]
Bug 1410839 - Add the ChromeMigrationUtils.jsm module and implement the getExtensionList and related functions to export all extensions information installed in a specific profile.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195630/diff/25-26/

Comment 47

[Evan Tseng [:evanxd]]

Comment on attachment 8924372 [details]
Bug 1410839 - Add the ChromeMigrationUtils.jsm module and implement the getExtensionList and related functions to export all extensions information installed in a specific profile.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195630/diff/26-27/

Comment 48

[Evan Tseng [:evanxd]]

Hi Gijs,

Thanks for previous review comments.
I've updated the patch for them.

Could you help review it again?
Thank you very much.

Comment 49

[:Gijs (he/him)]

Yep - you don't need to needinfo in addition to the review request. :-)

Comment 50

[:Gijs (he/him)]

Comment on attachment 8924372 [details]
Bug 1410839 - Add the ChromeMigrationUtils.jsm module and implement the getExtensionList and related functions to export all extensions information installed in a specific profile.

https://reviewboard.mozilla.org/r/195630/#review206880

::: browser/components/migration/ChromeMigrationUtils.jsm:28
(Diff revision 27)
> +  /**
> +   * Get all extensions installed in a specific profile.
> +   * @param {String} profileId - A Chrome user profile ID. For example, "Profile 1".
> +   * @returns {Array} All installed Chrome extensions information.
> +   */
> +  async getExtensionList(profileId = "Default") {

Hm, but "Default" isn't necessarily the best profile to use here. Can we instead make the non-argument version use the last selected profile for which we already have a utility function?

::: browser/components/migration/ChromeMigrationUtils.jsm:32
(Diff revision 27)
> +   */
> +  async getExtensionList(profileId = "Default") {
> +    let path = this.getExtensionPath(profileId);
> +    let iterator = new OS.File.DirectoryIterator(path);
> +    let extensionList = [];
> +    await iterator.forEach(async entry => {

This probably needs a catch() (see other comments).

::: browser/components/migration/ChromeMigrationUtils.jsm:35
(Diff revision 27)
> +    let iterator = new OS.File.DirectoryIterator(path);
> +    let extensionList = [];
> +    await iterator.forEach(async entry => {
> +      if (entry.isDir) {
> +        let extensionInformation = await this.getExtensionInformation(entry.name, profileId);
> +        extensionInformation && extensionList.push(extensionInformation);

Just use a normal if statement.

::: browser/components/migration/ChromeMigrationUtils.jsm:42
(Diff revision 27)
> +    });
> +    return extensionList;
> +  },
> +
> +  /**
> +   * Get informatin of a specific Chrome extension.

Nit: information

::: browser/components/migration/ChromeMigrationUtils.jsm:47
(Diff revision 27)
> +   * Get informatin of a specific Chrome extension.
> +   * @param {String} extensionId - The extension ID.
> +   * @param {String} profileId - The user profile's ID.
> +   * @retruns {Object} The Chrome extension information.
> +   */
> +  async getExtensionInformation(extensionId, profileId = "Default") {

Same issue with the profile selection as in `getExtensionList`. You probably want a utility function that gets the default profile, which you can probably just move out of ChromeProfileMigrator?

::: browser/components/migration/ChromeMigrationUtils.jsm:52
(Diff revision 27)
> +      let directories = await this._getSortedByVersionSubDirectoryNames(manifestPath);
> +      // If there are multiple sub-directories in the extension directory,
> +      // read the files in the latest directory.
> +      manifestPath = OS.Path.join(manifestPath, directories[0], "manifest.json");

This should check for null / empty array before using `directories[0]`, and return null if so. Otherwise we'll try to add "undefined" to the path, or something.

::: browser/components/migration/ChromeMigrationUtils.jsm:62
(Diff revision 27)
> +        let name = await this.getLocaleString(manifest.name, DEFAULT_LOCALE, extensionId, profileId);
> +        let description = await this.getLocaleString(manifest.description, DEFAULT_LOCALE, extensionId, profileId);

This should somehow filter out add-ons where we can't determine the name/description because the string fetch failed. We could survive not having a description, but we should probably just not return an extension object that doesn't even have a correct name.

::: browser/components/migration/ChromeMigrationUtils.jsm:84
(Diff revision 27)
> +   * @param {String} locale - The specific language of locale string.
> +   * @param {String} extensionId - The extension ID.
> +   * @param {String} profileId - The user profile's ID.
> +   * @retruns {String} The locale string.
> +   */
> +  async getLocaleString(key, locale, extensionId, profileId = "Default") {

TBH, I would make this private (prefix with `_`) and the `profileId` argument non-optional.

::: browser/components/migration/ChromeMigrationUtils.jsm:89
(Diff revision 27)
> +  async getLocaleString(key, locale, extensionId, profileId = "Default") {
> +    // Return the key string if it is not a locale key.
> +    // The key string starts with "__MSG_" and ends with "__".
> +    // For example, "__MSG_name__".
> +    // https://developer.chrome.com/apps/i18n
> +    if (!(key.startsWith("__MSG_") && key.endsWith("__"))) {

You can use de Morgan's law to change this to: !a || !b, which doesn't require the extra set of braces.

::: browser/components/migration/ChromeMigrationUtils.jsm:93
(Diff revision 27)
> +    // https://developer.chrome.com/apps/i18n
> +    if (!(key.startsWith("__MSG_") && key.endsWith("__"))) {
> +      return key;
> +    }
> +
> +    let localeString = key;

Does defaulting to the `__MSG__` key here really make sense? Because of the try...catch, this basically means you can end up with the extension information containing `{name: "___MSG_name__"}` which seems not useful. We should just omit that property and/or return an error. Maybe we should just return null or empty string?

::: browser/components/migration/ChromeMigrationUtils.jsm:101
(Diff revision 27)
> +      localeFilePath = OS.Path.join(localeFilePath, extensionId);
> +      let directories = await this._getSortedByVersionSubDirectoryNames(localeFilePath);
> +      // If there are multiple sub-directories in the extension directory,
> +      // read the files in the latest directory.
> +      localeFilePath = OS.Path.join(localeFilePath, directories[0], "_locales", locale, "messages.json");
> +      let localeFile = await OS.File.read(localeFilePath, { encoding: "utf-8" });

We're still reading locale files once for each string we're fetching. Can we avoid doing that? Maybe just fetch both strings (if any) at once?

::: browser/components/migration/ChromeMigrationUtils.jsm:234
(Diff revision 27)
> +    if (!await OS.File.exists(path)) {
> +      throw new Error("File does not exist.");
> +    }

You don't need this, the iterator will complain instead.

I keep banging on about this because of two things:
- we're doing extra IO that isn't necessary
- it's a logic error. It's basically never correct to check if a file exists before doing "something" (read/write/delete) with it, because the file can go away inbetween you checking and then doing something with it. In the worst cases, these can become security problems ( https://cwe.mitre.org/data/definitions/367.html ). Instead, catch errors caused by the operation you're actually interested in (reading/writing/deleting the file, iterating over the directory, ...). 

The only good reason to use a "does this file exist" check is if that (ie whether the file exists) is all the information you need - like in our case where we just want to know if the add-on's file exists on disk while we know the add-on ID already. We don't read it or care about its contents, we just want to know if at that point in time, the add-on existed (and we don't even really care if it goes away a split second later).

::: browser/components/migration/ChromeMigrationUtils.jsm:243
(Diff revision 27)
> +      return this._extensionVersionDirectoryNames[path];
> +    }
> +
> +    let iterator = new OS.File.DirectoryIterator(path);
> +    let entries = [];
> +    await iterator.forEach(async entry => {

This promise can be rejected, in which case the async method will reject/throw. I suspect you want a .catch() on the `forEach` that does the right thing (probably `Cu.reportError` and set entries to [] ?).

::: browser/components/migration/ChromeMigrationUtils.jsm:249
(Diff revision 27)
> +      if (entry.isDir) {
> +        // Chop off the "_0" substring from `entry.name`
> +        // since the version string ends with "_0".
> +        // For example, the version string is "1.0_0".
> +        let name = entry.name;
> +        let version = name.substring(0, name.indexOf("_0"));

This won't work if the add-on name includes `_0`. Maybe use `lastIndexOf` instead? Also, do we know what this `_0` bit means? Is the last digit ever not 0?

::: browser/components/migration/ChromeMigrationUtils.jsm:253
(Diff revision 27)
> +    entries.sort((a, b) => {
> +      return Services.vc.compare(b.version, a.version);
> +    });

Nit: I would just put this on one line and omit the braces:

```js
entries.sort((a, b) => Services.vc.compare(b.version, a.version));
```

::: browser/components/migration/ChromeMigrationUtils.jsm:256
(Diff revision 27)
> +    entries = entries.map(entry => {
> +      return entry.name;
> +    });

Same here.

::: browser/components/migration/ChromeProfileMigrator.js:153
(Diff revision 27)
>      if (!this._chromeUserDataFolder)
>        return [];
>  
>      let profiles = [];
>      try {
> -      // Local State is a JSON file that contains profile info.
> +      let info_cache = ChromeMigrationUtils.getLocalState().profile.info_cache;

Can you file a followup bug to make getting a list of profiles async, so we can use non-mainthread IO for this instead of the manual file inputstream stuff? We'd need to change migrator code to use `getSourceProfiles` instead of just `migrator.sourceProfiles`, and make that new method be async.

::: browser/components/migration/tests/unit/test_ChromeMigrationUtils.js:14
(Diff revision 27)
> +  return do_get_file("Library/Application Support/Google/Chrome/").path;
> +};
> +
> +add_task(async function test_getExtensionList_function() {
> +  let extensionList = await ChromeMigrationUtils.getExtensionList("Default");
> +  Assert.equal(extensionList.length, 2, "There should be a extension installed.");

Nit: update message to say there should be 2 extensions.

::: browser/components/migration/tests/unit/test_ChromeMigrationUtils.js:19
(Diff revision 27)
> +  Assert.equal(extensionList.length, 2, "There should be a extension installed.");
> +  Assert.deepEqual(extensionList[0], {
> +    id: "fake-extension-1",
> +    name: "Fake Extension 1",
> +    description: "It is the description of fake extension 1.",
> +  }, "There should be a extension installed.");

Nit: message should be something like "First extension should match expectations"

::: browser/components/migration/tests/unit/test_ChromeMigrationUtils.js:24
(Diff revision 27)
> +  }, "There should be a extension installed.");
> +  Assert.deepEqual(extensionList[1], {
> +    id: "fake-extension-2",
> +    name: "Fake Extension 2",
> +    description: "It is the description of fake extension 2.",
> +  }, "There should be a extension installed.");

Same, but "Second extension should..."

::: toolkit/components/osfile/modules/osfile_async_front.jsm:91
(Diff revision 27)
>  for (let [constProp, dirKey] of [
>    ["localProfileDir", "ProfLD"],
>    ["profileDir", "ProfD"],
>    ["userApplicationDataDir", "UAppData"],
>    ["winAppDataDir", "AppData"],
> +  ["winLocalAppDataDir", "LocalAppData"],

We probably need to update the MDN docs for this ( https://developer.mozilla.org/en-US/docs/JavaScript_OS.Constants#OS.Constants.Path ).

Comment 51

[Evan Tseng [:evanxd]]

Comment on attachment 8924372 [details]
Bug 1410839 - Add the ChromeMigrationUtils.jsm module and implement the getExtensionList and related functions to export all extensions information installed in a specific profile.

https://reviewboard.mozilla.org/r/195630/#review207180

::: browser/components/migration/ChromeMigrationUtils.jsm:32
(Diff revision 27)
> +   */
> +  async getExtensionList(profileId = "Default") {
> +    let path = this.getExtensionPath(profileId);
> +    let iterator = new OS.File.DirectoryIterator(path);
> +    let extensionList = [];
> +    await iterator.forEach(async entry => {

OK.

::: browser/components/migration/ChromeMigrationUtils.jsm:35
(Diff revision 27)
> +    let iterator = new OS.File.DirectoryIterator(path);
> +    let extensionList = [];
> +    await iterator.forEach(async entry => {
> +      if (entry.isDir) {
> +        let extensionInformation = await this.getExtensionInformation(entry.name, profileId);
> +        extensionInformation && extensionList.push(extensionInformation);

Sure.

::: browser/components/migration/ChromeMigrationUtils.jsm:42
(Diff revision 27)
> +    });
> +    return extensionList;
> +  },
> +
> +  /**
> +   * Get informatin of a specific Chrome extension.

Thanks.

::: browser/components/migration/ChromeMigrationUtils.jsm:52
(Diff revision 27)
> +      let directories = await this._getSortedByVersionSubDirectoryNames(manifestPath);
> +      // If there are multiple sub-directories in the extension directory,
> +      // read the files in the latest directory.
> +      manifestPath = OS.Path.join(manifestPath, directories[0], "manifest.json");

Sure.

::: browser/components/migration/ChromeMigrationUtils.jsm:62
(Diff revision 27)
> +        let name = await this.getLocaleString(manifest.name, DEFAULT_LOCALE, extensionId, profileId);
> +        let description = await this.getLocaleString(manifest.description, DEFAULT_LOCALE, extensionId, profileId);

Sure.

::: browser/components/migration/ChromeMigrationUtils.jsm:84
(Diff revision 27)
> +   * @param {String} locale - The specific language of locale string.
> +   * @param {String} extensionId - The extension ID.
> +   * @param {String} profileId - The user profile's ID.
> +   * @retruns {String} The locale string.
> +   */
> +  async getLocaleString(key, locale, extensionId, profileId = "Default") {

Sure, let's do it.

::: browser/components/migration/ChromeMigrationUtils.jsm:89
(Diff revision 27)
> +  async getLocaleString(key, locale, extensionId, profileId = "Default") {
> +    // Return the key string if it is not a locale key.
> +    // The key string starts with "__MSG_" and ends with "__".
> +    // For example, "__MSG_name__".
> +    // https://developer.chrome.com/apps/i18n
> +    if (!(key.startsWith("__MSG_") && key.endsWith("__"))) {

Sure.

::: browser/components/migration/ChromeMigrationUtils.jsm:253
(Diff revision 27)
> +    entries.sort((a, b) => {
> +      return Services.vc.compare(b.version, a.version);
> +    });

Sure.

::: browser/components/migration/ChromeMigrationUtils.jsm:256
(Diff revision 27)
> +    entries = entries.map(entry => {
> +      return entry.name;
> +    });

Sure.

::: browser/components/migration/tests/unit/test_ChromeMigrationUtils.js:14
(Diff revision 27)
> +  return do_get_file("Library/Application Support/Google/Chrome/").path;
> +};
> +
> +add_task(async function test_getExtensionList_function() {
> +  let extensionList = await ChromeMigrationUtils.getExtensionList("Default");
> +  Assert.equal(extensionList.length, 2, "There should be a extension installed.");

Sure.

::: browser/components/migration/tests/unit/test_ChromeMigrationUtils.js:19
(Diff revision 27)
> +  Assert.equal(extensionList.length, 2, "There should be a extension installed.");
> +  Assert.deepEqual(extensionList[0], {
> +    id: "fake-extension-1",
> +    name: "Fake Extension 1",
> +    description: "It is the description of fake extension 1.",
> +  }, "There should be a extension installed.");

Sure.

::: browser/components/migration/tests/unit/test_ChromeMigrationUtils.js:24
(Diff revision 27)
> +  }, "There should be a extension installed.");
> +  Assert.deepEqual(extensionList[1], {
> +    id: "fake-extension-2",
> +    name: "Fake Extension 2",
> +    description: "It is the description of fake extension 2.",
> +  }, "There should be a extension installed.");

Sure.

::: toolkit/components/osfile/modules/osfile_async_front.jsm:91
(Diff revision 27)
>  for (let [constProp, dirKey] of [
>    ["localProfileDir", "ProfLD"],
>    ["profileDir", "ProfD"],
>    ["userApplicationDataDir", "UAppData"],
>    ["winAppDataDir", "AppData"],
> +  ["winLocalAppDataDir", "LocalAppData"],

Good point. Let me update the doc after we land the patch.

Comment 52

[Evan Tseng [:evanxd]]

Comment on attachment 8924372 [details]
Bug 1410839 - Add the ChromeMigrationUtils.jsm module and implement the getExtensionList and related functions to export all extensions information installed in a specific profile.

https://reviewboard.mozilla.org/r/195630/#review207242

::: browser/components/migration/ChromeMigrationUtils.jsm:28
(Diff revision 27)
> +  /**
> +   * Get all extensions installed in a specific profile.
> +   * @param {String} profileId - A Chrome user profile ID. For example, "Profile 1".
> +   * @returns {Array} All installed Chrome extensions information.
> +   */
> +  async getExtensionList(profileId = "Default") {

Sure, let's resue the `getLastUsedProfileId` function here.

::: browser/components/migration/ChromeMigrationUtils.jsm:47
(Diff revision 27)
> +   * Get informatin of a specific Chrome extension.
> +   * @param {String} extensionId - The extension ID.
> +   * @param {String} profileId - The user profile's ID.
> +   * @retruns {Object} The Chrome extension information.
> +   */
> +  async getExtensionInformation(extensionId, profileId = "Default") {

Sure, let's resue the `getLastUsedProfileId` function here.

::: browser/components/migration/ChromeMigrationUtils.jsm:101
(Diff revision 27)
> +      localeFilePath = OS.Path.join(localeFilePath, extensionId);
> +      let directories = await this._getSortedByVersionSubDirectoryNames(localeFilePath);
> +      // If there are multiple sub-directories in the extension directory,
> +      // read the files in the latest directory.
> +      localeFilePath = OS.Path.join(localeFilePath, directories[0], "_locales", locale, "messages.json");
> +      let localeFile = await OS.File.read(localeFilePath, { encoding: "utf-8" });

Miss this. Let's cache the locale files.

::: browser/components/migration/ChromeMigrationUtils.jsm:234
(Diff revision 27)
> +    if (!await OS.File.exists(path)) {
> +      throw new Error("File does not exist.");
> +    }

Sure, let's remove the `OS.File.exists` function call.

::: browser/components/migration/ChromeMigrationUtils.jsm:243
(Diff revision 27)
> +      return this._extensionVersionDirectoryNames[path];
> +    }
> +
> +    let iterator = new OS.File.DirectoryIterator(path);
> +    let entries = [];
> +    await iterator.forEach(async entry => {

Sure, let's do it.

::: browser/components/migration/ChromeMigrationUtils.jsm:249
(Diff revision 27)
> +      if (entry.isDir) {
> +        // Chop off the "_0" substring from `entry.name`
> +        // since the version string ends with "_0".
> +        // For example, the version string is "1.0_0".
> +        let name = entry.name;
> +        let version = name.substring(0, name.indexOf("_0"));

After looked at the unit test[1] for chrome extension files, the "1.0_1" directory name is possibly existed if you install the version 1.0 again without removing the "1.0_0" directory.

I think the simply way to fix this is using `Services.vc.compare` to compare the version number strings directly, for example, `Services.vc.compare("1.0_1", "1.0_0")` will return `1`. The `Services.vc.compare` can deal with a version number with the `_` char.

What do you think?

[1]: https://chromium.googlesource.com/chromium/src/+/0b58a813992b539a6b555ad7959adfad744b095a/chrome/common/extensions/extension_file_util_unittest.cc

Comment 53

[Evan Tseng [:evanxd]]

Comment on attachment 8924372 [details]
Bug 1410839 - Add the ChromeMigrationUtils.jsm module and implement the getExtensionList and related functions to export all extensions information installed in a specific profile.

https://reviewboard.mozilla.org/r/195630/#review206880

> Does defaulting to the `__MSG__` key here really make sense? Because of the try...catch, this basically means you can end up with the extension information containing `{name: "___MSG_name__"}` which seems not useful. We should just omit that property and/or return an error. Maybe we should just return null or empty string?

Good point. Let's just return a `null` here.

Comment 54

[Evan Tseng [:evanxd]]

Comment on attachment 8924372 [details]
Bug 1410839 - Add the ChromeMigrationUtils.jsm module and implement the getExtensionList and related functions to export all extensions information installed in a specific profile.

https://reviewboard.mozilla.org/r/195630/#review206880

> Can you file a followup bug to make getting a list of profiles async, so we can use non-mainthread IO for this instead of the manual file inputstream stuff? We'd need to change migrator code to use `getSourceProfiles` instead of just `migrator.sourceProfiles`, and make that new method be async.

Sure, the bug is filed. https://bugzilla.mozilla.org/show_bug.cgi?id=1419700

Comment 55

[Evan Tseng [:evanxd]]

Comment on attachment 8924372 [details]
Bug 1410839 - Add the ChromeMigrationUtils.jsm module and implement the getExtensionList and related functions to export all extensions information installed in a specific profile.

https://reviewboard.mozilla.org/r/195630/#review206880

> This won't work if the add-on name includes `_0`. Maybe use `lastIndexOf` instead? Also, do we know what this `_0` bit means? Is the last digit ever not 0?

After looked at the unit test1 for chrome extension files, the "1.0_1" directory name is possibly existed if you install the version 1.0 again without removing the "1.0_0" directory.

I think the simply way to fix this is using Services.vc.compare to compare the version number strings directly, for example, Services.vc.compare("1.0_1", "1.0_0") will return 1. The Services.vc.compare can deal with a version number with the _ char.

What do you think?

Comment 56

[Evan Tseng [:evanxd]]

Comment on attachment 8924372 [details]
Bug 1410839 - Add the ChromeMigrationUtils.jsm module and implement the getExtensionList and related functions to export all extensions information installed in a specific profile.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195630/diff/27-28/

Comment 57

[Evan Tseng [:evanxd]]

Hi Gijs,

Thank very much for the previous review.
I've update the patch for them and filed the follow up bug.
Could you review it again?

And we still have one issue to discuss, what do you think of Comment55? Do you think it is a good way to deal with the version name directories, or you have any better idea? 

Thank you again.

Comment 58

[Evan Tseng [:evanxd]]

Comment on attachment 8924372 [details]
Bug 1410839 - Add the ChromeMigrationUtils.jsm module and implement the getExtensionList and related functions to export all extensions information installed in a specific profile.

https://reviewboard.mozilla.org/r/195630/#review206880

> We probably need to update the MDN docs for this ( https://developer.mozilla.org/en-US/docs/JavaScript_OS.Constants#OS.Constants.Path ).

Yes, let's update the doc after land the patch.

Comment 59

[:Gijs (he/him)]

(In reply to Evan Tseng [:evanxd] from comment #55)
> I think the simply way to fix this is using Services.vc.compare to compare
> the version number strings directly, for example,
> Services.vc.compare("1.0_1", "1.0_0") will return 1. The Services.vc.compare
> can deal with a version number with the _ char.
> 
> What do you think?

Oh, OK. If the version comparator service deals with it, I think that's OK. :-)

Comment 60

[:Gijs (he/him)]

Comment on attachment 8924372 [details]
Bug 1410839 - Add the ChromeMigrationUtils.jsm module and implement the getExtensionList and related functions to export all extensions information installed in a specific profile.

https://reviewboard.mozilla.org/r/195630/#review207342

Almost done. I'm not sure what to do about add-ons without names... maybe just a `Cu.reportError()` is good enough? What do you think?

::: browser/components/migration/ChromeMigrationUtils.jsm:41
(Diff revisions 27 - 28)
>     * Get all extensions installed in a specific profile.
>     * @param {String} profileId - A Chrome user profile ID. For example, "Profile 1".
>     * @returns {Array} All installed Chrome extensions information.
>     */
> -  async getExtensionList(profileId = "Default") {
> +  async getExtensionList(profileId) {
> +    profileId = profileId ? profileId : this.getLastUsedProfileId();

Here and below, you can use function invocations in the default argument notation in the method signature (they will only be run if the argument is not provided), so this is valid:

```js
async getExtensionList(profileId = this.getLastUsedProfileId())
```

::: browser/components/migration/ChromeMigrationUtils.jsm:45
(Diff revisions 27 - 28)
> +    try {
> -    await iterator.forEach(async entry => {
> +      await iterator.forEach(async entry => {
> -      if (entry.isDir) {
> +        if (entry.isDir) {
> -        let extensionInformation = await this.getExtensionInformation(entry.name, profileId);
> +          let extensionInformation = await this.getExtensionInformation(entry.name, profileId);
> -        extensionInformation && extensionList.push(extensionInformation);
> +          if (extensionInformation) {
> +            extensionList.push(extensionInformation);
> +          }
> -      }
> +        }
> -    });
> +      });
> +    } catch (ex) {
> +      Cu.reportError(ex);
> +    }

The iterator returns a promise, which will reject if something breaks. So you don't need a try...catch statement. Instead, do something like this:

```js
await iterator.forEach(async entry => {
  ...
}).catch(e => Cu.reportError(e));
```

::: browser/components/migration/ChromeMigrationUtils.jsm:87
(Diff revisions 27 - 28)
>        // No app attribute means this is a Chrome extension not a Chrome app.
>        if (!manifest.app) {
>          const DEFAULT_LOCALE = manifest.default_locale;
> -        let name = await this.getLocaleString(manifest.name, DEFAULT_LOCALE, extensionId, profileId);
> -        let description = await this.getLocaleString(manifest.description, DEFAULT_LOCALE, extensionId, profileId);
> +        let name = await this._getLocaleString(manifest.name, DEFAULT_LOCALE, extensionId, profileId);
> +        let description = await this._getLocaleString(manifest.description, DEFAULT_LOCALE, extensionId, profileId);
> +        if (name) {

Should we log an error (or even return an error?) for add-ons whose names we can't read?

::: browser/components/migration/ChromeMigrationUtils.jsm:276
(Diff revisions 27 - 28)
>        return this._extensionVersionDirectoryNames[path];
>      }
>  
>      let iterator = new OS.File.DirectoryIterator(path);
>      let entries = [];
> +    try {

Here too you can use promise.catch() instead of try...catch.

::: browser/components/migration/ChromeMigrationUtils.jsm:279
(Diff revisions 27 - 28)
> -        let name = entry.name;
> +          let name = entry.name;
> -        let version = name.substring(0, name.indexOf("_0"));
> +          entries.push(name);

Nit: just make this 1 line. :-)

Comment 61

[Evan Tseng [:evanxd]]

Comment on attachment 8924372 [details]
Bug 1410839 - Add the ChromeMigrationUtils.jsm module and implement the getExtensionList and related functions to export all extensions information installed in a specific profile.

https://reviewboard.mozilla.org/r/195630/#review207342

> Here and below, you can use function invocations in the default argument notation in the method signature (they will only be run if the argument is not provided), so this is valid:
> 
> ```js
> async getExtensionList(profileId = this.getLastUsedProfileId())
> ```

Thanks, learned.

> The iterator returns a promise, which will reject if something breaks. So you don't need a try...catch statement. Instead, do something like this:
> 
> ```js
> await iterator.forEach(async entry => {
>   ...
> }).catch(e => Cu.reportError(e));
> ```

Thanks, learned.

> Should we log an error (or even return an error?) for add-ons whose names we can't read?

Let's log the error.

> Here too you can use promise.catch() instead of try...catch.

Sure, let's do it.

> Nit: just make this 1 line. :-)

Yes, let's do it.

Comment 62

[Evan Tseng [:evanxd]]

Comment on attachment 8924372 [details]
Bug 1410839 - Add the ChromeMigrationUtils.jsm module and implement the getExtensionList and related functions to export all extensions information installed in a specific profile.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195630/diff/28-29/

Comment 63

[Evan Tseng [:evanxd]]

Hi Gijs,

I've updated the patch for the review comments.
Could you help review it again?

Thank you very much.
Learned a lot. :-)

Comment 64

[Evan Tseng [:evanxd]]

Needinfo myself, update the MDN docs for the new `winLocalAppDataDir` property in the OS.Constants.Path object (https://developer.mozilla.org/en-US/docs/JavaScript_OS.Constants#OS.Constants.Path) after landing the patch.

Comment 65

[:Gijs (he/him)]

Comment on attachment 8924372 [details]
Bug 1410839 - Add the ChromeMigrationUtils.jsm module and implement the getExtensionList and related functions to export all extensions information installed in a specific profile.

https://reviewboard.mozilla.org/r/195630/#review207788

This looks OK to me. Thanks!

Comment 66

[Evan Tseng [:evanxd]]

Hi Gijs,

Thank you very much for the review.

And the treeherder job[1] is good. Let's land the patch.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=94a9de1e1e88

Comment 67

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/be627b364b31
Add the ChromeMigrationUtils.jsm module and implement the getExtensionList and related functions to export all extensions information installed in a specific profile. r=Gijs

Comment 68

[Andrei Ciure[:aciure]]

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

Comment 69

[Evan Tseng [:evanxd]]

The new `winLocalAppDataDir` property of `OS.Constants.Path` object is added and updated on the mdn page[1].

[1]: https://developer.mozilla.org/en-US/docs/JavaScript_OS.Constants