ChromeActor should be updated to the new protocol.js It is instantiated via the root actor using: RootActor.getProcess(0)

I suppose this might be the first actor we've tried converting to protocol.js which extends from another actor? I can't recall an existing p.js actor with that setup, at least. Anyway, it sounds like Alex and Yulia have worked it out how to do it from today's meeting notes.

For clarity, here are my notes on the progression of this bug: - protocol.js refactor is very difficult due to the prototypical inheritance, TabActor methods would disappear for reasons that were unclear to me. - I found two different paths which lead to errors. The first leads to an error where `docShell` is not defined by the child (meaning that the tabActor is overriding the child). This happens when the inheritance is done via `Object.create(TabActor.prototype)`. Second error relates to `_shouldHaveGlobalDebuggee` being undefined. In this case TabActor seems to be empty. This occurs when merging TabActor with the chromActorPrototype `{...TabActor, ...chromeActorPrototype}`. this might be related to the lazy getters. The solution that Alex found: - if an actor is inheriting methods from another actor, such as the case between ChromeActor and TabActor, the methods from TabActor have to be exposed in the ChromeActor’s spec. - TabActor had to be updated so that the onAttach method is now `attach` - Another important detail for inheriting from other actors: you need to use `getOwnPropertyDescriptors` on the actor that you are inheriting from, because otherwise you break the lazy getters. See https://hg.mozilla.org/try/rev/79426ca7b98dbb42fae1286b0521c703e733343b#l2.35

I update main.js and protocol.js with a few warnings for if people use the wrong object when declaring a request type. it turns out that the following is not valid: > { > request: Option(0, "json"), > response: RetVal("json") > } but this is: > { > request: { > options: Option(0, "json") > }, > response: RetVal("json") > } Because when the request object is created, it has a type field based on the name of the method being declared. However, this is overwritten by the type present in the return value of Option or Arg. See here: https://searchfox.org/mozilla-central/source/devtools/shared/protocol.js#1063 . This is also used by the source spec, to overwrite onSource, though I am not sure it is a good idea to keep it that way. Next, I ran into a new issue - webextension extends chrome actor. this means that we now need to inherit from a protocol actor. this introduces another problem with inheritance: protocol.js throws if the same actorProto is instantiated twice. I am not sure what the thinking is behind this, maybe :ochameau or :jryans have further insights? https://searchfox.org/mozilla-central/source/devtools/shared/protocol.js#1098 removing this got me a little further, however there are other things that are no longer instantiated -- namely extra actors.

First of all, thanks for exploring these dark corners! We're covering some new ground here with protocol.js, so I am not surprised to see things like this pop up. Hopefully it is not discouraging! (In reply to Yulia Startsev [:yulia] from comment #9) > I update main.js and protocol.js with a few warnings for if people use the > wrong object when declaring a request type. it turns out that the following > is not valid: > > > { > > request: Option(0, "json"), > > response: RetVal("json") > > } > > but this is: > > > { > > request: { > > options: Option(0, "json") > > }, > > response: RetVal("json") > > } > > Because when the request object is created, it has a type field based on the > name of the method being declared. However, this is overwritten by the type > present in the return value of Option or Arg. See here: > https://searchfox.org/mozilla-central/source/devtools/shared/protocol. > js#1063 . It does seem like current conventions in p.js expect the request template to be object of Args, Options, etc. instead of a single Arg / Option directly. Testing for the unexpected thing and throwing seems useful. > This is also used by the source spec, to overwrite onSource, > though I am not sure it is a good idea to keep it that way. Hmm! I am not sure why the source spec was done that way. Perhaps it was just a conversion mistake? It seems like naming the method `source` and removing the type like elsewhere would be the same. I filed bug 1454827. > Next, I ran into a new issue - webextension extends chrome actor. this means > that we now need to inherit from a protocol actor. this introduces another > problem with inheritance: protocol.js throws if the same actorProto is > instantiated twice. I am not sure what the thinking is behind this, maybe > :ochameau or :jryans have further insights? > > https://searchfox.org/mozilla-central/source/devtools/shared/protocol.js#1098 At a basic level, I think protocol.js today assumes you _aren't_ using prototypal inheritance. This check that you hit is trying to guard for the case of creating the _same_ actor twice somehow. So, we could remove the check. However, we'll still need to check carefully that we get usable request handlers for methods in the leaf actor as well the parent actor it extends via prototype. We should also test overriding request methods that the parent had: when a request is received, does it go to the leaf actor? Is there a way for the leaf actor to call the parent's implementation? I can imagine there will be many little bits like this that need tweaking for actor inheritance. > removing this got me a little further, however there are other things that > are no longer instantiated -- namely extra actors. That's part of tab actor I guess? Would it potentially be easier to convert TabActor first, and then move to ones like this that extends from TabActor? (I am not sure.)

