Restart counters when the MD CBD dialog is reopened.

Restart counters when the MD CBD dialog is reopened.

We call Restart() on all counters in HandleInitialize(), which is called
every time the dialog is (re)opened.

Note that counters are added to ClearBrowsingDataHandler in
OnJavascriptAllowed() and removed in OnJavascriptDisallowed(), so we
know that if counters exist, JavaScript is allowed.

BUG=674040
Review-Url: https://codereview.chromium.org/2585633003
Cr-Commit-Position: refs/heads/master@{#442344}
Committed: https://chromium.googlesource.com/chromium/src/+/0eb377ef00cb7afd8392ff44f6ab94b7848b359f

[msramek, 2016-12-16 14:36:42]

Description was changed from

==========
Restart counters when the MD CBD dialog is reopened.

We call Restart() on all counters in HandleInitialize(), which is called
every time the dialog is (re)opened.

Note that counters are added to ClearBrowsingDataHandler in
OnJavascriptAllowed() and removed in OnJavascriptDisallowed(), so we
know that if counters exist, JavaScript is allowed. However, due to their
calculation being asynchronous, it is possible that JavaScript is
disallowed between the Restart() call and the callback. Therefore, we
also add a guard statement to the UpdateCounterText() method.

BUG=674040
==========

to

==========
Restart counters when the MD CBD dialog is reopened.

We call Restart() on all counters in HandleInitialize(), which is called
every time the dialog is (re)opened.

Note that counters are added to ClearBrowsingDataHandler in
OnJavascriptAllowed() and removed in OnJavascriptDisallowed(), so we
know that if counters exist, JavaScript is allowed.

BUG=674040
==========

[msramek, 2016-12-16 14:37:49]

msramek@chromium.org changed reviewers:
+ tommycli@chromium.org

[msramek, 2016-12-16 14:37:50]

Hi Tommy,

Please have a look!

Thanks,
Martin

[tommycli, 2016-12-16 17:39:14]

https://codereview.chromium.org/2585633003/diff/1/chrome/browser/ui/webui/set...
File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc
(right):

https://codereview.chromium.org/2585633003/diff/1/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:277:
counter->Restart();
how much performance penalty is it to create the counter in AllowJavascript and
then immediately restart it here?

Do counters kick off any I/O operations on a separate thread for instance?

[msramek, 2017-01-03 13:53:48]

The CQ bit was checked by msramek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-03 13:54:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msramek, 2017-01-03 13:56:39]

Sorry for the long delay, I was OOO over the holidays. I addressed your comment.

https://codereview.chromium.org/2585633003/diff/1/chrome/browser/ui/webui/set...
File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc
(right):

https://codereview.chromium.org/2585633003/diff/1/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:277:
counter->Restart();
On 2016/12/16 17:39:14, tommycli (OOO until Jan 9th) wrote:
> how much performance penalty is it to create the counter in AllowJavascript
and
> then immediately restart it here?
> 
> Do counters kick off any I/O operations on a separate thread for instance?

Oops, I didn't realize that AllowJavascript() is called in HandleInitialize()
*before* we do the restart. That would be indeed a duplication. Removing
Restart() from AddCounter() should do the trick. In the last patchset, Restart()
is called exactly once for each counter each time the dialog is opened.

And yes, counting is done asynchronously, for example reading the cache index on
the IO thread, or the passwords database on the DB thread.

[commit-bot: I haz the power, 2017-01-03 15:02:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-03 15:02:30]

Dry run: This issue passed the CQ dry run.

[tommycli, 2017-01-09 18:18:25]

lgtm

[msramek, 2017-01-09 20:11:47]

Thanks!

[msramek, 2017-01-09 20:11:51]

The CQ bit was checked by msramek@chromium.org

[commit-bot: I haz the power, 2017-01-09 20:12:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-09 21:09:27]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1483992711948350,
"parent_rev": "f6e9045bac175f482358c53e857cda6428b4700a", "commit_rev":
"0eb377ef00cb7afd8392ff44f6ab94b7848b359f"}

[commit-bot: I haz the power, 2017-01-09 21:09:56]

Description was changed from

==========
Restart counters when the MD CBD dialog is reopened.

We call Restart() on all counters in HandleInitialize(), which is called
every time the dialog is (re)opened.

Note that counters are added to ClearBrowsingDataHandler in
OnJavascriptAllowed() and removed in OnJavascriptDisallowed(), so we
know that if counters exist, JavaScript is allowed.

BUG=674040
==========

to

==========
Restart counters when the MD CBD dialog is reopened.

We call Restart() on all counters in HandleInitialize(), which is called
every time the dialog is (re)opened.

Note that counters are added to ClearBrowsingDataHandler in
OnJavascriptAllowed() and removed in OnJavascriptDisallowed(), so we
know that if counters exist, JavaScript is allowed.

BUG=674040

Review-Url: https://codereview.chromium.org/2585633003
Cr-Commit-Position: refs/heads/master@{#442344}
Committed:
https://chromium.googlesource.com/chromium/src/+/0eb377ef00cb7afd8392ff44f6ab...
==========

[commit-bot: I haz the power, 2017-01-09 21:09:57]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/0eb377ef00cb7afd8392ff44f6ab...