Closed
Bug 1094900
Opened 10 years ago
Closed 10 years ago
Livemarks service should use the new Bookmarks.jsm API
Categories
(Toolkit :: Places, defect)
Toolkit
Places
Tracking
()
Tracking | Status | |
---|---|---|
firefox40 | --- | verified |
People
(Reporter: mak, Assigned: mak)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
(deleted),
patch
|
ttaubert
:
review+
|
Details | Diff | Splinter Review |
converting it should not be too hard, since it's async already, though tests could break badly. So maybe it's better to convert tests first.
Assignee | ||
Updated•10 years ago
|
Flags: qe-verify+
Flags: firefox-backlog+
Comment 1•10 years ago
|
||
Are there two starter bugs here? Make the tests asynchronous, then the service?
Flags: needinfo?(mak77)
Assignee | ||
Comment 2•10 years ago
|
||
yes, but we are not ready for the conversion yet, we are still working on dependencies before we can start with this work.
Flags: needinfo?(mak77)
Assignee | ||
Comment 3•10 years ago
|
||
The work here can be done, won't be trivial as a good first bug, but should not be hard.
The scope is to replace any API from nsINavBookmarksService with the new API calls in Bookmarks.jsm.
Mentor: mak77
Whiteboard: [good next bug][lang=js]
Updated•10 years ago
|
Assignee: nobody → smacleod
Status: NEW → ASSIGNED
Iteration: --- → 39.2 - 23 Mar
Updated•10 years ago
|
Iteration: 39.2 - 23 Mar → 39.3 - 30 Mar
Updated•10 years ago
|
Iteration: 39.3 - 30 Mar → 40.1 - 13 Apr
Updated•10 years ago
|
Iteration: 40.1 - 13 Apr → ---
Assignee | ||
Comment 4•10 years ago
|
||
Steven, did you already start work here? I was evaluating to steal this bug :)
Flags: needinfo?(smacleod)
Comment 5•10 years ago
|
||
(In reply to Marco Bonardo [::mak] from comment #4)
> Steven, did you already start work here? I was evaluating to steal this bug
> :)
No code written yet, please do.
Assignee: smacleod → mak77
Flags: needinfo?(smacleod)
Assignee | ||
Comment 6•10 years ago
|
||
Sounds good, thanks for the quick reply.
Mentor: mak77
Whiteboard: [good next bug][lang=js]
Assignee | ||
Comment 7•10 years ago
|
||
I will try to do bug 1145650 at the same time, since it's related.
Updated•10 years ago
|
Iteration: --- → 40.2 - 27 Apr
Assignee | ||
Comment 8•10 years ago
|
||
I'm sorry, I ended up doing more refactoring than expected...
On the positive side, this removes a bunch of deprecated code, prepares the service to future improvements (deCOMify it, make the API closer to Bookmarks.jsm), I figured 1 improvement to Bookmarks.jsm API and one bug with its annotations removal. Plus this might improve reliability of import/export tests.
Locally I verified all the livemarks tests, but it's also on Try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=81c6d623f866
Attachment #8597141 -
Flags: review?(ttaubert)
Comment 9•10 years ago
|
||
Comment on attachment 8597141 [details] [diff] [review]
1145650.diff
Review of attachment 8597141 [details] [diff] [review]:
-----------------------------------------------------------------
Looks mostly good to me, great cleanup! The only real issue is that we should somehow test cache initialization with existing livemarks to cover the third comment below.
::: toolkit/components/places/BookmarkJSONUtils.jsm
@@ +423,5 @@
> if (aData.dateAdded)
> PlacesUtils.bookmarks.setItemDateAdded(id, aData.dateAdded);
> if (aData.annos && aData.annos.length)
> PlacesUtils.setAnnotationsForItem(id, aData.annos);
> }, Cu.reportError);
Couldn't we remove the error handler here too?
::: toolkit/components/places/nsLivemarkService.js
@@ +60,3 @@
>
> +XPCOMUtils.defineLazyGetter(this, "gLivemarksCachePromised",
> + () => Task.spawn(function* () {
Couldn't we use Task.async() here?
@@ +79,5 @@
> + lastModified: row.getResultByName("lastModified"),
> + feedURI: NetUtil.newURI(row.getResultByName("feedURI")),
> + siteURI: siteURI ? NetUtil.newURI(siteURI) : null
> + });
> + livemarksMap.set(guid, livemark);
"guid" shouldn't be defined here... and no tests are failing? :)
@@ +93,5 @@
> + * @param date
> + * the Date object to convert.
> + * @return microseconds from the epoch.
> + */
> +function toPRTime(date) date * 1000;
Should use a real function definition here (bug 1083458).
@@ +102,5 @@
> + * @param time
> + * microseconds from the epoch.
> + * @return a Date object or undefined if time was not defined.
> + */
> +function toDate(time) time ? new Date(parseInt(time / 1000)) : undefined;
Same here.
@@ +116,5 @@
> + PlacesUtils.addLazyBookmarkObserver(this, true);
> +}
> +
> +LivemarkService.prototype = {
> + _promiseLivemarksMap: () => gLivemarksCachePromised,
Is there any advantage in using |this._promiseLivemarksMap()| rather than just directly |gLivemarksCachePromised()| everywhere?
@@ +187,3 @@
> }
>
> + return new Task.spawn(function* () {
return Task.spawn(
@@ +249,3 @@
> }
>
> + return Task.spawn(function*() {
Nit: function* () {
@@ +291,3 @@
> }
>
> + return Task.spawn(function*() {
Nit: function* () {
@@ +427,3 @@
> this.index = aLivemarkInfo.index;
> + this.dateAdded = aLivemarkInfo.dateAdded;
> + this.lastModified = aLivemarkInfo.lastModified;
Nit: trailing white space
@@ +477,2 @@
> try {
> + Services.scriptSecurityManager
Nit: might want to put the scriptSecurityManager in a variable here?
::: toolkit/components/places/tests/unit/test_bookmarks_html_corrupt.js
@@ +102,5 @@
> Assert.equal(root.childCount, 3);
>
> + // For now some promises are resolved later, so we can't guarantee an order.
> + let foundLivemark = false;
> + for (let i = 0; i< root.childCount; ++i) {
Nit: i < root.childCount
::: toolkit/components/places/tests/unit/test_mozIAsyncLivemarks.js
@@ +235,5 @@
> , feedURI: FEED_URI
> });
> do_throw("Adding a livemark into a livemark should fail");
> + } catch(ex) {
> + do_check_eq(ex.result, Cr.NS_ERROR_INVALID_ARG);
Nit: trailing white space
@@ +451,5 @@
>
> + yield PlacesUtils.bookmarks.update({ guid: livemark.guid,
> + parentGuid: PlacesUtils.bookmarks.toolbarGuid,
> + index: PlacesUtils.bookmarks.DEFAULT_INDEX });
> + // Poll for the title change.
Nit: should update the comment.
@@ +468,4 @@
> , feedURI: FEED_URI } );
>
> + yield PlacesUtils.bookmarks.remove(livemark.guid);
> + // Poll for the title change.
Nit: should update the comment.
Attachment #8597141 -
Flags: review?(ttaubert) → review+
Comment 11•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Updated•10 years ago
|
QA Contact: andrei.vaida
Comment 12•9 years ago
|
||
This bug is verified fixed, following Smoke and Regression Testing performed on Beta 40.0b9 using Windows 10 Pro x64 (10240), Mac OS X 10.10.4 and Ubuntu 14.04 (x64). No major issues were found affecting the main functionality of Livemarks.
Two (2) new bugs were found during testing and filed separately: Bug 1190350 and Bug 1190354. They're still pending further investigation. Please note that these bugs might NOT be related to this patch/change.
You need to log in
before you can comment on or make changes to this bug.
Description
•