1157709 - Remove PlacesUtils.asyncGetBookmarkIds

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P2)

Description

[Marco Bonardo [:mak]]

since Bookmarks.jsm is async, this util doesn't make sense anymore.

Comment 1

[Marco Bonardo [:mak]]

This looks actionable regardless of the dependencies.
I see 2 uses:
http://searchfox.org/mozilla-central/search?q=asyncGetBookmarkIds&path=

in PlacesTransactions.jsm it is used to see if an url is bookmarked. For this we can just use PlacesUtils.bookmarks.fetch. so the whole promiseIsBookmarked can be removed.

in browser-places.js we can still use the onResult callback of PlacesUtils.bookmarks.fetch to collect all the guids, then loop through them and use PlacesUtils.promiseItemId to convert to ids.
To simplify the code, updateStarState should become updateStarState: Task.async(function* () {...

We will also have to replace the _pendingStmt management there, since we cannot interrupt the bookmarks fetch. For that we could instead use a unique object and compare with it to know if we are in the current iteration.
Basically when we start querying we set something like
let pendingUpdate = {};
this._pendingUpdate = pendingUpdate;
When we are about to handle a result aftera yield we check if pendingUpdate == this._pendingUpdate;
At the end of the update we delete this._pendingUpdate;
everywhere we check this._pendingStmt we can instead check this._pendingUpdate

Since I don't see a wide usage in add-ons (most matches check for its existence) we'll just remove it rather than deprecating.

Comment 2

[Alex]

I can go ahead and remove the instances if no one else is working on it.

Comment 3

[Marco Bonardo [:mak]]

nobody is assigned, so you're free to proceed with it.

Comment 4

[Alex]

So I haven't developed here before but I assume I can just follow the steps on the using mercurial page and just grep for the instances correct?

Comment 5

[Marco Bonardo [:mak]]

Primary reference pages:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch

from here you should be able to have a working Firefox build, an artifact build should be fine here since it's all javascript code, it will be faster since it can download pre-built binaries from remote.
For setting up stuff you can use "./mach bootstrap" from the src dir, it should also setup mercurial config for you.

Comment 6

[Pauline]

I would like to work on it; could you please assign it to me?

Comment 7

[Marco Bonardo [:mak]]

Hi, feel free to proceed, the bug will be assigned when the first patch is attached.

Comment 8

[Pauline]

Attachment: Removed promiseIsBookmarked; replaced it with PlacesUtils.bookmarks.fetch. All tests in tests/unit/test_async_transactions.js pass locally

***

Updated browser-places.js to remove asyncGetBookmarkIds

Comment 9

[Pauline]

Hi,

About PlacesTransactions.jsm - I have removed PlacesUtils.asyncGetBookmarkIds from that file and ran the tests locally doing `./mach test toolkit/components/places/`. All of those tests passed locally.

About browser-places.js - I have removed PlacesUtils.asyncGetBookmarkIds from that as well, however I could not run the tests to confirm everything is working properly (the tests took too long to run). I have some doubts that it is working properly, because when I built the project and `mach run` it, the bookmark functionality was working, however the start would not stay blue. I would appreciate your input on this, and I will work on fixing it ASAP :-)

PlacesUtils.asyncGetBookmarkIds itself was not removed from PlacesUtils.jsm because it is still directly referenced in some tests file and I wasn't sure whether I should remove it from those tests as well.

Best,
Pauline

Comment 10

[Marco Bonardo [:mak]]

(In reply to Pauline from comment #9)
> PlacesUtils.asyncGetBookmarkIds itself was not removed from PlacesUtils.jsm
> because it is still directly referenced in some tests file and I wasn't sure
> whether I should remove it from those tests as well.

test_PlacesUtils_asyncGetBookmarkIds.js should be removed, also from xpcshell.ini
test_async_transactions.js should be updated

then you should be able to remove the util.

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 8852707 [details] [diff] [review]
Removed promiseIsBookmarked; replaced it with PlacesUtils.bookmarks.fetch. All tests in tests/unit/test_async_transactions.js pass locally

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

::: browser/base/content/browser-places.js
@@ +1673,1 @@
>      this._uri = gBrowser.currentURI;

The comment and newline change don't seem necessary.

@@ +1680,2 @@
>  
> +    yield PlacesUtils.bookmarks.fetch({url: this._uri}, entry => {

Rather than yielding, I think we should use the .then() handler. Otherwise we may continue running after the yield after the pendingUpdate has been canceled.
The first thing to do in the callback would be to check pendingUpdate and check that the return value is an array of bookmark objects.
This means likely you won't need Task.async

@@ +1692,4 @@
>  
> +    let aItemIds = [];
> +    for (let guid of aItemGuids) {
> +      aItemIds.push(yield PlacesUtils.promiseItemId(guid));

We should try to change _itemIds to _itemGuids
The notifications like onItemAdded, onItemRemoved, etc, also have guids, even if not in the current code, you can just add the missing arguments, see nsINavBookmarkObserver

Comment 12

[Pauline]

Attachment: Removed all uses of PlacesUtils.asyncGetBookmarkIds

Hi Marco,

I've reached a point where I am stuck. I was wondering if you could help.

test_async_transactions.js - I have a failing test whereby `yield PlacesUtils.bookmarks.fetch({ url })` returns null when it should not (the one on line 1145). The test passed previously, when it was calling `yield promiseIsBookmarked(url)`. I have spent quite some on this and I haven't been able to pinpoint the issue. I can only assume that there is no bookmark corresponding to that URL but then `promiseIsBookmarked(url)` should have failed too. The other times when `bmsvc.fetch({ url })` is called (line 1137, 1152 and 1154), the tests pass.

browser-places.js - The current code I have submitted as been updated from last time. I no longer have the issue with the star I was describing. It seems to work well, though I would rather test it properly. I wasn't sure which tests to run to make sure it is working, so I looked for the tests that use BookmarkingUI and ran them all. They passed. If would be happy to learn more about how to test the function I am working on properly. The issue I have with that file is around integrating your last few comments.

I have not been able to use the .then() handler. I have tried several different ways but the callbacks never gets executed. When you mention the callback in your comment, which one do you refer to: the one from PlacesUtils.bookmarks.fetch, or the one from the .then() handler? I have tried either way but I couldn't get it to work. I was not able to achieve a way where the .then() handler was executed at any stage. On top of that, if I don't yield PlacesUtils.bookmarks.fetch, then its callback is never executed either. I have spent a few evenings on this and I am pretty disappointed I couldn't achieve that. Do you have any further guidance on how achieve what you are hopping for?

About ItemGuids: _updateStar uses this._itemIds to check whether the page should be starred or not (`if (this._itemIds.length > 0)`) therefore I am not sure that the function I am working on will work as intended if I do not retrieve all the itemIds from the itemGuids. Did you mean that I should update _updateStar as well, and all its references? Sorry if the question is redundant but I wanted to make sure that this is what is required before I start updating all of this.

Thanks,
Pauline

Comment 13

[Marco Bonardo [:mak]]

please, set a needinfo flag (at the bottom of this bug report page) to the person you have questions for, it's easier for us to track those.
I'll look at this asap.

Comment 14

[Marco Bonardo [:mak]]

(In reply to Pauline from comment #12)
> test_async_transactions.js - I have a failing test whereby `yield
> PlacesUtils.bookmarks.fetch({ url })` returns null when it should not (the
> one on line 1145). The test passed previously, when it was calling `yield
> promiseIsBookmarked(url)`. I have spent quite some on this and I haven't
> been able to pinpoint the issue. I can only assume that there is no bookmark
> corresponding to that URL but then `promiseIsBookmarked(url)` should have
> failed too. The other times when `bmsvc.fetch({ url })` is called (line
> 1137, 1152 and 1154), the tests pass.

I think I know why that happens.
asyncGetBookmarkIds returns any bookmark that points to that uri.
Now, tags are unfortunately implemented as "special" bookmarks, and asyncGetBookmarkIds doesn't differenciate, so if there's a tag, it will return true.
Bookmarks.fetch fixed that bug, so it filters out tags.
I suspect either the test is wrong, or PlacesTransactions.jsm is wrong, the old code was finding a tag and thinking it was a bookmark.
Don't worry too much about this, in the worst case I will look into it when we're closer to a final patch.

> I have not been able to use the .then() handler. I have tried several
> different ways but the callbacks never gets executed. When you mention the
> callback in your comment, which one do you refer to: the one from
> PlacesUtils.bookmarks.fetch, or the one from the .then() handler?

I was thinking something like this
let guids = [];
PlacesUtils.bookmarks.fetch({ url }, bm => guids.push(bm.guid))
                     .catch(Components.utils.reportError)
                     .then(() => {
  if (pendingUpdate != this._pendingUpdate)
    ...
});

The .catch is just in case

> About ItemGuids: _updateStar uses this._itemIds to check whether the page
> should be starred or not (`if (this._itemIds.length > 0)`) therefore I am
> not sure that the function I am working on will work as intended if I do not
> retrieve all the itemIds from the itemGuids.

it should use this._itemGuids
Everywhere here we should change from itemIds to itemGuids since guids is what the API provides.

Comment 15

[Pauline]

Attachment: Removed all uses of PlacesUtils.asyncGetBookmarkIds

Hi, thanks a lot for your last comment. This was super useful and I was able to progress as a result. I have integrated all the changes you mentioned. I ran the same tests as last time and they passed. It also works fine when I `./mach build` and `./mach run` it.

Comment 16

[Marco Bonardo [:mak]]

please, set the review flag on your attachment on me, if it's ready for review.

Comment 17

[Marco Bonardo [:mak]]

Also, I suppose I should still investigate the async transactions failure you mentioned in comment 12?

Comment 18

[Pauline]

Hi Mak, I tried to set the review flag yesterday, but I couldn't find it. Note: I am not assigned to this bug, maybe that's why? You are also correct on the async transaction failure.

Comment 19

[Marco Bonardo [:mak]]

when you attach the patch there should be fields like review and feedback close to it.
Now you can set flags by clicking details, next to the attachment.

Note that we also have a new workflow named MozReview (http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview-user.html) that allows you to directly work with multiple heads in your mercurial repository, and directly push to a special "review" repository, that will automatically create the attachment and ask for review on push.
Along with the Evolve Mercurial extension, that makes a quite clean workflow (create patch, push to review, address comments, amend changes, push to review,...).

Comment 20

[Marco Bonardo [:mak]]

Comment on attachment 8857653 [details] [diff] [review]
Removed all uses of PlacesUtils.asyncGetBookmarkIds

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

it looks almost ready, thank you

::: browser/base/content/browser-places.js
@@ +1671,4 @@
>      this._uri = gBrowser.currentURI;
> +    this._itemGuids = [];
> +
> +    let pendingUpdate = {};

probably worth a comment explaining how it's used.

@@ +1671,5 @@
>      this._uri = gBrowser.currentURI;
> +    this._itemGuids = [];
> +
> +    let pendingUpdate = {};
> +    this._pendingUpdate = pendingUpdate;

I think this would also work:
let pendingUpdate = this._pendingUpdate = {};

@@ +1680,2 @@
>  
> +                           if (pendingUpdate != this._pendingUpdate) {

let's keep the body indented just 2 spaces after PlacesUtils.
It's not great but it's better than this wide indentation

@@ -1686,4 @@
>  
> -      // It's possible that onItemAdded gets called before the async statement
> -      // calls back.  For such an edge case, retain all unique entries from both
> -      // arrays.

I think we still need this edge-case handling.
It's still possible that something adds a bookmark while you are waiting for fetch() to return, at least until we have 2 concurrent bookmarking APIs...
clearly you should update the code to guids.

@@ +1904,3 @@
>  
>    // nsINavBookmarkObserver
>    onItemAdded: function BUI_onItemAdded(aItemId, aParentId, aIndex, aItemType,

you can remove ": function BUI_onItemAdded" from here, to help reducing the line length

@@ +1916,5 @@
>        }
>      }
>    },
>  
> +  onItemRemoved: function BUI_onItemRemoved(aItemId, aParentId, aIndex,

and remove ": function BUI_onItemRemoved("

all of this boilerplate was needed when the stack walking had bugs and we didn't have ES6 shorthands.

@@ +1929,5 @@
>        }
>      }
>    },
>  
>    onItemChanged: function BUI_onItemChanged(aItemId, aProperty,

ditto

@@ +1987,1 @@
>                                                   : this._starButtonOverflowedLabel;

please fix indentation

PS: I'd also be fine if you want to introduce an .isBookmarked() helper here that internally will return this._itemGuids.length > 0 and use it all around, sounds like it would make things a bit more readable.

::: toolkit/components/places/tests/unit/test_async_transactions.js
@@ +1134,4 @@
>  
>      let tagWillAlsoBookmark = new Set();
>      for (let url of urls) {
> +      if (!(yield bmsvc.fetch({ url }))) {

Can you tell me if this still fails obscurely or not?

Comment 21

[Pauline]

Attachment: Removed all uses of PlacesUtils.asyncGetBookmarkIds

Hi Marco, Apologies for the delay. I have applied your comments. It seems to be working fine. The test in test_async_transactions.js still fails though.

Comment 22

[Marco Bonardo [:mak]]

Hm this latest patch looks line an interdiff, did you forget to merge changesets?

Comment 23

[Pauline]

Attachment: Removed all uses of PlacesUtils.asyncGetBookmarkIds

Looks like it... Here it is now.

Comment 24

[Marco Bonardo [:mak]]

Comment on attachment 8866782 [details] [diff] [review]
Removed all uses of PlacesUtils.asyncGetBookmarkIds

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

::: toolkit/components/places/PlacesTransactions.jsm
@@ +1505,4 @@
>      let onUndo = [], onRedo = [];
>      for (let uri of aURIs) {
>  
> +      if (yield PlacesUtils.bookmarks.fetch({ url: uri })) {

Looks like the error causing failures in test_async_transactions.js is here, this should be an if (!(... asyncGetBookmarkIds was also returning tags, so the check in test_async_transactions.js was confused by that.
This is good, we found a bug!

Dont' worry about this, I will directly fix it before landing.

Comment 25

[Marco Bonardo [:mak]]

checking tests on Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec8b117c10206226b5088540bd010c19e606f187

Comment 26

[Marco Bonardo [:mak]]

Attachment: updated patch

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ee414a52ab1
Removed all uses of PlacesUtils.asyncGetBookmarkIds. r=mak77

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4ee414a52ab1

Comment 29

[Joel Maher ( :jmaher ) (UTC -8)]

good news (to counter the bad new of the talos regression), there is a AWSY memory improvement:
== Change summary for alert #6592 (as of May 11 2017 22:23 UTC) ==

Improvements:

  4%  Explicit Memory summary windows7-32-vm opt      114,176,080.01 -> 109,609,005.84
  4%  Resident Memory summary windows7-32-vm opt      400,773,773.31 -> 385,822,445.00
  4%  Explicit Memory summary linux32 opt             115,980,741.01 -> 111,711,691.77
  4%  Explicit Memory summary linux64 opt             154,589,146.31 -> 149,081,308.47
  3%  Resident Memory summary windows10-64-vm opt     613,099,121.73 -> 592,227,480.40
  3%  Explicit Memory summary windows10-64-vm opt     149,483,483.78 -> 145,119,607.05
  2%  Heap Unclassified summary windows10-64-vm opt   42,192,367.33 -> 41,268,304.13

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=6592