Closed Bug 1383622 Opened 7 years ago Closed 7 years ago

Remove use of sync bookmarks API from TPS

Categories

(Firefox :: Sync, enhancement, P3)

Product:
Firefox
Component:
Sync
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: markh, Assigned: markh)

References

(
URL
)

Details

Attachments

(2 files)

Bug 1383622 (part 1) - convert most of TPS to async/await.
59 bytes, text/x-review-board-request
tcsc
: review+
Details
Bug 1383622 (part 2) - update TPS to use the async bookmark API.
59 bytes, text/x-review-board-request
tcsc
: review+
Details 
TPS still uses the sync bookmarks API - we should convert it to use the async API
Priority: -- → P3
Assignee: nobody → markh
Comment on attachment 8897254 [details]
Bug 1383622 (part 1) - convert most of TPS to async/await.

https://reviewboard.mozilla.org/r/168538/#review173796

This has been a long time coming, glad to see it happening.

::: services/sync/tps/extensions/tps/resource/tps.jsm:283
(Diff revision 1)
>        this._operations_pending--;
>      }
>      if (!this.operations_pending) {
>        this._currentAction++;
>        Utils.nextTick(function() {
> -        this.RunNextTestAction();
> +        this.RunNextTestAction().catch(err => {this.DumpError("RunNextTestActionFailed", err)});;

nit: Can you space this out with whitespace or newlines?

::: services/sync/tps/extensions/tps/resource/tps.jsm:1093
(Diff revision 1)
> -   * Synchronously wait for the named event to be observed.
> +   * Wait for the named event to be observed.
> -   *
> -   * When the event is observed, the function will wait an extra tick before
> -   * returning.
>     *
>     * Note that in general, you should probably use createEventWaiter unless you

nit: comment refers to deleted/renamed function createEventWaiter
Attachment #8897254 - Flags: review?(tchiovoloni) → review+
Comment on attachment 8897255 [details]
Bug 1383622 (part 2) - update TPS to use the async bookmark API.

https://reviewboard.mozilla.org/r/168540/#review173800

As with the other patch, this is a much needed change that's great to see. I have a comple concerns in the bookmark.jsm code, but I don't think any of them matter if the tests pass. 

(For example, I vaguely recall that SetPosition has weird behavior for it's index, due to http://searchfox.org/mozilla-central/source/toolkit/components/places/nsINavBookmarksService.idl#450-452, but it's entirely possible update works the same way)

::: services/sync/tps/extensions/tps/resource/tps.jsm:1221
(Diff revision 1)
>    skipValidation() {
>      TPS.shouldValidateAddons = false;
>    }
>  };
>  
>  var Bookmarks = {

These need to be async now, and should await HandleBookmarks. You already have `await action[0].apply(this, action.slice(1));` (which is the caller) in RunNextTestAction so that should be it.
Attachment #8897255 - Flags: review?(tchiovoloni) → review+
Comment on attachment 8897255 [details]
Bug 1383622 (part 2) - update TPS to use the async bookmark API.

I've no idea what went wrong, but that last patch was incomplete and didn't work at all. Here's a better one which also has a change that causes TPS to wait until the default browser bookmarks have been created, which was causing a race.
Attachment #8897255 - Flags: review+
Comment on attachment 8897255 [details]
Bug 1383622 (part 2) - update TPS to use the async bookmark API.

(I have no idea what I'm doing:)
Attachment #8897255 - Flags: review?(tchiovoloni)
Comment on attachment 8897255 [details]
Bug 1383622 (part 2) - update TPS to use the async bookmark API.

https://reviewboard.mozilla.org/r/168540/#review174690

Still looks fine.

::: services/sync/tps/extensions/tps/resource/modules/bookmarks.jsm:138
(Diff revisions 1 - 2)
> -        if (type == null || type == undefined || node.type == type)
> -          if (uri == undefined || uri == null || node.url.spec == uri.spec)
> -            return node.guid;
> +        let nodeType = this._typeMap.get(node.type);
> +        if (type == null || type == undefined || nodeType == type)
> +          if (uri == undefined || uri == null || node.uri.spec == uri.spec) {
> +            // Note that this is suspect as we return the *last* matching
> +            // child, which some tests rely on (ie, an early-return here causes
> +            // at least 1 test to fail). But that's a yak for another day.

Horrifying. It wouldn't surprise me if this were a bug though.

::: services/sync/tps/extensions/tps/resource/modules/bookmarks.jsm:378
(Diff revisions 1 - 2)
>     * Updates the position of this places item within this item's current
>     * folder.  Use SetLocation to change folders.
>     *
>     * @param position The new index this item should be moved to; if null,
>     *        no changes are made; if -1, this item is moved to the bottom of
> -   *        the current folder
> +   *        the current folder. Otherwise, must be a string which is the

Ah, ok. That makes the original code make much more sense.
Attachment #8897255 - Flags: review?(tchiovoloni) → review+
Pushed by mhammond@skippinet.com.au:
https://hg.mozilla.org/integration/autoland/rev/d229587489da
(part 1) - convert most of TPS to async/await. r=tcsc
https://hg.mozilla.org/integration/autoland/rev/60005e2d9ea5
(part 2) - update TPS to use the async bookmark API. r=tcsc
I really need to install that eslint outgoing hook :( Fix coming up
Flags: needinfo?(markh)
Pushed by mhammond@skippinet.com.au:
https://hg.mozilla.org/integration/autoland/rev/492d8960d85b
(part 1) - convert most of TPS to async/await. r=tcsc
https://hg.mozilla.org/integration/autoland/rev/aab3a105009e
(part 2) - update TPS to use the async bookmark API. r=tcsc
https://hg.mozilla.org/mozilla-central/rev/492d8960d85b
https://hg.mozilla.org/mozilla-central/rev/aab3a105009e
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: