1583749 - Remove webextension descriptor front "connect" API and cleanup backward compat code

Status: RESOLVED FIXED

(DevTools :: General, task, P3)

Description

[Julian Descottes [:jdescottes]]

Looking at the backward compat code from Bug 1575557, it seems we need to perform some cleanup:

• the code relies on a trait isDescriptor which doesn't exist anywhere
• we keep exposing connect on the webextension front/spect, but it's only used by a webextension test that we can update. All the client code should use getTarget
• the actor still defines a connect() method that maps to getTarget(). Since we don't support forward compatibility we should not expect old clients to connect to this actor.

Comment 1

[Julian Descottes [:jdescottes]]

Blocked on Bug 1581536 to avoid touching deprecated files during the cleanup

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: Bug 1583749 - Remove unnecessary trait webExtensionAddonConnect

Depends on D47050. Trait was added in 2017, all servers should now have support this by default

Comment 3

[Julian Descottes [:jdescottes]]

Attachment: Bug 1583749 - Add isDescriptor trait to webextension descriptor actor

Depends on D47051.

Without the trait, we keep calling the connect() wrapper on the actor, which is supposed to be deprecated

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: Bug 1583749 - Stop exposing deprecated connect() api on the webextension descriptor front

Depends on D47053

Comment 5

[Julian Descottes [:jdescottes]]

I noticed a difference between the webextension descriptor and the frame/process descriptors. For frame and process, the front is responsible for "caching" the result of getTarget and always return the same target when you call the method several times on a front. Code looks more or less like:

async getTarget() {
  if (this._targetFront && this._targetFront.actorID) {
    // already has the front 
    return this._targetFront;
  }
  if (this._targetFrontPromise) {
    // already requested the front
    return this._targetFrontPromise;
  }
  this._targetFrontPromise = (async () => {
    try {
      // try to get the front
      // [...]
      return this._targetFront;
    } catch (e) {
      // log some error
      // [...]
      return null;
    }
  })();
  return this._targetFrontPromise;
}

But the webextension one is not doing that. Instead this is handled by the webextension descriptor actor: https://searchfox.org/mozilla-central/rev/45f30e1d19bde27bf07e47a0a5dd0962dd27ba18/devtools/server/actors/descriptors/webextension.js#98-117
(and here, the descriptor actors for frame and processes don't do any "caching" since it's handled by the front).

Is there a reason why we don't want to have the same pattern for all three descriptors?

Comment 6

[Julian Descottes [:jdescottes]]

Meanwhile, try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2aaeb315002f22b44b4476d1c9177297f13cb60 for the current patches. Also tested backward against Fx 69, can connect to addons without problems

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b5e6eb959127
Remove unnecessary trait webExtensionAddonConnect r=ochameau
https://hg.mozilla.org/integration/autoland/rev/a649c4da961c
Add isDescriptor trait to webextension descriptor actor r=yulia
https://hg.mozilla.org/integration/autoland/rev/7a3383771d07
Stop exposing deprecated connect() api on the webextension descriptor front r=yulia,rpl

Comment 8

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/b5e6eb959127
https://hg.mozilla.org/mozilla-central/rev/a649c4da961c
https://hg.mozilla.org/mozilla-central/rev/7a3383771d07

Comment 9

[Yulia Startsev [:yulia] OOO until July 2026]

There isn't a reason that we do it differently in the three cases. I think I did it this way to keep it close to the original behavior, we should probably make it the same everywhere