Adding "Clear hosted app data" checkbox to browsing data removal UI.

Adding "Clear hosted app data" checkbox to browsing data removal UI.

Original patch from battre@chromium.org, I'm just sheparding it along as he's
out on vacation.

BUG=116372
TBR=rsimha@chromium.org, timsteele@chromium.org
TEST=Install a hosted application, then clear browsing data with the new checkbox checked: the hosted application's data should be cleared. Without the checkbox, the data should remain untouched.


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

[Mike West, 2012-06-04 14:19:39]

Hello, lovely reviewers! This CL adds a checkbox to the browsing data removal
panel to enable the removal of data associated with hosted apps.

Evan, you already looked at the backend half of this change last week
(http://codereview.chromium.org/10413072/). Would you mind taking a quick look
at the frontend webui code?

Scott: Would you mind taking a look at the ui/browser.cc change?

Tim: Could you please take a look at the sync changes?

Markus: Can you look at the pref bit?

Mihai, Jeff: FYI.

There. That should be enough people on a single CL. :)

Thanks all!

[Mike West, 2012-06-04 14:21:33]

On 2012/06/04 14:19:39, Mike West (chromium) wrote:
> Hello, lovely reviewers! This CL adds a checkbox to the browsing data removal
> panel to enable the removal of data associated with hosted apps.
> 
> Evan, you already looked at the backend half of this change last week
> (http://codereview.chromium.org/10413072/). Would you mind taking a quick look
> at the frontend webui code?
> 
> Scott: Would you mind taking a look at the ui/browser.cc change?
> 
> Tim: Could you please take a look at the sync changes?
> 
> Markus: Can you look at the pref bit?
> 
> Mihai, Jeff: FYI.
> 
> There. That should be enough people on a single CL. :)
> 
> Thanks all!

Hrm. Apparently the upload died? The unified diffs seem to work correctly, but
the side-by-side is borked. I'll see what I can do about that.

[sky, 2012-06-04 14:29:51]

browser.cc LGTM

[markusheintz_, 2012-06-04 14:50:51]

LGTM with nits prefs stuff

https://chromiumcodereview.appspot.com/10502015/diff/3001/chrome/browser/ui/w...
File chrome/browser/ui/webui/options2/clear_browser_data_handler2.cc (right):

https://chromiumcodereview.appspot.com/10502015/diff/3001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/clear_browser_data_handler2.cc:160: remover_ =
new BrowsingDataRemover(profile,
nit: If I interpret the style guide correctly then the parameter "profile" must
be moved to a new line.

https://chromiumcodereview.appspot.com/10502015/diff/3001/chrome/browser/ui/w...
File chrome/browser/ui/webui/options2/clear_browser_data_handler2.h (right):

https://chromiumcodereview.appspot.com/10502015/diff/3001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/clear_browser_data_handler2.h:36: // Clears the
data of hosted apps, which is usually protected from deletion
nit: Does "usually" mean:
A) always protected
B) sometimes protected and sometimes not.

In case of A please delete "usually"
In case of B please document when it is protected and when not.

[Evan Stade, 2012-06-04 22:03:40]

https://chromiumcodereview.appspot.com/10502015/diff/3001/chrome/app/generate...
File chrome/app/generated_resources.grd (right):

https://chromiumcodereview.appspot.com/10502015/diff/3001/chrome/app/generate...
chrome/app/generated_resources.grd:6622: +        Clear data from hosted apps
I don't have a better suggestion for this string, but I don't think users have
any idea what a hosted app is vs. any other kind of app.

https://chromiumcodereview.appspot.com/10502015/diff/3001/chrome/browser/ui/w...
File chrome/browser/ui/webui/options2/clear_browser_data_handler2.cc (right):

