Use Asynchronous FormHistory.jsm in place of nsIFormHistory2 in Suite

RESOLVED FIXED in seamonkey2.52

Status

RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: philip.chee, Assigned: iann_bugzilla)

Tracking

Trunk
seamonkey2.52
Dependency tree / graph

SeaMonkey Tracking Flags

(seamonkey2.51 fixed, seamonkey2.52 fixed)

Details

Attachments

(3 attachments, 7 obsolete attachments)

(Reporter)

Description

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

Comment 1

2 years ago
Created attachment 8735822 [details] [diff] [review]
WIP Patch covering sanitize

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)
(Assignee)

Updated

2 years ago
Component: General → Bookmarks & History
(Reporter)

Comment 2

2 years ago
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+
(Reporter)

Comment 3

2 years ago
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
(Reporter)

Comment 4

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

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

Comment 5

2 years ago
> +        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)
(Reporter)

Comment 6

2 years ago
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.
Blocks: 1334779
Created attachment 8835677 [details] [diff] [review]
912031-tempreststore.patch

Temporary restore nsIFormHistory2 until the patch from IanN is ready.
Created attachment 8835957 [details] [diff] [review]
WIP Patch covering sanitize adjusted for bit rot.

Bit rot fixed and added missing import. No other changes done.
Created attachment 8836507 [details] [diff] [review]
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.
Blocks: 1345770
Depends on: 1346850
Created attachment 8848863 [details] [diff] [review]
912031-formhistory-dataman.patch
Attachment #8836507 - Attachment is obsolete: true
Attachment #8848863 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 12

a year ago
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?
(Assignee)

Comment 15

a year ago
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.
Created attachment 8858657 [details] [diff] [review]
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.
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)
Created attachment 8858661 [details] [diff] [review]
912031-formhistory-sanitizer.patch
Attachment #8858661 - Flags: superreview?(iann_bugzilla)
Attachment #8858661 - Flags: review+
(Assignee)

Comment 21

a year ago
(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.
(Assignee)

Comment 22

a year ago
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+
Created attachment 8858857 [details] [diff] [review]
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.
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.
status-seamonkey2.51: --- → affected
status-seamonkey2.52: --- → fixed
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?
(Assignee)

Comment 27

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

a=me
Attachment #8858661 - Flags: approval-comm-beta? → approval-comm-beta+
status-seamonkey2.51: affected → fixed
(Assignee)

Comment 29

a year ago
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+
Created attachment 8863320 [details] [diff] [review]
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.
Attachment #8858857 - Attachment is obsolete: true
Attachment #8863320 - Flags: review?(iann_bugzilla)
Created attachment 8863321 [details] [diff] [review]
912031-formhistory-sanitizer-tests-V2.patch

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)
(Assignee)

Comment 32

a year ago
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
Last Resolved: a year ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.