Closed Bug 1583749 Opened 1 year ago Closed 1 year ago

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

Categories

(DevTools :: General, task, P3)

task

Tracking

(firefox71 fixed)

RESOLVED FIXED
Firefox 71
Tracking Status
firefox71 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

Details

Attachments

(3 files)

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.
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED

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

Depends on: 1581536

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

Depends on D47051.

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

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?

Flags: needinfo?(ystartsev)

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

Pushed by jdescottes@mozilla.com:
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
Flags: needinfo?(ystartsev)

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

You need to log in before you can comment on or make changes to this bug.