1094900 - Livemarks service should use the new Bookmarks.jsm API

Status: VERIFIED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

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.

Comment 1

[Mike Hoye [:mhoye]]

Are there two starter bugs here? Make the tests asynchronous, then the service?

Comment 2

[Marco Bonardo [:mak]]

yes, but we are not ready for the conversion yet, we are still working on dependencies before we can start with this work.

Comment 3

[Marco Bonardo [:mak]]

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.

Comment 4

[Marco Bonardo [:mak]]

Steven, did you already start work here? I was evaluating to steal this bug :)

Comment 5

[Steven MacLeod [: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.

Comment 6

[Marco Bonardo [:mak]]

Sounds good, thanks for the quick reply.

Comment 7

[Marco Bonardo [:mak]]

I will try to do bug 1145650 at the same time, since it's related.

Comment 8

[Marco Bonardo [:mak]]

Attachment: 1145650.diff

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

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/6126a2cc97c6

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6126a2cc97c6

Comment 12

[Andrei Vaida [:avaida]]

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.