Closed
Bug 878692
Opened 12 years ago
Closed 12 years ago
form history should use FormHistory.jsm instead of nsIFormHistory2
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 24
People
(Reporter: markh, Assigned: Margaret)
References
Details
Attachments
(1 file, 1 obsolete file)
5.49 KB,
patch
|
wesj
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 2•12 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.
Assignee | ||
Comment 3•12 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.
Flags: needinfo?(wjohnston)
Comment 4•12 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.
Flags: needinfo?(wjohnston)
Comment 5•12 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!
Assignee | ||
Comment 6•12 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
Attachment #757738 -
Flags: review?(wjohnston)
Assignee | ||
Comment 7•12 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.
Comment 8•12 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.
Assignee | ||
Comment 9•12 years ago
|
||
Addressed gavin's feedback.
Attachment #757738 -
Attachment is obsolete: true
Attachment #757738 -
Flags: review?(wjohnston)
Attachment #758151 -
Flags: review?(wjohnston)
Comment 10•12 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!
Attachment #758151 -
Flags: review?(wjohnston) → review+
Assignee | ||
Comment 11•12 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
Assignee | ||
Comment 12•12 years ago
|
||
Comment 13•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•