Initial check-in of gesture config WebUI.

Initial check-in of gesture config WebUI.

BUG=111993
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=142247

[Kevin Greer, 2012-06-06 14:17:15]

Initial version of new gesture configuration WebUI.  Appears under
chrome://gesture.  See issue for screenshot.

[flackr, 2012-06-06 14:50:53]

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/browser/brow...
File chrome/browser/browser_resources.grd (right):

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/browser/brow...
chrome/browser/browser_resources.grd:68: <include name="IDR_GESTURE_CONFIG_JS"
file="resources\gesture_config.js" type="BINDATA" />
Can we put these in chrome/browser/resources/chromeos?

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/browser/reso...
File chrome/browser/resources/gesture_config.css (right):

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/browser/reso...
chrome/browser/resources/gesture_config.css:14: width: 125px;
Use 2 space indent.

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/browser/reso...
File chrome/browser/resources/gesture_config.html (right):

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/browser/reso...
chrome/browser/resources/gesture_config.html:8: <!--  <script
src="chrome://resources/js/cr.js"></script> -->
Unnecessary I assume?

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/browser/reso...
File chrome/browser/resources/gesture_config.js (right):

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/browser/reso...
chrome/browser/resources/gesture_config.js:7: // browser directly for layout and
some testing purposes.
Try to avoid personal pronouns. i.e. Redefine '$' here rather than including
cr.js since this is the only function needed.

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/browser/reso...
chrome/browser/resources/gesture_config.js:24: label: 'Long Press Time',
These labels will need to be localized.

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/browser/reso...
chrome/browser/resources/gesture_config.js:121: buf.push('</div>');
Can you define and clone a template DOM node rather than constructing an HTML
string?

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/browser/reso...
chrome/browser/resources/gesture_config.js:191: $('resetButton').onclick =
onReset.bind(this);
On the settings page we update / fetch values immediately as they are changed,
are we sure that we want to use a delayed batch update model here?

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/browser/ui/w...
File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right):

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/browser/ui/w...
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:29: #include
"chrome/browser/ui/webui/gesture_config_ui.h"
Move to chromeos specific include block.

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/browser/ui/w...
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:188: return
&NewWebUI<GestureConfigUI>;
Move to ChromeOS specific block below.

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/browser/ui/w...
File chrome/browser/ui/webui/gesture_config_ui.cc (right):

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/browser/ui/w...
chrome/browser/ui/webui/gesture_config_ui.cc:5: #include
"chrome/browser/ui/webui/gesture_config_ui.h"
Move to chrome/browser/ui/webui/chromeos/

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/browser/ui/w...
File chrome/browser/ui/webui/gesture_config_ui.h (right):

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/browser/ui/w...
chrome/browser/ui/webui/gesture_config_ui.h:21: void SetPreferenceValue(const
base::ListValue* args);
nit: newline before private block

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/chrome_brows...
File chrome/chrome_browser.gypi (right):

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/chrome_brows...
chrome/chrome_browser.gypi:3676: 'browser/ui/webui/gesture_config_ui.h',
nit: feedback_ui and this seem out of place, can you sort these?

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/common/url_c...
File chrome/common/url_constants.h (right):

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/common/url_c...
chrome/common/url_constants.h:43: extern const char
kChromeUIGestureConfigHost[];
The Host variables are defined in a separate block below. Also these should only
be defined on CHROMEOS

[rjkroege, 2012-06-06 15:10:28]

drive by nit...

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/browser/reso...
File chrome/browser/resources/gesture_config.js (right):

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/browser/reso...
chrome/browser/resources/gesture_config.js:29: label: 'Max. Double Click
Interval',
why abbreviate Max, Min? Does it negatively effect layout.

[Kevin Greer, 2012-06-06 20:26:19]

Changed L&F to match other settings screens.
No longer batches updates.
Addressed most review comments.

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/browser/reso...
File chrome/browser/resources/gesture_config.css (right):

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/browser/reso...
chrome/browser/resources/gesture_config.css:14: width: 125px;
On 2012/06/06 14:50:54, flackr wrote:
> Use 2 space indent.

