makeRemote makes an existing target remote, so there is never a need to supply the (optional) options object that it expects, since the caller would have already used it to instantiate the target.

Well, that wasn't so hard. All tests pass locally and connecting to b2g works fine.

Comment on attachment 732349 [details] [diff] [review] Patch v1 Review of attachment 732349 [details] [diff] [review]: ----------------------------------------------------------------- I wonder if we should also add TargetFactory.forRemotedTab(options) -> promise(target) so we can begin the process of moving over? It would be fairly easy to do that here, I think. Thoughts? Also is forRemotedTab the best name?

(In reply to Joe Walker [:jwalker] from comment #2) > I wonder if we should also add TargetFactory.forRemotedTab(options) -> > promise(target) so we can begin the process of moving over? Good idea, I like that. > Also is forRemotedTab the best name? How about forRemoteTab, since this is not a regular tab that was remoted, but a remote one from the beginning?

Comment on attachment 732490 [details] [diff] [review] Patch v2 Review of attachment 732490 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/framework/Target.jsm @@ +60,5 @@ > + let deferred = Promise.defer(); > + target = new TabTarget(options); > + targets.set(options, target); > + target.makeRemote().then( () => deferred.resolve(target) ); > + return deferred.promise; I think we can use propagation from https://addons.mozilla.org/en-US/developers/docs/sdk/latest/modules/sdk/core/promise.html to simplify this to: let target = targets.get(options); if (target != null) { return Promise.resolve(target); } target = new TabTarget(options); targets.set(options, target); return target.makeRemote().then(() => target); However, I think there's a bug here where 2 people call this twice, in quick succession, and the second gets a resolved promise to a target that has not finished remoting yet, but where the contract says it should have. If we were to store promises to targets in 'targets' then we could safely do: let promise = targets.get(options); if (promise == null) { let target = new TabTarget(options); promise = target.makeRemote().then(() => target); targets.set(options, promise); } return promise; But that might require us to change other code? Perhaps this is getting too complex and we should do it in a follow-up.

(In reply to Joe Walker [:jwalker] from comment #4) > let promise = targets.get(options); > if (promise == null) { > let target = new TabTarget(options); > promise = target.makeRemote().then(() => target); > targets.set(options, promise); > } > return promise; > > But that might require us to change other code? Perhaps this is getting too > complex and we should do it in a follow-up. Nah, this is only used in a single place (connect.js) and works fine.

(In reply to Panos Astithas [:past] from comment #5) > > But that might require us to change other code? Perhaps this is getting too > > complex and we should do it in a follow-up. > > Nah, this is only used in a single place (connect.js) and works fine. I meant with something like this: let tab = ... TargetFactory.forRemoteTab(tab).then(...); let target = TargetFactory.forTab(tab); // Uses target only to discover that it's a promise. The solution, I think, is to have another WeakMap that holds promises to targets. forTab() then works as normal, but forRemoteTab() does: * if there is a promise for the tab in promiseTargets, return it. Done. * if don't have a target for the tab in targets, then create one. * create a promise for the remoting of the target, store it in promiseTargets and then return it.

(In reply to Joe Walker [:jwalker] from comment #6) > The solution, I think, is to have another WeakMap that holds promises to > targets. forTab() then works as normal, but forRemoteTab() does: > > * if there is a promise for the tab in promiseTargets, return it. Done. > * if don't have a target for the tab in targets, then create one. I agree with the rest, but this step doesn't make much sense to me. There will never be a target for the remote tab in |targets|, because the |options| object that corresponds to the remote tab is not a XUL tab and we are now going to explicitly separate the weakmaps according to object type (for values, but consequently also for keys). > * create a promise for the remoting of the target, store it in > promiseTargets and then return it. Doing (1) and (3) only works for me.

Is this what you had in mind?

(In reply to Panos Astithas [:past] from comment #7) > (In reply to Joe Walker [:jwalker] from comment #6) > > The solution, I think, is to have another WeakMap that holds promises to > > targets. forTab() then works as normal, but forRemoteTab() does: > > > > * if there is a promise for the tab in promiseTargets, return it. Done. > > * if don't have a target for the tab in targets, then create one. > > I agree with the rest, but this step doesn't make much sense to me. There > will never be a target for the remote tab in |targets|, because the > |options| object that corresponds to the remote tab is not a XUL tab and we > are now going to explicitly separate the weakmaps according to object type > (for values, but consequently also for keys). > > > * create a promise for the remoting of the target, store it in > > promiseTargets and then return it. > > Doing (1) and (3) only works for me. I guess I'm looking ahead to when there is only one type of target. That's ok for now.