777001 - [Developer Toolbar] "inspect" command refers to "node", should refer to "selector"

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Ian Bicking (:ianbicking)]

In the completion suggestion and in help the "inspect" command refers to invoking "inspect node" but the second argument is a selector, not some node object.  It would be easier to immediately understand the second argument if it was consistently named "selector".

Comment 1

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: propose

Like this?

Comment 2

[Ian Bicking (:ianbicking)]

Comment on attachment 647684 [details] [diff] [review]
propose

>      document.defaultView.InspectorUI.openInspectorUI(args.node);

I'm guessing that line has to be changed too?  And perhaps the help documents (inspectNodeDesc, inspectNodeManual) also need to be updated?

Comment 3

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: propose rev2

I fixed Ian's commented point.

Comment 4

[Ian Bicking (:ianbicking)]

Comment on attachment 649298 [details] [diff] [review]
propose rev2

I'm not really familiar enough with this code to review.

Comment 5

[Joe Walker [:jwalker]]

Hmm.

So technically, the thing that we're inspecting is a node, and not (the result of looking up) a selector, in that we demand a single node. We could argue that 'node' is a subset of 'selector', I guess.

In addition we have discussed (and I've pushed back on) allowing use of xpath to specify a node.

However, I think that the user can imply a singular result from the context, but they can't imply the syntax, so probably 'selector' is more helpful.

So I'm generally favourable to this, but with caveats, that I thought should be explained.

More in review...

Comment 6

[Joe Walker [:jwalker]]

Comment on attachment 649298 [details] [diff] [review]
propose rev2

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

r+ with the changes below, assuming my notes on 'node vs selector' don't change anyone's mind.
Thanks for the patch.

::: browser/devtools/commandline/GcliCommands.jsm
@@ +484,5 @@
>    manual: gcli.lookup("inspectManual"),
>    params: [
>      {
> +      name: "selector",
> +      type: "selector",

Changing the *type* to selector is wrong - the type really is "node" (unless you want to go into GCLI and change how the NodeType is registered).

@@ +486,5 @@
>      {
> +      name: "selector",
> +      type: "selector",
> +      description: gcli.lookup("inspectSelectorDesc"),
> +      manual: gcli.lookup("inspectSelectorManual")

Much as I think we should keep the naming consistent, I think the localizers will be cross with us if we needlessly change the lookup strings, so could we leave all of these as 'Node'.

Comment 7

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch rev.3

Fixed Joe's point.

I think that "node" signage mislead the user like this parameter accept JS expression, |document.getElementById()|.

Comment 8

[Joe Walker [:jwalker]]

Comment on attachment 653298 [details] [diff] [review]
patch rev.3

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

Thanks!

Comment 9

[Joe Walker [:jwalker]]

https://tbpl.mozilla.org/?tree=Fx-Team&rev=bbd267f16fc7

Comment 10

[Ryan VanderMeulen [:RyanVM]]

This was failing on Try? Thanks for at least not pushing inbound without Try results...
https://tbpl.mozilla.org/?tree=Try&rev=4c6632e2334a

https://tbpl.mozilla.org/php/getParsedLog.php?id=14693190&tree=Try

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/commandline/test/browser_gcli_inspect.js | emptyParameters[0] - Got  <selector>, expected  <node>

Comment 11

[Joe Walker [:jwalker]]

My test script wasn't working properly, so I didn't catch this before I pushed, but I fixed the patch, and pushed it again.
Thanks.

Comment 12

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Sorry. I had missed it.
And Thank you, Joe.

By the way, Joe, I detect that we need to change the argument of "screenshot" command "node",  to "selector", if we change "inspect" command.

Comment 13

[Joe Walker [:jwalker]]

(In reply to OHZEKI Tetsuharu [:saneyuki_s] from comment #12)
> Sorry. I had missed it.
> And Thank you, Joe.
> 
> By the way, Joe, I detect that we need to change the argument of
> "screenshot" command "node",  to "selector", if we change "inspect" command.

Is 'yes please' a valid answer?

Comment 14

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Joe Walker [:jwalker] from comment #13)
> Is 'yes please' a valid answer?

Yes. I would like to work it.
Needless to say, Should I file the new bug?

Comment 15

[Joe Walker [:jwalker]]

Yes please!

Comment 16

[Dave Camp (:dcamp)]

https://hg.mozilla.org/mozilla-central/rev/33945cb30eae