Closed Bug 912031 Opened 6 years ago Closed 3 years ago

Use Asynchronous FormHistory.jsm in place of nsIFormHistory2 in Suite

Categories

(SeaMonkey :: Bookmarks & History, defect)

defect
Not set

Tracking

(seamonkey2.51 fixed, seamonkey2.52 fixed)

RESOLVED FIXED
seamonkey2.52
Tracking Status
seamonkey2.51 --- fixed
seamonkey2.52 --- fixed

People

(Reporter: philip.chee, Assigned: iann_bugzilla)

References

Details

Attachments

(3 files, 7 obsolete files)

See Bug 566746 (Create new module FormHistory.jsm with handles asynchronous form history) and dependencies.
Attached patch WIP Patch covering sanitize (obsolete) — Splinter Review
This has not been tested as yet, so just another set of eyes on it.
Assignee: nobody → iann_bugzilla
Status: NEW → ASSIGNED
Attachment #8735822 - Flags: feedback?(philip.chee)
Component: General → Bookmarks & History
Comment on attachment 8735822 [details] [diff] [review]
WIP Patch covering sanitize

> +  canClearItem: function (aItemName, aCallback, aArg) {
> +    let canClear = this.items[aItemName].canClear;
"var" rather than "let" here.

> -        var formHistory = Components.classes["@mozilla.org/satchel/form-history;1"]
> -                                    .getService(Components.interfaces.nsIFormHistory2);
> -        formHistory.removeAllEntries();
> +        let change = { op: "remove" };
"var" rather than "let" here.

>            var findBar = currentDocument.getElementById("FindToolbar");
> -          if (findBar && findBar.canClear)
> -            return true;
> +          if (findBar && findBar.canClear) {
I think this property is now called findBar.hasTransactions

>              if (searchBar.value ||
> -                transactionMgr.numberOfUndoItems ||

>              if (sideSearchBar.value ||
> -                sidebarTm.numberOfUndoItems ||
Why these two changes?

> +        let count = 0;
> +        let countDone = {
"var" rather than "let" here.

> +          handleResult : function(aResult) count = aResult,
> +          handleError : function(aError) Components.utils.reportError(aError),
Don't use expression closures. But you can use fat arrow syntax:
  handleResult: aResult => count = aResult,
  handleError: aError => Components.utils.reportError(aError),
Also no space before the ":".

> +          handleCompletion :
> +            function(aReason) { aCallback("formdata", aReason == 0 && count > 0, aArg); }
aReason is always either 0 (success) or 1 (failure).
For count, aResult is an integer. So:
aCallback("formdata", !aReason && count, aArg)

Perhaps:
handleCompletion(aReason) {
  aCallback("formdata", !aReason && count, aArg);
}
Attachment #8735822 - Flags: feedback?(philip.chee) → feedback+
Comment on attachment 8735822 [details] [diff] [review]
WIP Patch covering sanitize

> +      function update(changes) {
> +        let deferred = Promise.defer();

DOM Promise doesn't have the defer method. So you will have to use:

1. Components.utils.import("resource://gre/modules/Promise.jsm"); OR
2. Components.utils.import("resource://gre/modules/PromiseUtils.jsm"); OR
3.Use a Promise Constructor:
    return new Promise(......);

https://developer.mozilla.org/docs/Mozilla/JavaScript_code_modules/Promise.jsm/Deferred
https://developer.mozilla.org/docs/Mozilla/JavaScript_code_modules/Promise.jsm
https://developer.mozilla.org/docs/Mozilla/JavaScript_code_modules/PromiseUtils.jsm
https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Promise
> 3.Use a Promise Constructor:
>     return new Promise(......);

Example: https://hg.mozilla.org/integration/mozilla-inbound/rev/8a367c93f87d
> +        var obj = {};
> +        if (aName !== null)
if (aName)

>        var sanitizePreferences = document.getElementById("sanitizePreferences");
>        for (var i = 0; i < sanitizePreferences.childNodes.length; ++i) {
>          var preference = sanitizePreferences.childNodes[i];
>          var name = preference.getAttribute("name");
for..of now works for nodeLists, so:

var preferences = document.getElementById("sanitizePreferences").childNodes;
for (var pref of preferences) {
  var name = pref.getAttribute("name");
  .....
Flags: needinfo?(iann_bugzilla)
Sat Apr 30 2016 00:29:02
Error: ReferenceError: XPCOMUtils is not defined
Source file: resource://app/modules/Sanitizer.jsm
Line: 9
Bug 876002 will remove nsIFormHistory2 today. This will break the Data manager and sanitizer.
Attached patch 912031-tempreststore.patch (obsolete) — Splinter Review
Temporary restore nsIFormHistory2 until the patch from IanN is ready.
Bit rot fixed and added missing import. No other changes done.
If I hadn't found Async.promiseSpinningly I would have probably shot myself now...

List is populated using the ASYNC search call. The remaining parts should be more or less only 'mop up' now.
Depends on: 1346850
Attachment #8836507 - Attachment is obsolete: true
Attachment #8848863 - Flags: review?(iann_bugzilla)
Comment on attachment 8848863 [details] [diff] [review]
912031-formhistory-dataman.patch

LGTM r=me
Flags: needinfo?(iann_bugzilla)
Attachment #8848863 - Flags: review?(iann_bugzilla) → review+
Comment on attachment 8848863 [details] [diff] [review]
912031-formhistory-dataman.patch

[Approval Request Comment]
Regression caused by (bug #): 876002
User impact if declined: Data Manager broken for formhistory
Testing completed (on m-c, etc.): c-c c-a
Risk to taking this patch (and alternatives if risky): no risk already broken.
String changes made by this patch: none
Attachment #8848863 - Flags: approval-comm-aurora?
Comment on attachment 8848863 [details] [diff] [review]
912031-formhistory-dataman.patch

a=me
Attachment #8848863 - Flags: approval-comm-aurora? → approval-comm-aurora+
I wonder if we shouldn't port bug 1211849 in a followup bug. Normally I am not keen about removing logic without need but this seems right here and would simplify the logic. Just let the remove routines deal with any empty results.
Put Rattys suggestions in the patch and retested it. 

>              if (searchBar.value ||
> -                transactionMgr.numberOfUndoItems ||

>              if (sideSearchBar.value ||
> -                sidebarTm.numberOfUndoItems ||
> Why these two changes?

+1 Why? I reverted these two changes and didn't put them in the patch.

Next the tests.
Attachment #8858657 - Flags: superreview?(iann_bugzilla)
Attachment #8858657 - Flags: review+
Comment on attachment 8858657 [details] [diff] [review]
912031-formhistory-sanitizer.patch

Missed one comment for sanitize.xul
Attachment #8858657 - Attachment is obsolete: true
Attachment #8858657 - Flags: superreview?(iann_bugzilla)
Attachment #8858661 - Flags: superreview?(iann_bugzilla)
Attachment #8858661 - Flags: review+
(In reply to Frank-Rainer Grahl from comment #17)
> I wonder if we shouldn't port bug 1211849 in a followup bug. Normally I am
> not keen about removing logic without need but this seems right here and
> would simplify the logic. Just let the remove routines deal with any empty
> results.

Yes, that should be ported too, but as you say in a follow-up bug. There are probably lots of other bugs that need porting for sanitize too.
Comment on attachment 8858661 [details] [diff] [review]
912031-formhistory-sanitizer.patch

sr=me not sure where my removal of numberOfUndoItems came from...
Attachment #8858661 - Flags: superreview?(iann_bugzilla) → superreview+
Final part for tests. I didn't change browser_bug409624.js but test_browser_sanitizer.js has extensive changes. I still do not know how to test this locally so chances are high that I screwed up somewhere.
Attachment #8735822 - Attachment is obsolete: true
Attachment #8835677 - Attachment is obsolete: true
Attachment #8835957 - Attachment is obsolete: true
Attachment #8858857 - Flags: review?(iann_bugzilla)
Comment on attachment 8858661 [details] [diff] [review]
912031-formhistory-sanitizer.patch

https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=378466f8c7c145273d16c5c551836507b8906d2e

[Approval Request Comment]
Regression caused by (bug #): 876002
User impact if declined: claring formhistory private data does not work.
Testing completed (on m-c, etc.): c-c
Risk to taking this patch (and alternatives if risky): no risk already broken.
String changes made by this patch: none
Attachment #8858661 - Flags: approval-comm-aurora?
Tests are still open but they are genrally broken anyway so marking it fixed for 2.52.
Target Milestone: --- → seamonkey2.52
Comment on attachment 8858661 [details] [diff] [review]
912031-formhistory-sanitizer.patch

After merge its now comm-beta
Attachment #8858661 - Flags: approval-comm-aurora? → approval-comm-beta?
Comment on attachment 8858661 [details] [diff] [review]
912031-formhistory-sanitizer.patch

a=me
Attachment #8858661 - Flags: approval-comm-beta? → approval-comm-beta+
Comment on attachment 8858857 [details] [diff] [review]
912031-formhistory-sanitizer-tests.patch

Is it worth basing the first test on the Firefox version of browser_bug409624.js?
f+ for the moment
Attachment #8858857 - Flags: review?(iann_bugzilla) → feedback+
> Is it worth basing the first test on the Firefox version of browser_bug409624.js?

Yes because bug 1211849 removed the canClear. Adapted the parts. I am quite sure this still won't work but when we come to finally come around and fix the tests it should be an easy fix.
Attachment #8858857 - Attachment is obsolete: true
Attachment #8863320 - Flags: review?(iann_bugzilla)
Sorry previous file missed an LF in test_browser_sanitizer.js
Attachment #8863320 - Attachment is obsolete: true
Attachment #8863320 - Flags: review?(iann_bugzilla)
Attachment #8863321 - Flags: review?(iann_bugzilla)
Comment on attachment 8863321 [details] [diff] [review]
912031-formhistory-sanitizer-tests-V2.patch

LGTM r=me thanks
Attachment #8863321 - Flags: review?(iann_bugzilla) → review+
912031-formhistory-sanitizer-tests-V2.patch
https://hg.mozilla.org/comm-central/rev/3288aba313695802ff20a9e23e88253adf415992
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.