Last Comment Bug 683506 - GCLI needs an 'inspect' command
: GCLI needs an 'inspect' command
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: All All
: P1 normal (vote)
: Firefox 10
Assigned To: Joe Walker [:jwalker] (needinfo me or ping on irc)
:
:
Mentors:
: 683508 (view as bug list)
Depends on: 693265
Blocks: GCLI-SHIP 697043 GCLICMD
  Show dependency treegraph
 
Reported: 2011-08-31 07:43 PDT by Joe Walker [:jwalker] (needinfo me or ping on irc)
Modified: 2012-06-27 11:34 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
upload 1 (2.54 KB, patch)
2011-10-19 03:30 PDT, Joe Walker [:jwalker] (needinfo me or ping on irc)
mihai.sucan: review-
Details | Diff | Splinter Review
upload 2 (13.32 KB, patch)
2011-10-20 10:21 PDT, Joe Walker [:jwalker] (needinfo me or ping on irc)
mihai.sucan: review+
Details | Diff | Splinter Review
upload 3 (11.19 KB, patch)
2011-10-25 08:57 PDT, Joe Walker [:jwalker] (needinfo me or ping on irc)
l10n: review+
Details | Diff | Splinter Review

Description Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-08-31 07:43:59 PDT
The highlight command opens the highlighter on a given node
Comment 1 Kevin Dangoor 2011-10-10 07:19:33 PDT
Since the menu items now say "inspect", which should probably make the command match.
Comment 2 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-10 10:00:59 PDT
Agreed.
And I should close bug 683508.
:)
Comment 3 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-10 10:01:17 PDT
*** Bug 683508 has been marked as a duplicate of this bug. ***
Comment 4 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-19 03:30:52 PDT
Created attachment 567998 [details] [diff] [review]
upload 1
Comment 5 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-19 03:39:31 PDT
Some review context: I don't expect the strings in this patch to change
I do expect the implementation to change in 2 ways:
- Added tests
- Possible move of 'node' type from GCLI to central
Comment 6 Mihai Sucan [:msucan] 2011-10-19 06:05:32 PDT
Comment on attachment 567998 [details] [diff] [review]
upload 1

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

Patch looks fine, but I do have some concerns:

1. The selector field doesn't show here. See:

http://img.i7m.de/show/1rw87.png

It also doesn't show when I try the echo command. Please file a follow up bug report to fix this issue.


2. When the selector I write matches multiple elements I just get an inline message that tells me how many matches have been found. I think most often users will type selectors that match multiple elements (this is really the most common use-case). Allow us to select the node we want from keyboard and from mouse (display the list and allow up/down through the list). If I press Enter inspect the node I picked, if I don't press up/down ... just select the first match.

Writing document.querySelector() is more productive in this case. Even querySelectorAll() because I can just type querySelectorAll()[2] to select the third match.

This is for a follow up bug or you might want to fix this here, in this patch. I leave this to be your choice.


3. Matched elements are highlighted by changing their element.style props. That is not desired. Please use Highlighter instances to highlight the matched elements.

Another way would be to land this patch without addressing point 3, but open a follow up bug report that would be required to be fixed before GCLI is enabled by default. You might want to ping dcamp/robcee about a decision. (are we in a hurry to land this patch?)


Giving the patch r- for point 3 (needs a decision) and because the patch needs a test (as you pointed out in comment 5).

Looking forward for an updated patch. Thank you!