(In reply to Yulia Startsev [:yulia] from comment #9) > I update main.js and protocol.js with a few warnings for if people use the > wrong object when declaring a request type. it turns out that the following > is not valid: > > > { > > request: Option(0, "json"), > > response: RetVal("json") > > } > > but this is: > > > { > > request: { > > options: Option(0, "json") > > }, > > response: RetVal("json") > > } > > Because when the request object is created, it has a type field based on the > name of the method being declared. However, this is overwritten by the type > present in the return value of Option or Arg. See here: > https://searchfox.org/mozilla-central/source/devtools/shared/protocol. > js#1063 . This is also used by the source spec, to overwrite onSource, > though I am not sure it is a good idea to keep it that way. Note that I may tweak Request.write in bug 1454899. I would like to remove the nested arguments in request object and throw error if we get anything else but an Arg or Option properties. If you happen to tweak protocol.js, please do it at least in another changeset or another bug if that is significant change. Otherwise, yes, let's remove this cryptic usage of request in source.js! > Next, I ran into a new issue - webextension extends chrome actor. this means > that we now need to inherit from a protocol actor. this introduces another > problem with inheritance: protocol.js throws if the same actorProto is > instantiated twice. I am not sure what the thinking is behind this, maybe > :ochameau or :jryans have further insights? As :jryans said, protocol.js isn't designed for inheritance *AT ALL*! You may be able to workaround that by using a WeakMap to store the specs. (But feel free to use another approach if you find any alternative) const actorSpecs = new WeakMap(); // WeakMap[Actor.prototype => Spec] var ActorClassWithSpec = function(actorSpec, actorProto) { ... cls.prototype = extend(Actor.prototype, generateRequestHandlers(actorSpec, cls.prototype)); actorSpecs.set(cls.prototype, actorSpec); // So you would need to remove _actorSpec check and set from generateRequestHandlers var Actor = function(conn) { Pool.call(this, conn); this._actorSpec = actorsSpec.get(Object.getPrototypeOf(this)); ... (This is pseudo code, you may tweak all that) > https://searchfox.org/mozilla-central/source/devtools/shared/protocol.js#1098 > > removing this got me a little further, however there are other things that > are no longer instantiated -- namely extra actors. I don't know how you do to attach two mozreview to one bug, but here you should be able to attach the two patches in a single mozreview. With git I do that by specifying a changeset range: git mozreview push f4d9dd8571eb9dd3d745b5a6a9e55595231236f6..3e43705c9fef7dd07be428a95f4d5403d340fc65 or git mozreview push f4d9dd8571eb9dd3d745b5a6a9e55595231236f6..HEAD (if my patch queue are the current branch and are the latest) Otherwise I applied your patches and I get a working Browser toolbox!! (FYI, your patches need to be rebase) Where do you see an error with extra actors?

Further issues with inheritance - webextension inherits from both chromeActor and tabActor, however in my attempts to get it to inherit in a similar way to chromeActor, i was unsuccessful, as it would still be missing a method: https://bugzilla.mozilla.org/show_bug.cgi?id=1450948 the diff can be found here: https://hg.mozilla.org/try/rev/9b5a1d858fc63123c92d2935d8460a303fe2d0a0 I am going ahead and continuing to update these two as a single item, and think about how we might do inheritance in a less complex way in the future.. or if we even need inheritance for these - we are already directly calling a number of the methods and this process of copying objects is undermining what protocol inheritance should be doing, which is sharing memory.

Comment on attachment 8970541 [details] fix types in tab.js https://reviewboard.mozilla.org/r/239306/#review244988 ::: devtools/shared/specs/tab.js:111 (Diff revision 1) > }, > response: {} > }, > switchToFrame: { > request: { > - request: Arg(0, "tab.switchtoframerequest") > + windowId: Arg(0, "string") changing this to Option will make the test pass

I have added two files with a passing and failing case, so as to document what i found about the request formatting for protocol.js. I focused on logInPage in actors/tab.js -- there is a log statement which logs the name of the function and the arguments passed to it the following can result in a silent error: ``` request: Arg(0, "json") ``` So does this: ``` request: Arg(0, "tab.loginpage") ``` Both of these tests pass, however they are not doing what is expected. logInPage is never called You can see the details in failing case.zip -- some more details: logInPage is never called because type is overwritten in protocol.js, i am working on a solution to this, but this is why it happens the following does work: ``` request: { text: Option(0, "string"), category: Option(0, "string"), flags: Option(0, "string") } ``` this is good to know for future refactorings

Comment on attachment 8970226 [details] Bug 1450948 - Convert ChromeActor to protocol.js https://reviewboard.mozilla.org/r/239024/#review244660 Thanks a lot Yulia, this patch is looking great. I only have a concern about `_actorSpec` in protocol.js. I would feel more confortable to address the underlying issue rather than only removing the exeption. ::: devtools/shared/specs/tab.js:9 (Diff revision 1) > +"use strict"; > + > +const {types, generateActorSpec, RetVal, Arg, Option} = require("devtools/shared/protocol"); > + > +types.addDictType("tab.attach", { > + type: "string", ::: devtools/server/actors/chrome.js:40 (Diff revision 3) > * The connection to the client. > */ > -function ChromeActor(connection) { > + > +/* We are creating a prototype object rather than a class here, so in order > + * to inherit from the TabActor, we extend from a normal object, and apply the > + * properties of the TabActor prototype In this comment you are describing "the what" rather than "the why". It is a bit like paraphrasing the code. It would be helpful to mention that we do this in order to prevent messing up with getters. ::: devtools/server/actors/tab.js (Diff revision 3) > if (!this.docShell) { > // The tab is already closed. > return null; > - } > + } return this.docShell.allowJavascript; > - > - return this.docShell.allowJavascript; There is a surprising modification here. ::: devtools/shared/protocol.js (Diff revision 3) > */ > var generateRequestHandlers = function(actorSpec, actorProto) { > - if (actorProto._actorSpec) { > - throw new Error("actorProto called twice on the same actor prototype!"); > - } > - Landing this looks like introducing a foot gun in protocol.js that could hurt us later on. That would be great to find a proper way to address this. I tried to suggest one way to fix that in comment 11. You can address that in a distinct changeset. ::: devtools/shared/specs/tab.js:9 (Diff revision 3) > +"use strict"; > + > +const {types, generateActorSpec, RetVal, Option} = require("devtools/shared/protocol"); > + > +types.addDictType("tab.attach", { > + type: "string", nit: This refactoring is interesting. It highlights the cruft of TabActor. This `type` attribute is misleading as `type` should be a reserved keyword. It may end up being considered as an event if the value set on `type` attribute matches an event name. `tabAttached` is only used in test and doc: https://searchfox.org/mozilla-central/search?q=tabAttached&case=false&regexp=false&path=devtools I'm not sure it is any useful. But keep this patch focused on 1:1 refactoring. I'm mentioning it as an opportunity to mention possible followup cleanups. ::: devtools/shared/specs/tab.js:23 (Diff revision 3) > + type: "nullable:string", > +}); > + > +types.addDictType("tab.switchtoframerequest", { > + windowId: "string" > +}); This is no longer used. ::: devtools/shared/specs/tab.js:43 (Diff revision 3) > + title: "string", > +}); > + > +types.addDictType("tab.workers", { > + error: "nullable:string", > + from: "nullable:string", `from` is part of RDP. This don't have to set this property. The transport layer is going to do that for you. So, here we should not have to specify `form` in the spec. And while refactoring tab.js we can stop returning `form` property. Also, this method isn't correctly typed. It should contain a definition of `workers`. You may start with `workers: "array:json",` for now. No test are failing as we never try to list worker from Chrome actor, nor WebExtension one. ::: devtools/shared/specs/tab.js:61 (Diff revision 3) > + > +types.addDictType("tab.loginpage", { > + text: "string", > + category: "string", > + flags: "string" > +}); This is no longer used.

Comment on attachment 8970226 [details] Bug 1450948 - Convert ChromeActor to protocol.js https://reviewboard.mozilla.org/r/239024/#review245356

Comment on attachment 8970872 [details] Bug 1450948 - collect actorSpecs in a weakmap. https://reviewboard.mozilla.org/r/239642/#review245344 Thanks for your first contribution to protocol.js :) ::: commit-message-4003f:1 (Diff revision 1) > + Bug 1450948 - collect actorSpecs in a weakmap. r=ochameau It looks like there is a space prefix before "Bug" ::: devtools/shared/protocol.js:1208 (Diff revision 1) > }; > cls.prototype = extend(Actor.prototype, generateRequestHandlers(actorSpec, actorProto)); > > + if (actorSpecs.get(cls.prototype)) { > + throw new Error("actorProto called twice on the same actor prototype!"); > + } This error isn't so easy to keep. `cls.prototype` is always going to be a different object here. `extend()` will always return a new object. If we want to keep logging this error we would have to have another weakmap whose keys are `actorProto`... I'm not sure it is worth the effort?

Comment on attachment 8970226 [details] Bug 1450948 - Convert ChromeActor to protocol.js https://reviewboard.mozilla.org/r/239024/#review244660 > nit: This refactoring is interesting. It highlights the cruft of TabActor. > This `type` attribute is misleading as `type` should be a reserved keyword. It may end up being considered as an event if the value set on `type` attribute matches an event name. > `tabAttached` is only used in test and doc: > https://searchfox.org/mozilla-central/search?q=tabAttached&case=false&regexp=false&path=devtools > I'm not sure it is any useful. > But keep this patch focused on 1:1 refactoring. > I'm mentioning it as an opportunity to mention possible followup cleanups. i will try to do this in the tab refactoring

Pushed by ystartsev@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f03bcb0ba0b4 Convert ChromeActor to protocol.js r=ochameau https://hg.mozilla.org/integration/autoland/rev/902a66d98a6a collect actorSpecs in a weakmap. r=ochameau

https://hg.mozilla.org/mozilla-central/rev/f03bcb0ba0b4 https://hg.mozilla.org/mozilla-central/rev/902a66d98a6a