Done.

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/browser/reso...
File chrome/browser/resources/gesture_config.html (right):

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/browser/reso...
chrome/browser/resources/gesture_config.html:8: <!--  <script
src="chrome://resources/js/cr.js"></script> -->
Removed.
On 2012/06/06 14:50:54, flackr wrote:
> Unnecessary I assume?

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/browser/reso...
File chrome/browser/resources/gesture_config.js (right):

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/browser/reso...
chrome/browser/resources/gesture_config.js:7: // browser directly for layout and
some testing purposes.
On 2012/06/06 14:50:54, flackr wrote:
> Try to avoid personal pronouns. i.e. Redefine '$' here rather than including
> cr.js since this is the only function needed.

Done.

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/browser/reso...
chrome/browser/resources/gesture_config.js:24: label: 'Long Press Time',
This is screen is meant to be an unsupported/internal feature which will not be
internationalized.
Does this mean it should appear under about:// instead of chrome://?
On 2012/06/06 14:50:54, flackr wrote:
> These labels will need to be localized.

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/browser/reso...
chrome/browser/resources/gesture_config.js:29: label: 'Max. Double Click
Interval',
Changed to the long forms.
On 2012/06/06 15:10:28, rjkroege wrote:
> why abbreviate Max, Min? Does it negatively effect layout.

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/browser/reso...
chrome/browser/resources/gesture_config.js:191: $('resetButton').onclick =
onReset.bind(this);
Changed.
On 2012/06/06 14:50:54, flackr wrote:
> On the settings page we update / fetch values immediately as they are changed,
> are we sure that we want to use a delayed batch update model here?

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/browser/ui/w...
File chrome/browser/ui/webui/gesture_config_ui.h (right):

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/browser/ui/w...
chrome/browser/ui/webui/gesture_config_ui.h:21: void SetPreferenceValue(const
base::ListValue* args);
On 2012/06/06 14:50:54, flackr wrote:
> nit: newline before private block

Done.

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/chrome_brows...
File chrome/chrome_browser.gypi (right):

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/chrome_brows...
chrome/chrome_browser.gypi:3676: 'browser/ui/webui/gesture_config_ui.h',
On 2012/06/06 14:50:54, flackr wrote:
> nit: feedback_ui and this seem out of place, can you sort these?

Done.

[Kevin Greer, 2012-06-07 19:07:00]

Fixed conditional compilation and addressed or replied to all outstanding review
comments.

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/browser/brow...
File chrome/browser/browser_resources.grd (right):

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/browser/brow...
chrome/browser/browser_resources.grd:68: <include name="IDR_GESTURE_CONFIG_JS"
file="resources\gesture_config.js" type="BINDATA" />
This is not chromeos specific.  I mistakenly put some earlier code under a
chromeos ifdef, but it should have been under 'aura'.
On 2012/06/06 14:50:54, flackr wrote:
> Can we put these in chrome/browser/resources/chromeos?

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/browser/reso...
File chrome/browser/resources/gesture_config.js (right):

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/browser/reso...
chrome/browser/resources/gesture_config.js:121: buf.push('</div>');
Given all of the conditional logic in the formatting, I think it would be a lot
more trouble than it would be worth.
On 2012/06/06 14:50:54, flackr wrote:
> Can you define and clone a template DOM node rather than constructing an HTML
> string?

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/browser/ui/w...
File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right):

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/browser/ui/w...
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:188: return
&NewWebUI<GestureConfigUI>;
Moved to AURA block.
On 2012/06/06 14:50:54, flackr wrote:
> Move to ChromeOS specific block below.

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/browser/ui/w...
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:188: return
&NewWebUI<GestureConfigUI>;
Moved to AURA block.
On 2012/06/06 14:50:54, flackr wrote:
> Move to ChromeOS specific block below.

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/common/url_c...
File chrome/common/url_constants.h (right):

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/common/url_c...
chrome/common/url_constants.h:43: extern const char
kChromeUIGestureConfigHost[];
Moved to AURA block.
On 2012/06/06 14:50:54, flackr wrote:
> The Host variables are defined in a separate block below. Also these should
only
> be defined on CHROMEOS

