Add support for time based deletion of browsing data on iOS

Add support for time based deletion of browsing data on iOS

Change existing browsing data deletion methods to take a browsing_data::TimePeriod as a parameter.

Keep old methods around until all call sites are changed downstream.

BUG=642334
Committed: https://crrev.com/f433931685d2eefb27a0e987c4361dea6fbb5e22
Cr-Commit-Position: refs/heads/master@{#419433}

[ioanap, 2016-08-25 15:06:57]

The CQ bit was checked by ioanap@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 15:07:16]

Dry run: 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-08-25 15:46:35]

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

[commit-bot: I haz the power, 2016-08-25 15:46:36]

Dry run: This issue passed the CQ dry run.

[ioanap, 2016-08-26 14:09:54]

The CQ bit was checked by ioanap@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-26 14:10:15]

Dry run: 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-08-26 14:56:12]

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

[commit-bot: I haz the power, 2016-08-26 14:56:13]

Dry run: This issue passed the CQ dry run.

[ioanap, 2016-08-29 13:40:02]

Description was changed from

==========
Add browsing_data::TimePeriod to browsing data remover on iOS

BUG=
==========

to

==========
Add browsing_data::TimePeriod to browsing data remover on iOS

Change existing browsing data deletion methods to take a
browsing_data::TimePeriod as a parameter. 

BUG=620318
==========

[ioanap, 2016-08-29 13:43:03]

Description was changed from

==========
Add browsing_data::TimePeriod to browsing data remover on iOS

Change existing browsing data deletion methods to take a
browsing_data::TimePeriod as a parameter. 

BUG=620318
==========

to

==========
Add support for time based deletion of browsing data on iOS

Change existing browsing data deletion methods to take a
browsing_data::TimePeriod as a parameter. 

BUG=620318
==========

[ioanap, 2016-08-29 13:44:40]

Description was changed from

==========
Add support for time based deletion of browsing data on iOS

Change existing browsing data deletion methods to take a
browsing_data::TimePeriod as a parameter. 

BUG=620318
==========

to

==========
Add support for time based deletion of browsing data on iOS

Change existing browsing data deletion methods to take a
browsing_data::TimePeriod as a parameter. 

Keep old methods around until all call sites are changed downstream.

BUG=620318
==========

[ioanap, 2016-08-29 13:48:06]

ioanap@google.com changed reviewers:
+ msramek@chromium.org

[ioanap, 2016-08-29 13:48:07]

Hi Martin,

Do you want to take a first look ?

Thank you,
Ioana

[msramek, 2016-08-29 14:21:00]

*confusedly looks over the right shoulder* O.o

Thanks, this is on the right path. Are those all backends from your spreadsheet
that needed update?

Also, one general comment is that we normally don't have overloaded methods with
different parameter count - although that is of course inevitable here to be
compatible with downstream. It might be a good idea to mark those that you plan
to remove after updating downstream as "DEPRECATED".

https://codereview.chromium.org/2270063005/diff/40001/ios/chrome/browser/brow...
File ios/chrome/browser/browsing_data/browsing_data_remover_helper.cc (right):

https://codereview.chromium.org/2270063005/diff/40001/ios/chrome/browser/brow...
ios/chrome/browser/browsing_data/browsing_data_remover_helper.cc:98:
browsing_data::TimePeriod time_period = current_removal_info_->time_period;
nit: I would perhaps just inline this.

https://codereview.chromium.org/2270063005/diff/40001/ios/chrome/browser/brow...
File ios/chrome/browser/browsing_data/browsing_data_remover_helper.h (right):

https://codereview.chromium.org/2270063005/diff/40001/ios/chrome/browser/brow...
ios/chrome/browser/browsing_data/browsing_data_remover_helper.h:43: void
Remove(ios::ChromeBrowserState* browser_state,
I would recommend not copy-pasting the comment, just something along the lines
of "Same as above, with |time_period| being ALL_TIME."

https://codereview.chromium.org/2270063005/diff/40001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/commands/clear_browsing_data_command.mm (right):

https://codereview.chromium.org/2270063005/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/commands/clear_browsing_data_command.mm:34: -
(instancetype)initWithBrowserState:(ios::ChromeBrowserState*)browserState
Shouldn't we explicitly initialize _timePeriod to all time here?

[ioanap, 2016-08-30 11:12:04]

The CQ bit was checked by ioanap@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-30 11:12:30]

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

[ioanap, 2016-08-30 11:32:08]

Addressed comments.

Thanks,
Ioana

https://codereview.chromium.org/2270063005/diff/40001/ios/chrome/browser/brow...
File ios/chrome/browser/browsing_data/browsing_data_remover_helper.cc (right):

https://codereview.chromium.org/2270063005/diff/40001/ios/chrome/browser/brow...
ios/chrome/browser/browsing_data/browsing_data_remover_helper.cc:98:
browsing_data::TimePeriod time_period = current_removal_info_->time_period;
On 2016/08/29 14:20:59, msramek wrote:
> nit: I would perhaps just inline this.

