Closed Bug 1148467 Opened 10 years ago Closed 10 years ago

Use new keywords API in promiseBookmarksTree

Categories

(Toolkit :: Places, defect)

defect
Not set
normal
Points:
1

Tracking

()

RESOLVED DUPLICATE of bug 1148466

People

(Reporter: mak, Unassigned, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good first bug][lang=js])

Should be easy
the scope here is to replace the call to getKeywordForBookmark in promiseBookmarksTree, with a call to yield PlacesUtils.keywords.fetch API.
Mentor: mak77
Whiteboard: [good first bug][lang=js]
Hi Marco, I am interested in the bug, I have Firefox built, please could you tell me where to find the code? Thanks Jesal
Flags: needinfo?(mak77)
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)
Depends on: 1149488
Flags: needinfo?(mak77)
If Jesal is not assigned this problem I would like to work on it if possible.
Jesal asked first, so he's the current assignee (though, we formally assign bugs only when the first patch is attached)
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.
Flags: needinfo?(mak77)
(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.
Flags: needinfo?(mak77)
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
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
Flags: needinfo?(mak77)
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.
Flags: needinfo?(mak77)
Depends on: 1148466
No longer depends on: 1095426, 1095427
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?
Status: NEW → RESOLVED
Closed: 10 years ago
No longer depends on: 1148466
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.