[MD settings] disable clear browsing data while it is running

[MD settings] disable clear browsing data while it is running

This CL will disable the buttons in the clear browsing data dialog
if clear browsing data is in progress.

Based largely on https://crrev.com/2118503005/ by
Dave Schuyler <dschuyler@chromium.org>;

BUG=622998
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/3b09ab93aed1759180313cec0ce435646b93225e
Cr-Commit-Position: refs/heads/master@{#405518}

[Dan Beam, 2016-07-08 22:06:14]

Patchset #2 (id:20001) has been deleted

[Dan Beam, 2016-07-08 22:06:22]

Patchset #1 (id:1) has been deleted

[Dan Beam, 2016-07-08 22:07:39]

dbeam@chromium.org changed reviewers:
+ dpapad@chromium.org

[Dan Beam, 2016-07-08 22:07:39]

has the stuff we talked about

TODO(dbeam): write a test that responds to clearing from another tab.

[dpapad, 2016-07-08 23:09:43]

https://codereview.chromium.org/2133893002/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js
(right):

https://codereview.chromium.org/2133893002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:82:
attached: function() {
Nit: @override

https://codereview.chromium.org/2133893002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc
(right):

https://codereview.chromium.org/2133893002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:228:
AddCounter(base::WrapUnique(new AutofillCounter(profile_)));
initialize() can be called multiple times (close dialog, re-open the dialog). Is
this OK? It seems that we are re-adding the counters in such case.

[Dan Beam, 2016-07-08 23:26:58]

https://codereview.chromium.org/2133893002/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js
(right):

https://codereview.chromium.org/2133893002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:82:
attached: function() {
On 2016/07/08 23:09:43, dpapad wrote:
> Nit: @override

Done.

https://codereview.chromium.org/2133893002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc
(right):

https://codereview.chromium.org/2133893002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:228:
AddCounter(base::WrapUnique(new AutofillCounter(profile_)));
On 2016/07/08 23:09:43, dpapad wrote:
> initialize() can be called multiple times (close dialog, re-open the dialog).
Is
> this OK? It seems that we are re-adding the counters in such case.

Done.

[dpapad, 2016-07-09 00:26:22]

LGTM.

[msramek, 2016-07-11 11:19:22]

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

[msramek, 2016-07-11 11:19:23]

Oh no - this is yet another case of "two teams on the other side of the planet
are independently working on the same thing at the same time".

I appreciate that you're handling this case, but I want to ask you not to go
forward with this CL, because I'm planning changes that will ultimately
overwrite yours.

This exclusivity is a wider problem with BrowsingDataRemover that I'm planning
to solve for all callsites. I've been investigating this last week and after
collecting initial feedback, I sent out the plan to chromium-dev today
(https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/Jei32zQtJP0).
Sorry for not doing that at least one day earlier :-/ . Feel free to comment in
that doc if it doesn't contain what you need for this usecase.

For now, please do not add another callback to BrowsingDataRemover, as I'm
actually trying to simplify the mechanism. Instead, consider a simpler solution
of testing BrowsingDataRemover::is_removing() at the time when the "Clear data"
button is pressed and silently failing (without closing the dialog!) until my
solution is implemented.

Cheers,
Martin

[msramek, 2016-07-11 17:00:01]

As discussed offline - proceed with this custom solution, so you don't have to
be blocked on me or rely on my plans not being delayed. But please make it clear
in the code that this is not to be reused.

Happy to move the discussion to
https://docs.google.com/document/d/1oIbmZJlq5m5FvHcN7trCpp0h4mtydA0UfZasxEIe3P8/
.

With that, browsing_data/ LGTM.

https://codereview.chromium.org/2133893002/diff/60001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_remover.h (right):

https://codereview.chromium.org/2133893002/diff/60001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover.h:184: virtual void
OnBrowsingDataRemoving(bool is_removing) {}
Please add a comment here to note that this method is DEPRECATED and not to be
used. I want to avoid that people start using it and I'll have to hunt down the
callsites.

[Dan Beam, 2016-07-11 21:10:58]

https://codereview.chromium.org/2133893002/diff/60001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_remover.h (right):

https://codereview.chromium.org/2133893002/diff/60001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover.h:184: virtual void
OnBrowsingDataRemoving(bool is_removing) {}
On 2016/07/11 17:00:01, msramek wrote:
> Please add a comment here to note that this method is DEPRECATED and not to be
> used. I want to avoid that people start using it and I'll have to hunt down
the
> callsites.

Done.

[Dan Beam, 2016-07-14 16:58:04]

Description was changed from

==========
[MD settings] disable clear browsing data while it is running

This CL will disable the buttons in the clear browsing data dialog
if clear browsing data is in progress.

Based largely on https://crrev.com/2118503005/ by
Dave Schuyler <dschuyler@chromium.org>

BUG=622998
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD settings] disable clear browsing data while it is running

This CL will disable the buttons in the clear browsing data dialog
if clear browsing data is in progress.

Based largely on https://crrev.com/2118503005/ by
Dave Schuyler <dschuyler@chromium.org>

BUG=622998
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation
==========

[Dan Beam, 2016-07-14 16:58:58]

The CQ bit was checked by dbeam@chromium.org

[Dan Beam, 2016-07-14 16:58:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
msramek@chromium.org, dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/2133893002/#ps100001
(title: "merge")

[commit-bot: I haz the power, 2016-07-14 16:59:17]

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, 2016-07-14 16:59:19]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-14 18:18:56]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-07-14 18:19:06]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-14 18:21:30]

Description was changed from

==========
[MD settings] disable clear browsing data while it is running

This CL will disable the buttons in the clear browsing data dialog
if clear browsing data is in progress.

Based largely on https://crrev.com/2118503005/ by
Dave Schuyler <dschuyler@chromium.org>

BUG=622998
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD settings] disable clear browsing data while it is running

This CL will disable the buttons in the clear browsing data dialog
if clear browsing data is in progress.

Based largely on https://crrev.com/2118503005/ by
Dave Schuyler <dschuyler@chromium.org>

BUG=622998
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/3b09ab93aed1759180313cec0ce435646b93225e
Cr-Commit-Position: refs/heads/master@{#405518}
==========

[commit-bot: I haz the power, 2016-07-14 18:21:32]

Patchset 4 (id:??) landed as
https://crrev.com/3b09ab93aed1759180313cec0ce435646b93225e
Cr-Commit-Position: refs/heads/master@{#405518}