The highlight command opens the highlighter on a given node

Since the menu items now say "inspect", which should probably make the command match.

Agreed. And I should close bug 683508. :)

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 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()?

(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()?

Sure, go for follow up bug reports. Please mention here the new bug numbers. Thanks!

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.

This is ready to land if we get an r+ from l10n.

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?

It's worth adding that this patch is in https://tbpl.mozilla.org/?tree=Try&rev=b65f1a65c796

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).

(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

(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'...

I raised bug 704789 BTW.

(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.

(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.

flod - I'd like to revert bug 704789. Could you confirm that it's OK to do that? Thanks.

(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.

(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?