Closed Bug 1157253 Opened 5 years ago Closed 5 years ago

Port ListenerProxy to use Proxy instead of __noSuchMethod__

Categories

(Testing :: Marionette, defect)

defect
Not set
normal

Tracking

(firefox40 fixed)

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: ato, Assigned: ato)

Details

(Keywords: pi-marionette-server)

Attachments

(1 file, 1 obsolete file)

ListenerProxy currently uses the recently deprecated __noSuchMethod__ trick to accept any method called on an instance of itself.  We need to move to use the new ES6 Proxy concept.
Attached file MozReview Request: bz://1157253/ato (obsolete) —
/r/7509 - Bug 1157253: Port ListenerProxy to use Proxy instead of __noSuchMethod__

Pull down this commit:

hg pull -r 6eb95acb67fe1ee61b87bb775853b7e397b8f8b1 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8596630 - Flags: review?(cmanchester)
Comment on attachment 8596630 [details]
MozReview Request: bz://1157253/ato

https://reviewboard.mozilla.org/r/7507/#review6327

::: testing/marionette/driver.js:105
(Diff revision 1)
> + *
> + * @return {Promise}
> + *     A promise that can be yielded to retrieve await and get its value.

This comment seems out of date.

::: testing/marionette/driver.js
(Diff revision 1)
> -    let msg;
> -    if (args.length == 1 && typeof args[0] == "object" && args[0] !== null) {
> -      msg = args[0];
> -    } else {
> -      msg = Array.prototype.slice.call(args);

Something in here was needed... try is complaining a lot about what look like calls into the listener never coming back.

::: testing/marionette/driver.js:115
(Diff revision 1)
> +
> +  let handler = {
> +    get: (obj, prop) => args => this.propagate.bind(obj)(prop, args)
> +  }
> +  return new Proxy(this, handler);

Is returning a value from a constructor considered good style? I find this weird, but if it's guaranteed to be transparent to callers I guess it's ok. Elsewhere we're doing "listener.curCmdId ="... is that proxy, not the object, now?
Attachment #8596630 - Flags: review?(cmanchester)
Assignee: nobody → ato
Status: NEW → ASSIGNED
https://reviewboard.mozilla.org/r/7507/#review6369

> This comment seems out of date.

Oh sorry, this was meant to go under #propagate.
Comment on attachment 8596630 [details]
MozReview Request: bz://1157253/ato

/r/7509 - Bug 1157253: Port ListenerProxy to use Proxy instead of __noSuchMethod__

Pull down this commit:

hg pull -r e055f1762239c654a8ab8177f5317a36de27e806 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8596630 - Flags: review?(cmanchester)
https://reviewboard.mozilla.org/r/7507/#review6373

> Something in here was needed... try is complaining a lot about what look like calls into the listener never coming back.

I was trying to be too clever for my own good here.  Added back this logic in a slightly altered form since the `args` argument to `ListenerProxy#propagate` isn't an `Arguments` object anymore, but an `Array`.
Comment on attachment 8596630 [details]
MozReview Request: bz://1157253/ato

https://reviewboard.mozilla.org/r/7507/#review6393

Try is still failing, and there's still an open issue from the last review.
Attachment #8596630 - Flags: review?(cmanchester)
Comment on attachment 8596630 [details]
MozReview Request: bz://1157253/ato

/r/7509 - Bug 1157253: Port ListenerProxy to use Proxy instead of __noSuchMethod__

Pull down this commit:

hg pull -r 7b9a013df6c2963ebb7766802b8e4d9b2ab6b83a https://reviewboard-hg.mozilla.org/gecko/
Attachment #8596630 - Flags: review?(cmanchester)
https://reviewboard.mozilla.org/r/7507/#review6455

> Is returning a value from a constructor considered good style? I find this weird, but if it's guaranteed to be transparent to callers I guess it's ok. Elsewhere we're doing "listener.curCmdId ="... is that proxy, not the object, now?

Agree with this and implemented the changes we discussed on IRC Friday evening.
Comment on attachment 8596630 [details]
MozReview Request: bz://1157253/ato

https://reviewboard.mozilla.org/r/7507/#review6467

::: testing/marionette/driver.js:106
(Diff revision 3)
>  let ListenerProxy = function(mmFn, sendAsyncFn, curBrowserFn) {
> +  let sender = new CurrentContentSender(mmFn, sendAsyncFn, curBrowserFn);

We should stop using "new" where this is invoked, it's more like a factory now.

::: testing/marionette/driver.js:124
(Diff revision 3)
> + * Calls to content space using this sender are guaranteed to block.

This comment is slightly misleading. Maybe just state that the promise returned is guaranteed not to resolve until the conten command completes?

::: testing/marionette/driver.js:162
(Diff revision 3)
> + *     A promise that can be yielded to retrieve await and get its value.

I think there's a typo in here... "A promise that resolves to the result of the command." would be clearer.

::: testing/marionette/driver.js:116
(Diff revision 3)
> + * The CurrentContentSender allows one to make synchronous calls to the
> + * message listener of the content frame of the current browsing context.

I would just call it "ContentSender", but up to you.

Looks good!
Attachment #8596630 - Flags: review?(cmanchester) → review+
https://reviewboard.mozilla.org/r/7507/#review6471

::: testing/marionette/driver.js:126
(Diff revision 3)
> + * @param {function(): nsIMessageManager} mmFn
> + *     Function returning the current message manager.

It's a little funny to see nsIMessageManager here because there isn't literally an interface called that. We either have a nsIMessageSender or nsIMessageBroadcaster here.
https://hg.mozilla.org/mozilla-central/rev/c30b5528ce96
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Attachment #8596630 - Attachment is obsolete: true
Attachment #8620109 - Flags: review+
You need to log in before you can comment on or make changes to this bug.