[flackr, 2012-06-07 21:07:54]

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/browser/reso...
File chrome/browser/resources/gesture_config.js (right):

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/browser/reso...
chrome/browser/resources/gesture_config.js:121: buf.push('</div>');
I don't think this should be that bad, you would just clone the node, and
populate fields / attributes (some only if you have data for them). Then perhaps
add a class to toggle visibility of the units div.

This way you could see and edit the structure of a row in the HTML file.

On 2012/06/07 19:07:00, Kevin Greer wrote:
> Given all of the conditional logic in the formatting, I think it would be a
lot
> more trouble than it would be worth.
> On 2012/06/06 14:50:54, flackr wrote:
> > Can you define and clone a template DOM node rather than constructing an
HTML
> > string?
>

https://chromiumcodereview.appspot.com/10532005/diff/12001/chrome/browser/bro...
File chrome/browser/browser_resources.grd (right):

https://chromiumcodereview.appspot.com/10532005/diff/12001/chrome/browser/bro...
chrome/browser/browser_resources.grd:196: <include name="IDR_GESTURE_CONFIG_CSS"
file="resources\gesture_config.css" type="BINDATA" />
nit: Put this before the html file to be in sorted order.

https://chromiumcodereview.appspot.com/10532005/diff/12001/chrome/browser/res...
File chrome/browser/resources/gesture_config.css (right):

https://chromiumcodereview.appspot.com/10532005/diff/12001/chrome/browser/res...
chrome/browser/resources/gesture_config.css:7: }
What's this for?

https://chromiumcodereview.appspot.com/10532005/diff/12001/chrome/browser/res...
chrome/browser/resources/gesture_config.css:21: }
We probably want to include shared/css/widgets.css for consistent input and
button styles.

https://chromiumcodereview.appspot.com/10532005/diff/12001/chrome/browser/res...
chrome/browser/resources/gesture_config.css:32: color: rgb(48, 57,66);
nit: Space before 66.

https://chromiumcodereview.appspot.com/10532005/diff/12001/chrome/browser/ui/...
File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right):

https://chromiumcodereview.appspot.com/10532005/diff/12001/chrome/browser/ui/...
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:29: #include
"chrome/browser/ui/webui/gesture_config_ui.h"
Move include to if defined as well.

[Kevin Greer, 2012-06-08 11:51:25]

Addressed review comments except use of a DOM template, which I'll address in
next CL.

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/browser/reso...
File chrome/browser/resources/gesture_config.js (right):

https://chromiumcodereview.appspot.com/10532005/diff/6001/chrome/browser/reso...
chrome/browser/resources/gesture_config.js:121: buf.push('</div>');
Will do in next CL.
On 2012/06/07 21:07:54, flackr wrote:
> I don't think this should be that bad, you would just clone the node, and
> populate fields / attributes (some only if you have data for them). Then
perhaps
> add a class to toggle visibility of the units div.
> 
> This way you could see and edit the structure of a row in the HTML file.
> 
> On 2012/06/07 19:07:00, Kevin Greer wrote:
> > Given all of the conditional logic in the formatting, I think it would be a
> lot
> > more trouble than it would be worth.
> > On 2012/06/06 14:50:54, flackr wrote:
> > > Can you define and clone a template DOM node rather than constructing an
> HTML
> > > string?
> >

https://chromiumcodereview.appspot.com/10532005/diff/12001/chrome/browser/bro...
File chrome/browser/browser_resources.grd (right):

https://chromiumcodereview.appspot.com/10532005/diff/12001/chrome/browser/bro...
chrome/browser/browser_resources.grd:196: <include name="IDR_GESTURE_CONFIG_CSS"
file="resources\gesture_config.css" type="BINDATA" />
On 2012/06/07 21:07:54, flackr wrote:
> nit: Put this before the html file to be in sorted order.

