663571 - OS agnostic webapps backend

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[[:fabrice] Fabrice Desré]

Attachment: patch

Uses a sqlite database to store webapps uri and icons.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

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?)]

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)]

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?)]

Attachment: 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?)]

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)]

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?)]

(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?)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/9e99bd095fe1

Comment 9

[Daniel Holbert [:dholbert]]

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]]

(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?)]

This patch was not the cause of the failures

http://hg.mozilla.org/integration/mozilla-inbound/rev/b1707b7835e2

Comment 12

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/b1707b7835e2

Comment 13

[u279076]

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.