1450960 - Convert AddonConsoleActor to protocol.js

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P2)

Description

[Yulia Startsev [:yulia]]

Needs to be updated to protocol.js as part of the work to update BrowserAddonActor

Comment 1

[Yulia Startsev [:yulia]]

Attachment: Bug 1450960 - split AddonConsole into its own file;

Review commit: https://reviewboard.mozilla.org/r/242688/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/242688/

Comment 2

[Yulia Startsev [:yulia]]

Attachment: Bug 1450960 - Convert AddonConsoleActor to protocol.js;

Review commit: https://reviewboard.mozilla.org/r/242690/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/242690/

Comment 3

[Yulia Startsev [:yulia]]

Comment on attachment 8974348 [details]
Bug 1450960 - Convert AddonConsoleActor to protocol.js;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242690/diff/1-2/

Comment 4

[Alexandre Poirot [:ochameau]]

Comment on attachment 8974348 [details]
Bug 1450960 - Convert AddonConsoleActor to protocol.js;

https://reviewboard.mozilla.org/r/242690/#review248534

Thanks Yulia, it looks like a good iteration.
Please address the actorPrefix/typeName issue before landing.

We should followup on:
* having more explicit specs,
* convert childs longstrings to protocol.js

::: devtools/server/actors/addon-console.js:38
(Diff revision 2)
>   *        The parent BrowserAddonActor actor.
>   */
> -function AddonConsoleActor(addon, connection, parentActor) {
> +addonConsolePrototype.initialize = function(addon, connection, parentActor) {
>    this.addon = addon;
> +  Actor.prototype.initialize.call(this, connection);
>    WebConsoleActor.call(this, connection, parentActor);

I think that once WebConsoleActor is refactored to protocol.js, we will have to remember removing the call to Actor.prototype.initialize here.

::: devtools/server/actors/addon-console.js:42
(Diff revision 2)
> -
> -update(AddonConsoleActor.prototype, {
> -  constructor: AddonConsoleActor,
>  
> -  actorPrefix: "addonConsole",
> +update(addonConsolePrototype, {
> +  typeName: "addonConsole",

Unfortunately, this is not going to be picked up as the "actorPrefix" set by WebConsoleActor prototype is going to be preferred over "typeName", per:
https://searchfox.org/mozilla-central/source/devtools/server/actors/common.js#238

Do you think we can override actorPrefix to null somehow? (we should then remember to remove this override once the WebConsoleActor is also refactored to protocol.js)

::: devtools/server/actors/addon-console.js:60
(Diff revision 2)
>  
>    /**
>     * Destroy the current AddonConsoleActor instance.
>     */
>    destroy() {
>      WebConsoleActor.prototype.destroy.call(this);

You should also call Actor.prototype.destroy here.

::: devtools/shared/specs/webconsole.js:43
(Diff revision 2)
> +      request: {
> +        messageTypes: Option(0, "array:string"),
> +      },
> +      // the return value here has a field "string" which can either be a longStringActor
> +      // or a plain string. Since we do not have union types, we cannot fully type this
> +      // response

Note that this uses old fashion long string actors.
When switching to protocol.js's version, you will have to type it correctly in order to mashall them correctly. Otherwise, it will just try to convert them to JSON and fail with a "cycle value" exception.
(I know that because I just got into that while working on netmonitor actors)

::: devtools/shared/specs/webconsole.js:55
(Diff revision 2)
> +        frameActor: Option(0, "string"),
> +        url: Option(0, "string"),
> +        selectedNodeActor: Option(0, "string"),
> +        selectedObjectActor: Option(0, "string"),
> +      },
> +      response: RetVal("json")

You should try to type this if possible, or open a  followup to focus on just that. This one may be complex and could depend on typing grips correctly.

::: devtools/shared/specs/webconsole.js:60
(Diff revision 2)
> +      response: RetVal("json")
> +    },
> +    evaluateJSAsync: {
> +      request: {},
> +      response: RetVal("json")
> +    },

I think we can better type this as it is only an object with a `resultID` attribute being set to `Date().now()`.

::: devtools/shared/specs/webconsole.js:95
(Diff revision 2)
> +    }
> +  },
> +  events: {
> +    "evaluationResult": {
> +      response: RetVal("json")
> +    }

There is other kind of events like: `pageError`, `logMessage`, `consoleAPICall`, `networkEvent`, `fileActivity`, `reflowActivity`, `lastPrivateContextExited`.

But it will only be useful to mention events in the spec like this, once you are going to convert the call to sendActorEvent, as well as manual packet sending like this one:
https://searchfox.org/mozilla-central/source/devtools/server/actors/webconsole.js#1520-1534
to:
  this.emit("logMessage", {...});

Given that the addon console doesn't emit any event itself, we can wait for a later patch to specify them.

Comment 5

[Alexandre Poirot [:ochameau]]

Comment on attachment 8974347 [details]
Bug 1450960 - split AddonConsole into its own file;

https://reviewboard.mozilla.org/r/242688/#review249900

Oops, I forgot to r+ this simple patch.

Comment 6

[Yulia Startsev [:yulia]]

Comment on attachment 8974348 [details]
Bug 1450960 - Convert AddonConsoleActor to protocol.js;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242690/diff/2-3/

Comment 7

[Yulia Startsev [:yulia]]

Comment on attachment 8974348 [details]
Bug 1450960 - Convert AddonConsoleActor to protocol.js;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242690/diff/3-4/

Comment 8

[Yulia Startsev [:yulia]]

Comment on attachment 8974348 [details]
Bug 1450960 - Convert AddonConsoleActor to protocol.js;

https://reviewboard.mozilla.org/r/242690/#review248534

> I think that once WebConsoleActor is refactored to protocol.js, we will have to remember removing the call to Actor.prototype.initialize here.

will add this to the todolist here: https://bugzilla.mozilla.org/show_bug.cgi?id=1452920

> You should try to type this if possible, or open a  followup to focus on just that. This one may be complex and could depend on typing grips correctly.

follow up will be finished here: https://bugzilla.mozilla.org/show_bug.cgi?id=1461715

> There is other kind of events like: `pageError`, `logMessage`, `consoleAPICall`, `networkEvent`, `fileActivity`, `reflowActivity`, `lastPrivateContextExited`.
> 
> But it will only be useful to mention events in the spec like this, once you are going to convert the call to sendActorEvent, as well as manual packet sending like this one:
> https://searchfox.org/mozilla-central/source/devtools/server/actors/webconsole.js#1520-1534
> to:
>   this.emit("logMessage", {...});
> 
> Given that the addon console doesn't emit any event itself, we can wait for a later patch to specify them.

i will remove this for now since we dont need it yet

Comment 9

[Yulia Startsev [:yulia]]

Comment on attachment 8974348 [details]
Bug 1450960 - Convert AddonConsoleActor to protocol.js;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242690/diff/4-5/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/979280d2d611
split AddonConsole into its own file; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/931d5cd50662
Convert AddonConsoleActor to protocol.js; r=ochameau

Comment 11

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/979280d2d611
https://hg.mozilla.org/mozilla-central/rev/931d5cd50662