Done.

https://chromiumcodereview.appspot.com/10532005/diff/12001/chrome/browser/res...
File chrome/browser/resources/gesture_config.css (right):

https://chromiumcodereview.appspot.com/10532005/diff/12001/chrome/browser/res...
chrome/browser/resources/gesture_config.css:21: }
Ironically, if I do that, then the button no longer matches the style of the
buttons on the other settings screens.

On 2012/06/07 21:07:54, flackr wrote:
> We probably want to include shared/css/widgets.css for consistent input and
> button styles.

https://chromiumcodereview.appspot.com/10532005/diff/12001/chrome/browser/res...
chrome/browser/resources/gesture_config.css:32: color: rgb(48, 57,66);
Removed.
On 2012/06/07 21:07:54, flackr wrote:
> nit: Space before 66.

[Kevin Greer, 2012-06-08 12:10:34]

Missed one comment.

https://chromiumcodereview.appspot.com/10532005/diff/12001/chrome/browser/res...
File chrome/browser/resources/gesture_config.css (right):

https://chromiumcodereview.appspot.com/10532005/diff/12001/chrome/browser/res...
chrome/browser/resources/gesture_config.css:7: }
I set the font globally rather than repeating it in multiple CSS rules.
On 2012/06/07 21:07:54, flackr wrote:
> What's this for?

[Kevin Greer, 2012-06-08 20:11:31]

Now addresses Rob's last outstanding comment and uses a DOM template.

[flackr, 2012-06-11 14:27:35]

Looking good, a few nits.

http://codereview.chromium.org/10532005/diff/4003/chrome/browser/resources/ge...
File chrome/browser/resources/gesture_config.css (right):

http://codereview.chromium.org/10532005/diff/4003/chrome/browser/resources/ge...
chrome/browser/resources/gesture_config.css:7: }
Just define this once on the body, it should be inherited by all elements below
automatically.

http://codereview.chromium.org/10532005/diff/4003/chrome/browser/resources/ge...
chrome/browser/resources/gesture_config.css:42: font-size: 12px;
Is the default (body) font size not 12? if not? Can it be?

http://codereview.chromium.org/10532005/diff/4003/chrome/browser/resources/ge...
File chrome/browser/resources/gesture_config.html (right):

http://codereview.chromium.org/10532005/diff/4003/chrome/browser/resources/ge...
chrome/browser/resources/gesture_config.html:2: <html
i18n-values="dir:textdirection">
i18n-values will do nothing if you don't include shared/js/i18n_template.js and
call i18nTemplate.process(document, templateData). I assume given that you're
not using localized strings there's no need to support rtl either.

[Kevin Greer, 2012-06-11 17:47:11]

Addresses latest review.  Adds tooltips to show default values in case user
wants to revert individual fields.

[flackr, 2012-06-11 18:06:52]

https://chromiumcodereview.appspot.com/10532005/diff/2004/chrome/browser/reso...
File chrome/browser/resources/gesture_config.js (right):

https://chromiumcodereview.appspot.com/10532005/diff/2004/chrome/browser/reso...
chrome/browser/resources/gesture_config.js:140: label.innerHTML = field.label;
Prefer .textContent over .innerHTML (below as well).

https://chromiumcodereview.appspot.com/10532005/diff/2004/chrome/browser/reso...
chrome/browser/resources/gesture_config.js:152: $('gesture-form-row').outerHTML
= '';
Can you avoid having to delete this if you put 'gesture-form-row' in a <div
class="templates"> somewhere (at the end of the body is the convention) where
you have the CSS .templates { display: none; }?

https://chromiumcodereview.appspot.com/10532005/diff/2004/chrome/browser/reso...
chrome/browser/resources/gesture_config.js:214: }
nit: no braces necessary here.

[Kevin Greer, 2012-06-11 19:56:32]

Addressed latest review comments.

