Closed Bug 1379570 Opened 4 years ago Closed 4 years ago

Change `inspect` command behavior

Categories

(DevTools :: Console, enhancement, P1)

enhancement

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 56
Iteration:
56.3 - Jul 24

People

(Reporter: nchevobbe, Assigned: nchevobbe)

References

Details

(Whiteboard: [console-html])

Attachments

(1 file)

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`.
Depends on: 1308566
Flags: qe-verify+
Priority: -- → P2
Whiteboard: [console-html]
QA Contact: iulia.cristescu
(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
Depends on: 1307922
No longer depends on: 1308566
Assignee: nobody → nchevobbe
Priority: P2 → P1
Status: NEW → ASSIGNED
Iteration: --- → 56.3 - Jul 24
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?
Attachment #8886348 - Flags: review?(bgrinstead)
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 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
Attachment #8886348 - Flags: review?(bgrinstead) → review+
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c676af6947ac
Adapt the inspect command to the new console frontend. r=bgrins
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.
Flags: needinfo?(nchevobbe)
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4dd3191931e0
Adapt the inspect command to the new console frontend. r=bgrins
https://hg.mozilla.org/mozilla-central/rev/4dd3191931e0
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
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
QA Whiteboard: [bugday-20170719]
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]
As per Comment 14 and Comment 15, I am marking this bug as verified fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Product: Firefox → DevTools
Duplicate of this bug: 1243966
You need to log in before you can comment on or make changes to this bug.