Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Description • 7 years ago

The sidebar setExpression API method should behave differently from inspectedWindow.eval API method when the value returned by the expression is a DOM object or a cyclic object value:

while inspectedWindow.eval should raise a protocol error when the result of the evaluation is not a JSON serializable object, the seidebar setExpression should support the inspection of this results in the registered inspector sidebar.

To support this behavior the following changes are needed:

- the inspectedWindow actor should optionally return a Grip of the evaluated results instead of a JSON representation of the entire object, by using a Grip we will be able to return a Remote Debugging "object actor" which can be used to inspect DOM nodes and cyclic objects

- the sidebar setExpression method should request the result to be returned as a Grip on its call to the RDP inspectedWindow actor's eval method

- the ExtensionSidebar defined inside 'devtools/client/inspector/extensions/' should support an additional "remote-object-treeview" mode which integrated an ObjectInspector (http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/devtools/client/shared/components/reps/reps.js#4713, which is the same component internally used by the debugger and the new console panel for the same purpose)

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 6 • 7 years ago
mozreview-review

Comment on attachment 8917895 [details]
Bug 1403130 - Support ObjectInspector-based object value grip view in ExtensionSidebar

https://reviewboard.mozilla.org/r/188826/#review194134

::: devtools/client/inspector/extensions/components/ObjectValueGripView.js:80
(Diff revision 2)
> +    // TODO: Once the ObjectInspector is mounted for the first time, two warnings are logged
> +    // in the console:
> +    // - Warning: getInitialState was defined on Tree, a plain JavaScript class.
> +    //   This is only supported for classes created using React.createClass.
> +    //   Did you mean to define a state property instead?
> +    //
> +    //   logged from react-dev.js:22807:9 for the Tree component defined in the reps module)
> +    //
> +    // - Error: addComponentAsRefTo(...): Only a ReactOwner can have refs.
> +    //   You might be adding a ref to a component that was not created inside a component's `render`
> +    //   method, or you have multiple copies of React loaded
> +    //   (details: https://fb.me/react-refs-must-have-owner).
> +    //
> +    //   logged from react-dev.js:22473:15 for the same Tree component defined in the reps module)
> +    //
> +    // Once the ObjectInspector instance is mounted, the updates to the store doesn't seem
> +    // to be able to force the extension sidebar to be re-rendered as expected.
> +    //
> +    // To verify that this issues are only happening when we mount an ObjectInspector,
> +    // uncomment the following line:
> +    //
> +    // return dom.pre(null, JSON.stringify(objectValueGrip, null, 2));
> +    return ObjectInspector(objectInspectorProps);

Hi Nicolas,
like I anticipated over IRC, these are an initial version of the patches where I'd like to integrate the ObjectInspector (these are not yet finalized, but I pushed the current version of them here on mozreview so that you can be able to reproduce the issue locally).

In this comment (temporarily added to the patch itself) there are more details about the logged React warnings and the issue with the re-rendering on the updates applied to the store.

In like 101 there is a commented line that, once uncommented, renders a pre tab instead of the ObjectInspector, you can try it to verify that the issues are only happening when the ObjectInspector is mounted in the tree of react elements.

(I'm also going to attach a small test extension which creates a sidebar panel that can be used to easily reproduce the issue).

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 7 • 7 years ago

Hi Nicolas,

This is a small extension which creates an inspector sidebar named "TestPanel", and updates the sidebar using the sidebar.setExpression API method on every inspector "onSelectChanged" event.

Using this extension, and the patches attached in mozreview, you will be able to reproduce locally the issues that we briefly discussed over IRC today.

In Comment 6 there are some additional notes and the code fragment that integrates the ObjectInspector.

	Nicolas Chevobbe [:nchevobbe]
	Comment 8 • 7 years ago

This is looking good, ping me for review again when the new reps bundle land :)
One thing to keep in mind, as said on irc : between each "evaluation", we should release the actor so we don't fill up the server memory (where actors are kept).

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 12 • 7 years ago

Patches rebased on a recent mozilla-central tip (currently waiting for https://github.com/devtools-html/devtools-core/pull/814 to be included in an updated devtools-reps bundle).

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 19 • 6 years ago
mozreview-review

Comment on attachment 8917895 [details]
Bug 1403130 - Support ObjectInspector-based object value grip view in ExtensionSidebar

https://reviewboard.mozilla.org/r/188826/#review218144

::: devtools/client/inspector/extensions/components/ObjectValueGripView.js:14
(Diff revision 5)
> +  createFactory,
> +  PropTypes,
> +  Component,
> +} = require("devtools/client/shared/vendor/react");
> +
> +const Accordion = createFactory(require("devtools/client/inspector/layout/components/Accordion"));

Hi :gl,
I'd like to give you some additional context about the Accordion React component imported here: 

The extension sidebar API supports an optional title for the rendered content, using the Accordion component (the one used by the layout sidebar) is the nicer approach that I've currently found (and it also provide a UI element with a look and feel consistent with the other inspector sidebars).

Reusing the Accordion component already works correctly, but I'm not really happy about importing it from the layout sidebar dir, I'd like to move it into a dir that would make it clear that it is a shared component, how about moving it into "devtools/client/inspector/shared/components" dir?

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 20 • 6 years ago

Hi :nchevobbe,

I've added you are an additional reviewer on the second patch from the mozreview patch queue attached to this issue, 
for the pieces related to the usage of the ObjectInspector react component (which I confirm that in the last reps bundle works like a charm, thanks!).

	Nicolas Chevobbe [:nchevobbe]
	Comment 23 • 6 years ago
mozreview-review

Comment on attachment 8917895 [details]
Bug 1403130 - Support ObjectInspector-based object value grip view in ExtensionSidebar

https://reviewboard.mozilla.org/r/188826/#review218208

Things are looking good for the ObjectInspector side.
I find the test a bit lengthy and doing a lot of things, so maybe splitting it would be nice.

::: devtools/client/inspector/extensions/components/ObjectValueGripView.js:63
(Diff revision 6)
> +          value: objectValueGrip,
> +        }
> +      }],
> +      createObjectClient: serviceContainer.createObjectClient,
> +      releaseActor: serviceContainer.releaseActor,
> +      // TODO: evaluate if there should also be a serviceContainer.openLink.