https://chromiumcodereview.appspot.com/10502015/diff/3001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/clear_browser_data_handler2.cc:149: void
ClearBrowserDataHandler::HandleClearHostedAppData() {
it's confusing to call this Handle... because that makes it sound like it's
handling a chrome.send message.

https://chromiumcodereview.appspot.com/10502015/diff/3001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/clear_browser_data_handler2.cc:155: if
(remover_)
I don't think that remover_ can be non-NULL here because it's always set to NULL
on L171

https://chromiumcodereview.appspot.com/10502015/diff/3001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/clear_browser_data_handler2.cc:175:
HandleClearHostedAppData();
why do you defer this until after the first set of data is removed? Why not
remove it in parallel?

[Mike West, 2012-06-05 08:55:02]

Thanks Evan and Markus. Feedback addressed, and rebased onto ToT.

http://codereview.chromium.org/10502015/diff/3001/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10502015/diff/3001/chrome/app/generated_resour...
chrome/app/generated_resources.grd:6622: +        Clear data from hosted apps
On 2012/06/04 22:03:40, Evan Stade wrote:
> I don't have a better suggestion for this string, but I don't think users have
> any idea what a hosted app is vs. any other kind of app.

1. I agree completely.
2. This is the best Brian and Glen could come up with/agree upon.

http://codereview.chromium.org/10502015/diff/3001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options2/clear_browser_data_handler2.cc (right):

http://codereview.chromium.org/10502015/diff/3001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/clear_browser_data_handler2.cc:149: void
ClearBrowserDataHandler::HandleClearHostedAppData() {
On 2012/06/04 22:03:40, Evan Stade wrote:
> it's confusing to call this Handle... because that makes it sound like it's
> handling a chrome.send message.

Dropping "Handle" from this and the above "HandleClearBrowserData".

http://codereview.chromium.org/10502015/diff/3001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/clear_browser_data_handler2.cc:155: if
(remover_)
On 2012/06/04 22:03:40, Evan Stade wrote:
> I don't think that remover_ can be non-NULL here because it's always set to
NULL
> on L171

I think you're right. That's the only callsite for ClearHostedAppData, so
there's no need for this check here.

http://codereview.chromium.org/10502015/diff/3001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/clear_browser_data_handler2.cc:160: remover_ =
new BrowsingDataRemover(profile,
On 2012/06/04 14:50:52, markusheintz_ wrote:
> nit: If I interpret the style guide correctly then the parameter "profile"
must
> be moved to a new line.

Done.

http://codereview.chromium.org/10502015/diff/3001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/clear_browser_data_handler2.cc:175:
HandleClearHostedAppData();
On 2012/06/04 22:03:40, Evan Stade wrote:
> why do you defer this until after the first set of data is removed? Why not
> remove it in parallel?

An excellent question, and one I should have read before fixing the rest of the
comments.

I'm not entirely sure that the BDR won't explode if we run two at once. But
trying it seems like a good way to find out.

http://codereview.chromium.org/10502015/diff/3001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/clear_browser_data_handler2.cc:175:
HandleClearHostedAppData();
On 2012/06/04 22:03:40, Evan Stade wrote:
> why do you defer this until after the first set of data is removed? Why not
> remove it in parallel?

It explodes. BRD is actually set up to explicitly block concurrent removals: one
BRD runs at a time, period.

http://codereview.chromium.org/10502015/diff/3001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options2/clear_browser_data_handler2.h (right):

http://codereview.chromium.org/10502015/diff/3001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/clear_browser_data_handler2.h:36: // Clears the
data of hosted apps, which is usually protected from deletion
On 2012/06/04 14:50:52, markusheintz_ wrote:
> nit: Does "usually" mean:
> A) always protected
> B) sometimes protected and sometimes not.
> 
> In case of A please delete "usually"
> In case of B please document when it is protected and when not. 

Changed to "otherwise", which I think makes things more clear. The data is
protected from deletion, unless Clear Hosted App Data is called.

[Evan Stade, 2012-06-05 18:07:44]

http://codereview.chromium.org/10502015/diff/10001/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options2/clear_browser_data_handler2.cc (right):

http://codereview.chromium.org/10502015/diff/10001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/clear_browser_data_handler2.cc:102: void
ClearBrowserDataHandler::ClearBrowserData(const ListValue* value) {
this one actually does handle a chrome.send message (see L98), so it should
start with Handle.

http://codereview.chromium.org/10502015/diff/10001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/clear_browser_data_handler2.cc:130: int
period_selected = prefs->GetInteger(prefs::kDeleteTimePeriod);
this should be declared where it's used.

http://codereview.chromium.org/10502015/diff/10001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/clear_browser_data_handler2.cc:134: state);
(read this comment after the one right below)

OK, I think I see why you could get two performClearBrowserData messages right
in a row: we don't disable the UI until here. I cannot fathom why. Should remove
this completely and do setClearingState(true) in
$('clearBrowserDataCommit').onclick.

http://codereview.chromium.org/10502015/diff/10001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/clear_browser_data_handler2.cc:139:
remover_->RemoveObserver(this);
I don't really like this pattern. How would (Handle)ClearBrowserData get called
again when it already has a |remover_|? It should be impossible because the UI
is disabled until the first removal is complete.

The problem is that now that OnBrowsingDataRemoverDone does something important
(i.e. continues removing more data that the user has chosen to remove), ignoring
the previous remover_ now has real consequences.

Since I don't understand how remover_ could legitimately be non-NULL here, I'd
just as soon return early.

http://codereview.chromium.org/10502015/diff/10001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/clear_browser_data_handler2.cc:146:
remover_->Remove(remove_mask, BrowsingDataHelper::UNPROTECTED_WEB);
it seems that remove_mask can be 0 here, in which case this call is a bit
pointless.

http://codereview.chromium.org/10502015/diff/10001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/clear_browser_data_handler2.cc:155: if
(remover_)
this is still here. intentional?

[Evan Stade, 2012-06-05 18:10:15]

also, I just noticed that the cancel button in the overlay is still active when
the data is removing. This is a red herring because there's no way you can
actually cancel the deletion of your data. It should be disabled the same as the
"Clear browsing data" button.

[battre, 2012-06-07 21:17:24]

Just some quick replies. Mike, please continue driving this.

Thanks,
Dominic

http://codereview.chromium.org/10502015/diff/10001/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options2/clear_browser_data_handler2.cc (right):

http://codereview.chromium.org/10502015/diff/10001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/clear_browser_data_handler2.cc:146:
remover_->Remove(remove_mask, BrowsingDataHelper::UNPROTECTED_WEB);
On 2012/06/05 18:07:45, Evan Stade wrote:
> it seems that remove_mask can be 0 here, in which case this call is a bit
> pointless.

We rely on the callback even if remove_mask is 0 here to trigger
ClearHostedAppData().

http://codereview.chromium.org/10502015/diff/10001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/clear_browser_data_handler2.cc:155: if
(remover_)
On 2012/06/05 18:07:45, Evan Stade wrote:
> this is still here. intentional?

This was just for defensive programming. There is currently no code path in
which remover_ would be != NULL.

[Evan Stade, 2012-06-07 21:50:20]

http://codereview.chromium.org/10502015/diff/10001/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options2/clear_browser_data_handler2.cc (right):

http://codereview.chromium.org/10502015/diff/10001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/clear_browser_data_handler2.cc:146:
remover_->Remove(remove_mask, BrowsingDataHelper::UNPROTECTED_WEB);
On 2012/06/07 21:17:24, battre wrote:
> On 2012/06/05 18:07:45, Evan Stade wrote:
> > it seems that remove_mask can be 0 here, in which case this call is a bit
> > pointless.
> 
> We rely on the callback even if remove_mask is 0 here to trigger
> ClearHostedAppData().

yea, I can see that. Doesn't mean it makes sense. Instead we should jump
straight to ClearHostedAppData().

http://codereview.chromium.org/10502015/diff/10001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/clear_browser_data_handler2.cc:155: if
(remover_)
On 2012/06/07 21:17:24, battre wrote:
> On 2012/06/05 18:07:45, Evan Stade wrote:
> > this is still here. intentional?
> 
> This was just for defensive programming. There is currently no code path in
> which remover_ would be != NULL.

then DCHECK(!remover_) is more appropriate

[Mike West, 2012-06-11 09:35:44]

Thanks for your feedback, Evan and Dominic.

I spent some time Friday trying to get BrowsingDataRemover to accept parallel
runs; I ended up deciding that it's not a good idea. Multiple callers walking
through lists of origins that might or might not have been deleted by the other
BDR instance is just asking for scary bugs. It's something that would probably
be nice to get working in the future, but I hope you don't feel strongly enough
about it to block this CL until it's done.

The rest of the comments have been addressed in the patch I just uploaded. Would
you mind taking another look?

Thanks!

http://codereview.chromium.org/10502015/diff/10001/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options2/clear_browser_data_handler2.cc (right):

http://codereview.chromium.org/10502015/diff/10001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/clear_browser_data_handler2.cc:102: void
ClearBrowserDataHandler::ClearBrowserData(const ListValue* value) {
On 2012/06/05 18:07:45, Evan Stade wrote:
> this one actually does handle a chrome.send message (see L98), so it should
> start with Handle.

Done.

http://codereview.chromium.org/10502015/diff/10001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/clear_browser_data_handler2.cc:130: int
period_selected = prefs->GetInteger(prefs::kDeleteTimePeriod);
On 2012/06/05 18:07:45, Evan Stade wrote:
> this should be declared where it's used.

Done.

http://codereview.chromium.org/10502015/diff/10001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/clear_browser_data_handler2.cc:134: state);
On 2012/06/05 18:07:45, Evan Stade wrote:
> OK, I think I see why you could get two performClearBrowserData messages right
> in a row: we don't disable the UI until here.

Good catch. I've moved this out into the JavaScript file.

http://codereview.chromium.org/10502015/diff/10001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/clear_browser_data_handler2.cc:139:
remover_->RemoveObserver(this);
On 2012/06/05 18:07:45, Evan Stade wrote:
> Since I don't understand how remover_ could legitimately be non-NULL here, I'd
> just as soon return early.

If `remover_` is non-NULL, then something unexpected has happened. Since it
shouldn't ever happen, I think a DCHECK is appropriate here.

http://codereview.chromium.org/10502015/diff/10001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/clear_browser_data_handler2.cc:146:
remover_->Remove(remove_mask, BrowsingDataHelper::UNPROTECTED_WEB);
On 2012/06/07 21:50:21, Evan Stade wrote:
> On 2012/06/07 21:17:24, battre wrote:
> > On 2012/06/05 18:07:45, Evan Stade wrote:
> > > it seems that remove_mask can be 0 here, in which case this call is a bit
> > > pointless.
> > 
> > We rely on the callback even if remove_mask is 0 here to trigger
> > ClearHostedAppData().
> 
> yea, I can see that. Doesn't mean it makes sense. Instead we should jump
> straight to ClearHostedAppData().

You're right. I've added a skip. I've also handled the case where no checkboxes
are checked (which JS shouldn't allow, but better safe than sorry).

http://codereview.chromium.org/10502015/diff/10001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/clear_browser_data_handler2.cc:155: if
(remover_)
On 2012/06/07 21:50:21, Evan Stade wrote:
> On 2012/06/07 21:17:24, battre wrote:
> > On 2012/06/05 18:07:45, Evan Stade wrote:
> > > this is still here. intentional?
> > 
> > This was just for defensive programming. There is currently no code path in
> > which remover_ would be != NULL.
> 
> then DCHECK(!remover_) is more appropriate

Done.

[battre, 2012-06-11 13:16:37]

lgtm

[Evan Stade, 2012-06-11 19:15:01]

lgtm

[commit-bot: I haz the power, 2012-06-11 19:22:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkwst@chromium.org/10502015/17003

[commit-bot: I haz the power, 2012-06-11 19:22:36]

Presubmit check for 10502015-17003 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/sync

Presubmit checks took 4.3s to calculate.

[battre, 2012-06-11 19:27:46]

+rsimha for OWNERS approval of changes in chrome/browser/sync

[battre, 2012-06-12 10:49:11]

I think you should just TBR rsimha. The change is really trivial and the CL
blocks some followup work (update of default apps, sign-off for privacy review
of launch bug, ...).

[Mike West, 2012-06-12 10:51:59]

On 2012/06/12 10:49:11, battre wrote:
> I think you should just TBR rsimha. The change is really trivial and the CL
> blocks some followup work (update of default apps, sign-off for privacy review
> of launch bug, ...).

After talking with you about it, I agree. I'm TBRing both Tim and Raghu, as I
haven't been able to get feedback from them through now. Hi Tim and Raghu!
Please do take a look at this change; if there's anything at all that I can
improve after the fact, I will.

I'll drop a line to Mihai asking him to proceed with the default app portion of
this change.

[commit-bot: I haz the power, 2012-06-12 10:52:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkwst@chromium.org/10502015/17003

[commit-bot: I haz the power, 2012-06-12 12:02:45]

Change committed as 141646