OS agnostic webapps backend

RESOLVED FIXED in Firefox 8

Status

Fennec Graveyard
General
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: fabrice, Assigned: mfinkle)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 8

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Created attachment 538644 [details] [diff] [review]
patch

Uses a sqlite database to store webapps uri and icons.
(Reporter)

Updated

6 years ago
Assignee: nobody → fabrice
(Reporter)

Updated

6 years ago
Blocks: 583750
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
Attachment #538644 - Flags: review+
(Assignee)

Updated

6 years ago
tracking-fennec: --- → 7+
Comment on attachment 538644 [details] [diff] [review]
patch

Second review. We want to keep this simple and add to it later, as needed.
Attachment #538644 - Flags: review?(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.
Attachment #538644 - Flags: review?(mbrubeck) → review-
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
Assignee: fabrice → mark.finkle
Attachment #538644 - Attachment is obsolete: true
Attachment #540126 - Flags: review?(mbrubeck)
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 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.
Attachment #540126 - Flags: review?(mbrubeck) → review+
(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()
http://hg.mozilla.org/integration/mozilla-inbound/rev/9e99bd095fe1
Whiteboard: [inbound]
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
Whiteboard: [inbound]
(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 )
This patch was not the cause of the failures

http://hg.mozilla.org/integration/mozilla-inbound/rev/b1707b7835e2
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/b1707b7835e2
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 8
(Assignee)

Updated

6 years ago
tracking-fennec: 7+ → 8+
Whiteboard: [inbound]
(Assignee)

Updated

6 years ago
status-firefox8: --- → fixed

Updated

6 years ago
Whiteboard: QA?
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.
Whiteboard: QA? → [qa-]
You need to log in before you can comment on or make changes to this bug.