Reland r55881: Disable the 'Clear browsing Data' button when no checkboxes are checked.

Reland r55881: Disable the 'Clear browsing Data' button when no checkboxes are checked.

BUG=49037
TEST=out/Debug/chrome --enable-tabbed-options. Go to 'Under the Hood' tab, open the
dialog, check some checkbox, the button should become enabled, uncheck all the
checkboxes, the button should become disabled.

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

[tfarina, 2010-08-14 22:32:08]

Please, Eric, Stuart, review this to me.

I think I have addressed all your concerns.

Thanks in advance.

[arv (Not doing code reviews), 2010-08-16 17:17:39]

http://codereview.chromium.org/3143012/diff/2001/3003
File chrome/browser/resources/options/clear_browser_data_overlay.js (right):

http://codereview.chromium.org/3143012/diff/2001/3003#newcode39
chrome/browser/resources/options/clear_browser_data_overlay.js:39:
Preferences.getInstance().addEventListener(
I think a loop would be cleaner here.

var f = cr.bind(this.updateButtonState_, this);
var types = ['browser.clear_data.browsing_history', ...];
types.forEach(function(type) {
  Preferences.getInstance().addEventListener(type, f);
});

http://codereview.chromium.org/3143012/diff/2001/3003#newcode60
chrome/browser/resources/options/clear_browser_data_overlay.js:60: var handler =
cr.bind(this.updateButtonState_, this);
This is same as f in my previous comment. You can reuse this.

http://codereview.chromium.org/3143012/diff/2001/3003#newcode75
chrome/browser/resources/options/clear_browser_data_overlay.js:75: var
is_checked = false;
no underscores in js

[stuartmorgan, 2010-08-16 19:10:42]

http://codereview.chromium.org/3143012/diff/2001/3001
File chrome/browser/resources/options/clear_browser_data_overlay.css (right):

http://codereview.chromium.org/3143012/diff/2001/3001#newcode4
chrome/browser/resources/options/clear_browser_data_overlay.css:4: */
Why add this? The core DOMUI CSS files don't have it.

[tfarina, 2010-08-17 04:35:06]

On 2010/08/16 19:10:42, stuartmorgan wrote:
> http://codereview.chromium.org/3143012/diff/2001/3001
> File chrome/browser/resources/options/clear_browser_data_overlay.css (right):
> 
> http://codereview.chromium.org/3143012/diff/2001/3001#newcode4
> chrome/browser/resources/options/clear_browser_data_overlay.css:4: */
> Why add this? The core DOMUI CSS files don't have it.

Removed, but why not? All the source files in our code base has a license
header. Why just the .css files shouldn't have one?

[tfarina, 2010-08-17 04:35:22]

http://codereview.chromium.org/3143012/diff/2001/3003
File chrome/browser/resources/options/clear_browser_data_overlay.js (right):

http://codereview.chromium.org/3143012/diff/2001/3003#newcode39
chrome/browser/resources/options/clear_browser_data_overlay.js:39:
Preferences.getInstance().addEventListener(
On 2010/08/16 17:17:39, arv wrote:
> I think a loop would be cleaner here.
> 
> var f = cr.bind(this.updateButtonState_, this);
> var types = ['browser.clear_data.browsing_history', ...];
> types.forEach(function(type) {
>   Preferences.getInstance().addEventListener(type, f);
> });

Done.

http://codereview.chromium.org/3143012/diff/2001/3003#newcode60
chrome/browser/resources/options/clear_browser_data_overlay.js:60: var handler =
cr.bind(this.updateButtonState_, this);
On 2010/08/16 17:17:39, arv wrote:
> This is same as f in my previous comment. You can reuse this.

Done.

http://codereview.chromium.org/3143012/diff/2001/3003#newcode75
chrome/browser/resources/options/clear_browser_data_overlay.js:75: var
is_checked = false;
On 2010/08/16 17:17:39, arv wrote:
> no underscores in js

Done.

[stuartmorgan, 2010-08-17 22:21:00]

http://codereview.chromium.org/3143012/diff/10001/9003
File chrome/browser/resources/options/clear_browser_data_overlay.html (right):

http://codereview.chromium.org/3143012/diff/10001/9003#newcode36
chrome/browser/resources/options/clear_browser_data_overlay.html:36: <table
class="option-control-table">
As long as you are de-tabling this file, you should do the rest of the file as
well; there's no reason for it to use tables later either.

http://codereview.chromium.org/3143012/diff/10001/9005
File chrome/browser/resources/options/import_data_overlay.html (right):

http://codereview.chromium.org/3143012/diff/10001/9005#newcode26
chrome/browser/resources/options/import_data_overlay.html:26: <button
id="import-data-commit" i18n-content="import_commit"></button>
Rather than changing this from Mac style to Windows style, please use platform
conditions to have it be in the right place on all platforms. See add_start_page
for an example.

[tfarina, 2010-08-20 13:54:14]

Sorry for the delay. Please, could you take another look?

http://codereview.chromium.org/3143012/diff/10001/9003
File chrome/browser/resources/options/clear_browser_data_overlay.html (right):

http://codereview.chromium.org/3143012/diff/10001/9003#newcode36
chrome/browser/resources/options/clear_browser_data_overlay.html:36: <table
class="option-control-table">
On 2010/08/17 22:21:00, stuartmorgan wrote:
> As long as you are de-tabling this file, you should do the rest of the file as
> well; there's no reason for it to use tables later either.

Done.

http://codereview.chromium.org/3143012/diff/10001/9005
File chrome/browser/resources/options/import_data_overlay.html (right):

http://codereview.chromium.org/3143012/diff/10001/9005#newcode26
chrome/browser/resources/options/import_data_overlay.html:26: <button
id="import-data-commit" i18n-content="import_commit"></button>
On 2010/08/17 22:21:00, stuartmorgan wrote:
> Rather than changing this from Mac style to Windows style, please use platform
> conditions to have it be in the right place on all platforms. See
add_start_page
> for an example.

Done.

[stuartmorgan, 2010-08-20 18:19:22]

http://codereview.chromium.org/3143012/diff/18001/19002
File chrome/browser/resources/options/clear_browser_data_overlay.html (right):

http://codereview.chromium.org/3143012/diff/18001/19002#newcode4
chrome/browser/resources/options/clear_browser_data_overlay.html:4: <div
id="checkboxListData">
This entire block is indented too far.

http://codereview.chromium.org/3143012/diff/18001/19002#newcode41
chrome/browser/resources/options/clear_browser_data_overlay.html:41:
</label><br>
Make the label block, or wrap it in a div, rather than using <br>

http://codereview.chromium.org/3143012/diff/18001/19002#newcode43
chrome/browser/resources/options/clear_browser_data_overlay.html:43: <if
expr="os == 'win32' or os == 'darwin'">
This is wrong; Mac should be [cancel] [commit]. (I'm not sure what Linux style
is.)

http://codereview.chromium.org/3143012/diff/18001/19002#newcode50
chrome/browser/resources/options/clear_browser_data_overlay.html:50: <if
expr="os == 'linux2'">
Also, I don't like that the button conditions aren't negations of eachother; it
needs to be impossible to end up without a commit button.

http://codereview.chromium.org/3143012/diff/18001/19004
File chrome/browser/resources/options/import_data_overlay.html (right):

http://codereview.chromium.org/3143012/diff/18001/19004#newcode26
chrome/browser/resources/options/import_data_overlay.html:26: <if expr="os ==
'win32' or os == 'darwin'">
Same here.

[tfarina, 2010-08-21 00:07:15]

Ready for another round of review.

http://codereview.chromium.org/3143012/diff/18001/19002
File chrome/browser/resources/options/clear_browser_data_overlay.html (right):

http://codereview.chromium.org/3143012/diff/18001/19002#newcode4
chrome/browser/resources/options/clear_browser_data_overlay.html:4: <div
id="checkboxListData">
On 2010/08/20 18:19:22, stuartmorgan wrote:
> This entire block is indented too far.

Done.

http://codereview.chromium.org/3143012/diff/18001/19002#newcode41
chrome/browser/resources/options/clear_browser_data_overlay.html:41:
</label><br>
On 2010/08/20 18:19:22, stuartmorgan wrote:
> Make the label block, or wrap it in a div, rather than using <br>

Done.

http://codereview.chromium.org/3143012/diff/18001/19002#newcode43
chrome/browser/resources/options/clear_browser_data_overlay.html:43: <if
expr="os == 'win32' or os == 'darwin'">
On 2010/08/20 18:19:22, stuartmorgan wrote:
> This is wrong; Mac should be [cancel] [commit]. (I'm not sure what Linux style
> is.)
Linux uses the same style of Mac [cancel] [commit].

http://codereview.chromium.org/3143012/diff/18001/19004
File chrome/browser/resources/options/import_data_overlay.html (right):

http://codereview.chromium.org/3143012/diff/18001/19004#newcode26
chrome/browser/resources/options/import_data_overlay.html:26: <if expr="os ==
'win32' or os == 'darwin'">
On 2010/08/20 18:19:22, stuartmorgan wrote:
> Same here.

Done.

[stuartmorgan, 2010-08-23 23:01:56]

http://codereview.chromium.org/3143012/diff/18001/19002
File chrome/browser/resources/options/clear_browser_data_overlay.html (right):

http://codereview.chromium.org/3143012/diff/18001/19002#newcode50
chrome/browser/resources/options/clear_browser_data_overlay.html:50: <if
expr="os == 'linux2'">
On 2010/08/20 18:19:22, stuartmorgan wrote:
> Also, I don't like that the button conditions aren't negations of eachother;
it
> needs to be impossible to end up without a commit button.

You didn't address this comment.

[tfarina, 2010-08-23 23:48:43]

http://codereview.chromium.org/3143012/diff/23001/24002
File chrome/browser/resources/options/clear_browser_data_overlay.html (right):

http://codereview.chromium.org/3143012/diff/23001/24002#newcode52
chrome/browser/resources/options/clear_browser_data_overlay.html:52: <if
expr="os == 'linux2' or os == 'darwin'">
What you want here is the negation:

<if expr="os != 'win32'">

?

[stuartmorgan, 2010-08-23 23:57:58]

On 2010/08/23 23:48:43, tfarina wrote:
> http://codereview.chromium.org/3143012/diff/23001/24002
> File chrome/browser/resources/options/clear_browser_data_overlay.html (right):
> 
> http://codereview.chromium.org/3143012/diff/23001/24002#newcode52
> chrome/browser/resources/options/clear_browser_data_overlay.html:52: <if
> expr="os == 'linux2' or os == 'darwin'">
> What you want here is the negation:
> 
> <if expr="os != 'win32'">
> 
> ?

Yes. Especially since I'm not sure 'linux2' actually covers everything, so some
platforms might have no submit button with your current change.

[tfarina, 2010-08-24 02:38:24]

On 2010/08/23 23:57:58, stuartmorgan wrote:
> Yes. Especially since I'm not sure 'linux2' actually covers everything, so
some
> platforms might have no submit button with your current change.

Done.

[stuartmorgan, 2010-08-24 16:10:34]

LGTM