Mark Hammond [:markh] [:mhammond] Reporter
	Description • 11 years ago

We'd like to remove nsIFormHistory in-favour of the async FormHistory.jsm.  Android code still uses nsIFormHistory2 - this bug is to replace that usage with the new module.

	:Margaret Leibovic Assignee
	Comment 1 • 11 years ago

I can take a look at this.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 2 • 11 years ago

For mobile/android/modules/Sanitizer.jsm, I think you just need to port the changes from http://hg.mozilla.org/mozilla-central/diff/0b21e902f7a0/browser/base/content/sanitize.js (also the followup: http://hg.mozilla.org/mozilla-central/rev/96fe69d53f35).

For browser.js, the "// Init FormHistory" thing should just be removed - the form history constructor doesn't do any interesting initialization on creation, so that's kind of pointless (http://hg.mozilla.org/mozilla-central/annotate/198e38876f7e/toolkit/components/satchel/nsFormHistory.js#l83).

The FormHistory:Init thing seems complicated. I don't know how FormHistoryProvider.java works. Maybe if it's compatible with FormHistory.jsm, you can just make FormHistory:Init work by calling the new service and doing some dummy operation to initialize things (rather than just getting .DBConnection). But that will result in async initialization, and so you may need to invent a FormHistory:Init:Response and have FormHistoryProvider block on that.

	:Margaret Leibovic Assignee
	Comment 3 • 11 years ago

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #2)

> The FormHistory:Init thing seems complicated. I don't know how
> FormHistoryProvider.java works. Maybe if it's compatible with
> FormHistory.jsm, you can just make FormHistory:Init work by calling the new
> service and doing some dummy operation to initialize things (rather than
> just getting .DBConnection). But that will result in async initialization,
> and so you may need to invent a FormHistory:Init:Response and have
> FormHistoryProvider block on that.

Looking through this code, it doesn't look like we depend on this initialization being synchronous, since this "FormHistory:Init" message is already asynchronous, but I'm not sure. I'm not familiar with this history of this code, so maybe Wes can help us understand what we should do.

	Wesley Johnston (:wesj)
	Comment 4 • 11 years ago

We don't care if this is sync. We just want to ensure the form history database is created at some point, or sync's will fail (the sync attempt that kicked off the call to FormHistory:Init will fail too, but the next attempt will hopefully pass). If there's some way to force the async provider to create the database, it would work.

In fact, this "message" may not be needed anymore. Sync can try to sync information when Gecko has never been started, or if it has been started, but form history has never been initialized (i.e. there is no db). In those cases, Sync will STILL just fail, but this would kick off the initialization (assuming Gecko was running). I initially figured we could live with that, but was wrong. Now we force the form history database to be created on first run to ensure the database is there for sync. I think I left the old code in just in case something had deleted your form history database, and you were trying to sync again... but I'm not exactly sure how that can happen.

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 5 • 11 years ago

(In reply to Wesley Johnston (:wesj) from comment #4)
 
> In fact, this "message" may not be needed anymore. Sync can try to sync
> information when Gecko has never been started, or if it has been started,
> but form history has never been initialized (i.e. there is no db). In those
> cases, Sync will STILL just fail, but this would kick off the initialization
> (assuming Gecko was running).

15 Mozilla Bucks if we can remove that message!

	:Margaret Leibovic Assignee
	Comment 6 • 11 years ago

I started to remove the "FormHistory:Init" and "Passwords:Init" notifications, but that started to feel out of scope for this bug, so we can just do that in a separate bug.

Pushed this to try:
https://tbpl.mozilla.org/?tree=Try&rev=6e1331aea705

	:Margaret Leibovic Assignee
	Comment 7 • 11 years ago

(In reply to Mark Finkle (:mfinkle) from comment #5)
> (In reply to Wesley Johnston (:wesj) from comment #4)
>  
> > In fact, this "message" may not be needed anymore. Sync can try to sync
> > information when Gecko has never been started, or if it has been started,
> > but form history has never been initialized (i.e. there is no db). In those
> > cases, Sync will STILL just fail, but this would kick off the initialization
> > (assuming Gecko was running).
> 
> 15 Mozilla Bucks if we can remove that message!

I filed bug 879097. I didn't want to block the changes here from landing if there ended up being some problem with this.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 8 • 11 years ago

Comment on attachment 757738 [details] [diff] [review]
patch

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>-    // Init LoginManager
>-    Cc["@mozilla.org/login-manager;1"].getService(Ci.nsILoginManager);

Login Manager is a different beast and does still do useful stuff on construction:

http://hg.mozilla.org/mozilla-central/annotate/7e3a4ebcf067/toolkit/components/passwordmgr/nsLoginManager.js#l143

(at least until bug 839961 lands), so I don't think you can remove this (yet)?

>       case "FormHistory:Init": {
>-        let fh = Cc["@mozilla.org/satchel/form-history;1"].getService(Ci.nsIFormHistory2);
>         // Force creation/upgrade of formhistory.sqlite
>-        let db = fh.DBConnection;
>+        FormHistory.schemaVersion;

Hmm, kind of unfortunate that this is exposed by FormHistory.jsm. We want to make DB migration entirely async eventually (see bug 876002 comment 4), so it would probably be best to use something else here (like "get") to be future proof... though now that I've written this all I remembered about bug 879097, so I guess this doesn't need to be future-proof.

	:Margaret Leibovic Assignee
	Comment 9 • 11 years ago

Addressed gavin's feedback.

	Wesley Johnston (:wesj)
	Comment 10 • 11 years ago

Comment on attachment 758151 [details] [diff] [review]
patch v2

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

::: mobile/android/modules/Sanitizer.jsm
@@ +55,5 @@
>    clearItem: function (aItemName)
>    {
> +    let item = this.items[aItemName];
> +    let canClear = item.canClear;
> +    if (typeof canClear == "function") {

Did you tell me you hated functions like this!

	:Margaret Leibovic Assignee
	Comment 11 • 11 years ago

(In reply to Wesley Johnston (:wesj) from comment #10)
> Comment on attachment 758151 [details] [diff] [review]
> patch v2
> 
> Review of attachment 758151 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/modules/Sanitizer.jsm
> @@ +55,5 @@
> >    clearItem: function (aItemName)
> >    {
> > +    let item = this.items[aItemName];
> > +    let canClear = item.canClear;
> > +    if (typeof canClear == "function") {
> 
> Did you tell me you hated functions like this!

Probably, but I'm just porting over the desktop version:
http://hg.mozilla.org/mozilla-central/diff/0b21e902f7a0/browser/base/content/sanitize.js#l1.28

	:Margaret Leibovic Assignee
	Comment 12 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/1dca30a5eaf7

	Ed Morley [:emorley]
	Comment 13 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/1dca30a5eaf7