Implement the 'Clear Browser Data' dom-ui overlay....

Implement the 'Clear Browser Data' dom-ui overlay.

BUG=49037
TEST=Exercise the Clear Browser Data dialog with --enable-tabbed-options.

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

[csilv, 2010-07-21 20:40:31]

+zel for review.

[arv (Not doing code reviews), 2010-07-21 21:13:08]

http://codereview.chromium.org/2856053/diff/3002/12005
File chrome/browser/resources/options/clear_browser_data_overlay.css (right):

http://codereview.chromium.org/2856053/diff/3002/12005#newcode5
chrome/browser/resources/options/clear_browser_data_overlay.css:5:
background-position: 0px;
no need for this one (this is the default)

http://codereview.chromium.org/2856053/diff/3002/12006
File chrome/browser/resources/options/clear_browser_data_overlay.html (right):

http://codereview.chromium.org/2856053/diff/3002/12006#newcode45
chrome/browser/resources/options/clear_browser_data_overlay.html:45: <span
id="cbdThrobberContainer" style="visibility:hidden">
move to css

http://codereview.chromium.org/2856053/diff/3002/12006#newcode46
chrome/browser/resources/options/clear_browser_data_overlay.html:46: <span
id="cbdThrobber">
do we really need 2 spans here? Can you use the outer one?

http://codereview.chromium.org/2856053/diff/3002/12007
File chrome/browser/resources/options/clear_browser_data_overlay.js (right):

http://codereview.chromium.org/2856053/diff/3002/12007#newcode55
chrome/browser/resources/options/clear_browser_data_overlay.js:55:
$('cbdThrobberContainer').style.visibility = state ? "visible" : "hidden";
use single quotes for consistency

http://codereview.chromium.org/2856053/diff/3002/12007#newcode60
chrome/browser/resources/options/clear_browser_data_overlay.js:60:
((parseInt(getComputedStyle(throbber).backgroundPositionX) - 16) %
parseFloat or parseInt(str, 10)

http://codereview.chromium.org/2856053/diff/3002/12007#newcode60
chrome/browser/resources/options/clear_browser_data_overlay.js:60:
((parseInt(getComputedStyle(throbber).backgroundPositionX) - 16) %
Can you make this time based? (Not required but it does give much smoother
animations)

http://codereview.chromium.org/2856053/diff/3002/12008
File chrome/browser/resources/options/pref_ui.js (right):

http://codereview.chromium.org/2856053/diff/3002/12008#newcode112
chrome/browser/resources/options/pref_ui.js:112: if (this.dataType == "number")
{
single quotes

Maybe use a switch?

[csilv, 2010-07-23 00:08:19]

http://codereview.chromium.org/2856053/diff/3002/12005
File chrome/browser/resources/options/clear_browser_data_overlay.css (right):

http://codereview.chromium.org/2856053/diff/3002/12005#newcode5
chrome/browser/resources/options/clear_browser_data_overlay.css:5:
background-position: 0px;
On 2010/07/21 21:13:09, arv wrote:
> no need for this one (this is the default)

Done.

http://codereview.chromium.org/2856053/diff/3002/12006
File chrome/browser/resources/options/clear_browser_data_overlay.html (right):

http://codereview.chromium.org/2856053/diff/3002/12006#newcode45
chrome/browser/resources/options/clear_browser_data_overlay.html:45: <span
id="cbdThrobberContainer" style="visibility:hidden">
On 2010/07/21 21:13:09, arv wrote:
> move to css

Done.

http://codereview.chromium.org/2856053/diff/3002/12006#newcode46
chrome/browser/resources/options/clear_browser_data_overlay.html:46: <span
id="cbdThrobber">
On 2010/07/21 21:13:09, arv wrote:
> do we really need 2 spans here? Can you use the outer one?

Done.

http://codereview.chromium.org/2856053/diff/3002/12007
File chrome/browser/resources/options/clear_browser_data_overlay.js (right):

http://codereview.chromium.org/2856053/diff/3002/12007#newcode55
chrome/browser/resources/options/clear_browser_data_overlay.js:55:
$('cbdThrobberContainer').style.visibility = state ? "visible" : "hidden";
On 2010/07/21 21:13:09, arv wrote:
> use single quotes for consistency

Done.

http://codereview.chromium.org/2856053/diff/3002/12007#newcode60
chrome/browser/resources/options/clear_browser_data_overlay.js:60:
((parseInt(getComputedStyle(throbber).backgroundPositionX) - 16) %
On 2010/07/21 21:13:09, arv wrote:
> parseFloat or parseInt(str, 10)

Done.

http://codereview.chromium.org/2856053/diff/3002/12007#newcode60
chrome/browser/resources/options/clear_browser_data_overlay.js:60:
((parseInt(getComputedStyle(throbber).backgroundPositionX) - 16) %
On 2010/07/21 21:13:09, arv wrote:
> Can you make this time based? (Not required but it does give much smoother
> animations)

I'll come back to this.  In local testing the animation is pretty smooth, but I
can see how using a time calculation could make it smoother.  FYI, this
technique was borrowed from the Sync UI.

http://codereview.chromium.org/2856053/diff/3002/12008
File chrome/browser/resources/options/pref_ui.js (right):

http://codereview.chromium.org/2856053/diff/3002/12008#newcode112
chrome/browser/resources/options/pref_ui.js:112: if (this.dataType == "number")
{
On 2010/07/21 21:13:09, arv wrote:
> single quotes
> 
> Maybe use a switch?

Done.

[arv (Not doing code reviews), 2010-07-23 00:13:56]

LGTM

http://codereview.chromium.org/2856053/diff/20001/10007
File chrome/browser/resources/options/clear_browser_data_overlay.html (right):

http://codereview.chromium.org/2856053/diff/20001/10007#newcode55
chrome/browser/resources/options/clear_browser_data_overlay.html:55: <span
i18n-content="flash_storage_settings"></span></a>
You can skip the span here.

[csilv, 2010-07-23 00:33:54]

http://codereview.chromium.org/2856053/diff/20001/10007
File chrome/browser/resources/options/clear_browser_data_overlay.html (right):

http://codereview.chromium.org/2856053/diff/20001/10007#newcode55
chrome/browser/resources/options/clear_browser_data_overlay.html:55: <span
i18n-content="flash_storage_settings"></span></a>
On 2010/07/23 00:13:57, arv wrote:
> You can skip the span here.

Done.

[zel, 2010-07-23 01:18:14]

http://codereview.chromium.org/2856053/diff/14002/27001
File chrome/browser/dom_ui/clear_browser_data_handler.cc (right):

http://codereview.chromium.org/2856053/diff/14002/27001#newcode115
chrome/browser/dom_ui/clear_browser_data_handler.cc:115:
remover_->AddObserver(this);
I don't see you removing this observer in this class' destructor. If the remover
does not finish before we close options page, we can get AV.

[csilv, 2010-07-26 19:21:52]

http://codereview.chromium.org/2856053/diff/14002/27001
File chrome/browser/dom_ui/clear_browser_data_handler.cc (right):

http://codereview.chromium.org/2856053/diff/14002/27001#newcode115
chrome/browser/dom_ui/clear_browser_data_handler.cc:115:
remover_->AddObserver(this);
On 2010/07/23 01:18:14, zel wrote:
> I don't see you removing this observer in this class' destructor. If the
remover
> does not finish before we close options page, we can get AV.

Done. (will be put in new CL).