Done.

https://codereview.chromium.org/2270063005/diff/40001/ios/chrome/browser/brow...
File ios/chrome/browser/browsing_data/browsing_data_remover_helper.h (right):

https://codereview.chromium.org/2270063005/diff/40001/ios/chrome/browser/brow...
ios/chrome/browser/browsing_data/browsing_data_remover_helper.h:43: void
Remove(ios::ChromeBrowserState* browser_state,
On 2016/08/29 14:20:59, msramek wrote:
> I would recommend not copy-pasting the comment, just something along the lines
> of "Same as above, with |time_period| being ALL_TIME."

Done.

https://codereview.chromium.org/2270063005/diff/40001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/commands/clear_browsing_data_command.mm (right):

https://codereview.chromium.org/2270063005/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/commands/clear_browsing_data_command.mm:34: -
(instancetype)initWithBrowserState:(ios::ChromeBrowserState*)browserState
On 2016/08/29 14:20:59, msramek wrote:
> Shouldn't we explicitly initialize _timePeriod to all time here?

Done.

[msramek, 2016-08-30 11:39:55]

LGTM!

[ioanap, 2016-08-30 12:03:30]

Description was changed from

==========
Add support for time based deletion of browsing data on iOS

Change existing browsing data deletion methods to take a
browsing_data::TimePeriod as a parameter. 

Keep old methods around until all call sites are changed downstream.

BUG=620318
==========

to

==========
Add support for time based deletion of browsing data on iOS

Change existing browsing data deletion methods to take a
browsing_data::TimePeriod as a parameter. 

Keep old methods around until all call sites are changed downstream.

BUG=
==========

[commit-bot: I haz the power, 2016-08-30 12:14:14]

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

[commit-bot: I haz the power, 2016-08-30 12:14:15]

Dry run: This issue passed the CQ dry run.

[ioanap, 2016-08-30 12:17:10]

Description was changed from

==========
Add support for time based deletion of browsing data on iOS

Change existing browsing data deletion methods to take a
browsing_data::TimePeriod as a parameter. 

Keep old methods around until all call sites are changed downstream.

BUG=
==========

to

==========
Add support for time based deletion of browsing data on iOS

Change existing browsing data deletion methods to take a
browsing_data::TimePeriod as a parameter. 

Keep old methods around until all call sites are changed downstream.

BUG=642334
==========

[ioanap, 2016-08-30 12:19:38]

ioanap@google.com changed reviewers:
+ droger@chromium.org

[ioanap, 2016-08-30 12:19:39]

+droger

Please take a look!

Thank you,
Ioana

[droger, 2016-08-30 13:00:48]

lgtm

https://codereview.chromium.org/2270063005/diff/60001/ios/chrome/browser/brow...
File ios/chrome/browser/browsing_data/ios_chrome_browsing_data_remover.mm
(right):

https://codereview.chromium.org/2270063005/diff/60001/ios/chrome/browser/brow...
ios/chrome/browser/browsing_data/ios_chrome_browsing_data_remover.mm:213: //
they can't be more than a day old, so we can simply clear them all.
Should this be changed to avoid deleting everything, especially in the LAST_HOUR
case?

https://codereview.chromium.org/2270063005/diff/60001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/commands/clear_browsing_data_command.mm (right):

https://codereview.chromium.org/2270063005/diff/60001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/commands/clear_browsing_data_command.mm:35: mask:(int)mask
{
Should this be implemented by calling the other constructor?

i.e.:

return [self initWithBrowserState:browserState
                             mask:mask
                       timePeriod:browsing_data::ALL_TIME];

[msramek, 2016-08-30 14:14:35]

https://codereview.chromium.org/2270063005/diff/60001/ios/chrome/browser/brow...
File ios/chrome/browser/browsing_data/ios_chrome_browsing_data_remover.mm
(right):

https://codereview.chromium.org/2270063005/diff/60001/ios/chrome/browser/brow...
ios/chrome/browser/browsing_data/ios_chrome_browsing_data_remover.mm:213: //
they can't be more than a day old, so we can simply clear them all.
On 2016/08/30 13:00:48, droger wrote:
> Should this be changed to avoid deleting everything, especially in the
LAST_HOUR
> case?

This implementation is consistent with Desktop and Android, where we always wipe
the last session (even during LAST_HOUR deletions).

It would be possible to honor timestamps, but it would lead to strange results -
e.g. we would remove some tabs from the session; from those that remain, we
would remove only the latest navigation history... this is probably not the
desired effect.

Furthermore, this should be kept in sync with the session stored on disk (as
opposed to "recent tabs" in memory), and that's a lot more complicated.

[ioanap, 2016-08-30 15:08:51]

Addressed comments.

Thank you,
Ioana

https://codereview.chromium.org/2270063005/diff/60001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/commands/clear_browsing_data_command.mm (right):

https://codereview.chromium.org/2270063005/diff/60001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/commands/clear_browsing_data_command.mm:35: mask:(int)mask
{
On 2016/08/30 13:00:48, droger wrote:
> Should this be implemented by calling the other constructor?
> 
> i.e.:
> 
> return [self initWithBrowserState:browserState
>                              mask:mask
>                        timePeriod:browsing_data::ALL_TIME];

Done.

[ioanap, 2016-09-06 16:18:14]

ioanap@google.com changed reviewers:
+ msarda@chromium.org

[ioanap, 2016-09-06 16:18:14]

msarda@chromium.org: Please review changes in 
    - components/signin/ios/browser/account_consistency_service.h

Thank you,
Ioana

[msarda, 2016-09-06 16:32:56]

CC+ bzanotti (FYI)

[msarda, 2016-09-06 16:36:40]

lgtm

[Eugene But (OOO till 7-30), 2016-09-07 16:16:03]

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

[Eugene But (OOO till 7-30), 2016-09-07 16:16:04]

Ioana, are you sure that this CL actually changes the behavior on iOS? After
switching to WKWebView, Chrome does not control cookies, caches, etc.. This
downstream code does the actual browsing data deletion:


      [[WKWebsiteDataStore defaultDataStore]
          removeDataOfTypes:dataTypesToRemove
              modifiedSince:[NSDate distantPast]
          completionHandler:afterRemovalFromWKWebsiteDataStore];

[ioanap, 2016-09-07 16:26:22]

On 2016/09/07 16:16:04, Eugene But wrote:
> Ioana, are you sure that this CL actually changes the behavior on iOS? After
> switching to WKWebView, Chrome does not control cookies, caches, etc.. This
> downstream code does the actual browsing data deletion:
> 
> 
>       [[WKWebsiteDataStore defaultDataStore]
>           removeDataOfTypes:dataTypesToRemove
>               modifiedSince:[NSDate distantPast]
>           completionHandler:afterRemovalFromWKWebsiteDataStore];

I am changing this particular call and other browsing data remover methods in a
corresponding downstream CL:  498667015 .
Feel free to take a look!

Thank you,
Ioana

[Eugene But (OOO till 7-30), 2016-09-07 16:29:45]

On 2016/09/07 16:26:22, ioanap wrote:
> On 2016/09/07 16:16:04, Eugene But wrote:
> > Ioana, are you sure that this CL actually changes the behavior on iOS? After
> > switching to WKWebView, Chrome does not control cookies, caches, etc.. This
> > downstream code does the actual browsing data deletion:
> > 
> > 
> >       [[WKWebsiteDataStore defaultDataStore]
> >           removeDataOfTypes:dataTypesToRemove
> >               modifiedSince:[NSDate distantPast]
> >           completionHandler:afterRemovalFromWKWebsiteDataStore];
> 
> I am changing this particular call and other browsing data remover methods in
a
> corresponding downstream CL:  498667015 .
> Feel free to take a look!
> 
> Thank you,
> Ioana
Thank you for explanation.

[ioanap, 2016-09-19 11:44:11]

The CQ bit was checked by ioanap@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-19 11:44:19]

Dry run: 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-09-19 11:52:10]

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

[commit-bot: I haz the power, 2016-09-19 11:52:11]

Dry run: This issue passed the CQ dry run.

[ioanap, 2016-09-19 12:08:41]

The CQ bit was checked by ioanap@google.com

[ioanap, 2016-09-19 12:08:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
msramek@chromium.org, droger@chromium.org
Link to the patchset: https://codereview.chromium.org/2270063005/#ps100001
(title: "Updated comment in AccountConsistencyService")

[commit-bot: I haz the power, 2016-09-19 12:08:51]

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-09-19 12:13:28]

Description was changed from

==========
Add support for time based deletion of browsing data on iOS

Change existing browsing data deletion methods to take a
browsing_data::TimePeriod as a parameter. 

Keep old methods around until all call sites are changed downstream.

BUG=642334
==========

to

==========
Add support for time based deletion of browsing data on iOS

Change existing browsing data deletion methods to take a
browsing_data::TimePeriod as a parameter. 

Keep old methods around until all call sites are changed downstream.

BUG=642334
==========

[commit-bot: I haz the power, 2016-09-19 12:13:30]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-19 12:15:41]

Description was changed from

==========
Add support for time based deletion of browsing data on iOS

Change existing browsing data deletion methods to take a
browsing_data::TimePeriod as a parameter. 

Keep old methods around until all call sites are changed downstream.

BUG=642334
==========

to

==========
Add support for time based deletion of browsing data on iOS

Change existing browsing data deletion methods to take a
browsing_data::TimePeriod as a parameter.

Keep old methods around until all call sites are changed downstream.

BUG=642334

Committed: https://crrev.com/f433931685d2eefb27a0e987c4361dea6fbb5e22
Cr-Commit-Position: refs/heads/master@{#419433}
==========

[commit-bot: I haz the power, 2016-09-19 12:15:42]

Patchset 6 (id:??) landed as
https://crrev.com/f433931685d2eefb27a0e987c4361dea6fbb5e22
Cr-Commit-Position: refs/heads/master@{#419433}