1379570 - Change `inspect` command behavior

Status: VERIFIED FIXED

(DevTools :: Console, enhancement, P1)

Description

[Nicolas Chevobbe [:nchevobbe]]

Until https://bugzilla.mozilla.org/show_bug.cgi?id=1308566 lands, the `inspect` command would open the variable view with the object passed as an argument.

Since we want to get rid of the variable view, we should take care of handling the inspect command in a different way.

The idea that makes the most sense to me at the moment would be to log the object, expanded in the object inspector, like we are liikely to do for `console.dir`.

Comment 1

[Brian Grinstead [:bgrins]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #0)
> The idea that makes the most sense to me at the moment would be to log the
> object, expanded in the object inspector, like we are liikely to do for
> `console.dir`.

I agree with that

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1379570 - Adapt the inspect command to the new console frontend.

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

Comment 3

[Brian Grinstead [:bgrins]]

Comment on attachment 8886348 [details]
Bug 1379570 - Adapt the inspect command to the new console frontend.

https://reviewboard.mozilla.org/r/157088/#review162644

::: devtools/client/webconsole/new-console-output/components/message-types/evaluation-result.js:70
(Diff revision 1)
>      }
>    } else {
>      messageBody = GripMessageBody({
>        dispatch,
>        messageId,
> -      grip: parameters,
> +      grip: isInspectCommandResult ? parameters.object : parameters,

I like the approach in this patch overall. I'd prefer if we didn't need to make any changes to this file and could instead contain special casing into the transformPacket function in utils/messages.js.

This would mean replacing the `helperResult` property with `helperResult.object` when transforming, so that in this scope  `parameters` is pointing to the object grip, just like it would on a normal evaluation result. This would lead to a wrinkle around the `type` name, since we use it also to check if the OI should be expanded in the GripMessageBody. However, we could rename the `type: "inspectObject"` property to something like `helperType: "inspectObject"`.  Then in GripMessageBody we could check `helperType` instead of `type`.

Does that seem reasonable?

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8886348 [details]
Bug 1379570 - Adapt the inspect command to the new console frontend.

https://reviewboard.mozilla.org/r/157088/#review162644

> I like the approach in this patch overall. I'd prefer if we didn't need to make any changes to this file and could instead contain special casing into the transformPacket function in utils/messages.js.
> 
> This would mean replacing the `helperResult` property with `helperResult.object` when transforming, so that in this scope  `parameters` is pointing to the object grip, just like it would on a normal evaluation result. This would lead to a wrinkle around the `type` name, since we use it also to check if the OI should be expanded in the GripMessageBody. However, we could rename the `type: "inspectObject"` property to something like `helperType: "inspectObject"`.  Then in GripMessageBody we could check `helperType` instead of `type`.
> 
> Does that seem reasonable?

It does. Let me try that.

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8886348 [details]
Bug 1379570 - Adapt the inspect command to the new console frontend.

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

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8886348 [details]
Bug 1379570 - Adapt the inspect command to the new console frontend.

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

Comment 7

[Brian Grinstead [:bgrins]]

Comment on attachment 8886348 [details]
Bug 1379570 - Adapt the inspect command to the new console frontend.

https://reviewboard.mozilla.org/r/157088/#review162990

Looks good to me, thanks

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c676af6947ac
Adapt the inspect command to the new console frontend. r=bgrins

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=114978017&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/923d39f15f371d06a5726e511346847369dd353c

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8886348 [details]
Bug 1379570 - Adapt the inspect command to the new console frontend.

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

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

Sorry about that, I should have look more carefully, I thought the failures were an intermittent we have on this (which we should get fixed too).
I fixed the error and pushed to TRY again https://treeherder.mozilla.org/#/jobs?repo=try&revision=6316b20b6e388f298134ed464b7bdb73fa4be65d , everything seems okay now.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4dd3191931e0
Adapt the inspect command to the new console frontend. r=bgrins

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4dd3191931e0

Comment 14

[Md. Majedul islam]

I have reproduced this bug with Nightly 56.0a1 (2017-07-10) on Ubuntu 16.04, 64 bit!

The fix is now verified on Latest Nightly.

Build ID 	20170720100139
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:56.0) Gecko/20100101 Firefox/56.0

Comment 15

[Md. Rahimul Islam]

I have successfully reproduced this bug with Nightly 56.0a1 (2017-07-10) (32-bit) on windows 10(32bit)

this bug is verified fix with  latest nightly 56.0a1 (2017-07-20) (32-bit)

Build ID:  	20170720030203
Mozilla/5.0 (Windows NT 10.0; rv:56.0) Gecko/20100101 Firefox/56.0
[bugday-20170719]

Comment 16

[Md. Majedul islam]

As per Comment 14 and Comment 15, I am marking this bug as verified fixed.