769575 - GCLI needs page manipulation commands

Status: RESOLVED FIXED

(DevTools :: Console, defect, P2)

Description

[Joe Walker [:jwalker]]

Source:
https://github.com/joewalker/mozcmd/pull/1

We've done a quick security review, and decided that as they stand these commands don't need a major security review to proceed. If we add writing to disk, we need to double check that what we do is ok.

Outstanding:
- Checking that we've organized these commands well
- Unit tests
  (See https://developer.mozilla.org/en/Tools/GCLI/Writing_GCLI_Commands)
- L10n

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Mihai has created the command at:
https://github.com/joewalker/mozcmd/blob/master/rm.mozcmd

Comment 3

[Mihai Sucan [:msucan]]

Attachment: proposed patch

This is the proposed patch. As agreed on IRC:

- this patch is against Mozilla's codebase.
- merged `replace text` and `replace attribute` into one `replace` command.
- moved `replace` and `remove` into a `pagemod` command.
- removed regex and xpath support. I'd like these added back, when gcli allows such types.
- `export page` is now `export html`.
- made the commands localizable.
- wrote tests.

Try push:
https://tbpl.mozilla.org/?tree=Try&rev=db30908f34ad

Please let me know if further changes are needed. Thanks!

Comment 4

[Joe Walker [:jwalker]]

Comment on attachment 639431 [details] [diff] [review]
proposed patch

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

Thanks Mihai. I've given this a quick once over, and it looks ok. I'd like to have a play tomorrow when I've a few less things to think about. Will r+ then.

Comment 5

[Joe Walker [:jwalker]]

Comment on attachment 639431 [details] [diff] [review]
proposed patch

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

::: browser/locales/en-US/chrome/browser/devtools/gclicommands.properties
@@ +472,5 @@
> +
> +# LOCALIZATION NOTE (pagemodremoveattributeIgnoreCaseDesc) A very short string
> +# to describe the 'ignoreCase' parameter to the 'pagemod remove attribute'
> +# command, which is displayed in a dialog when the user is using this command.
> +pagemodremoveattributeIgnoreCaseDesc=Tells that the search for attributes needs to ignore the string case

English nit: I've been racking my brains to work out what's strange to me about this. I think it's that in English 'tell' generally has an explicit subject and object.
So this feels more natural as "This tells the system that the search..."

Or perhaps just avoid 'tell' in this situation: "Ignore the case of the string in the search for attributes".

Another solution is to use 'specify' instead. This solution could be used more generally above.

Also, I apologise for how utterly bizarre and incomprehensible English is ;-)

@@ +477,5 @@
> +
> +# LOCALIZATION NOTE (pagemodremoveattributeResult) A string displayed as the
> +# result of the 'pagemod remove attribute' command.
> +pagemodremoveattributeResult=Elements matched by selector: %1$S. Attributes removed: %2$S.
> +

GCLI supports 2 descriptive strings:
  description -> Very short string designed to show up in a menu
  manual -> Longer more verbose, potentially html snippet
    which is shown in help etc.

I think we need to s/Desc/Manual/g and add some shorter versions in.

Also some places use multipartcommandAttributeDesc and some use multiPartCommandAttributeDesc. We need a standard, and as GCLI owner I get to pick, and I've been selecting common camel case for a while. It's a lot harder to change when we've got this in. It should be a simple S&R job, I think.

Comment 6

[Mihai Sucan [:msucan]]

(In reply to Joe Walker from comment #5)
> Comment on attachment 639431 [details] [diff] [review]
> proposed patch
> 
> Review of attachment 639431 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/locales/en-US/chrome/browser/devtools/gclicommands.properties
> @@ +472,5 @@
> > +
> > +# LOCALIZATION NOTE (pagemodremoveattributeIgnoreCaseDesc) A very short string
> > +# to describe the 'ignoreCase' parameter to the 'pagemod remove attribute'
> > +# command, which is displayed in a dialog when the user is using this command.
> > +pagemodremoveattributeIgnoreCaseDesc=Tells that the search for attributes needs to ignore the string case
> 
> English nit: I've been racking my brains to work out what's strange to me
> about this. I think it's that in English 'tell' generally has an explicit
> subject and object.
> So this feels more natural as "This tells the system that the search..."
> 
> Or perhaps just avoid 'tell' in this situation: "Ignore the case of the
> string in the search for attributes".

I think I'll use this suggestion. I knew when I wrote these descriptions that they sound weird, but as a non-native English speaker no better ideas came to me. Thanks for yours!

> Another solution is to use 'specify' instead. This solution could be used
> more generally above.
> 
> Also, I apologise for how utterly bizarre and incomprehensible English is ;-)

Oh, I find English easy. French, Romanian, German and Arabic are much harder. ;)

