Closed Bug 1094900 Opened 5 years ago Closed 4 years ago

Livemarks service should use the new Bookmarks.jsm API

Categories

(Toolkit :: Places, defect)

defect
Not set
Points:
3

Tracking

()

VERIFIED FIXED
mozilla40
Iteration:
40.2 - 27 Apr
Tracking Status
firefox40 --- verified

People

(Reporter: mak, Assigned: mak)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
Flags: qe-verify+
Flags: firefox-backlog+
Are there two starter bugs here? Make the tests asynchronous, then the service?
Flags: needinfo?(mak77)
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)
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]
Assignee: nobody → smacleod
Status: NEW → ASSIGNED
Iteration: --- → 39.2 - 23 Mar
Blocks: 1145650
Iteration: 39.2 - 23 Mar → 39.3 - 30 Mar
Iteration: 39.3 - 30 Mar → 40.1 - 13 Apr
Iteration: 40.1 - 13 Apr → ---
Steven, did you already start work here? I was evaluating to steal this bug :)
Flags: needinfo?(smacleod)
(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)
Sounds good, thanks for the quick reply.
Mentor: mak77
Whiteboard: [good next bug][lang=js]
I will try to do bug 1145650 at the same time, since it's related.
No longer blocks: 1145650
Depends on: 1145650
Iteration: --- → 40.2 - 27 Apr
No longer depends on: 1145650
Blocks: 1145653
Attached patch 1145650.diffSplinter Review
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 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+
https://hg.mozilla.org/mozilla-central/rev/6126a2cc97c6
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
QA Contact: andrei.vaida
Depends on: 1159381
Depends on: 1159346
Depends on: 1158887
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1194568
You need to log in before you can comment on or make changes to this bug.