Last Comment Bug 751904 - [responsive] design view GCLI commands
: [responsive] design view GCLI commands
Status: RESOLVED FIXED
[gclicommands]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: 14 Branch
: x86 All
: P2 normal (vote)
: Firefox 16
Assigned To: Michael Ratcliffe [:miker] [:mratcliffe]
:
Mentors:
Depends on: 710159
Blocks: GCLICMD
  Show dependency treegraph
 
Reported: 2012-05-04 08:34 PDT by Kevin Dangoor
Modified: 2012-07-16 05:11 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v0.999 (7.05 KB, patch)
2012-06-26 03:48 PDT, Paul Rouget [:paul]
no flags Details | Diff | Review
v1 (7.03 KB, patch)
2012-06-26 03:50 PDT, Paul Rouget [:paul]
no flags Details | Diff | Review
v2 (9.26 KB, patch)
2012-07-06 08:09 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Review
v3 (10.46 KB, patch)
2012-07-09 07:56 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
jwalker: review+
Details | Diff | Review
v4 (12.08 KB, patch)
2012-07-10 03:13 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Review

Description Kevin Dangoor 2012-05-04 08:34:07 PDT
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] 2012-06-26 03:48:56 PDT
Created attachment 636646 [details] [diff] [review]
v0.999
Comment 2 Paul Rouget [:paul] 2012-06-26 03:50:23 PDT
Created attachment 636648 [details] [diff] [review]
v1
Comment 3 Paul Rouget [:paul] 2012-06-26 05:44:59 PDT
Maybe I should use a selection type? ("on" "off" "toggle")?
Comment 4 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-06-27 06:37:13 PDT
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] 2012-06-28 09:51:14 PDT
Mike, do you mind taking over this?
Comment 6 Michael Ratcliffe [:miker] [:mratcliffe] 2012-06-28 09:54:11 PDT
Sure
Comment 7 Michael Ratcliffe [:miker] [:mratcliffe] 2012-07-06 08:09:46 PDT
Created attachment 639673 [details] [diff] [review]
v2

Now with tests
Comment 8 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-07-09 03:26:19 PDT
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 Michael Ratcliffe [:miker] [:mratcliffe] 2012-07-09 07:56:00 PDT
Created attachment 640214 [details] [diff] [review]
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] (needinfo me or ping on irc) 2012-07-09 08:54:00 PDT
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] 2012-07-09 08:58:17 PDT
s/Alter Responsive Design Mode size/Alter page size ?

Beside that, everything looks fine.
Comment 12 Michael Ratcliffe [:miker] [:mratcliffe] 2012-07-10 03:13:53 PDT
Created attachment 640552 [details] [diff] [review]
v4

Done
Comment 13 Michael Ratcliffe [:miker] [:mratcliffe] 2012-07-10 04:00:44 PDT
Added to master patch in bug 768998
Comment 14 Michael Ratcliffe [:miker] [:mratcliffe] 2012-07-10 04:03:26 PDT
Copy / paste error ... added to master patch in but 771555.

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