1492434 - Simplify the code around target.isBrowsingContext

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement, P2)

Description

[Alexandre Poirot [:ochameau]]

On the client side, we do have to make a clear distinctions between target actors that inherit from BrowsingContextActor from others.
The one that inherit from it can debug documents. It enables various features like the inspector, accessibility panels, the frame switcher, ...

But for now, the way we distinguish the two set of target actors is clunky.
The main issue is that the distinction has to be done on the client side, when calling TargetFactory.forRemoteTab. 
  https://searchfox.org/mozilla-central/search?q=isBrowsingContext%3A&case=false&regexp=false&path=
But in these callsites, we don't necessarily know if the target actor is a browsing context. It is ultimately defined in the server, depending on what kind of actor it returns.
We could potentially look at the target actor's id and guess what kind of actor it is, but that feels weak.
It may be more robust to have a "isBrowsingContext" trait being set on the actor's form.
The only issue with the traits is that is will only work with recent runtimes and not the previous ones, for which we will have to fallback on some other mechanism...

On top of that, the behavior of isBrowsingContext argument passed to TabTarget is quite complex for no valid reasons. It was complex only to support old xul addons which no longer exists:
https://searchfox.org/mozilla-central/rev/a0333927deabfe980094a14d0549b589f34cbe49/devtools/client/framework/target.js#136-141
Investigation on bug 1492265 proved that this argument was hard to follow.

Simplifying this particular flag is going to help merging TabClient with TabTarget so I'm blocking but 1465635.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1492434 - Computes isBrowsingContext from TabTarget out of actor's form

Instead of requiring all TargetFactory.forRemoteTab to compute isBrowsingContext flag,
we can compute it out of the actor's form as we know which types are browsing context
inherited or not.

MozReview-Commit-ID: H5zLo5nZSm6

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1492434 - Convert _getContentProcessTarget to async in order to correctly forward rejections

MozReview-Commit-ID: 50HYZyUkflN

Depends on D7413

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1492434 - Use a trait to differenciate browsing context target actors

MozReview-Commit-ID: Gp0cjWDHCou

Depends on D7414

Comment 4

[Alexandre Poirot [:ochameau]]

* This first patch only moves isBrowsingContext computation from forRemoteTab callsites to TabTarget constructor.
It still involves client side computation as we would like to keep this computation until all the server support the new trait.
I pushed to try with just this changeset in order to ensure it works fine:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=60dd3ff9c79c7b9b4ef18cc6290ea064fb7bc17e
(I also tried manually by opening all kinds of toolboxes)

* The second patch isn't really related to this bug, but help seeing rejections happening during browser content toolbox creation. Without that, exception happening from TabTarget constructor are silent.

* The third patch introduces the traits and should help us to cleanup the client codebase once 64 is the last officially supported remote runtime. I used `isBrowsingContext: !!this.docShell`, but we may use `isBrowsingContext: true` as I'm trying to avoid using the ParentProcessTargetActor for xpcshell and so avoid having a BrowsingContextTargetActor without a browsing context...

Comment 5

[Alexandre Poirot [:ochameau]]

First try push, with the two first patches:
  https://hg.mozilla.org/try/rev/60dd3ff9c79c7b9b4ef18cc6290ea064fb7bc17e
(testing the client-code-only codepath)

And for third patch, to test the traits:
  https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=202864565&revision=5d5004a4c3d010a16ff8add0ea82954afaacb74c

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/79065fca7cc1
Computes isBrowsingContext from TabTarget out of actor's form r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/9cd5d357528d
Convert _getContentProcessTarget to async in order to correctly forward rejections r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/9368d2715723
Use a trait to differenciate browsing context target actors r=jdescottes

Comment 7

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/79065fca7cc1
https://hg.mozilla.org/mozilla-central/rev/9cd5d357528d
https://hg.mozilla.org/mozilla-central/rev/9368d2715723

Comment 8

[Chris Peterson [:cpeterson]]

Retroactively moving fixed bugs whose summaries mention "Fission" (or other Fission-related keywords) but are not assigned to a Fission Milestone to an appropriate Fission Milestone.

This will generate a lot of bugmail, so you can filter your bugmail for the following UUID and delete them en masse:

0ee3c76a-bc79-4eb2-8d12-05dc0b68e732