All of the consumers here are async, so we should be able to convert them to the new async connection.

or use Sqlite.jsm

The only remaining call is here: http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/browser/components/migration/tests/unit/test_Chrome_passwords.js#150 it could be converted to Sqlite.jsm

Hi. Newbie here. Can I work on this?

(In reply to George Veneel Dogga from comment #3) > Hi. Newbie here. > Can I work on this? Sure you can. comment 2 is still valid, that test should be refactored to use Sqlite.jsm API instead of the existing calls to mozIStorageConnection. You can see an example of that at: http://searchfox.org/mozilla-central/rev/790b2cb423ea1ecb5746722d51633caec9bab95a/browser/components/migration/MigrationUtils.jsm#602

Hey mak, I was on linux so the test was skipped. Please let me know about the errors. :)

Comment on attachment 8827889 [details] [diff] [review] V1.patch Feedback and review should only be given by reviewers

Comment on attachment 8827889 [details] [diff] [review] V1.patch Review of attachment 8827889 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/migration/tests/unit/test_Chrome_passwords.js @@ +81,3 @@ > > function promiseSetPassword(login) { > return new Promise((resolve, reject) => { The new execute method already returns a promise, so there should be no need to further wrap its call into a new Promise constructor, you should just return dbConn.execute.... @@ +85,3 @@ > let passwordValue = crypto.stringToArray(crypto.encryptData(login.password)); > + let params={password_value: passwordValue, rowid: login.id }; > + dbConn.execute(stmt,params); This not mandatory clearly, just a coding style thing. I'd prefer to have the arguments inline, something like return dbConn.execute(`UPDATE... SET... WHERE `, { param1:..., param2:... }); The backticks ` is a special char that indicates a template literal (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals) that allows to go multi-line. @@ +85,4 @@ > let passwordValue = crypto.stringToArray(crypto.encryptData(login.password)); > + let params={password_value: passwordValue, rowid: login.id }; > + dbConn.execute(stmt,params); > + dbConn.close(); why are you closing the connection here? the old code was not doing that, it was only finalizing the statement. Sqlite.jsm finalizes the statement for you already. @@ +129,5 @@ > > add_task(function* setup() { > let loginDataFile = do_get_file("AppData/Local/Google/Chrome/User Data/Default/Login Data"); > + let dbConn = yield Sqlite.openConnection({path: "loginDataFile"}); > + //dbConn = Services.storage.openUnsharedDatabase(loginDataFile); the path should be a real path to the file, not just a string. loginDataFile is an instance of nsIFile: http://searchfox.org/mozilla-central/source/xpcom/io/nsIFile.idl it has a path attribute, so it should be { path: loginDataFile.path } (clearly you should also remove the commented out code) @@ +134,5 @@ > registerFakePath("LocalAppData", do_get_file("AppData/Local/")); > > do_register_cleanup(() => { > Services.logins.removeAllLogins(); > + dbConn.close(); this is asynchronous and returns a promise, thus you should also change do_register_cleanup(() => { to do_register_cleanup(function* () { and yield dbConn.close();

Before I forget, please add a commit message, the reviewer and yourself as patch author. See https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

I hope I've added all the changes as you said. Please let me know about the errors :)

Comment on attachment 8828122 [details] [diff] [review] V1.1 patch Review of attachment 8828122 [details] [diff] [review]: ----------------------------------------------------------------- The patch is still missing a commit message, the reviewer must be at the end of the commit message. The style is usually: Bug 887876 - Use Sqlite.jsm in browser/components/migration. r=mak ::: browser/components/migration/tests/unit/test_Chrome_passwords.js @@ +83,5 @@ > > + let stmt = "UPDATE logins SET password_value = :password_value WHERE rowid = :rowid"; > + let passwordValue = crypto.stringToArray(crypto.encryptData(login.password)); > + let params={password_value: passwordValue, rowid: login.id }; > + return dbConn.execute(stmt,params); you didn't address my cosing style comment. It may also be ok like this, but please always add a whitespace after commas and around =, we have eslint support for that and coding style tests would fail. @@ +132,3 @@ > registerFakePath("LocalAppData", do_get_file("AppData/Local/")); > > + do_register_cleanup(function* finalize_cleanup() { Sorry, I just checked and do_register_cleanup doesn't support generators :( It supports promises though, so here you should do_register_cleanup(() => { Services.logins.removeAllLogins(); crypto.finalize(); return dbConn.close(); });

Comment on attachment 8828349 [details] [diff] [review] Proposed patch Review of attachment 8828349 [details] [diff] [review]: ----------------------------------------------------------------- it looks good, pushing to Try now.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=08bff13589b0f475368f76daee27250a1408b555

The ES test noticed a couple problems: TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/browser/components/migration/tests/unit/test_Chrome_passwords.js:133:7 | 'dbConn' is already declared in the upper scope. (no-shadow) indeed the let dbConn is shadowing the var dbConn at the top, please just remove "let". TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/browser/components/migration/tests/unit/test_Chrome_passwords.js:133:22 | 'Sqlite' is not defined. (no-undef) Ehr right, you must import the Sqlite.jsm module, but I'd suggest to do that in the header file: http://searchfox.org/mozilla-central/source/browser/components/migration/tests/unit/head_migration.js Here just add a: XPCOMUtils.defineLazyModuleGetter(this, "Sqlite", "resource://gre/modules/Sqlite.jsm"); close to the other imports at the top. This way all the tests in this folder will be able to use the Sqlite object without having to import it (the head file is loaded before each xpcshell test). To understand the X failure, just click on it, then click on the [LOG] icon, then you should be able to see this: 10:58:05 INFO - PROCESS | 5908 | A coding exception was thrown and uncaught in a Task. 10:58:05 INFO - PROCESS | 5908 | Full message: ReferenceError: Sqlite is not defined ... That is what we are already fixing.

Comment on attachment 8828773 [details] [diff] [review] V2.0.patch Review of attachment 8828773 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Let's try again :)

I have confirmed that the test passes once Bug 1247602 is fixed, I'll push that patch today.

tested locally.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/6328f5d01711 Use Sqlite.jsm in browser/components/migration. r=mak