912031 - Use Asynchronous FormHistory.jsm in place of nsIFormHistory2 in Suite

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[Philip Chee]

See Bug 566746 (Create new module FormHistory.jsm with handles asynchronous form history) and dependencies.

Comment 1

[Ian Neal]

Attachment: WIP Patch covering sanitize

This has not been tested as yet, so just another set of eyes on it.

Comment 2

[Philip Chee]

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);
}

Comment 3

[Philip Chee]

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

Comment 4

[Philip Chee]

> 3.Use a Promise Constructor:
>     return new Promise(......);

Example: https://hg.mozilla.org/integration/mozilla-inbound/rev/8a367c93f87d

Comment 5

[Philip Chee]

> +        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");
  .....

Comment 6

[Philip Chee]

Sat Apr 30 2016 00:29:02
Error: ReferenceError: XPCOMUtils is not defined
Source file: resource://app/modules/Sanitizer.jsm
Line: 9

Comment 7

[Frank-Rainer Grahl (:frg)]

Bug 876002 will remove nsIFormHistory2 today. This will break the Data manager and sanitizer.

Comment 8

[Frank-Rainer Grahl (:frg)]

Attachment: 912031-tempreststore.patch

Temporary restore nsIFormHistory2 until the patch from IanN is ready.

Comment 9

[Frank-Rainer Grahl (:frg)]

Attachment: WIP Patch covering sanitize adjusted for bit rot.

Bit rot fixed and added missing import. No other changes done.

Comment 10

[Frank-Rainer Grahl (:frg)]

Attachment: WIP patch for Data Manager (just the start)

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.

Comment 11

[Frank-Rainer Grahl (:frg)]

Attachment: 912031-formhistory-dataman.patch

Comment 12

[Ian Neal]

Comment on attachment 8848863 [details] [diff] [review]
912031-formhistory-dataman.patch

LGTM r=me

Comment 13

[Frank-Rainer Grahl (:frg)]

https://hg.mozilla.org/comm-central/rev/d18c7965d883598dfc4d7120643073a172e29c03

Looking at the sanitizer part now.

Comment 14

[Frank-Rainer Grahl (:frg)]

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

Comment 15

[Ian Neal]

Comment on attachment 8848863 [details] [diff] [review]
912031-formhistory-dataman.patch

a=me

Comment 16

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8848863 [details] [diff] [review]
912031-formhistory-dataman.patch

https://hg.mozilla.org/releases/comm-aurora/rev/a393a71df2386f8ec09aef7aece8b2bb680f5208

Comment 17

[Frank-Rainer Grahl (:frg)]

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.

Comment 18

[Frank-Rainer Grahl (:frg)]

Attachment: 912031-formhistory-sanitizer.patch

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.

Comment 19

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8858657 [details] [diff] [review]
912031-formhistory-sanitizer.patch

Missed one comment for sanitize.xul

Comment 20

[Frank-Rainer Grahl (:frg)]

Attachment: 912031-formhistory-sanitizer.patch

Comment 21

[Ian Neal]

(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 22

[Ian Neal]

Comment on attachment 8858661 [details] [diff] [review]
912031-formhistory-sanitizer.patch

sr=me not sure where my removal of numberOfUndoItems came from...

Comment 23

[Frank-Rainer Grahl (:frg)]

Attachment: 912031-formhistory-sanitizer-tests.patch

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.

Comment 24

[Frank-Rainer Grahl (:frg)]

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

Comment 25

[Frank-Rainer Grahl (:frg)]

Tests are still open but they are genrally broken anyway so marking it fixed for 2.52.

Comment 26

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8858661 [details] [diff] [review]
912031-formhistory-sanitizer.patch

After merge its now comm-beta

Comment 27

[Ian Neal]

Comment on attachment 8858661 [details] [diff] [review]
912031-formhistory-sanitizer.patch

a=me

Comment 28

[Frank-Rainer Grahl (:frg)]

https://hg.mozilla.org/releases/comm-beta/rev/d0bd0343ed6ddbd6c6a5626ed25120becd4947e7

Comment 29

[Ian Neal]

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

Comment 30

[Frank-Rainer Grahl (:frg)]

Attachment: 912031-formhistory-sanitizer-tests-V2.patch

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

Comment 31

[Frank-Rainer Grahl (:frg)]

Attachment: 912031-formhistory-sanitizer-tests-V2.patch

Sorry previous file missed an LF in test_browser_sanitizer.js

Comment 32

[Ian Neal]

Comment on attachment 8863321 [details] [diff] [review]
912031-formhistory-sanitizer-tests-V2.patch

LGTM r=me thanks

Comment 33

[Frank-Rainer Grahl (:frg)]

912031-formhistory-sanitizer-tests-V2.patch
https://hg.mozilla.org/comm-central/rev/3288aba313695802ff20a9e23e88253adf415992