Closed Bug 942288 Opened 6 years ago Closed 6 years ago

Allow panel add-on to push data to content provider from JS

Categories

(Firefox for Android :: Data Providers, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: Margaret, Assigned: Margaret)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 3 obsolete files)

This may need to be broken up into separate bugs, but there are a few questions to answer here:
* How often should we let the add-on update its data? Should we just send it some "hey, update yourself" notification?
* What will the API look like for this? I don't think we want to give the add-on direct access to the list content provider, since it should only be able to update its own data.
* How will we do this from JS? Provider a generic JS content provider wrapper? Make our own helper functions that write directly to the DB?
Now that bug 941318 and bug 862805 landed, we should start working on this.
Whiteboard: shovel-ready
I'll start working on this, starting with thinking about what the API should look like.
Assignee: nobody → margaret.leibovic
Summary: Allow list add-on to push data to list content provider from JS → Allow panel add-on to push data to content provider from JS
Lots of discussion about this happening here: https://mobile.etherpad.mozilla.org/panel-storage-sync
Attached patch WIP - HomeProvider.jsm (obsolete) — Splinter Review
This is a first stab at an implementation of the API that we discussed earlier today. Instead of using callbacks, I used promises, since that's what the Sqlite.jsm API uses, and honestly, it's nicer :)

For now the schema is simple - it just supports a URL. But that's a detail we can sort out independently of getting the basics in place here :)

For reference:
https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Sqlite.jsm
http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/Sqlite.jsm
http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/tests/xpcshell/test_sqlite.js
Attachment #8361458 - Flags: feedback?(wjohnston)
Attachment #8361458 - Flags: feedback?(lucasr.at.mozilla)
Tests! The check in test_delete is failing, so that needs some investigation, but everything else is passing. We can just work on improving this in parallel with improving HomeProvider.jsm itself.
Attachment #8361459 - Flags: feedback?(wjohnston)
Attachment #8361459 - Flags: feedback?(lucasr.at.mozilla)
Also note: we need to rename/repurpose HomeListsProvider on the Java side, but let's note worry about that for now :)
Comment on attachment 8361458 [details] [diff] [review]
WIP - HomeProvider.jsm

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

Looks good as a starting point. Doesn't gecko give us any tools/APIs to handle schema versioning, migration, etc? Will we have to maintain all this manually with an ad-hoc framework? It's not the end of world but it would be nice if could lean on some existing framework in gecko for these things.

