Closed Bug 1308257 Opened 3 years ago Closed 3 years ago

Implement LayoutActor and GridActor to fetch all grid containers

Categories

(DevTools :: Inspector, defect, P3)

defect

Tracking

(firefox52 fixed)

RESOLVED FIXED
Firefox 52
Tracking Status
firefox52 --- fixed

People

(Reporter: gl, Assigned: gl)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 9 obsolete files)

9.56 KB, patch
pbro
: review+
Details | Diff | Splinter Review
18.32 KB, patch
Details | Diff | Splinter Review
The purpose of this bug is to do the following:
(1) Add a new CSSLayoutActor in server/actors/layout.js to get all grid containers (getAllGridContainers)
(2) Connect the CSSLayoutActor to the new layout panel
Blocks: dt-grid
Priority: -- → P3
Blocks: 1308260
Assignee: nobody → gl
Status: NEW → ASSIGNED
Attached patch 1308257.patch (obsolete) — Splinter Review
Running into some problems with how to return the grid container domnodes from the actor: 
17 INFO TEST-UNEXPECTED-FAIL | devtools/server/tests/browser/browser_layout_getAllGridContainers.js | Uncaught exception - at resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/fronts/inspector.js:134 - TypeError: ctx.marshallPool(...).ensureParentFront is not a function
Stack trace:
    NodeFront<.form@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/fronts/inspector.js:134:29
    types.addActorType/type<.read@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/protocol.js:282:7
    types.addArrayType/<.read/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/protocol.js:197:39
    types.addArrayType/<.read@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/protocol.js:197:23
    RetVal<.read@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/protocol.js:532:12
    Response<.read@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/protocol.js:700:12
    generateRequestMethods/</frontProto[name]/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/protocol.js:1366:17
    Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:937:23
    this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:816:7
    Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:747:11
    this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:779:7
    this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:714:7
    Front<.onPacket/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/protocol.js:1283:9
    DevTools RDP*Front<.request@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/protocol.js:1225:14
    generateRequestMethods/</frontProto[name]@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/protocol.js:1363:14
    @chrome://mochitests/content/browser/devtools/server/tests/browser/browser_layout_getAllGridContainers.js:9:21
    Tester_execTest@chrome://mochikit/content/browser-test.js:733:9
    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:653:7
    SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:743:59
    Tester_execTest@chrome://mochikit/content/browser-test.js:733:9
    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:653:7
    SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:743:59
SUITE-END | took 8s

I had to add a new reflow.js for the ReflowActor since you can only construct one actor using registerModule. So, the LayoutActor couldn't be constructed using the same method registerModule if it existed in the same file.
Attachment #8805897 - Flags: feedback?(pbrosset)
We've already had that problem a couple of times.

One architecture decision that was made at the very start was that the WalkerActor was the only actor that could manage NodeActors. The WalkerActor must be the parent of each and every NodeActor.

It creates them and sends them to the front-end. In the sending process, once NodeActors become NodeFronts on the front-end, something special happens that requires NodeFronts to have their parents be the WalkerFront, otherwise things don't work and you end up with "ensureParentFront" errors.

That's because NodeFronts assume their parent (in the sense of protocol.js actor parent) to be the WalkerFront (this happens here: http://searchfox.org/mozilla-central/rev/445097654c1cc1098ee0171a29c439afe363a588/devtools/shared/fronts/inspector.js#130-135)

This is important because the Walker maintains its list of known Nodes, and makes sure each of them is related to the root node and that the list of ancestors leading up to it is known too.

So, bottom line is, NodeActors can't be sent to the front-end by any actor, only the WalkerActor can.
We could work on making this a little bit easier maybe, but the WalkerActor would still need to be involved one way or another.

I can suggest 2 solutions:

1. Add a new method to the WalkerActor. We already have stuff like querySelectorAll, which returns a list of NodeActors essentially. We could introduce a getGridContainers method there too, and it would also simply return a list of NodeActors.
But, I'd prefer if we kept the WalkerActor as simple as possible and only about navigating the DOM tree, and moved domain-specific methods to other actors.
Also, if we did this, we'd have to still request data about each grid from the server at some stage.

2. Or, the CSSLayoutActor that you're introducing could have a getGrids method (instead of just getGridContainers). This method would return children actors: CSSGridActor which would contain a bunch of information about each grid layout found on the page.

