1148467 - Use new keywords API in promiseBookmarksTree

Status: RESOLVED DUPLICATE of bug 1148466

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Should be easy

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

the scope here is to replace the call to getKeywordForBookmark in promiseBookmarksTree, with a call to yield PlacesUtils.keywords.fetch API.

Comment 2

[Jesal]

Hi Marco, I am interested in the bug, I have Firefox built, please could you tell me where to find the code? Thanks

Jesal

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Use mxr (http://mxr.mozilla.org/) or dxr (http://dxr.mozilla.org/) so search through the codebase.
You should find the promiseBookmarksTree implementation in PlacesUtils.jsm and you should find a getKeywordForBookmark call into it. That is what we need to replace.

the keywords.fetch() method is here
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/PlacesUtils.jsm#2027

the problem is that I just noticed fetch doesn't yet implement a way to get keywords for a uri, so the first thing to do here is to implement that.
I'll do the implementation of that new API in Bug 1149488, you'll need that to complete this bug, but you can begin putting all the stuff in place as if it was there and then will just have to test it.

the API will be like let entry = yield PlacesUtils.keywords.fetch({ url }, onResult=null); (where onResult allows to get all the results, while the return value will be just one of them)

Comment 4

[Cjewett92]

If Jesal is not assigned this problem I would like to work on it if possible.

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Jesal asked first, so he's the current assignee (though, we formally assign bugs only when the first patch is attached)

Comment 6

[Cjewett92]

Alright, in case Jesal does not get to it I am attempting it now (first bug for me) to familiarize myself with the process. As you said we are replacing getKeywordForBookmark with PlacesUtils.keywords.fetch({ url }, onResult=null); - Looking at the code we are replacing the url with the same uri the getKeywordForBookmark function uses? What about onResult=null? Is it always null or is there some other parameters available.

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Cjewett92 from comment #6)
> Alright, in case Jesal does not get to it I am attempting it now (first bug
> for me) to familiarize myself with the process. As you said we are replacing
> getKeywordForBookmark with PlacesUtils.keywords.fetch({ url },
> onResult=null); - Looking at the code we are replacing the url with the same
> uri the getKeywordForBookmark function uses? What about onResult=null? Is it
> always null or is there some other parameters available.

Sorry, I understand it's not your intention to step on others foot, but please find a free bug in the bugs list, since there are many
http://www.joshmatthews.net/bugsahoy/?js=1&unowned=1
That way the mentor can follow you more closely, and we don't confuse the bug flow.

Comment 8

[Jesal]

Hi Marco, Im having issues with my build, just re downloading the source code, once that is done I will have a look at this.

Thanks

Jesal

Comment 9

[Jesal]

Hi Marco

I just had a look at the placesutils.jsm file, found that there are four instances of getKeywordForBookmark. 

From what I understand, I need to replace the four instances with this new fetch implementation? So for example one would be:

      var keyword = PlacesUtils.bookmarks.getKeywordForBookmark(aJSNode.id);

to:

      1.var keyword = PlacesUtils.keywords.fetch(aJSNode.id);
The other instances?: 

      2.let keyword = PlacesUtils.keywords.fetch(itemId);
      3. PlacesUtils.keywords.fetch(this.item.id);
      4.  this.item.keyword = PlacesUtils.keywords.fetch(this.item.id);

Is this right? 
I am slightly confused at where the let entry implementation needs to go, or is that something that depends on your eventual implementation of the new api?

Thanks

Jesal

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

No, please only replace the instances in promiseBookmarksTree, we will replace the others in other bugs.

Note PlacesUtils.keywords.fetch has a different interface.

First of all it returns a Promise (https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Promise.jsm/Promise) so you must handle the promise with a yield (if you are inside a Task, that I think it's your case) that will make the code wait for the promise to be resolved before proceeding.

Then, fetch cannot fetch by bookmark id, it can fetch by keyword, or by url (I'm adding the latter in bug 1149488 that should land very soon, like today-ish).
it works this way:
let entry = yield PlacesUtils.keywords.fetch({ url: a_valid_url });
entry will be an object like { keyword, url, postData } or null, if no keyword was found.
So you must adapt the code to this new API, that takes the bookmark url instead of its id, and returns an object or null.

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I'm sorry, in the end I had to do this conversion in bug 1148466 along with the other fixes :(

Maybe you could help me completing bug 1108019?