https://chromiumcodereview.appspot.com/10532005/diff/2004/chrome/browser/reso...
File chrome/browser/resources/gesture_config.js (right):

https://chromiumcodereview.appspot.com/10532005/diff/2004/chrome/browser/reso...
chrome/browser/resources/gesture_config.js:140: label.innerHTML = field.label;
Changed.
On 2012/06/11 18:06:52, flackr wrote:
> Prefer .textContent over .innerHTML (below as well).

https://chromiumcodereview.appspot.com/10532005/diff/2004/chrome/browser/reso...
chrome/browser/resources/gesture_config.js:152: $('gesture-form-row').outerHTML
= '';
I've changed it to the same design used by ntp4 and put the template in a <div
hidden>.
On 2012/06/11 18:06:52, flackr wrote:
> Can you avoid having to delete this if you put 'gesture-form-row' in a <div
> class="templates"> somewhere (at the end of the body is the convention) where
> you have the CSS .templates { display: none; }?

https://chromiumcodereview.appspot.com/10532005/diff/2004/chrome/browser/reso...
chrome/browser/resources/gesture_config.js:214: }
Removed.
On 2012/06/11 18:06:52, flackr wrote:
> nit: no braces necessary here.

[James Hawkins, 2012-06-12 15:39:00]

Note this was sent to my @google.com address, which is why it didn't hit my
inbox.

[rjkroege, 2012-06-12 15:42:36]

gesture interface: lgtm.