::: mobile/android/modules/HomeProvider.jsm
@@ +60,5 @@
> +        for (let item of data) {
> +          // XXX: Factor out SQL statement, batch insert?
> +          let params = {
> +            dataset_id: this.datasetId,
> +            url: item.url

It would be nice if we could express the schema in a more human-readable format instead of having to figure it out by reading the queries e.g. an object with the list of properties or something. This same representation could then be used to create the DB and perform queries. Maybe I'm thinking of something like a simpler more JS-ey version of our BrowserContract in java.
Attachment #8361458 - Flags: feedback?(lucasr.at.mozilla) → feedback+
Comment on attachment 8361459 [details] [diff] [review]
WIP - Tests for HomeProvider.jsm!

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

Nice :-)
Attachment #8361459 - Flags: feedback?(lucasr.at.mozilla) → feedback+
(In reply to Lucas Rocha (:lucasr) from comment #7)

> Looks good as a starting point. Doesn't gecko give us any tools/APIs to
> handle schema versioning, migration, etc? Will we have to maintain all this
> manually with an ad-hoc framework? It's not the end of world but it would be
> nice if could lean on some existing framework in gecko for these things.

Yeah, it would be nice if there was an onUpgrade() type of thing we could implement like in our Java code, but it looks like Sqlite.jsm only gives us minimal helper methods. You can get/set a schema version however, so I don't think it would be too complicated to make a helper function that just checks to make sure the schema matches whatever version we set as a constant, and if not, follow the appropriate upgrade path.

> ::: mobile/android/modules/HomeProvider.jsm
> @@ +60,5 @@
> > +        for (let item of data) {
> > +          // XXX: Factor out SQL statement, batch insert?
> > +          let params = {
> > +            dataset_id: this.datasetId,
> > +            url: item.url
> 
> It would be nice if we could express the schema in a more human-readable
> format instead of having to figure it out by reading the queries e.g. an
> object with the list of properties or something. This same representation
> could then be used to create the DB and perform queries. Maybe I'm thinking
> of something like a simpler more JS-ey version of our BrowserContract in
> java.

Yeah, this sounds good. The only thing I'd want to avoid is giant concatenated queries that become hard to read for a different reason. Fortunately, I don't think we'll have giant crazy queries like we do in BrowserProvider :)
Blocks: 961092
Realized I just made a SQL mistake in test_delete. Fixing that made it pass for me locally, so I pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=29140117e1d2
(In reply to :Margaret Leibovic from comment #10)
> Realized I just made a SQL mistake in test_delete. Fixing that made it pass
> for me locally, so I pushed to try:
> https://tbpl.mozilla.org/?tree=Try&rev=29140117e1d2

Made another mistake in that try push... this one should work:
https://tbpl.mozilla.org/?tree=Try&rev=3a594a28f640

I also updated my patch for bug 949039 to disable testHomeListsProvider, since that also caused problems in that try push.
Comment on attachment 8361458 [details] [diff] [review]
WIP - HomeProvider.jsm

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

::: mobile/android/modules/HomeProvider.jsm
@@ +45,5 @@
> +   * @resolves When the operation has completed.
> +   */
> +  save: function(data) {
> +    return Task.spawn(function save_task() {
> +      let path = OS.Path.join(OS.Constants.Path.profileDir, "home.sqlite");

You should move this filename to a const in here somewhere.

@@ +52,5 @@
> +      try {
> +        // XXX: Factor this out to some migration path.
> +        if (!(yield db.tableExists("items"))) {
> +          // XXX: Factor out SQL statement, add more columns for full schema.
> +          yield db.execute("CREATE TABLE items (id INTEGER PRIMARY KEY AUTOINCREMENT, dataset_id TEXT, url TEXT)");

I think you might as well start out with what we talked about in the meeting this week. id, primary_text, secondary_text, url, image? As you said on IRC, pulling these statements out into some constants would be nice :)

@@ +62,5 @@
> +          let params = {
> +            dataset_id: this.datasetId,
> +            url: item.url
> +          };
> +          yield db.execute("INSERT INTO items (dataset_id, url) VALUES (:dataset_id, :url)", params);

Yeah, executeCached would be our friend here, since this should happen a bunch. Might be good to wrap in a transaction: http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/tests/xpcshell/test_sqlite.js#336

@@ +83,5 @@
> +      let db = yield Sqlite.openConnection({ path: path });
> +
> +      try {
> +        let params = { dataset_id: this.datasetId };
> +        yield db.execute("DELETE FROM items WHERE dataset_id = :dataset_id", params);

Same with executeCached here.

@@ +87,5 @@
> +        yield db.execute("DELETE FROM items WHERE dataset_id = :dataset_id", params);
> +      } finally {
> +        yield db.close();
> +      }
> +    }.bind(this));

i hate bind now that we have big arrows :)
Attachment #8361458 - Flags: feedback?(wjohnston) → feedback+
Attached patch HomeProvider.jsm (obsolete) — Splinter Review
This doesn't address wesj's most recent comments, but I think this is a good enough start to land, in case anyone feels in a rush to land this and address the follow-ups in a separate bug.
Attachment #8361458 - Attachment is obsolete: true
Attachment #8362004 - Flags: review?(wjohnston)
Here's an updated test that passes on try.

I decided to put both the save and the delete bits in the same task, so that we avoid any potential races.
Attachment #8361459 - Attachment is obsolete: true
Attachment #8361459 - Flags: feedback?(wjohnston)
Attachment #8362007 - Flags: review?(michael.l.comella)
Attachment #8362004 - Flags: review?(wjohnston) → review+
Here's an updated version that just addresses some of the feedback here, rather than in a follow-up.

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=2d6ea654833f

(I also think I fixed the issue with testHomeListsProvider from bug 949039, so I don't think we need to disable that anymore.)
Attachment #8362004 - Attachment is obsolete: true
Attachment #8364051 - Flags: review?(wjohnston)
Comment on attachment 8362007 [details] [diff] [review]
Test for HomeProvider.jsm

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

lgtm with the disclaimer that I looked up promises, generators, yield, and Task.jsm for the first time specifically for this review. I think I understand it well enough to say my word is good enough, though you can assign another reviewer if you feel it's necessary.

r+, though I would like it if the test was more thorough, as per my comments below. This can be a followup, however, so we can continue to progress.

::: mobile/android/base/tests/testHomeProvider.java
@@ +1,3 @@
> +package org.mozilla.gecko.tests;
> +
> +import org.mozilla.gecko.*;

nit: Unneeded import.

::: mobile/android/base/tests/testHomeProvider.js
@@ +29,5 @@
> +  // Make sure the correct values for the item ended up in there.
> +  let result = yield db.execute("SELECT * FROM items", null, function onRow(row){
> +    do_check_eq(row.getResultByName("dataset_id"), TEST_DATASET_ID);
> +    do_check_eq(row.getResultByName("url"), TEST_URL);
> +  });

If we're expecting the db of TEST_DATASET_ID to be empty to start, we should probably verify the data is saved once, i.e. `do_check_eq(result.length, 1);`. Otherwise we don't need to store result above.

@@ +32,5 @@
> +    do_check_eq(row.getResultByName("url"), TEST_URL);
> +  });
> +
> +  // Use the HomeProvider API to delete the data.
> +  yield storage.deleteAll();

To be thorough, before deleting, I would also:
* Insert multiple items (I usually stop at 3 total)
* Insert one item with a different value for the 'url' property and ensure the correct value is inserted.
* Insert one item with a different attribute than 'url' and ensure the correct value is inserted (if applicable - I noticed this attribute is hard-coded and the only value).
* On each insertion, verify the length of the result set, as per the comment above.

This will also better test deleteAll, to ensure it's deleting multiple values.

More importantly, you may also want to:
* Call deleteAll() with an empty database.
* Call save with a "malicious" arg (undefined, null, object w/ null values, object without null column, etc.) - you can probably store these in an array and loop over them in an insertion to ensure nothing stupid is inserted (or an exception is thrown, if applicable)

Perhaps this is stupidly over-thorough but to consider:
* Run deleteAll multiple times and ensure it correctly deletes everything each time
Attachment #8362007 - Flags: review?(michael.l.comella) → review+
Blocks: 952310
Blocks: 963352
Comment on attachment 8364051 [details] [diff] [review]
HomeProvider.jsm (v2)

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

::: mobile/android/modules/HomeProvider.jsm
@@ +83,5 @@
> +          let params = {
> +            dataset_id: this.datasetId,
> +            url: item.url
> +          };
> +          yield db.executeCached(SQL.insertItem, params);

I really think you should wrap this loop in a transaction since our API basically begs you to send a whole bunch of these at once. Follow up I guess...
Attachment #8364051 - Flags: review?(wjohnston) → review+
Depends on: 963817
https://hg.mozilla.org/mozilla-central/rev/7dcb7216e880
https://hg.mozilla.org/mozilla-central/rev/53c80ea7f4c9
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
You need to log in before you can comment on or make changes to this bug.