> @@ +477,5 @@
> > +
> > +# LOCALIZATION NOTE (pagemodremoveattributeResult) A string displayed as the
> > +# result of the 'pagemod remove attribute' command.
> > +pagemodremoveattributeResult=Elements matched by selector: %1$S. Attributes removed: %2$S.
> > +
> 
> GCLI supports 2 descriptive strings:
>   description -> Very short string designed to show up in a menu
>   manual -> Longer more verbose, potentially html snippet
>     which is shown in help etc.
> 
> I think we need to s/Desc/Manual/g and add some shorter versions in.

Here you point to the result string, not to the command description string. I find the descriptions are short, not sure how much shorter to make them, or how much longer the Manual versions should be. Can you please make your suggestions here?

> Also some places use multipartcommandAttributeDesc and some use
> multiPartCommandAttributeDesc. We need a standard, and as GCLI owner I get
> to pick, and I've been selecting common camel case for a while. It's a lot
> harder to change when we've got this in. It should be a simple S&R job, I
> think.

Oh, will do. I just followed the existing style.

Thank you for the review!

Comment 7

[Joe Walker [:jwalker]]

(In reply to Mihai Sucan [:msucan] from comment #6)
> (In reply to Joe Walker from comment #5)
> >
> > ...
> >
> > @@ +477,5 @@
> > > +
> > > +# LOCALIZATION NOTE (pagemodremoveattributeResult) A string displayed as the
> > > +# result of the 'pagemod remove attribute' command.
> > > +pagemodremoveattributeResult=Elements matched by selector: %1$S. Attributes removed: %2$S.
> > > +
> > 
> > GCLI supports 2 descriptive strings:
> >   description -> Very short string designed to show up in a menu
> >   manual -> Longer more verbose, potentially html snippet
> >     which is shown in help etc.
> > 
> > I think we need to s/Desc/Manual/g and add some shorter versions in.
> 
> Here you point to the result string, not to the command description string.

I was intending simply to put a comment at the end of the file.

> I find the descriptions are short, not sure how much shorter to make them,
> or how much longer the Manual versions should be. Can you please make your
> suggestions here?

I'll head back to the review and add suggestions.

Comment 8

[Joe Walker [:jwalker]]

Comment on attachment 639431 [details] [diff] [review]
proposed patch

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

::: browser/locales/en-US/chrome/browser/devtools/gclicommands.properties
@@ +350,5 @@
> +
> +# LOCALIZATION NOTE (exporthtmlDesc) A very short description of the 'export
> +# html' command. This string is designed to be shown in a menu alongside the
> +# command name, which is why it should be as short as possible.
> +exporthtmlDesc=Export the current page to HTML

I suggest 'Export HTML from page'

@@ +355,5 @@
> +
> +# LOCALIZATION NOTE (pagemodDesc) A very short description of the 'pagemod'
> +# command. This string is designed to be shown in a menu alongside the command
> +# name, which is why it should be as short as possible.
> +pagemodDesc=Make changes in the page

"Make page changes"

@@ +360,5 @@
> +
> +# LOCALIZATION NOTE (pagemodreplaceDesc) A very short description of the
> +# 'pagemod replace' command. This string is designed to be shown in a menu
> +# alongside the command name, which is why it should be as short as possible.
> +pagemodreplaceDesc=Search and replace attributes and text nodes in the page

"Search and replace in page elements"

@@ +370,5 @@
> +
> +# LOCALIZATION NOTE (pagemodreplaceReplaceDesc) A very short string to describe
> +# the 'replace' parameter to the 'pagemod replace' command, which is displayed in
> +# a dialog when the user is using this command.
> +pagemodreplaceReplaceDesc=What you want to put instead of the given search string

"Replacement string"

@@ +375,5 @@
> +
> +# LOCALIZATION NOTE (pagemodreplaceReplaceDesc) A very short string to describe
> +# the 'replace' parameter to the 'pagemod replace' command, which is displayed in
> +# a dialog when the user is using this command.
> +pagemodreplaceReplaceDesc=What you want to put instead of the given search string

A duplicate?

@@ +380,5 @@
> +
> +# LOCALIZATION NOTE (pagemodreplaceIgnoreCaseDesc) A very short string to
> +# describe the 'ignoreCase' parameter to the 'pagemod replace' command, which is
> +# displayed in a dialog when the user is using this command.
> +pagemodreplaceIgnoreCaseDesc=Tells if you want to ignore the case of the search string

"Perform case-insensitive search"

@@ +385,5 @@
> +
> +# LOCALIZATION NOTE (pagemodreplaceRootDesc) A very short string to describe the
> +# 'root' parameter to the 'pagemod replace' command, which is displayed in
> +# a dialog when the user is using this command.
> +pagemodreplaceRootDesc=CSS selector pointing to the root element from where the search starts

"CSS selector to root of search"

@@ +390,5 @@
> +
> +# LOCALIZATION NOTE (pagemodreplaceSelectorDesc) A very short string to describe
> +# the 'selector' parameter to the 'pagemod replace' command, which is displayed
> +# in a dialog when the user is using this command.
> +pagemodreplaceSelectorDesc=CSS selector that tells which elements you want to include in the search and replace operation

"CSS selector to match in search"

@@ +395,5 @@
> +
> +# LOCALIZATION NOTE (pagemodreplaceAttributesDesc) A very short string to
> +# describe the 'attributes' parameter to the 'pagemod replace' command, which is
> +# displayed in a dialog when the user is using this command.
> +pagemodreplaceAttributesDesc=When searching through attributes, limit the search to specific attribute names that match the string you give here

"Attribute match regexp"

I'm concerned that in a TL;DR age, if we can't describe a feature simply that there's a problem with the feature. This attribute is making warning bells go off.

I'm not saying I think we should remove it, just ask ourselves if this is the best way to do this.

@@ +400,5 @@
> +
> +# LOCALIZATION NOTE (pagemodreplaceAttrOnlyDesc) A very short string to describe
> +# the 'attrOnly' parameter to the 'pagemod replace' command, which is displayed
> +# in a dialog when the user is using this command.
> +pagemodreplaceAttrOnlyDesc=Search only through attributes

"Restrict search to attributes"

@@ +405,5 @@
> +
> +# LOCALIZATION NOTE (pagemodreplaceContentOnlyDesc) A very short string to
> +# describe the 'contentOnly' parameter to the 'pagemod replace' command, which
> +# is displayed in a dialog when the user is using this command.
> +pagemodreplaceContentOnlyDesc=Search only through text nodes

"Restrict search to text nodes"

@@ +420,5 @@
> +# LOCALIZATION NOTE (pagemodremoveelementDesc) A very short description of the
> +# 'pagemod remove element' command. This string is designed to be shown in
> +# a menu alongside the command name, which is why it should be as short as
> +# possible.
> +pagemodremoveelementDesc=Remove elements from the page

"Remove elements from page"

@@ +425,5 @@
> +
> +# LOCALIZATION NOTE (pagemodremoveelementSearchDesc) A very short string to
> +# describe the 'search' parameter to the 'pagemod remove element' command, which
> +# is displayed in a dialog when the user is using this command.
> +pagemodremoveelementSearchDesc=CSS selector that tells which elements you want to remove

"CSS selector specifying elements to remove"

@@ +430,5 @@
> +
> +# LOCALIZATION NOTE (pagemodremoveelementRootDesc) A very short string to
> +# describe the 'root' parameter to the 'pagemod remove element' command, which
> +# is displayed in a dialog when the user is using this command.
> +pagemodremoveelementRootDesc=CSS selector pointing to the root element from where the search starts

"CSS selector specifying root of search"

@@ +435,5 @@
> +
> +# LOCALIZATION NOTE (pagemodremoveelementStripOnlyDesc) A very short string to
> +# describe the 'stripOnly' parameter to the 'pagemod remove element' command,
> +# which is displayed in a dialog when the user is using this command.
> +pagemodremoveelementStripOnlyDesc=Tells that you want to remove the element, but not the content

"Remove element, but leave content"

@@ +450,5 @@
> +# LOCALIZATION NOTE (pagemodremoveattributeDesc) A very short description of the
> +# 'pagemod remove attribute' command. This string is designed to be shown in
> +# a menu alongside the command name, which is why it should be as short as
> +# possible.
> +pagemodremoveattributeDesc=Remove specific attributes from elements in the page

"Remove matching attributes"

@@ +456,5 @@
> +# LOCALIZATION NOTE (pagemodremoveattributeSearchAttributesDesc) A very short
> +# string to describe the 'searchAttributes' parameter to the 'pagemod remove
> +# attribute' command, which is displayed in a dialog when the user is using this
> +# command.
> +pagemodremoveattributeSearchAttributesDesc=String that matches the attribute names you want to remove

"Regexp specifying attributes to remove"

@@ +462,5 @@
> +# LOCALIZATION NOTE (pagemodremoveattributeSearchElementsDesc) A very short
> +# string to describe the 'searchElements' parameter to the 'pagemod remove
> +# attribute' command, which is displayed in a dialog when the user is using this
> +# command.
> +pagemodremoveattributeSearchElementsDesc=CSS selector that tells which elements you want to search for

"CSS selector of elements to include"

@@ +467,5 @@
> +
> +# LOCALIZATION NOTE (pagemodremoveattributeRootDesc) A very short string to
> +# describe the 'root' parameter to the 'pagemod remove attribute' command, which
> +# is displayed in a dialog when the user is using this command.
> +pagemodremoveattributeRootDesc=A CSS selector pointing to the root element from where the search starts

"CSS selector of root of search"

Comment 9

[Mihai Sucan [:msucan]]

Attachment: addressed review comments

Thank you Joe! Updated the strings as suggested. Much better, indeed.

Comment 10

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Added to master patch in bug 768998

Comment 11

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Copy / paste error ... added to master patch in but 771555.