[James Hawkins, 2012-06-12 15:48:14]

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/reso...
File chrome/browser/resources/gesture_config.css (right):

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/reso...
chrome/browser/resources/gesture_config.css:11: background-image:
-webkit-linear-gradient(#E7E7E7, #E7E7E7 38%, #D7D7D7);
nit: rgb should be lowercase.

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/reso...
chrome/browser/resources/gesture_config.css:23: 
nit: Remove extra blank line.

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/reso...
chrome/browser/resources/gesture_config.css:40: display: block;
nit: Wrap the input in a div if you want it to block.

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/reso...
chrome/browser/resources/gesture_config.css:50: display: block;
nit: Wrap the label in a div if you want it to block.

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/reso...
chrome/browser/resources/gesture_config.css:60: display: block;
nit: display: block is unnecessary since row-units is already a div.

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/reso...
File chrome/browser/resources/gesture_config.html (right):

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/reso...
chrome/browser/resources/gesture_config.html:5: <title>Gesture
Preferences</title>
Title needs to be internationalized.

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/reso...
chrome/browser/resources/gesture_config.html:10: <h2>Gesture Preferences</h2>
Internationalized.

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/reso...
chrome/browser/resources/gesture_config.html:11: <hr/>
No trailing slash on self-closing blocks.

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/reso...
chrome/browser/resources/gesture_config.html:14: 
nit: Remove blank line.

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/reso...
chrome/browser/resources/gesture_config.html:16: <button
id="resetButton">Reset</button>
Internationalized.

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/reso...
chrome/browser/resources/gesture_config.html:21: <label
class="row-label"></label>
nit: <label> needs a for= attribute specified.

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/reso...
chrome/browser/resources/gesture_config.html:22: <input class="row-input"
type="number" size="20"/>
No trailing slash on self-closing blocks:

<input ...>

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/reso...
File chrome/browser/resources/gesture_config.js (right):

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/reso...
chrome/browser/resources/gesture_config.js:18: var GESTURE_PREFIX = 'gesture.';
/** const */

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/ui/w...
File chrome/browser/ui/webui/gesture_config_ui.cc (right):

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/ui/w...
chrome/browser/ui/webui/gesture_config_ui.cc:87:
GestureConfigUI::~GestureConfigUI() {
Method ordering in the implementation does not match the ordering in the
interface.

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/ui/w...
File chrome/browser/ui/webui/gesture_config_ui.h (right):

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/ui/w...
chrome/browser/ui/webui/gesture_config_ui.h:13: } // namespace base
nit: Two spaces before start of comment.

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/ui/w...
chrome/browser/ui/webui/gesture_config_ui.h:15: // The WebUI for
chrome://gesture
nit: Missing period at end of sentence.

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/ui/w...
chrome/browser/ui/webui/gesture_config_ui.h:18: explicit
GestureConfigUI(content::WebUI* web_ui);
nit: Document |web_ui|.

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/ui/w...
chrome/browser/ui/webui/gesture_config_ui.h:20: void GetPreferenceValue(const
base::ListValue* args);
nit: Document methods.

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/ui/w...
chrome/browser/ui/webui/gesture_config_ui.h:20: void GetPreferenceValue(const
base::ListValue* args);
nit: These don't need to be public, I imagine.

[Kevin Greer, 2012-06-12 18:01:53]

Please review.

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/reso...
File chrome/browser/resources/gesture_config.css (right):

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/reso...
chrome/browser/resources/gesture_config.css:11: background-image:
-webkit-linear-gradient(#E7E7E7, #E7E7E7 38%, #D7D7D7);
On 2012/06/12 15:48:14, James Hawkins wrote:
> nit: rgb should be lowercase.

Done.

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/reso...
chrome/browser/resources/gesture_config.css:23: 
On 2012/06/12 15:48:14, James Hawkins wrote:
> nit: Remove extra blank line.

Done.

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/reso...
chrome/browser/resources/gesture_config.css:40: display: block;
Removed. Not needed.
On 2012/06/12 15:48:14, James Hawkins wrote:
> nit: Wrap the input in a div if you want it to block.

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/reso...
chrome/browser/resources/gesture_config.css:50: display: block;
Removed.
On 2012/06/12 15:48:14, James Hawkins wrote:
> nit: Wrap the label in a div if you want it to block.

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/reso...
chrome/browser/resources/gesture_config.css:60: display: block;
Removed
On 2012/06/12 15:48:14, James Hawkins wrote:
> nit: display: block is unnecessary since row-units is already a div.

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/reso...
File chrome/browser/resources/gesture_config.html (right):

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/reso...
chrome/browser/resources/gesture_config.html:5: <title>Gesture
Preferences</title>
This is an unsupported feature which does not need internationalization.
On 2012/06/12 15:48:14, James Hawkins wrote:
> Title needs to be internationalized.

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/reso...
chrome/browser/resources/gesture_config.html:10: <h2>Gesture Preferences</h2>
Not needed.
On 2012/06/12 15:48:14, James Hawkins wrote:
> Internationalized.

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/reso...
chrome/browser/resources/gesture_config.html:11: <hr/>
Removed.
On 2012/06/12 15:48:14, James Hawkins wrote:
> No trailing slash on self-closing blocks.

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/reso...
chrome/browser/resources/gesture_config.html:14: 
On 2012/06/12 15:48:14, James Hawkins wrote:
> nit: Remove blank line.

Done.

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/reso...
chrome/browser/resources/gesture_config.html:16: <button
id="resetButton">Reset</button>
Not needed.
On 2012/06/12 15:48:14, James Hawkins wrote:
> Internationalized.

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/reso...
chrome/browser/resources/gesture_config.html:21: <label
class="row-label"></label>
On 2012/06/12 15:48:14, James Hawkins wrote:
> nit: <label> needs a for= attribute specified.

Done (in gesture_config.js).

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/reso...
chrome/browser/resources/gesture_config.html:22: <input class="row-input"
type="number" size="20"/>
Removed.
On 2012/06/12 15:48:14, James Hawkins wrote:
> No trailing slash on self-closing blocks:
> 
> <input ...>

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/reso...
File chrome/browser/resources/gesture_config.js (right):

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/reso...
chrome/browser/resources/gesture_config.js:18: var GESTURE_PREFIX = 'gesture.';
On 2012/06/12 15:48:14, James Hawkins wrote:
> /** const */

Done.

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/ui/w...
File chrome/browser/ui/webui/gesture_config_ui.h (right):

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/ui/w...
chrome/browser/ui/webui/gesture_config_ui.h:20: void GetPreferenceValue(const
base::ListValue* args);
Documentation moved from .cc to .h.
On 2012/06/12 15:48:14, James Hawkins wrote:
> nit: Document methods.

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/ui/w...
chrome/browser/ui/webui/gesture_config_ui.h:20: void GetPreferenceValue(const
base::ListValue* args);
Made private.
On 2012/06/12 15:48:14, James Hawkins wrote:
> nit: These don't need to be public, I imagine.

https://chromiumcodereview.appspot.com/10532005/diff/7014/chrome/browser/ui/w...
chrome/browser/ui/webui/gesture_config_ui.h:20: void GetPreferenceValue(const
base::ListValue* args);
On 2012/06/12 15:48:14, James Hawkins wrote:
> nit: Document methods.

Done.

[James Hawkins, 2012-06-12 18:04:15]

LGTM with nits.

https://chromiumcodereview.appspot.com/10532005/diff/12037/chrome/browser/res...
File chrome/browser/resources/gesture_config.html (right):

https://chromiumcodereview.appspot.com/10532005/diff/12037/chrome/browser/res...
chrome/browser/resources/gesture_config.html:15: <button
id="resetButton">Reset</button>
nit: IDs use dash-form.

https://chromiumcodereview.appspot.com/10532005/diff/12037/chrome/browser/ui/...
File chrome/browser/ui/webui/gesture_config_ui.h (right):

https://chromiumcodereview.appspot.com/10532005/diff/12037/chrome/browser/ui/...
chrome/browser/ui/webui/gesture_config_ui.h:21: private:
nit: Blank line above private.

https://chromiumcodereview.appspot.com/10532005/diff/12037/chrome/browser/ui/...
chrome/browser/ui/webui/gesture_config_ui.h:35:
DISALLOW_COPY_AND_ASSIGN(GestureConfigUI);
nit: Blank line above DISALLOW..

[flackr, 2012-06-12 18:07:00]

LGTM other than james' nits.

[Kevin Greer, 2012-06-12 18:10:02]

Fixed final nits.

https://chromiumcodereview.appspot.com/10532005/diff/12037/chrome/browser/res...
File chrome/browser/resources/gesture_config.html (right):

https://chromiumcodereview.appspot.com/10532005/diff/12037/chrome/browser/res...
chrome/browser/resources/gesture_config.html:15: <button
id="resetButton">Reset</button>
changed
On 2012/06/12 18:04:15, James Hawkins wrote:
> nit: IDs use dash-form.

https://chromiumcodereview.appspot.com/10532005/diff/12037/chrome/browser/ui/...
File chrome/browser/ui/webui/gesture_config_ui.h (right):

https://chromiumcodereview.appspot.com/10532005/diff/12037/chrome/browser/ui/...
chrome/browser/ui/webui/gesture_config_ui.h:21: private:
On 2012/06/12 18:04:15, James Hawkins wrote:
> nit: Blank line above private.

Done.

https://chromiumcodereview.appspot.com/10532005/diff/12037/chrome/browser/ui/...
chrome/browser/ui/webui/gesture_config_ui.h:35:
DISALLOW_COPY_AND_ASSIGN(GestureConfigUI);
On 2012/06/12 18:04:15, James Hawkins wrote:
> nit: Blank line above DISALLOW..

Done.

[commit-bot: I haz the power, 2012-06-12 18:10:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kgr@chromium.org/10532005/3027

[commit-bot: I haz the power, 2012-06-12 18:35:53]

Try job failure for 10532005-3027 (retry) on linux_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-06-14 18:14:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kgr@chromium.org/10532005/15005

[commit-bot: I haz the power, 2012-06-14 19:35:58]

Try job failure for 10532005-15005 (retry) on win_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-06-14 20:20:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kgr@chromium.org/10532005/6007

[commit-bot: I haz the power, 2012-06-14 22:02:22]

Change committed as 142247