Last Comment Bug 777001 - [Developer Toolbar] "inspect" command refers to "node", should refer to "selector"
: [Developer Toolbar] "inspect" command refers to "node", should refer to "sele...
Status: RESOLVED FIXED
[fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 17
Assigned To: Nobody; OK to take it and work on it
:
: J. Ryan Stinnett [:jryans] (use ni?)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-24 11:07 PDT by Ian Bicking (:ianb)
Modified: 2012-08-25 17:04 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
propose (929 bytes, patch)
2012-07-31 14:40 PDT, Tetsuharu OHZEKI [:tetsuharu] [UTC+9]
no flags Details | Diff | Splinter Review
propose rev2 (3.16 KB, patch)
2012-08-06 09:47 PDT, Tetsuharu OHZEKI [:tetsuharu] [UTC+9]
jwalker: review+
Details | Diff | Splinter Review
patch rev.3 (1.21 KB, patch)
2012-08-20 00:52 PDT, Tetsuharu OHZEKI [:tetsuharu] [UTC+9]
jwalker: review+
Details | Diff | Splinter Review

Description Ian Bicking (:ianb) 2012-07-24 11:07:45 PDT
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] 2012-07-31 14:40:46 PDT
Created attachment 647684 [details] [diff] [review]
propose

Like this?
Comment 2 Ian Bicking (:ianb) 2012-08-01 09:01:28 PDT
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] 2012-08-06 09:47:21 PDT
Created attachment 649298 [details] [diff] [review]
propose rev2

I fixed Ian's commented point.
Comment 4 Ian Bicking (:ianb) 2012-08-12 08:13:46 PDT
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] (needinfo me or ping on irc) 2012-08-13 02:48:58 PDT
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] (needinfo me or ping on irc) 2012-08-13 02:55:55 PDT
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] 2012-08-20 00:52:23 PDT
Created attachment 653298 [details] [diff] [review]
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] (needinfo me or ping on irc) 2012-08-24 11:49:29 PDT
Comment on attachment 653298 [details] [diff] [review]
patch rev.3

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

Thanks!
Comment 9 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-08-24 22:57:35 PDT
https://tbpl.mozilla.org/?tree=Fx-Team&rev=bbd267f16fc7
Comment 10 Ryan VanderMeulen [:RyanVM] 2012-08-25 04:32:34 PDT
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] (needinfo me or ping on irc) 2012-08-25 06:23:57 PDT
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] 2012-08-25 06:32:59 PDT
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] (needinfo me or ping on irc) 2012-08-25 07:45:23 PDT
(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] 2012-08-25 07:58:50 PDT
(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] (needinfo me or ping on irc) 2012-08-25 08:07:25 PDT
Yes please!
Comment 16 Dave Camp (:dcamp) 2012-08-25 17:04:19 PDT
https://hg.mozilla.org/mozilla-central/rev/33945cb30eae

Note You need to log in before you can comment on or make changes to this bug.