Incorporate reset button in post-automatic-reset settings-banner.

Incorporate reset button in post-automatic-reset settings-banner.

(see bug for details and design doc)

BUG=397248
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=286265

[gab, 2014-07-24 20:42:28]

Alexei PTAL @ histograms
Robert PTAL at code (since you wrote it) before I send to true UI peoplez.

[Alexei Svitkine (slow), 2014-07-24 20:44:09]

lgtm

[robertshield, 2014-07-28 21:09:42]

lgtm

https://codereview.chromium.org/411273006/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/automatic_settings_reset_banner.js
(right):

https://codereview.chromium.org/411273006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/automatic_settings_reset_banner.js:47: };
not sure, but seems like this should line up with the $( on line 42.

[gab, 2014-07-28 21:22:53]

Bernhard PTAL (as owner and reviewer of initial CL @
https://codereview.chromium.org/151003004).

Also see question about JS style below.

Thanks!
Gab

https://codereview.chromium.org/411273006/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/automatic_settings_reset_banner.js
(right):

https://codereview.chromium.org/411273006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/automatic_settings_reset_banner.js:47: };
On 2014/07/28 21:09:42, robertshield wrote:
> not sure, but seems like this should line up with the $( on line 42. 

Wasn't sure either but since I indented "function(event) {" above so I indented
this "}" to match that.

I couldn't find an example of such a wrapping caused by the onclick target's
name being too long...

UI owners will know I assume.

[Bernhard Bauer, 2014-07-28 22:29:38]

LGTM with the formatting nit below.

https://codereview.chromium.org/411273006/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/automatic_settings_reset_banner.js
(right):

https://codereview.chromium.org/411273006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/automatic_settings_reset_banner.js:47: };
On 2014/07/28 21:22:52, gab wrote:
> On 2014/07/28 21:09:42, robertshield wrote:
> > not sure, but seems like this should line up with the $( on line 42. 
> 
> Wasn't sure either but since I indented "function(event) {" above so I
indented
> this "}" to match that.
> 
> I couldn't find an example of such a wrapping caused by the onclick target's
> name being too long...
> 
> UI owners will know I assume.

I think what you do here is indent line 43 four spaces, then continue with two
space indent for the rest of the function (and then no space for the closing
brace and semicolon).

[gab, 2014-07-29 14:54:40]

https://codereview.chromium.org/411273006/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/automatic_settings_reset_banner.js
(right):

https://codereview.chromium.org/411273006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/automatic_settings_reset_banner.js:47: };
On 2014/07/28 22:29:38, Bernhard Bauer wrote:
> On 2014/07/28 21:22:52, gab wrote:
> > On 2014/07/28 21:09:42, robertshield wrote:
> > > not sure, but seems like this should line up with the $( on line 42. 
> > 
> > Wasn't sure either but since I indented "function(event) {" above so I
> indented
> > this "}" to match that.
> > 
> > I couldn't find an example of such a wrapping caused by the onclick target's
> > name being too long...
> > 
> > UI owners will know I assume.
> 
> I think what you do here is indent line 43 four spaces, then continue with two
> space indent for the rest of the function (and then no space for the closing
> brace and semicolon).

Okay so just like I have it, but bring the closing "};" back.

Done.

[gab, 2014-07-29 14:55:23]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2014-07-29 14:56:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gab@chromium.org/411273006/20001

[Bernhard Bauer, 2014-07-29 15:01:14]

The CQ bit was unchecked by bauerb@chromium.org

[Bernhard Bauer, 2014-07-29 15:01:43]

Sorry, I have to stop your CQ here :)

https://codereview.chromium.org/411273006/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/automatic_settings_reset_banner.js
(right):

https://codereview.chromium.org/411273006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/automatic_settings_reset_banner.js:47: };
On 2014/07/29 14:54:40, gab wrote:
> On 2014/07/28 22:29:38, Bernhard Bauer wrote:
> > On 2014/07/28 21:22:52, gab wrote:
> > > On 2014/07/28 21:09:42, robertshield wrote:
> > > > not sure, but seems like this should line up with the $( on line 42. 
> > > 
> > > Wasn't sure either but since I indented "function(event) {" above so I
> > indented
> > > this "}" to match that.
> > > 
> > > I couldn't find an example of such a wrapping caused by the onclick
target's
> > > name being too long...
> > > 
> > > UI owners will know I assume.
> > 
> > I think what you do here is indent line 43 four spaces, then continue with
two
> > space indent for the rest of the function (and then no space for the closing
> > brace and semicolon).
> 
> Okay so just like I have it, but bring the closing "};" back.
> 
> Done.

Sorry, what I meant was this:

$('foo').onclick =
    function() {
  chrome.send('moose!');
};

[gab, 2014-07-29 15:41:55]

https://codereview.chromium.org/411273006/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/automatic_settings_reset_banner.js
(right):

https://codereview.chromium.org/411273006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/automatic_settings_reset_banner.js:47: };
On 2014/07/29 15:01:43, Bernhard Bauer wrote:
> On 2014/07/29 14:54:40, gab wrote:
> > On 2014/07/28 22:29:38, Bernhard Bauer wrote:
> > > On 2014/07/28 21:22:52, gab wrote:
> > > > On 2014/07/28 21:09:42, robertshield wrote:
> > > > > not sure, but seems like this should line up with the $( on line 42. 
> > > > 
> > > > Wasn't sure either but since I indented "function(event) {" above so I
> > > indented
> > > > this "}" to match that.
> > > > 
> > > > I couldn't find an example of such a wrapping caused by the onclick
> target's
> > > > name being too long...
> > > > 
> > > > UI owners will know I assume.
> > > 
> > > I think what you do here is indent line 43 four spaces, then continue with
> two
> > > space indent for the rest of the function (and then no space for the
closing
> > > brace and semicolon).
> > 
> > Okay so just like I have it, but bring the closing "};" back.
> > 
> > Done.
> 
> Sorry, what I meant was this:
> 
> $('foo').onclick =
>     function() {
>   chrome.send('moose!');
> };

Ah I see, thanks for clarifying, done!

[gab, 2014-07-29 15:42:10]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2014-07-29 15:42:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gab@chromium.org/411273006/40001

[commit-bot: I haz the power, 2014-07-29 18:30:36]

Change committed as 286265