Last Comment Bug 663571 - OS agnostic webapps backend
: OS agnostic webapps backend
Status: RESOLVED FIXED
[qa-]
:
Product: Fennec Graveyard
Classification: Graveyard
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 8
Assigned To: Mark Finkle (:mfinkle) (use needinfo?)
:
Mentors:
Depends on:
Blocks: 583750
  Show dependency treegraph
 
Reported: 2011-06-10 17:08 PDT by [:fabrice] Fabrice Desré
Modified: 2011-10-13 10:19 PDT (History)
7 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (7.31 KB, patch)
2011-06-10 17:08 PDT, [:fabrice] Fabrice Desré
mark.finkle: review+
mbrubeck: review-
Details | Diff | Splinter Review
patch 2 (7.18 KB, patch)
2011-06-17 14:02 PDT, Mark Finkle (:mfinkle) (use needinfo?)
mbrubeck: review+
Details | Diff | Splinter Review

Description [:fabrice] Fabrice Desré 2011-06-10 17:08:37 PDT
Created attachment 538644 [details] [diff] [review]
patch

Uses a sqlite database to store webapps uri and icons.
Comment 1 Mark Finkle (:mfinkle) (use needinfo?) 2011-06-16 21:45:12 PDT
Comment on attachment 538644 [details] [diff] [review]
patch

>diff --git a/mobile/components/Makefile.in b/mobile/components/Makefile.in

>+DIRS += webapps \
>+        $(NULL)

No need for a sub folder. Just leave the component in the components folder

>diff --git a/mobile/components/webapps/Makefile.in b/mobile/components/webapps/Makefile.in

Remove

>diff --git a/mobile/components/webapps/WebappsComponents.manifest b/mobile/components/webapps/WebappsComponents.manifest

>+# webapps
>+component {cb1107c1-1e15-4f11-99c8-27b9ec221a2a} WebappsGeneric.js
>+contract @mozilla.org/webapps/installer;1 {cb1107c1-1e15-4f11-99c8-27b9ec221a2a}

Put this in the current manifest
I am tempted to use "@mozilla.org/webapps/support;1" since "installer" seems so limited and not quite true.

>diff --git a/mobile/components/webapps/WebappsGeneric.js b/mobile/components/webapps/WebappsGeneric.js

Rename to WebappsSupport.js

>+ * The Initial Developer of the Original Code is Mozilla Foundation.
>+ * Portions created by the Initial Developer are Copyright (C) 2008

2011

>+function GenericInstaller() {

WebappsSupport

>+  init: function() {

>+    if (mustCreate) {
>+      dump("XxXxX GenericInstaller creating webapps db\n");
>+      this.db.executeSimpleSQL("CREATE TABLE webapps (title TEXT, uri TEXT PRIMARY KEY, icon TEXT)");
>+    }

Remove the dump and the {}

>diff --git a/mobile/installer/package-manifest.in b/mobile/installer/package-manifest.in

>+@BINPATH@/components/WebappsComponents.manifest
>+@BINPATH@/components/WebappsGeneric.js

No need for the manifest since we are consolidating into MobileComponents.manifest
Comment 2 Mark Finkle (:mfinkle) (use needinfo?) 2011-06-16 21:46:28 PDT
Comment on attachment 538644 [details] [diff] [review]
patch

Second review. We want to keep this simple and add to it later, as needed.
Comment 3 Matt Brubeck (:mbrubeck) 2011-06-17 11:14:35 PDT
Comment on attachment 538644 [details] [diff] [review]
patch

>+    this.db = Cc["@mozilla.org/storage/service;1"].getService(Ci.mozIStorageService).openDatabase(file);

The docs for openDatabase say, "Consumers should check mozIStorageConnection::connectionReady to ensure that they can use the database"

Should we also close() the connection on shutdown?

>+  isApplicationInstalled: function(aURI) {
>+    var stmt = this.db.createStatement("SELECT uri FROM webapps where uri = :uri");
>+    stmt.params.uri = aURI;
>+    return (stmt.executeStep());
>+  },

"Reset must be called on the statement after the last call of executeStep."  (I'm not sure whether this is actually true if we aren't re-using the statement, however.)

I think we are also supposed to call finalize() for all the statements we create.
Comment 4 Mark Finkle (:mfinkle) (use needinfo?) 2011-06-17 14:02:13 PDT
Created attachment 540126 [details] [diff] [review]
patch 2

* Adds some use of async statments
* Adds a DB schema version (mostly for later)
* Caches and finalizes stmts
* Closes the DB

I don't see any other JS consumers using "connectionReady" so I skipped it
Comment 5 Mark Finkle (:mfinkle) (use needinfo?) 2011-06-17 14:08:41 PDT
Comment on attachment 540126 [details] [diff] [review]
patch 2

Crap. I missed:
* 2008 -> 2011
* @mozilla.org/webapps/installer;1 -> @mozilla.org/webapps/support;1

fixed locally
Comment 6 Matt Brubeck (:mbrubeck) 2011-06-17 14:25:29 PDT
Comment on attachment 540126 [details] [diff] [review]
patch 2

>+  isApplicationInstalled: function(aURI) {
>+    let stmt = this._findQuery;
>+    stmt.params.uri = aURI;
>+    let found = stmt.executeStep();
>+    return found;
>+  },

Needs a "stmt.reset()" before the return.
Comment 7 Mark Finkle (:mfinkle) (use needinfo?) 2011-07-07 14:14:29 PDT
(In reply to comment #6)
> Comment on attachment 540126 [details] [diff] [review] [review]
> patch 2
> 
> >+  isApplicationInstalled: function(aURI) {
> >+    let stmt = this._findQuery;
> >+    stmt.params.uri = aURI;
> >+    let found = stmt.executeStep();
> >+    return found;
> >+  },
> 
> Needs a "stmt.reset()" before the return.

Added a try / finally for the reset()
Comment 8 Mark Finkle (:mfinkle) (use needinfo?) 2011-07-07 15:00:18 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/9e99bd095fe1
Comment 9 Daniel Holbert [:dholbert] 2011-07-07 16:02:16 PDT
Backed out because this push turned Android talos perma-orange (in a way that can be caused by bugs in code, according to aki):
http://hg.mozilla.org/integration/mozilla-inbound/rev/bf2c04e63a29
Comment 10 Daniel Holbert [:dholbert] 2011-07-07 16:29:43 PDT
(looks like the push turned all unittests orange, actually - talos was just the first to finish. http://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=89ef5bf1e3d2 )
Comment 11 Mark Finkle (:mfinkle) (use needinfo?) 2011-07-07 19:39:10 PDT
This patch was not the cause of the failures

http://hg.mozilla.org/integration/mozilla-inbound/rev/b1707b7835e2
Comment 12 Marco Bonardo [::mak] 2011-07-08 05:59:35 PDT
http://hg.mozilla.org/mozilla-central/rev/b1707b7835e2
Comment 13 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-10-13 10:19:09 PDT
qa- as I don't think there is anything specific QA can do to verify this fix. Please reset to qa+ if a testcase can be provided.

Note You need to log in before you can comment on or make changes to this bug.