Closed Bug 1124185 Opened 5 years ago Closed 4 years ago

Replace removeAllPages with history.clear() and deprecate it

Categories

(Toolkit :: Places, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: mak, Assigned: akrawchyk, Mentored)

References

Details

(Keywords: addon-compat, perf, Whiteboard: [good first bug][lang=js])

Attachments

(1 file, 7 obsolete files)

now that we have history.clear() and it has tests coverage, it's time to replace removeAllPages usage in code and mark it as deprecated.
Blocks: 739219
No longer blocks: 854886
Depends on: 1113178
Should not be too hard.
The scope here is to replace the calls to removeAllPages() with PlacesUtils.history.clear(), eventually waiting for the promise, or using .catch(Components.utils.reportError) where we don't need to wait.
The files under metro/ folder can be ignored.
Finally, need to add PLACES_WARN_DEPRECATED(); to RemoveAllPages() in nsNavHistory.cpp
Mentor: mak77
Whiteboard: [good first bug][lang=js]
Hi Marco,
I'd like to work on this bug. I am coming to this on the advice of the person who mentored my first bug (https://bugzilla.mozilla.org/show_bug.cgi?id=699860#c33). I am going to start wrapping my head around the code, but is there anything you'd like me to be aware of before I start working on a patch?

Thanks,
Daniel
Hi,
Yes, I'd suggest to create 3 separate patches here:
1. changes in services/sync/
2. changes in addon-sdk/
3. all of the other changes

I will feedback part 1 and 2, and will review part 3, I will ask other reviewers for part 1 and 2.

this is the current list of consumers:
http://mxr.mozilla.org/mozilla-central/search?string=removeAllPages

browser/base/content/sanitize.js
In this case you will likely need to .catch(Cu.reportError) since you cannot yield, 
bug 1089695 will change that but it's currently still being worked on

services/sync/modules/engines/history.js
you should likely do the same here, but I'm not sure if tests will be happy... so the first patch should just do the replacement like in sanitize... if tests will fail we will have to make this sync method use an async callback... this can be done using
let cb = Async.makeSpinningCallback();
cb.wait();
Then just invoke cb() when done.

toolkit/components/places/nsNavHistory.cpp
This is where you should put the PLACES_WARN_DEPRECATED(); call

toolkit/components/places/nsIBrowserHistory.idl 
Here you should update the javadoc with
@deprecated Please use PlacesUtils.history.clear() instead.

toolkit/components/places/nsPlacesExpiration.js
Here there is only one comment to s/removeAllPages/History.clear()

addon-sdk/source/test/addons/places/lib/places-helper.js
Here you must invoke the done callback at the right time, I think there's no need for this to wait for the expiration topic, so you can remove that and we'll ask review to the appropriate person.
Thanks Marco.

Is there an expected/preferred timeline for completion?
no, this is not urgent. Clearly it's better if it doesn't take months, or we will both forget what we are doing :)
Hi Daniel, 
Would you please confirm your interest on this bug ? So that I can search for another bug to work on.
Hi Daniel and Amod,

I'm also interested by this bug. Give me its status now ? Taken or not taken ?

Thx
(In reply to Amod [:AMoz] from comment #7)
> Hi Daniel, 
> Would you please confirm your interest on this bug ? So that I can search
> for another bug to work on.

Hi Amod,

I have not yet been able to spend time on this bug, so please do take it.

Daniel
Thank you Daniel. :)
Voltaire, I am returning to Bugzilla after a very long time, so thought of taking few simple bugs to get started. If you need any help in searching a bug for you, so let me know.
Marco, Hi. I will upload the first patch within a day or two.
sounds good, thanks
Assignee: nobody → amod.narvekar
Status: NEW → ASSIGNED
Attached patch DemoPatch (obsolete) — Splinter Review
Hi Mak,
I got introduced to | Promises | through this Bug. So, as of now, I have changed only one instance.
Apologies in advance if my changes deviate from acceptable limits.
Attachment #8621951 - Flags: feedback?(mak77)
Comment on attachment 8621951 [details] [diff] [review]
DemoPatch

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

