751904 - [responsive] design view GCLI commands

Status: RESOLVED FIXED

(DevTools :: General, defect, P2)

Description

[Kevin Dangoor]

We should have commands to turn responsive design view on/off and to change the screen size. (Zoom and other options would be nice as well)

Comment 1

[Paul Rouget [:paul]]

Attachment: v0.999

Comment 2

[Paul Rouget [:paul]]

Attachment: v1

Comment 3

[Paul Rouget [:paul]]

Maybe I should use a selection type? ("on" "off" "toggle")?

Comment 4

[Joe Walker [:jwalker]]

Comment on attachment 636648 [details] [diff] [review]
v1

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

I do think we need to add some tests for these commands. We've skipped it in the past and got bitten.
It's not hard, because there is a framework to help.

Examples:
- browser/devtools/commandline/test/browser_gcli_commands.js
- browser/devtools/commandline/test/browser_gcli_edit.js

There is even documentation (which I should perhaps move to MDN):
- browser/devtools/commandline/test/head.js
  See the doc comments for DeveloperToolbarTest.checkInputStatus()
  and DeveloperToolbarTest.exec()

Comment 5

[Paul Rouget [:paul]]

Mike, do you mind taking over this?

Comment 6

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

Sure

Comment 7

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

Attachment: v2

Now with tests

Comment 8

[Joe Walker [:jwalker]]

Comment on attachment 639673 [details] [diff] [review]
v2

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

::: browser/devtools/commandline/GcliCommands.jsm
@@ +599,5 @@
> +                          this.name,
> +                          args);
> +  }
> +});
> +

We've got 2 commands in different namespaces here, which is going to make it hard to remember.

How about:

>> resize on
    # resizes to the default or whatever we were on last

>> resize off
    # obvious

>> resize toggle
    # obvious

>> resize to x y
    # obvious

?

::: browser/devtools/commandline/test/head.js
@@ +249,5 @@
> + */
> +DeveloperToolbarTest.checkInputStatusNow = function DTT_checkInputStatusNow(checks) {
> +  DeveloperToolbarTest.checkInputStatus(checks);
> +  DeveloperToolbarTest.exec();
> +};

I think the actual tests are missing?

I'm also not sure about this function. Generally exec will have checks that it makes, and this prevents us from making those checks.

::: browser/devtools/responsivedesign/responsivedesign.jsm
@@ +41,5 @@
> +   * @param aWindow the browser window.
> +   * @param aTab the tab targeted.
> +   * @param aArgs command arguments.
> +   */
> +  handleGcliCommand: function(aWindow, aTab, aCommand, aArgs) {

Nit: aCommand is missing from docs

Also I think we'd be better off having separate function rather than one function that does 2 totally different things based on a switch.

Comment 9

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

Attachment: v3

(In reply to Joe Walker from comment #8)
> Comment on attachment 639673 [details] [diff] [review]
> v2
> 
> Review of attachment 639673 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/commandline/GcliCommands.jsm
> @@ +599,5 @@
> > +                          this.name,
> > +                          args);
> > +  }
> > +});
> > +
> 
> We've got 2 commands in different namespaces here, which is going to make it
> hard to remember.
> 
> How about:
> 
> >> resize on
>     # resizes to the default or whatever we were on last
> 
> >> resize off
>     # obvious
> 
> >> resize toggle
>     # obvious
> 
> >> resize to x y
>     # obvious
> 
> ?
> 

Done

> ::: browser/devtools/commandline/test/head.js
> @@ +249,5 @@
> > + */
> > +DeveloperToolbarTest.checkInputStatusNow = function DTT_checkInputStatusNow(checks) {
> > +  DeveloperToolbarTest.checkInputStatus(checks);
> > +  DeveloperToolbarTest.exec();
> > +};
> 
> I think the actual tests are missing?
> 

Yes, I hadn't added the file, added.

> I'm also not sure about this function. Generally exec will have checks that
> it makes, and this prevents us from making those checks.
> 

Agreed, removed

> ::: browser/devtools/responsivedesign/responsivedesign.jsm
> @@ +41,5 @@
> > +   * @param aWindow the browser window.
> > +   * @param aTab the tab targeted.
> > +   * @param aArgs command arguments.
> > +   */
> > +  handleGcliCommand: function(aWindow, aTab, aCommand, aArgs) {
> 
> Nit: aCommand is missing from docs
> 

Added

> Also I think we'd be better off having separate function rather than one
> function that does 2 totally different things based on a switch.

Now that they are all resize commands hopefully it makes more sense ... it is certainly a lot easier to read and expand upon.

Comment 10

[Joe Walker [:jwalker]]

Comment on attachment 640214 [details] [diff] [review]
v3

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

::: browser/locales/en-US/chrome/browser/devtools/gclicommands.properties
@@ +340,5 @@
> +
> +# LOCALIZATION NOTE (resizePageArgWidthDesc) A very short string to describe the
> +# 'width' parameter to the 'resizepage' command, which is displayed in a dialog
> +# when the user is using this command.
> +resizePageArgWidthDesc=width in pixels

l10n strings are hard to change, so it's worth doing some tweaks as we put this together: I think we need a capital 'W' (and 'H' in the next string)

@@ +350,5 @@
> +
> +# LOCALIZATION NOTE (resizeModeOnDesc) A very short string to describe the
> +# 'resizeon ' 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.
> +resizeModeOnDesc=Turn the Responsive Design Mode on.

These strings need to fit in menus, so they should be short.
How about '[Enter|Exit|Toggle] Responsive Design Mode'?

@@ +365,5 @@
> +
> +# LOCALIZATION NOTE (resizeModeToDesc) A very short string to describe the
> +# 'resize to' 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.
> +resizeModeToDesc=Change the Responsive Design Mode window size.

For brevity, how about 'Alter Responsive Design Mode size'

@@ +374,5 @@
> +resizeModeDesc=Control the Responsive Design Mode.
> +
> +# LOCALIZATION NOTE (resizeModeManual) A fuller description of the 'resize'
> +# command, displayed when the user asks for help on what it does.
> +resizeModeManual=Turn the Responsive Design Mode on and off.

We can afford to be as long as we like here, so how about:

"Responsive websites respond to their environment, so they look good on a mobile display and a cinema display and everything in-between. Responsive Design Mode allows you to easily test a variety of page sizes in Firefox without needing to resize your whole browser"

Comment 11

[Paul Rouget [:paul]]

s/Alter Responsive Design Mode size/Alter page size ?

Beside that, everything looks fine.

Comment 12

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

Attachment: v4

Done

Comment 13

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

Added to master patch in bug 768998

Comment 14

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

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