Not a requirement for now but that would be nice (and not much work I think).

We could also have an `onViewSourceInDebigger` props (https://searchfox.org/mozilla-central/rev/7fb999d1d39418fd331284fab909df076b967ac6/devtools/client/webconsole/new-console-output/utils/object-inspector.js#66), which when provided, shows an icon to jump to the definition of a logged function (and can be handy).

::: devtools/client/inspector/extensions/reducers/sidebar.js:33
(Diff revision 6)
> +  [EXTENSION_SIDEBAR_OBJECT_GRIP_VIEW_UPDATE](
> +    sidebar, {sidebarId, objectValueGrip, rootTitle}
> +  ) {
> +    // Update the sidebar to a "object-treeview" which shows
> +    // the passed object.
> +    return Object.assign({}, sidebar, {

not really in the scope of this review, but you might want to use the object spread operator which gives us more terse code than Object.assign : 

```js
return {
  ...sidebar,
  [sidebarId]: {
    viewMode: "object-value-grip-view",
    objectValueGrip,
    rootTitle,
  }
}
```

::: devtools/client/inspector/extensions/test/browser_inspector_extension_sidebar.js:250
(Diff revision 6)
> +  EventUtils.synthesizeMouseAtCenter(nodeOpenInspector, {type: "mousedown"}, view);
> +  EventUtils.synthesizeMouseAtCenter(nodeOpenInspector, {type: "mouseup"}, view);

does nodeOpenInspector.click() works ?

	Andrew Swan [:aswan]
	Comment 24 • 6 years ago
mozreview-review

Comment on attachment 8917896 [details]
Bug 1403130 - Allow DOMNodes and cyclic objects to be rendered with the sidebar.setExpression API method.

https://reviewboard.mozilla.org/r/188828/#review218372

The actual API bits look fine to me.  And the test looks fine to the extent that I understand it, but it has a ton of detail about the structure of devtools.  Can you get a devtools peer to sign off on that part?  ie, is embedding all those selectors in tests okay or do they want to abstract some of that away through some sort of devtools test utils module?

	Alexandre Poirot [:ochameau]
	Comment 25 • 6 years ago
mozreview-review

Comment on attachment 8917895 [details]
Bug 1403130 - Support ObjectInspector-based object value grip view in ExtensionSidebar

https://reviewboard.mozilla.org/r/188826/#review218556

::: devtools/client/inspector/extensions/extension-sidebar.js:100
(Diff revision 6)
> +              front, "inspector-extension-sidebar"
> +            );
> +
> +            return Promise.all([onNodeFrontSet, onInspectorUpdated]);
> +          }
> +        },

This look very generic and nothing seem specific to WebExtension here. Could this be shared with devtools/console/toolbox codebase?

	Alexandre Poirot [:ochameau]
	Comment 26 • 6 years ago
mozreview-review

Comment on attachment 8917894 [details]
Bug 1403130 - Support result as value grip on WebExtension InspectorWindow actor eval method.

https://reviewboard.mozilla.org/r/188824/#review218562

Looks good to me, thanks Luca.

::: devtools/server/actors/webextension-inspected-window.js:514
(Diff revision 4)
> +                exceptionInfo: {
> +                  isError: true,
> +                  code: "E_PROTOCOLERROR",
> +                  description: "Inspector protocol error: %s",
> +                  details: [
> +                    "Unexpected invalid sidebar panel expression request"

Don't you want to return a more explicit error message or is there any security concern?
From this message it is hard to understand there is something wrong with toolbox / console actor.

	Gabriel Luong [:gl] (ΦωΦ)
	Comment 27 • 6 years ago
mozreview-review

Comment on attachment 8917895 [details]
Bug 1403130 - Support ObjectInspector-based object value grip view in ExtensionSidebar

https://reviewboard.mozilla.org/r/188826/#review218718

::: devtools/client/inspector/extensions/components/ObjectValueGripView.js:22
(Diff revision 6)
> +const { REPS, MODE } = reps;
> +const { Grip } = REPS;
> +
> +const ObjectInspector = createFactory(reps.ObjectInspector);
> +
> +class ObjectValueGripView extends Component {

Can we use a PureComponent?

::: devtools/client/inspector/extensions/components/ObjectValueGripView.js:32
(Diff revision 6)
> +        PropTypes.string,
> +        PropTypes.number,
> +        PropTypes.object,
> +      ]).isRequired,
> +      // Helpers injected as props by extension-sidebar.js.
> +      serviceContainer: PropTypes.shape({

I would define a types.js file and export this serviceContainer shape.

	Code Review Bot [:reviewbot]
	Comment 33 • 6 years ago
mozreview-review

Comment on attachment 8917895 [details]
Bug 1403130 - Support ObjectInspector-based object value grip view in ExtensionSidebar

https://reviewboard.mozilla.org/r/188826/#review222154


Static analysis found 1 defect in this patch.
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/inspector/extensions/test/head.js:16
(Diff revision 7)
>  // Import the inspector's head.js first (which itself imports shared-head.js).
>  Services.scriptloader.loadSubScript(
>    "chrome://mochitests/content/browser/devtools/client/inspector/test/head.js",
>    this);
> +
> +Services.scriptloader.loadSubScript(new URL("head_devtools_inspector_sidebar.js", gTestPath).href,

Error: Line 16 exceeds the maximum line length of 90. [eslint: max-len]

	Code Review Bot [:reviewbot]
	Comment 36 • 6 years ago
mozreview-review

Comment on attachment 8917896 [details]
Bug 1403130 - Allow DOMNodes and cyclic objects to be rendered with the sidebar.setExpression API method.

https://reviewboard.mozilla.org/r/188828/#review222156


Static analysis found 1 defect in this patch.
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/extensions/test/browser/browser_ext_devtools_panels_elements_sidebar.js:149
(Diff revision 7)
> -  is(sidebarPanel1.querySelectorAll("table.treeTable").length, 1,
> +  info("Waiting for the first panel to be rendered");
> -     "The first sidebar panel contains a rendered TreeView component");
>  
> -  is(sidebarPanel1.querySelectorAll("table.treeTable .stringCell").length, 1,
> -     "The TreeView component contains the expected number of string cells.");
> -
> +  // Verify that the panel contains an ObjectInspector, with the expected number of nodes
> +  // and with the expected property names.
> +  await testSetExpressionSidebarPanel(sidebarPanel1, {

Error: 'testSetExpressionSidebarPanel' is not defined. [eslint: no-undef]

	Code Review Bot [:reviewbot]
	Comment 37 • 6 years ago
mozreview-review

Comment on attachment 8917895 [details]
Bug 1403130 - Support ObjectInspector-based object value grip view in ExtensionSidebar

https://reviewboard.mozilla.org/r/188826/#review222160


Static analysis found 1 defect in this patch.
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/inspector/extensions/test/head.js:16
(Diff revision 8)
>  // Import the inspector's head.js first (which itself imports shared-head.js).
>  Services.scriptloader.loadSubScript(
>    "chrome://mochitests/content/browser/devtools/client/inspector/test/head.js",
>    this);
> +
> +Services.scriptloader.loadSubScript(new URL("head_devtools_inspector_sidebar.js", gTestPath).href,

Error: Line 16 exceeds the maximum line length of 90. [eslint: max-len]

	Code Review Bot [:reviewbot]
	Comment 38 • 6 years ago
mozreview-review

Comment on attachment 8917896 [details]
Bug 1403130 - Allow DOMNodes and cyclic objects to be rendered with the sidebar.setExpression API method.

https://reviewboard.mozilla.org/r/188828/#review222162


Static analysis found 1 defect in this patch.
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/extensions/test/browser/browser_ext_devtools_panels_elements_sidebar.js:149
(Diff revision 8)
> -  is(sidebarPanel1.querySelectorAll("table.treeTable").length, 1,
> +  info("Waiting for the first panel to be rendered");
> -     "The first sidebar panel contains a rendered TreeView component");
>  
> -  is(sidebarPanel1.querySelectorAll("table.treeTable .stringCell").length, 1,
> -     "The TreeView component contains the expected number of string cells.");
> -
> +  // Verify that the panel contains an ObjectInspector, with the expected number of nodes
> +  // and with the expected property names.
> +  await testSetExpressionSidebarPanel(sidebarPanel1, {

Error: 'testSetExpressionSidebarPanel' is not defined. [eslint: no-undef]

	Gabriel Luong [:gl] (ΦωΦ)
	Comment 39 • 6 years ago
mozreview-review

Comment on attachment 8917895 [details]
Bug 1403130 - Support ObjectInspector-based object value grip view in ExtensionSidebar

https://reviewboard.mozilla.org/r/188826/#review222228

::: devtools/client/inspector/extensions/components/ExtensionSidebar.js:32
(Diff revision 8)
>    static get propTypes() {
>      return {
>        id: PropTypes.string.isRequired,
>        extensionsSidebar: PropTypes.object.isRequired,
> +      // Helpers injected as props by extension-sidebar.js.
> +      serviceContainer: PropTypes.shape({

This can just use the servicesContainer type.

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 44 • 6 years ago
mozreview-review-reply

Comment on attachment 8917894 [details]
Bug 1403130 - Support result as value grip on WebExtension InspectorWindow actor eval method.

https://reviewboard.mozilla.org/r/188824/#review218562

> Don't you want to return a more explicit error message or is there any security concern?
> From this message it is hard to understand there is something wrong with toolbox / console actor.

Good point, it could be helpful to report an issue related to this scenario, I've added an additional "missing consoleToolboxActorID" string to provide for information about the actual issue.

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 45 • 6 years ago
mozreview-review-reply

Comment on attachment 8917895 [details]
Bug 1403130 - Support ObjectInspector-based object value grip view in ExtensionSidebar

https://reviewboard.mozilla.org/r/188826/#review218208

I've splitted the test a bit and refactored some of the steps into test helpers (so that we can also reuse them for the webextensions tests).

> Not a requirement for now but that would be nice (and not much work I think).
> 
> We could also have an `onViewSourceInDebigger` props (https://searchfox.org/mozilla-central/rev/7fb999d1d39418fd331284fab909df076b967ac6/devtools/client/webconsole/new-console-output/utils/object-inspector.js#66), which when provided, shows an icon to jump to the definition of a logged function (and can be handy).

Yeah, that would be pretty nice, I've filed as a follow up (Bug 1434329)

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 46 • 6 years ago
mozreview-review-reply

Comment on attachment 8917895 [details]
Bug 1403130 - Support ObjectInspector-based object value grip view in ExtensionSidebar

https://reviewboard.mozilla.org/r/188826/#review218556

> This look very generic and nothing seem specific to WebExtension here. Could this be shared with devtools/console/toolbox codebase?

I've briefly discussed about this with nchevobbe, and we agreed that it would be better to defer it to a follow up so that we can evaluate how much is reasonable to share and where the shared code should live (in the meantime I filed it as Bug 1434335)

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 47 • 6 years ago
mozreview-review-reply

Comment on attachment 8917896 [details]
Bug 1403130 - Allow DOMNodes and cyclic objects to be rendered with the sidebar.setExpression API method.

https://reviewboard.mozilla.org/r/188828/#review218372

Once I have finalized the tests that included in the devtools side of this changes, I've moved some shared test helpers into a shared file which is now used in both the devtools and webextensions tests.

I've also applied some additional change to the ext-devtools-panels.js module (and an additional assertion to the test cases) to ensure that the sidebar releases the actors created on the remote debugging server by the sidebar.setExpression API calls (I'm also going to ask nchevobbe to take a very brief look at the code that release the actors for a feedback on it from the devtools point of view).

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 48 • 6 years ago

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #8)
> This is looking good, ping me for review again when the new reps bundle land
> :)
> One thing to keep in mind, as said on irc : between each "evaluation", we
> should release the actor so we don't fill up the server memory (where actors
> are kept).

Hi Nicolas,
In the last version of the attached patches I've added some additional changes which are taking care of releasing the
objectValueGrip.actor when it changes (because of a new sidebar.setExpression / sidebar.setObject API call) and when the sidebar is being destroyed).

The code that releases the actor lives in the ext-devtools-panels.js module (as one of the responsibilities of the ParentDevToolsInspectorSidebar class).

Do you mind to take a brief look at it for a feedback from a DevTools / ObjectInspector point of view?

Follows the link to the interdiff which include (mostly) just these changes:

- https://reviewboard.mozilla.org/r/188822/diff/9-10/

	Nicolas Chevobbe [:nchevobbe]
	Comment 49 • 6 years ago

Hello Luca, This looks good to me :)

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 50 • 6 years ago

Comment on attachment 8917896 [details]
Bug 1403130 - Allow DOMNodes and cyclic objects to be rendered with the sidebar.setExpression API method.

Hi Andrew, 
as anticipated I'm adding a needinfo assigned to you on this patch
only to double-check that the final version of this patch is ok,
the following is a link to the interdiff:

- https://reviewboard.mozilla.org/r/188828/diff/9-10/

(from the last time you looked it I have: moved most of the code that
uses selectors specific to the devtools internals into a file shared between
the test added at "WebExtensions" level and the ones at "devtools" level, applied some further tweaks to release the selected Remote Debugging actor when the sidebar is updated or destroyed, and asked to :nchevobbe to take a look at it from a "devtools internals" point of view)

	Andrew Swan [:aswan]
	Comment 51 • 6 years ago

Looks good to me, thanks!

	Pulsebot
	Comment 58 • 6 years ago

Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/bc28e2788fba
Support result as value grip on WebExtension InspectorWindow actor eval method. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/0fb730d2b346
Support ObjectInspector-based object value grip view in ExtensionSidebar r=gl,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/a2adbb81f2a2
Allow DOMNodes and cyclic objects to be rendered with the sidebar.setExpression API method. r=aswan

	Cristina Coroiu [:ccoroiu]
	Comment 59 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/bc28e2788fba
https://hg.mozilla.org/mozilla-central/rev/0fb730d2b346
https://hg.mozilla.org/mozilla-central/rev/a2adbb81f2a2

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 60 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/bc28e2788fba
https://hg.mozilla.org/mozilla-central/rev/0fb730d2b346
https://hg.mozilla.org/mozilla-central/rev/a2adbb81f2a2

	marius.santa
	Comment 61 • 6 years ago

Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 62 • 6 years ago

Follows the STR:

- install the attached extension temporarily from "about:debugging#addons"
- open a tab and load a regular webpage (e.g. https://developer.mozilla.org)
- open the devtools toolbox on the tab where the webpage has been loaded
- select the "Inspector" panel in the devtools toolbox
- select the body element in the "Inspector" markup view
- select the "TestPanel" sidebar in the "Inspector" panel

- Expected behavior:
  - an object preview should be rendered in the sidebar
  - the object preview should include:
    - $0 (which point to body#home, and the icon near to it should highlight the body when the mouse is over the icon)
    - body (which point to body#home like the above)
    - currentTagName: "BODY"
    - cyclicReference (which if expanded shows a self property, which should contain all the above properties, because this property is a reference to the entire object itself)

- select a different DOM element in the "Inspector" markup view (e.g. the html tag)
- Expected behavior:
  - the object preview is updated in the sidebar ($0 points to the html tag and currentTagName becomes "HTML")

	marius.santa
	Comment 63 • 6 years ago

I was able to reproduce the issue on Windows 10 64Bit Firefox 58.0a1(20171101220120).
Tested and verified on Windows 10 64Bit and Mac OS X 10.13.2 in Firefox  	60.0a1 (20180222101929).