1515862 - Review protocol.js form handling

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement, P3)

Description

[Alexandre Poirot [:ochameau]]

Protocol.js is currently implementing a complex API around form handling.
Fronts may overload form method.
This form method is then called whenever the backend refers to its related actor.
Whenever we return an actor on the server side, we don't pass only its actor ID, but it's "form".
The form includes the actor ID and optional JSON values set by the actor.
So form isn't passed only the first time the actor is returned, but everytime we return a reference to it.
This is the main reason why form isn't only a constructor argument.
And this is actually a key difference with chrome's CDP protocol (but that's another story).

What is still to be reviewed are the two last argument of the form method: detail and ctx
https://searchfox.org/mozilla-central/rev/9528360768d81b1fc84258b5fb3601b5d4f40076/devtools/shared/protocol.js#342
https://searchfox.org/mozilla-central/rev/9528360768d81b1fc84258b5fb3601b5d4f40076/devtools/shared/protocol.js#1305
  front.form(v, detail, ctx);

Here is all the overloads of form method:
https://searchfox.org/mozilla-central/search?q=form(&case=false&regexp=false&path=devtools%2Fshared%2Ffronts

The second argument "detail" looks like is only used to implement an "actorid" case, which can very likely be factored into protocol.js Pool or Front.
The third argument "ctx" seems to be only used by NodeFront in order to instantiate child node fronts and be able to called a WalkerFront method.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1515862 - Remove form passed as Front constructor.

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1515862 - Remove protocol.js's formType.

None of our actor define a formType in its specification.
This feature has never been used, so remove it and its test.

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1515862 - Remove unecessary usages of actorid checks in actor and front's form method.

Remove all actorid checks in actors and fronts as we never pass a defined detail argument in them.
Only domstylerule and node are using detail and that, always with detail="actorid".
Also remove ctx and detail in Front constructor as that's not used by these two actors.

Depends on D17612

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1515862 - Remove detail feature from protocol.js.

This feature is only used by node and domrulestyle actors and that, only for actor ID.
Instead of this, support this "pass only actor ID rather than full form" feature
in a more dedicated way.

We might followup on that to clarify/simplify doing this: passing only the actor ID
rather than its full form.

Depends on D17613

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1515862 - Set global and target scope front IDs from getFront helper.

Ideally, formAttributeName would be a field on the prototype of each front,
but unfortunately, Firefox doesn't support ES Class's fields yet. So it is
put as an instance attribute instead.

This patch streamline the manually set actorID and the retrieval of actor ID
from root or target front's form into getFront helper method.
So that all the specific related to the lazy actor is now inside of this helper.
It also moves the special "this.manage(this)" to this helper.
We might be able to followup on this one and finally have only the root front
being self managed. But the initialize method makes it hard.

Note the special tweak made to devtools/client/responsive.html/manager.js
Hopefully this can be removed once the target classes are merged.

Depends on D15832

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1515862 - Remove Front's form argument.

Now that form argument is no longer used by any front to set its actor ID,
we can remove this argument.

Have a particular look at:

• devtools/client/shared/test/test-actor-registry.js
  which was the last Front to be manually instantiated and need some tweaks,
• canvas head.js to create canvas front via getFront,
• RDM manager.js, which requires the EmulationFront to be self managed.

Depends on D17615

Comment 7

[Alexandre Poirot [:ochameau]]

Hopefully green try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e863e640153dea6eab3fad70253f53cf151fd13e

Note that all intermediate revision are having a green try, I'm just not sure about the very last one.

Please have a first overall look on the whole patch queue as I'm cleaning up the same thing multiple time, so that intermediate state may look imperfect.

It is great to see that bug 1503628 should help us use Target.getFront in RDM and simplify the special case of Emulation Front in it!

Here is the overall story of this patch queue:

• remove unused formType
• remove detail argument from form() methods. It was only used with detail=actorid. I kept this usecase, but it is now hardcoded into protocol.js. I wish we could followup on this particular subject as I believe that should be the default behavior and we shouldn't have implicit form update. This is what makes the Front very stateful and introduce confusion and complexity!
• rebased previous patch to remove the call to form() from Front constructor
• Simplify the story around lazy actor by moving this.manage(this) from each front constructor to protocol.js:getFront and set the actorID from protocol.js:getFront rather than depending on Front constructor to do it
• This allows to drop completely the form argument from Front constructor \o/ This helps highlighting all the places where we were manually instantiating a front. It now requires some more manual work, but ideally we should avoid doing that!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8d0fc09f446c
Remove protocol.js's formType. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/e51c403a53dd
Remove unecessary usages of actorid checks in actor and front's form method. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/41c84424b5f5
Remove detail feature from protocol.js. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/b974bf8f0374
Remove form passed as Front constructor. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/96d4f1748866
Set global and target scope front IDs from getFront helper. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/936be7433144
Remove Front's form argument. r=jdescottes

Comment 9

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d90632282c2544c92956e19942cb0d9e7afe60e5

Comment 10

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/8d0fc09f446c
https://hg.mozilla.org/mozilla-central/rev/e51c403a53dd
https://hg.mozilla.org/mozilla-central/rev/41c84424b5f5
https://hg.mozilla.org/mozilla-central/rev/b974bf8f0374
https://hg.mozilla.org/mozilla-central/rev/96d4f1748866
https://hg.mozilla.org/mozilla-central/rev/936be7433144