::: browser/devtools/webconsole/GcliCommands.jsm
@@ +134,5 @@
> +    }
> +  ],
> +  exec: function Command_inspect(args, context) {
> +    let hud = HUDService.getHudReferenceById(context.environment.hudId);
> +    let InspectorUI = hud.gcliterm.document.defaultView.InspectorUI;

hud.chromeWindow.InspectorUI

::: browser/locales/en-US/chrome/browser/devtools/gclicommands.properties
@@ +55,5 @@
> +# LOCALIZATION NOTE (inspectNodeManual) A fuller description of the 'node'
> +# parameter to the 'inspect' command, displayed when the user asks for help
> +# on what it does.
> +inspectNodeManual=A CSS selector for use with Document.querySelector which \
> +identifies a single element

Nit: shouldn't that be document.querySelector()?
Comment 7 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-20 10:20:21 PDT
(In reply to Mihai Sucan [:msucan] from comment #6)
> Comment on attachment 567998 [details] [diff] [review] [diff] [details] [review]
> upload 1
> 
> Review of attachment 567998 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> Patch looks fine, but I do have some concerns:
> 
> 1. The selector field doesn't show here. See:
> 
> http://img.i7m.de/show/1rw87.png
> 
> It also doesn't show when I try the echo command. Please file a follow up
> bug report to fix this issue.
> 
> 
> 2. When the selector I write matches multiple elements I just get an inline
> message that tells me how many matches have been found. I think most often
> users will type selectors that match multiple elements (this is really the
> most common use-case). Allow us to select the node we want from keyboard and
> from mouse (display the list and allow up/down through the list). If I press
> Enter inspect the node I picked, if I don't press up/down ... just select
> the first match.
> 
> Writing document.querySelector() is more productive in this case. Even
> querySelectorAll() because I can just type querySelectorAll()[2] to select
> the third match.
> 
> This is for a follow up bug or you might want to fix this here, in this
> patch. I leave this to be your choice.

You're kidding me - there's no way I'm doing that in this bug. It can be a followup.

> 3. Matched elements are highlighted by changing their element.style props.
> That is not desired. Please use Highlighter instances to highlight the
> matched elements.
> 
> Another way would be to land this patch without addressing point 3, but open
> a follow up bug report that would be required to be fixed before GCLI is
> enabled by default. You might want to ping dcamp/robcee about a decision.
> (are we in a hurry to land this patch?)

Likewise - as i said in IRC this isn't going to be done in this bug. I'll probably disable the highlighting if we can't easily use the Highlighter. It's abstracted in the next patch


> Giving the patch r- for point 3 (needs a decision) and because the patch
> needs a test (as you pointed out in comment 5).
> 
> Looking forward for an updated patch. Thank you!
> 
> ::: browser/devtools/webconsole/GcliCommands.jsm
> @@ +134,5 @@
> > +    }
> > +  ],
> > +  exec: function Command_inspect(args, context) {
> > +    let hud = HUDService.getHudReferenceById(context.environment.hudId);
> > +    let InspectorUI = hud.gcliterm.document.defaultView.InspectorUI;
> 
> hud.chromeWindow.InspectorUI
> 
> ::: browser/locales/en-US/chrome/browser/devtools/gclicommands.properties
> @@ +55,5 @@
> > +# LOCALIZATION NOTE (inspectNodeManual) A fuller description of the 'node'
> > +# parameter to the 'inspect' command, displayed when the user asks for help
> > +# on what it does.
> > +inspectNodeManual=A CSS selector for use with Document.querySelector which \
> > +identifies a single element
> 
> Nit: shouldn't that be document.querySelector()?
Comment 8 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-20 10:21:26 PDT
Created attachment 568433 [details] [diff] [review]
upload 2
Comment 9 Mihai Sucan [:msucan] 2011-10-20 10:24:10 PDT
Sure, go for follow up bug reports. Please mention here the new bug numbers. Thanks!
Comment 10 Mihai Sucan [:msucan] 2011-10-21 03:11:27 PDT
Comment on attachment 568433 [details] [diff] [review]
upload 2

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

Patch looks fine. Please address the comment below. Thanks!

::: browser/devtools/webconsole/test/browser/browser_gcli_inspect.js
@@ +38,5 @@
> +    testSetup();
> +    testCreateCommands();
> +  }
> +  catch (ex) {
> +    gcli._internal.console.error('Test Failure', ex);

This needs to be a real test failure, otherwise even if the whole test fails very badly the mochitest will show as pass: 0, fail: 0.
Comment 11 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-25 08:57:59 PDT
Created attachment 569393 [details] [diff] [review]
upload 3

This is ready to land if we get an r+ from l10n.
Comment 12 Axel Hecht [:Pike] 2011-10-25 09:11:22 PDT
Comment on attachment 569393 [details] [diff] [review]
upload 3

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

r=me.

Just to clarify, not every patch that lands in l10n needs my review, and should be gated on my time and attention.

If you like to have a l10n buddy for your patches, maybe hit the .l10n newsgroup and ask a community member to step up?
Comment 13 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-25 14:59:08 PDT
It's worth adding that this patch is in https://tbpl.mozilla.org/?tree=Try&rev=b65f1a65c796
Comment 14 Rob Campbell [:rc] (:robcee) 2011-10-26 07:28:24 PDT
https://hg.mozilla.org/integration/fx-team/rev/332d8f5f5886
Comment 15 :Margaret Leibovic 2011-10-27 11:43:48 PDT
https://hg.mozilla.org/mozilla-central/rev/332d8f5f5886
Comment 16 Francesco Lodolo [:flod] 2011-10-28 09:20:44 PDT
Can someone explain why these strings use two lines with this strange "\" at the of the first line? Honestly it's the first time I see something like this and it's messing with my localization tool ("\" is displayed as a normal character).
Comment 17 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-28 09:52:05 PDT
(In reply to flod (Francesco Lodolo) from comment #16)
> Can someone explain why these strings use two lines with this strange "\" at
> the of the first line? Honestly it's the first time I see something like
> this and it's messing with my localization tool ("\" is displayed as a
> normal character).

The short answer is - because it's a properties file, and that's how you specify continuations lines in a properties file [1]

However I guess I'm making a huge assumption about how we handle properties files. Maybe there is a different (or no) way to specify a continuation line in the Mozilla case?

[1] https://secure.wikimedia.org/wikipedia/en/wiki/.properties
Comment 18 Rimas Kudelis 2011-10-29 00:54:54 PDT
(In reply to Joe Walker from comment #17)
> (In reply to flod (Francesco Lodolo) from comment #16)
> > Can someone explain why these strings use two lines with this strange "\" at
> > the of the first line? Honestly it's the first time I see something like
> > this and it's messing with my localization tool ("\" is displayed as a
> > normal character).
> 
> The short answer is - because it's a properties file, and that's how you
> specify continuations lines in a properties file [1]
> 
> However I guess I'm making a huge assumption about how we handle properties
> files. Maybe there is a different (or no) way to specify a continuation line
> in the Mozilla case?
> 
> [1] https://secure.wikimedia.org/wikipedia/en/wiki/.properties

We don't usually split lines in .properties files like that. Or we use \n to add hard linebreaks.


I have another question about this string:
Investigate the dimensions and properties of an element using a CSS selector to open the DOM highlighter
what does the second part of it mean? Would it not have the same meaning if we just said 'Investigate the dimensions and properties of an element using the CSS inspector' or something similar? I have no idea how I can 'use a CSS selector to open the DOM highlighter'...
Comment 19 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-25 03:32:48 PST
I raised bug 704789 BTW.
Comment 20 Axel Hecht [:Pike] 2011-12-14 15:11:31 PST
(In reply to flod (Francesco Lodolo) from comment #16)
> Can someone explain why these strings use two lines with this strange "\" at
> the of the first line? Honestly it's the first time I see something like
> this and it's messing with my localization tool ("\" is displayed as a
> normal character).

Bad tool, bad tool. Can I haz you fix that?

As the \ line ending escapes are totally valid.
Comment 21 [:rickiees] Ricardo Palomares 2011-12-15 00:18:36 PST
(In reply to Axel Hecht [:Pike] from comment #20)
> (In reply to flod (Francesco Lodolo) from comment #16)
> > Can someone explain why these strings use two lines with this strange "\" at
> > the of the first line? Honestly it's the first time I see something like
> > this and it's messing with my localization tool ("\" is displayed as a
> > normal character).
> 
> Bad tool, bad tool. Can I haz you fix that?
> 
> As the \ line ending escapes are totally valid.


It is fixed long time ago (but after I released MT v5.26). flod, I'll send you an interim version with all new features.
Comment 22 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-12-15 07:56:07 PST
flod - I'd like to revert bug 704789. Could you confirm that it's OK to do that?
Thanks.
Comment 23 Francesco Lodolo [:flod] 2011-12-15 08:05:33 PST
(In reply to Joe Walker from comment #22)
> flod - I'd like to revert bug 704789. Could you confirm that it's OK to do
> that?

No problem here since Axel explained that "\" is correct.
Comment 24 Rimas Kudelis 2011-12-15 14:19:53 PST
(In reply to flod (Francesco Lodolo) from comment #23)
> (In reply to Joe Walker from comment #22)
> > flod - I'd like to revert bug 704789. Could you confirm that it's OK to do
> > that?
> 
> No problem here since Axel explained that "\" is correct.

I personally wouldn't revert it anyway. There are thousands of strings in our properties files, and none of them are broken like that. What's the point in breaking this one?

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