Closed
Bug 1157253
Opened 9 years ago
Closed 9 years ago
Port ListenerProxy to use Proxy instead of __noSuchMethod__
Categories
(Remote Protocol :: Marionette, defect)
Remote Protocol
Marionette
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.
Assignee | ||
Updated•9 years ago
|
Keywords: ateam-marionette-server
Assignee | ||
Comment 1•9 years ago
|
||
/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)
Assignee | ||
Comment 2•9 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebfde6c2a725
Comment 3•9 years ago
|
||
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 | ||
Updated•9 years ago
|
Assignee: nobody → ato
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•9 years ago
|
||
https://reviewboard.mozilla.org/r/7507/#review6369 > This comment seems out of date. Oh sorry, this was meant to go under #propagate.
Assignee | ||
Comment 5•9 years ago
|
||
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)
Assignee | ||
Comment 6•9 years ago
|
||
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`.
Assignee | ||
Comment 7•9 years ago
|
||
New try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4165822b4ea6
Comment 8•9 years ago
|
||
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)
Assignee | ||
Comment 9•9 years ago
|
||
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)
Assignee | ||
Comment 10•9 years ago
|
||
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.
Assignee | ||
Comment 11•9 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce28266cce80
Comment 12•9 years ago
|
||
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+
Comment 13•9 years ago
|
||
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.
Comment 15•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c30b5528ce96
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Assignee | ||
Comment 16•9 years ago
|
||
Attachment #8596630 -
Attachment is obsolete: true
Attachment #8620109 -
Flags: review+
Assignee | ||
Comment 17•9 years ago
|
||
Updated•1 year ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•