857082 - TabTarget.makeRemote doesn't need any arguments

Status: RESOLVED FIXED

(DevTools :: Framework, defect, P2)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

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.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v1

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

Comment 2

[Joe Walker [:jwalker]]

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?

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v2

(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 4

[Joe Walker [:jwalker]]

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.

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v3

(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.

Comment 6

[Joe Walker [:jwalker]]

(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.

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

(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.

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v4

Is this what you had in mind?

Comment 9

[Joe Walker [:jwalker]]

(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.

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/integration/fx-team/rev/89f2c56f3caf

Comment 11

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/mozilla-central/rev/89f2c56f3caf