Each CSSGridActor would come with a form that essentially gives you the content of what getGridFragments returns.
This is similar to what the AnimationActor does, it returns a collection of AnimationPlayerActors, one for each animation on the page, with data about them.
It's useful because you don't need to send an extra request to get information about the grid later (of course the grid definition can change over time, but CSSGridActors could send events with updated data when that happens).

Now, to get the NodeActor for the grid container, this could be done as a separate async request. That's where the WalkeActor comes in. Say CSSGridActor has a this.containerEl property, then you can do this on the front-end: walker.getNodeFromActor(cssGridFront.actorID, ["containerEl"]);

This will return a promise that resolves to the NodeFront, and then you can use DomNodePreview to render it in the UI.
This is also what the animation-inspector panel does.

There's one improvement you can make to this that avoids the extra async request.
On the server, in CSSGridActor, if you have a reference to the WalkerActor (which you do if you keep the setWalkeActor method you have added in your patch), then you can check if it knows about the element referenced by this.containerEl (I think the walker has a hasNode function or similar).
If it knows it, it means it has already come across it while walking the DOM, and it has already sent it to the front-end. So you could simply add its actorID to the form, and then on the client, you can use this ID to retrieve the NodeFront without requiring an extra server-side round-trip (see http://searchfox.org/mozilla-central/rev/445097654c1cc1098ee0171a29c439afe363a588/devtools/client/animationinspector/components/animation-target-node.js#44-56).

Needless to say I prefer option 2. But, again, maybe there's something we can do to make the whole process simpler somehow. Make it possible for other actors (with a reference to the WalkerActor) to send NodeActors directly and have them hooked up correctly to the right WalkerFront on the client-side. I haven't investigated this option though. Feel free to give it a shot. I don't think this should be too hard.
Comment on attachment 8805897 [details] [diff] [review]
1308257.patch

Review of attachment 8805897 [details] [diff] [review]:
-----------------------------------------------------------------

See comment 2.
Attachment #8805897 - Flags: feedback?(pbrosset)
Attachment #8805897 - Attachment is obsolete: true
Attachment #8805955 - Flags: review?(pbrosset)
Comment on attachment 8805955 [details] [diff] [review]
Part 1: Move ReflowActor to reflow.js

Review of attachment 8805955 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks.
Attachment #8805955 - Flags: review?(pbrosset) → review+
Keywords: leave-open
Attached patch 1308257-2.patch (obsolete) — Splinter Review
Attached patch 1308257-2.patch (obsolete) — Splinter Review
Attachment #8806360 - Attachment is obsolete: true
Attachment #8806655 - Flags: feedback?(pbrosset)
Attachment #8806655 - Attachment is obsolete: true
Attachment #8806655 - Flags: feedback?(pbrosset)
Attachment #8806661 - Flags: review?(pbrosset)
Comment on attachment 8806661 [details] [diff] [review]
Part 2: Implement LayoutActor and GridActor [1.0]

Review of attachment 8806661 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks this looks good.

::: devtools/server/actors/layout.js
@@ +5,5 @@
> +"use strict";
> +
> +const {Actor, ActorClassWithSpec} = require("devtools/shared/protocol");
> +const {gridSpec, layoutSpec} = require("devtools/shared/specs/layout");
> +const { stringifyGridFragments } = require("devtools/server/actors/utils/css-grid-utils");

Please be consistent in the file for object literals: {prop} or { prop }

@@ +7,5 @@
> +const {Actor, ActorClassWithSpec} = require("devtools/shared/protocol");
> +const {gridSpec, layoutSpec} = require("devtools/shared/specs/layout");
> +const { stringifyGridFragments } = require("devtools/server/actors/utils/css-grid-utils");
> +
> +const {NodeActor} = require("devtools/server/actors/inspector");

Why an extra line feed before this import?

@@ +12,5 @@
> +
> +/**
> + * Set of actors the expose the CSS layout information to the devtools protocol clients.
> + *
> + * The |Layout| actor is the main entry point. It is used to fetch all the grid containers

Right now, yes, it's only used to get all grid containers. But the plan is to expand its capabilities in the future. So there's a risk we forget to update that comment in the future and it going obsolete. I'd simply rephrase this to: "The |Layout| actor is the main entry point. It is used to get various CSS layout-related information from the document".

@@ +31,5 @@
> +   */
> +  initialize: function (layoutActor, containerEl, gridFragments) {
> +    Actor.prototype.initialize.call(this, layoutActor.conn);
> +
> +    this._containerEl = containerEl;

I don't think there's a a need to call this _containerEl rather than containerEl and have a getter that just returns it.
Getters are useful when extra logic is required, or when true private variables exist. This is not the case here and there's no need to make containerEl private.
So just removing the getter and calling this this.containerEl would work fine.

@@ +97,5 @@
> +    this.walker = null;
> +  },
> +
> +  /**
> +   * Sets an instance of the WalkerActor for the LayoutActor.

Please explain in this comment why it may sometimes be needed to set the walker actor. The comment should answer these questions: Is it mandatory? When should it be done? Why is it useful?

@@ +113,5 @@
> +   * @param  {Node|NodeActor} rootNode
> +   *         The root node to start iterating at.
> +   * @return {Array} An array of GridActor objects.
> +   */
> +  getGrids: function (rootNode) {

I think this function should be called getGridActors instead

@@ +127,5 @@
> +
> +      if (currentNode.getGridFragments && currentNode.getGridFragments().length > 0) {
> +        // Seralize the grid fragment data into JSON so protocol.js knows how to write
> +        // and read the data.
> +        let gridFragments = currentNode.getGridFragments();

I don't think you should do this here. I think the GridActor should do this when creating its form.
The reason is that grid definitions can change over time, and when they do, GridActors need to be aware of it and send an event with an update form. So GridActors live longer then a given grid fragments data. So I think constructing a GridActor should be done with GridActor(this, currentNode) only, and then the actor will execute the getGridFragments function and do the rest.

@@ +141,5 @@
> +  /**
> +   * Returns an array of GridActor objects for all existing grid containers found by
> +   * iterating below the given rootNode and optionally including nested frames.
> +   *
> +   * @param  {} rootNode

missing type information here {NodeActor}

::: devtools/server/actors/utils/css-grid-utils.js
@@ +18,5 @@
> +  if (fragments[0] && Cu.isDeadWrapper(fragments[0])) {
> +    return {};
> +  }
> +
> +  return JSON.stringify(fragments.map(getStringifiableFragment));

In layout.js, you are calling stringifyGridFragments and then doing JSON.parse on the result, which seems bad because we just did a JSON.stringify on the data you need.
So I think you should add a new function here:

function getStringifiableFragments(fragments = []) {
  if (fragments[0] && Cu.isDeadWrapper(fragments[0])) {
    return {};
  }
 
  return fragments.map(getStringifiableFragment);
}

And then, change stringifyGridFragments to be:

function stringifyGridFragments(fragments) {
  return JSON.stringify(getStringifiableFragments(fragments));
}

This way, in layout.js, you can call getStringifiableFragments directly and avoid doing JSON.parse

::: devtools/server/tests/browser/browser_layout_getAllGrids.js
@@ +3,5 @@
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +// Check the output of getAllGrids

Please complete this comment a bit: "Check the output of getAllGrids when the document contains only one grid in the root window".
And then please add a couple more test cases: one where there is a grid in the root window and another one a child frame (and check the output of the method when passing true, and then false to the second parameter). And one where there are multiple grids in the root window, and check that the grid fragments is correct.
Attachment #8806661 - Flags: review?(pbrosset)
I have moved the unit tests to Part 3.
Attachment #8806661 - Attachment is obsolete: true
Attachment #8806721 - Flags: review?(pbrosset)
Comment on attachment 8806721 [details] [diff] [review]
Part 2: Implement LayoutActor and GridActor [2.0]

Review of attachment 8806721 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for addressing my comments.
Just one last thing that I think we should do. See below.

::: devtools/server/actors/layout.js
@@ +103,5 @@
> +   *
> +   * @param {WalkerActor} walker
> +   *        The WalkerActor instance.
> +   */
> +  setWalkerActor: function (walker) {

I'm starting to wonder if we shouldn't make the LayoutActor a child of the WalkerActor in fact.
By child I mean, an actor that is returned by the WalkerActor, like so:
Somewhere in the WalkerActor:
getLayoutActor: function () {
  if (!this.layoutActor) {
    this.layoutActor = new LayoutActor(this.conn, tabActor, this);
  }
  return this.layoutActor;
}

This would help because the WalkerActor would be the one constructing the LayoutActor, so it could pass itself when doing so instead of requiring the front-end to do it.
What do you think?

We only ever need one instance of the LayoutActor, and in order to exist, it needs the WalkerActor anyway.
In fact, the fact that it uses getDocumentWalker isn't a strong requirement, it could just as well create its own document walker instance instead of using the one from WalkerActor. But the real reason why the LayoutActor may need the WalkerActor is when it starts sending information about DOM nodes to the front-end. Then it's very useful to check if they already exists in the WalkerActor in order to avoid having to attach and send them.

::: devtools/server/actors/utils/css-grid-utils.js
@@ +6,5 @@
> +
> +const { Cu } = require("chrome");
> +
> +/**
> + * Returns the grid fragment array with all the grid fragment data stringified.

s/stringified/stringifiable
In this function, nothing is stringified yet, it's just a plain object that can be stringified.

::: devtools/server/tests/browser/head.js
@@ +53,5 @@
>    let walker = yield inspector.getWalker();
>    let animations = AnimationsFront(client, form);
>    return {inspector, walker, animations, client};
>  }
> +function* initLayoutFrontForUrl(url) {

Since you moved the test to an other patch, then also move this function to the other patch.
Attachment #8806721 - Flags: review?(pbrosset)
Attachment #8806721 - Attachment is obsolete: true
Attachment #8807454 - Flags: review?(pbrosset)
Summary: Implement CSSLayout actor to fetch all grid containers → Implement LayoutActor and GridActor to fetch all grid containers
Comment on attachment 8807454 [details] [diff] [review]
Part 2: Implement LayoutActor and GridActor [3.0]

Review of attachment 8807454 [details] [diff] [review]:
-----------------------------------------------------------------

Alright, R+ assuming you address the final comments below. Basically 2 small cleanups, and 2 method renaming.

::: devtools/server/actors/inspector.js
@@ +2587,5 @@
> +   * information.
> +   *
> +   * @return {LayoutActor}
> +   */
> +  getLayoutActor: function () {

I'm a bit unsure about the function name. We usually don't have protocol methods with the word "Actor" in them. Actors and Fronts are just the technical way that things happen at the protocol level, but have nothing to do with the actual underlying functionality. 
Like the InspectorActor has a getWalker method, not a getWalkerActor method.
Having said this, getLayout sounds like a method that would just give you layout information about a node. Also, I think the PageStyleActor already has a getLayout method, so this would be confusing.
How about getCSSLayoutInspector, getCSSLayoutHelper, getCSSLayoutWalker? Open to suggestions.

::: devtools/server/actors/layout.js
@@ +82,5 @@
> +    this.walker = walker;
> +  },
> +
> +  /**
> +   * Since LayoutActor doesn't have a protocol.js parent actor that takes

This is not true anymore, the LayoutActor now has a parent: the WalkerActor. So you need to keep the destroy method, so you can cleanup after yourself, but the disconnect method isn't needed anymore.

::: devtools/server/main.js
@@ +511,5 @@
>        prefix: "reflow",
>        constructor: "ReflowActor",
>        type: { tab: true }
>      });
> +    this.registerModule("devtools/server/actors/layout", {

The layout actor isn't a top-level actor anymore, the walker is its parent. So this entry should not exist anymore.

::: devtools/shared/specs/layout.js
@@ +18,5 @@
> +const layoutSpec = generateActorSpec({
> +  typeName: "layout",
> +
> +  methods: {
> +    getAllGridActors: {

Same remark as in inspector.js, we usually don't have method names that contain the word "Actor". So, although the internal function getGridActors is fine, because that's something that's not visible at the protocol level, this method should probably be named just getAllGrids. Just like in animation.js getAnimationPlayersForNode is not named getAnimationPlayerActorsForNode.
Attachment #8807454 - Flags: review?(pbrosset) → review+
(In reply to Patrick Brosset <:pbro> from comment #17)
> Comment on attachment 8807454 [details] [diff] [review]
> Part 2: Implement LayoutActor and GridActor [3.0]
> 
> Review of attachment 8807454 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Alright, R+ assuming you address the final comments below. Basically 2 small
> cleanups, and 2 method renaming.
> 
> ::: devtools/server/actors/inspector.js
> @@ +2587,5 @@
> > +   * information.
> > +   *
> > +   * @return {LayoutActor}
> > +   */
> > +  getLayoutActor: function () {
> 
> I'm a bit unsure about the function name. We usually don't have protocol
> methods with the word "Actor" in them. Actors and Fronts are just the
> technical way that things happen at the protocol level, but have nothing to
> do with the actual underlying functionality. 
> Like the InspectorActor has a getWalker method, not a getWalkerActor method.
> Having said this, getLayout sounds like a method that would just give you
> layout information about a node. Also, I think the PageStyleActor already
> has a getLayout method, so this would be confusing.
> How about getCSSLayoutInspector, getCSSLayoutHelper, getCSSLayoutWalker?
> Open to suggestions.
> 
I chose to go with getLayoutInspector()
jryans, can you do a superreview to make sure everything we did with the actors are alright?
Attachment #8807454 - Attachment is obsolete: true
Attachment #8807552 - Flags: superreview?(jryans)
Attachment #8807552 - Flags: review+
Comment on attachment 8807552 [details] [diff] [review]
Part 2: Implement LayoutActor and GridActor [4.0]

Review of attachment 8807552 [details] [diff] [review]:
-----------------------------------------------------------------

Various small things, seems reasonable overall.

However, I am curious about the need for `release` here.  Is it truly required?  I'd like to know more about how it will be used.

::: devtools/server/actors/layout.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

.eslintignore currently ignores all actors by default.  Add an exception for this new file, so it's linted properly from the start.

@@ +33,5 @@
> +    this.containerEl = containerEl;
> +    this.walker = layoutActor.walker;
> +  },
> +
> +  disconnect: function () {

`disconnect` is only needed for top-level actors, it can be removed here since the parent is `LayoutActor`.

While we're on the subject, I just filed bug 1315391 to finally end this `disconnect` vs. `destroy` madness.

@@ +49,5 @@
> +  /**
> +   * Release the actor, when it isn't needed anymore.
> +   * Protocol.js uses this release method to call the destroy method.
> +   */
> +  release: function () {},

When do you expect to call `release` from the client?  Is it possible for the server to know when to destroy this instead of the client telling it to?  That would be a preferable design, if it is possible.

@@ +92,5 @@
> +  /**
> +   * Release the actor, when it isn't needed anymore.
> +   * Protocol.js uses this release method to call the destroy method.
> +   */
> +  release: function () {},

When do you expect to call `release` from the client?  Is it possible for the server to know when to destroy this instead of the client telling it to?  That would be a preferable design, if it is possible.

@@ +110,5 @@
> +    while (treeWalker.nextNode()) {
> +      let currentNode = treeWalker.currentNode;
> +
> +      if (currentNode.getGridFragments && currentNode.getGridFragments().length > 0) {
> +        let gridActor = GridActor(this, currentNode);

I think ESLint will require `new` with this constructor call.

::: devtools/shared/fronts/layout.js
@@ +7,5 @@
> +const { Front, FrontClassWithSpec } = require("devtools/shared/protocol");
> +const { gridSpec, layoutSpec } = require("devtools/shared/specs/layout");
> +
> +const GridFront = FrontClassWithSpec(gridSpec, {
> +  initialize: function (conn, form, detail, ctx) {

Pretty sure this isn't needed since you have no custom behavior here.

@@ +19,5 @@
> +    }
> +    this._form = form;
> +  },
> +
> +  destroy: function () {

This method also seems unnecessary.

@@ +32,5 @@
> +  }
> +});
> +
> +const LayoutFront = FrontClassWithSpec(layoutSpec, {
> +  initialize: function (conn, form, detail, ctx) {

I believe all methods here could be removed.

::: devtools/shared/specs/layout.js
@@ +14,5 @@
> +    release: { release: true },
> +  },
> +});
> +
> +const layoutSpec = generateActorSpec({

This spec doesn't define a `release` method to match the actor.  However, see the actor comments, it would be even better if you don't need it at all.
Attachment #8807552 - Flags: superreview?(jryans) → superreview-
Comment on attachment 8807552 [details] [diff] [review]
Part 2: Implement LayoutActor and GridActor [4.0]

Review of attachment 8807552 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/server/actors/layout.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Fixed. Added .eslintignore entry

@@ +33,5 @@
> +    this.containerEl = containerEl;
> +    this.walker = layoutActor.walker;
> +  },
> +
> +  disconnect: function () {

Fixed. Removed disconnect

@@ +49,5 @@
> +  /**
> +   * Release the actor, when it isn't needed anymore.
> +   * Protocol.js uses this release method to call the destroy method.
> +   */
> +  release: function () {},

Debugging this. I found that WalkerActor.destroy -> protocol.Actor.prototype.destroy.call(this) -> will destroy the LayoutActor and GridActor as well. This will also destroy the actors without having the release method. So, I have removed release() since the server does appear to know when to destroy these child actors by design.

@@ +92,5 @@
> +  /**
> +   * Release the actor, when it isn't needed anymore.
> +   * Protocol.js uses this release method to call the destroy method.
> +   */
> +  release: function () {},

See above.

@@ +110,5 @@
> +    while (treeWalker.nextNode()) {
> +      let currentNode = treeWalker.currentNode;
> +
> +      if (currentNode.getGridFragments && currentNode.getGridFragments().length > 0) {
> +        let gridActor = GridActor(this, currentNode);

Fixed. Added 'new' to the constructor call.

::: devtools/shared/fronts/layout.js
@@ +7,5 @@
> +const { Front, FrontClassWithSpec } = require("devtools/shared/protocol");
> +const { gridSpec, layoutSpec } = require("devtools/shared/specs/layout");
> +
> +const GridFront = FrontClassWithSpec(gridSpec, {
> +  initialize: function (conn, form, detail, ctx) {

Not fully sure what you meant here, but I removed form, detail, and ctx here since they are null.

@@ +19,5 @@
> +    }
> +    this._form = form;
> +  },
> +
> +  destroy: function () {

Fixed. Removed destroy method.

@@ +32,5 @@
> +  }
> +});
> +
> +const LayoutFront = FrontClassWithSpec(layoutSpec, {
> +  initialize: function (conn, form, detail, ctx) {

Removed form, detail, ctx

::: devtools/shared/specs/layout.js
@@ +14,5 @@
> +    release: { release: true },
> +  },
> +});
> +
> +const layoutSpec = generateActorSpec({

Should be fixed since we removed release.
Attachment #8807552 - Attachment is obsolete: true
Attachment #8807856 - Flags: superreview?(jryans)
Attachment #8807856 - Flags: review+
Comment on attachment 8807856 [details] [diff] [review]
1308257-2.patchPart 2: Implement LayoutActor and GridActor [5.0]

Review of attachment 8807856 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for looking into `release`, good to see it's not needed.

::: devtools/server/actors/layout.js
@@ +106,5 @@
> +  /**
> +   * Returns an array of GridActor objects for all existing grid containers found by
> +   * iterating below the given rootNode and optionally including nested frames.
> +   *
> +   * @param  {NoodeActor} rootNode

Nit: Noode -> Node

::: devtools/shared/fronts/layout.js
@@ +7,5 @@
> +const { Front, FrontClassWithSpec } = require("devtools/shared/protocol");
> +const { gridSpec, layoutSpec } = require("devtools/shared/specs/layout");
> +
> +const GridFront = FrontClassWithSpec(gridSpec, {
> +  initialize: function (conn) {

The initialize method here is not overridden with a specific implementation for `GridFront`, instead it only calls the default implementation defined on `Front`[1].  Since you have not customized it, you should be able to remove it from here and everything should still work.

[1]: http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/devtools/shared/protocol.js#1149

@@ +28,5 @@
> +  }
> +});
> +
> +const LayoutFront = FrontClassWithSpec(layoutSpec, {
> +  initialize: function (conn) {

The initialize method here is not overridden with a specific implementation for `LayoutFront`, instead it only calls the default implementation defined on `Front`[1].  Since you have not customized it, you should be able to remove it from here and everything should still work.

It looks like `FrontClassWithSpec` requires _something_ as the second arg, so I guess just `{}` would do.

[1]: http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/devtools/shared/protocol.js#1149

::: devtools/shared/specs/layout.js
@@ +10,5 @@
> +const gridSpec = generateActorSpec({
> +  typeName: "grid",
> +
> +  methods: {
> +    release: { release: true },

Remove `release` here, it no longer exists.
Attachment #8807856 - Flags: superreview?(jryans) → superreview+
Attachment #8807856 - Attachment is obsolete: true
Attachment #8808573 - Flags: review+
Attachment #8808573 - Attachment is obsolete: true
Attachment #8808579 - Flags: review+
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.