Hi, first of all, I must unfortunately tell that sanitize.js is being fixed in bug 1089695, so there should be nothing left to do here on that file.

But I'm going to feedback regardless cause it might be useful for the other conversions.

::: browser/base/content/sanitize.js
@@ +265,5 @@
>  
>          if (this.range)
>            PlacesUtils.history.removeVisitsByTimeframe(this.range[0], this.range[1]);
> +        else {
> +          var promise = new Promise(function(resolve, reject) {

Assigning this to a promise is not useful cause then you are not doing anything with the promise.
Usually on a promise you either want to wait or you want to at least .catch() failures

@@ +266,5 @@
>          if (this.range)
>            PlacesUtils.history.removeVisitsByTimeframe(this.range[0], this.range[1]);
> +        else {
> +          var promise = new Promise(function(resolve, reject) {
> +            PlacesUtils.history.clear().done(function(res){

Which promises documentation are you looking at?
Please have a look here https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise
there no .done() method, there is a .then method.

@@ +268,5 @@
> +        else {
> +          var promise = new Promise(function(resolve, reject) {
> +            PlacesUtils.history.clear().done(function(res){
> +              try {
> +                fulfill(JSON.parse(res));

the resolving method you passed to new Promise is resolve, not fulfill
And you don't get back a JSON string. I wonder if you are looking at some node.js promise library documentation...

@@ +269,5 @@
> +          var promise = new Promise(function(resolve, reject) {
> +            PlacesUtils.history.clear().done(function(res){
> +              try {
> +                fulfill(JSON.parse(res));
> +              } catch (Components.utils.reportError Ex) {

what I meant with catching with reportError was
{promise}.catch(Components.utils.reportError), not a try/catch construct

@@ +272,5 @@
> +                fulfill(JSON.parse(res));
> +              } catch (Components.utils.reportError Ex) {
> +                reject(Ex);
> +              }
> +            }, reject);

In any case, clear() already gives you back a promise, so there should be no need to wrap it with another promise
Attachment #8621951 - Flags: feedback?(mak77)
Priority: -- → P1
Keywords: perf
Looks like this hasn't been worked on in a while so I'm going to take a stab at it :)

I'm not sure what to do in the case of http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/sync/xpcshell.ini#12 so any help would be appreciated with it. My other notes are inline below.


(In reply to Marco Bonardo [::mak] from comment #4)
> Hi,
> Yes, I'd suggest to create 3 separate patches here:
> 1. changes in services/sync/
> 2. changes in addon-sdk/
> 3. all of the other changes
> 
> I will feedback part 1 and 2, and will review part 3, I will ask other
> reviewers for part 1 and 2.
> 
> this is the current list of consumers:
> http://mxr.mozilla.org/mozilla-central/search?string=removeAllPages
> 
> browser/base/content/sanitize.js
> In this case you will likely need to .catch(Cu.reportError) since you cannot
> yield, 
> bug 1089695 will change that but it's currently still being worked on


I don't think this file needs to be updated because bug 1089695 has been fixed and `browser/base/content/sanitize.js` has been updated with a yield statement: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/sanitize.js#345


> services/sync/modules/engines/history.js
> you should likely do the same here, but I'm not sure if tests will be
> happy... so the first patch should just do the replacement like in
> sanitize... if tests will fail we will have to make this sync method use an
> async callback... this can be done using
> let cb = Async.makeSpinningCallback();
> cb.wait();
> Then just invoke cb() when done.


I used a yield statement, like above, here as well. I tried running automated tests and everything seemed OK, but it's my first time doing so locally. Is try server access hard to get?


> addon-sdk/source/test/addons/places/lib/places-helper.js
> Here you must invoke the done callback at the right time, I think there's no
> need for this to wait for the expiration topic, so you can remove that and
> we'll ask review to the appropriate person.


I'm not sure what to do with this file either, I don't know what to import but I'm thinking I should `require('sdk/places/utils')`?
Flags: needinfo?(mak77)
Attachment #8672429 - Flags: review?(mak77)
thanks, changing assignee

(In reply to akrawchyk from comment #15)
> I'm not sure what to do in the case of
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/
> tests/sync/xpcshell.ini#12 so any help would be appreciated with it.

Ugh! I had no idea this stuff was still around.
The whole /sync folder must be removed, it's old stuff, not even built.

> I don't think this file needs to be updated because bug 1089695 has been
> fixed and `browser/base/content/sanitize.js` has been updated with a yield
> statement:
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/sanitize.
> js#345

Right

> > services/sync/modules/engines/history.js
> > you should likely do the same here, but I'm not sure if tests will be
> > happy... so the first patch should just do the replacement like in
> > sanitize... if tests will fail we will have to make this sync method use an
> > async callback... this can be done using
> > let cb = Async.makeSpinningCallback();
> > cb.wait();
> > Then just invoke cb() when done.
> 
> 
> I used a yield statement, like above, here as well. I tried running
> automated tests and everything seemed OK, but it's my first time doing so
> locally. Is try server access hard to get?

Usually not, but I think you should first have a couple patches reviewed and landed to get vouched. I can push to Try for you though. just needinfo me when you need that.

> > addon-sdk/source/test/addons/places/lib/places-helper.js
> > Here you must invoke the done callback at the right time, I think there's no
> > need for this to wait for the expiration topic, so you can remove that and
> > we'll ask review to the appropriate person.
> 
> 
> I'm not sure what to do with this file either, I don't know what to import
> but I'm thinking I should `require('sdk/places/utils')`?

the clearHistory call is using hsrv.removeAllPages(); I think it would be better to import PlacesUtils.jsm and start from there...but I'm not sure if there's a requirement to not use jsms in the sdk, since I see it's still using the old service getters. Maybe Dave knows.
Assignee: amod.narvekar → akrawchyk
Flags: needinfo?(mak77) → needinfo?(dtownsend)
(In reply to Marco Bonardo [::mak] from comment #16)
> > > addon-sdk/source/test/addons/places/lib/places-helper.js
> > > Here you must invoke the done callback at the right time, I think there's no
> > > need for this to wait for the expiration topic, so you can remove that and
> > > we'll ask review to the appropriate person.
> > 
> > 
> > I'm not sure what to do with this file either, I don't know what to import
> > but I'm thinking I should `require('sdk/places/utils')`?
> 
> the clearHistory call is using hsrv.removeAllPages(); I think it would be
> better to import PlacesUtils.jsm and start from there...but I'm not sure if
> there's a requirement to not use jsms in the sdk, since I see it's still
> using the old service getters. Maybe Dave knows.

It's fine to use jsms.
Flags: needinfo?(dtownsend)
Comment on attachment 8672429 [details] [diff] [review]
replacing removeAllPages with history.clear() progress

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

there is also a call in /browser/components/newtab/tests/xpcshell/test_RemoteDirectoryLinksProvider.js

::: browser/modules/test/xpcshell/test_DirectoryLinksProvider.js
@@ +1687,5 @@
>  
>    testObserver = new UrlDeletionTester();
>    DirectoryLinksProvider.addObserver(testObserver);
> +  // remove all history
> +  PlacesUtils.history.clear()

To stay on the safe side, this should import PlacesTestUtils and yield PlacesTestUtils.clearHistory(), that way it will properly wait for the operation to complete.
Generally tests should do this

::: services/sync/modules/engines/history.js
@@ +364,5 @@
>      return record;
>    },
>  
>    wipe: function HistStore_wipe() {
> +    yield PlacesUtils.history.clear();

wipe is not a generator, and it's not used like a generator, so using yield here is not going to work.
it is used ina bunch of places (http://mxr.mozilla.org/mozilla-central/search?string=.wipe%28%29&find=sync) and thus I'm not comfortable blindly making it async.
For now let's use the Async.waitForSyncCallback() trick (see on mxr), I know there's an ongoing effort to convert engines to be async, so that will also remove the need for this...
Attachment #8672429 - Flags: review?(mak77)
Flags: needinfo?(mak77)
Attached patch Bug_1124185.diff (obsolete) — Splinter Review
Attachment #8672429 - Attachment is obsolete: true
Comment on attachment 8674020 [details] [diff] [review]
Bug_1124185.diff

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

it's mostly good, just some minor details.

::: addon-sdk/source/test/addons/places/lib/places-helper.js
@@ +13,5 @@
>  const tagsrv = Cc['@mozilla.org/browser/tagging-service;1'].
>                getService(Ci.nsITaggingService);
>  const asyncHistory = Cc['@mozilla.org/browser/history;1'].
>                getService(Ci.mozIAsyncHistory);
> +const PlacesUtils = Cu.import("resource://gre/modules/PlacesUtils.jsm");

I'd suggest to use defineLazyModuleGetter for this.

If you wish, in a separate bug to be filed yet, you could remove all of the above service instantiations and directly use PlacesUtils getters all around this file. It would make future conversions easier.

::: services/sync/modules/engines/history.js
@@ +365,5 @@
>    },
>  
>    wipe: function HistStore_wipe() {
> +    let cb = Async.makeSyncCallback();
> +    PlacesUtils.history.clear().then(cb).catch(Cu.reportError);

I'd invert the catch and the then, the idea is that we should continue regardless, so if you put the catch before, it will handle eventual issues, a following then would be invoked regardless of success or failure.
It would also be wise to ask feedback to :markh about this change, so that at least he's aware of it.
Attachment #8674020 - Flags: feedback+
Flags: needinfo?(mak77)
Attachment #8674020 - Attachment is obsolete: true
Flags: needinfo?(mak77)
Attachment #8674649 - Flags: feedback?(markh)
Comment on attachment 8674649 [details] [diff] [review]
Use defineLazyModuleGetter and rework promises order

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

Gotta love fixing deprecated code :) But I think Mak should have a look over this too...

::: addon-sdk/source/test/addons/places/lib/places-helper.js
@@ +25,5 @@
>    save, search,
>    MENU, TOOLBAR, UNSORTED
>  } = require('sdk/places/bookmarks');
>  
> +XPCOMUtils.defineLazyModuleGetter(LoopUI, "PlacesUtils",

LoopUI doesn't seem right here - I'm surprised it works at all?

@@ +47,5 @@
>    [MENU, TOOLBAR, UNSORTED].forEach(clearBookmarks);
>  }
>  
>  function clearHistory (done) {
> +  PlacesUtils.history.clear();

I'd think that this would be better written as:

PlacesUtils.history.clear().then(done, err => <whatever makes sense on error, even if just Cu.reportError>);

as it removes an somewhat obscure and hidden assumption on observers (and it's not immediately clear to me that the notification will now be sent - at the very least, it looks like history.clear() may well do more work after it is. Mak might know more here.

::: services/sync/modules/engines/history.js
@@ +366,5 @@
>  
>    wipe: function HistStore_wipe() {
> +    let cb = Async.makeSyncCallback();
> +    PlacesUtils.history.clear().catch(Cu.reportError).then(cb);
> +    return Async.waitForSyncCallback();

waitForSyncCallback should be passed |cb| - but given wipe could (presumably) throw before, I think this should be:

let cb = Async.makeSyncCallback();
PlacesUtils.history.clear().then(result => {cb(null, result)}, err => {cb(err)});
Async.waitForSyncCallback(cb);

to keep the same semantics as before.
Attachment #8674649 - Flags: feedback?(markh) → feedback?(mak77)
(In reply to Mark Hammond [:markh] from comment #22)
>  function clearHistory (done) {
> > +  PlacesUtils.history.clear();
> 
> I'd think that this would be better written as:
> 
> PlacesUtils.history.clear().then(done, err => <whatever makes sense on
> error, even if just Cu.reportError>);

I think Cu.reportError is fine

> as it removes an somewhat obscure and hidden assumption on observers (and
> it's not immediately clear to me that the notification will now be sent - at
> the very least, it looks like history.clear() may well do more work after it
> is. Mak might know more here.

So far it's basically acting the same as the old API (modulo sync/async), it may change in the future since I'd like to remove some pieces from the expiration component and merge them back to clear.
Flags: needinfo?(mak77)
Comment on attachment 8674649 [details] [diff] [review]
Use defineLazyModuleGetter and rework promises order

please address Mark's feedback first.
Attachment #8674649 - Flags: feedback?(mak77)
Attachment #8621951 - Attachment is obsolete: true
Attached patch changeset_adjustments.patch (obsolete) — Splinter Review
(In reply to Mark Hammond [:markh] from comment #22)
> Comment on attachment 8674649 [details] [diff] [review]
> 
> ::: addon-sdk/source/test/addons/places/lib/places-helper.js
> @@ +25,5 @@
> >    save, search,
> >    MENU, TOOLBAR, UNSORTED
> >  } = require('sdk/places/bookmarks');
> >  
> > +XPCOMUtils.defineLazyModuleGetter(LoopUI, "PlacesUtils",
> 
> LoopUI doesn't seem right here - I'm surprised it works at all?
> 

Whoops, you're right, that's a copy pasta error. Meant to replace `LoopUI` with `this`, is that correct?


(In reply to Marco Bonardo [::mak] from comment #24)
> Comment on attachment 8674649 [details] [diff] [review]
> Use defineLazyModuleGetter and rework promises order
> 
> please address Mark's feedback first.


I've adjusted the changes to reflect Mark's feedback :)
Attachment #8674649 - Attachment is obsolete: true
Flags: needinfo?(mak77)
Comment on attachment 8676469 [details] [diff] [review]
changeset_adjustments.patch

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

Can you please make this fix and push a try run? Then upload a new patch and request feedback on the patch from :mak.

::: addon-sdk/source/test/addons/places/lib/places-helper.js
@@ +47,5 @@
>    [MENU, TOOLBAR, UNSORTED].forEach(clearBookmarks);
>  }
>  
>  function clearHistory (done) {
> +  PlacesUtils.history.clear().then(done, err => Cu.reportError);

This will call |done| twice - I think the |once(...)| call should be removed.
Flags: needinfo?(mak77)
Attached patch removed call to once (obsolete) — Splinter Review
(In reply to Mark Hammond [:markh] from comment #26)
> Comment on attachment 8676469 [details] [diff] [review]
> changeset_adjustments.patch
> 
> Review of attachment 8676469 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you please make this fix and push a try run? Then upload a new patch and
> request feedback on the patch from :mak.

I uploaded the updated patch. I can't push to the try server though, I've filed a bug to request access though https://bugzilla.mozilla.org/show_bug.cgi?id=1216841
Attachment #8676469 - Attachment is obsolete: true
Attachment #8676593 - Flags: feedback?(mak77)
Comment on attachment 8676593 [details] [diff] [review]
removed call to once

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

Thank you, it looks mostly good.

r=me with the following fixed, and a green-er Try run.

::: addon-sdk/source/test/addons/places/lib/places-helper.js
@@ +26,5 @@
>    MENU, TOOLBAR, UNSORTED
>  } = require('sdk/places/bookmarks');
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils",
> +  "resource://gre/modules/PlacesUtils.jsm");

For this to work you must also Cu.import XPCOMUtils. this should solve the JP failures you see on the Try run

@@ +47,5 @@
>    [MENU, TOOLBAR, UNSORTED].forEach(clearBookmarks);
>  }
>  
>  function clearHistory (done) {
> +  PlacesUtils.history.clear().then(done, err => Cu.reportError);

I'd rather do
PlacesUtils.history.clear().catch(Cu.reportError).then(done);
so we call done in any case.

::: browser/components/newtab/tests/xpcshell/test_RemoteDirectoryLinksProvider.js
@@ +1287,5 @@
>  
>    testObserver = new UrlDeletionTester();
>    RemoteDirectoryLinksProvider.addObserver(testObserver);
> +  // remove all history
> +  yield PlacesTestUtils.clearHistory();

nit: the comment is likely pointless now, the API speaks for itself

::: browser/modules/test/xpcshell/test_DirectoryLinksProvider.js
@@ +1689,5 @@
>  
>    testObserver = new UrlDeletionTester();
>    DirectoryLinksProvider.addObserver(testObserver);
> +  // remove all history
> +  yield PlacesTestUtils.clearHistory();

nit: ditto
Attachment #8676593 - Flags: feedback?(mak77) → review+
(In reply to Marco Bonardo [::mak] from comment #29)
> Comment on attachment 8676593 [details] [diff] [review]
> removed call to once
> 
> Review of attachment 8676593 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you, it looks mostly good.
> 
> r=me with the following fixed, and a green-er Try run.


Great! I've made the requested updates and have pushed another try run. I wasn't sure about the `r=me` part, so I added a review flag to you for the updated patch. Should I have committed with the `r=` in the message?

Also, is there a resource/documentation for reading Treeherder output? I'm struggling to follow the test results in the output... there's a lot going on there.
Attachment #8676593 - Attachment is obsolete: true
Attachment #8678342 - Flags: review?(mak77)
(In reply to Andrew Krawchyk from comment #31)
> Great! I've made the requested updates and have pushed another try run. I
> wasn't sure about the `r=me` part, so I added a review flag to you for the
> updated patch. Should I have committed with the `r=` in the message?

having a commit message is mandatory, and usually you append . r=reviewer to it
in this case the commit message could be:

Bug 1124185 - Replace removeAllPages with history.clear() and deprecate it. r=mak

> Also, is there a resource/documentation for reading Treeherder output? I'm
> struggling to follow the test results in the output... there's a lot going
> on there.

I honestly don't know, it mostly comes from experience with it. Basically the orange/red are what you care about, clicking on them you get a failure summary that usually points out the failure reason.
Some reasons may be recognized as intermittent failures (we have some) and a bug is suggested, in that case it's likely not your failure. Other cases could be network problems or similar.
Until you are not actively contributing from some time and you get experienced with it, you can just ask your reviewer to look at the results and eventually comment on them.
From the ui you can also retrigger a failed test, if it succeeds the next run it's likely an intermittent.
Due to the many intermittent failures it can happen that something is missed and the patch breaks the tree, but generally the system works, and in the worst case the patch gets backed out and needs some more work.
Comment on attachment 8678342 [details] [diff] [review]
import XPCOMUtils and cleanup comments

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

Are you sure this is the right patch? It looks like a backout patch...
Attachment #8678342 - Flags: review?(mak77) → review-
Bug 1124185 - Replace removeAllPages with history.clear() and deprecate it. r=mak
Attachment #8680144 - Flags: review?(mak77)
Attachment #8678342 - Attachment is obsolete: true
Comment on attachment 8680144 [details]
MozReview Request: Bug 1124185 - Replace removeAllPages with history.clear() and deprecate it. r=mak

https://reviewboard.mozilla.org/r/23575/#review21185

yep, this looks perfect, thank you!
please set the checkin-needed keyword in the keywords field of this bug, so it will be pushed to the fx-team tree.
Attachment #8680144 - Flags: review?(mak77) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5fc57433f68a
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
Keywords: addon-compat
Blocks: 1254923
See Also: → 1293618
You need to log in before you can comment on or make changes to this bug.