[LanguageModel] Clear the model when clearing full history

[LanguageModel] Clear the model when clearing full history

Before this CL, the LanguageModel did not react in any way to user
clearing browsing data.

This CL adds wiping the model whenever the user clears the complete
browsing history.

BUG=683875
Review-Url: https://codereview.chromium.org/2649163003
Cr-Commit-Position: refs/heads/master@{#446350}
Committed: https://chromium.googlesource.com/chromium/src/+/afb5fba70d41e1cc8971942d1d78cffa491933a0

[jkrcal, 2017-01-24 14:29:40]

jkrcal@chromium.org changed reviewers:
+ groby@chromium.org, msramek@chromium.org

[jkrcal, 2017-01-24 14:29:40]

Rachel, could you PTAL at components/translate/*?
Martin, could you PTAL at browsing_data_remover?

Jon in cc: as we handed over the model to you :)

[msramek, 2017-01-24 15:35:34]

browsing_data/ LGTM % comments

https://codereview.chromium.org/2649163003/diff/20001/chrome/browser/browsing...
File chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc
(right):

https://codereview.chromium.org/2649163003/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc:382:
LanguageModelFactory::GetInstance()->GetForBrowserContext(profile_);
style: 4 spaces on continuation of the previous line

https://codereview.chromium.org/2649163003/diff/20001/chrome/browser/browsing...
File
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc
(right):

https://codereview.chromium.org/2649163003/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc:1757:
BlockUntilOriginDataRemoved(base::Time(), base::Time::Max(),
You can just use BlockUntilBrowsingDataRemoved since you're not actually
filtering origins.

https://codereview.chromium.org/2649163003/diff/20001/components/translate/co...
File components/translate/core/browser/language_model.cc (right):

https://codereview.chromium.org/2649163003/diff/20001/components/translate/co...
components/translate/core/browser/language_model.cc:136: // Ignore all partial
removals and react only to "entire" history removal.
This might be worth documenting in the CBDRD unittest. Before you run the
(base::Time(), base::Time::Max()) removal, please add another step where the
time interval is finite and test that the model is unchanged.

[jkrcal, 2017-01-25 08:48:55]

Thanks, Martin!

Rachel, PTAL at components/translate/* changes.

https://codereview.chromium.org/2649163003/diff/20001/chrome/browser/browsing...
File chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc
(right):

https://codereview.chromium.org/2649163003/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc:382:
LanguageModelFactory::GetInstance()->GetForBrowserContext(profile_);
On 2017/01/24 15:35:34, msramek wrote:
> style: 4 spaces on continuation of the previous line

Done.

https://codereview.chromium.org/2649163003/diff/20001/chrome/browser/browsing...
File
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc
(right):

https://codereview.chromium.org/2649163003/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc:1757:
BlockUntilOriginDataRemoved(base::Time(), base::Time::Max(),
On 2017/01/24 15:35:34, msramek wrote:
> You can just use BlockUntilBrowsingDataRemoved since you're not actually
> filtering origins.

Done.

https://codereview.chromium.org/2649163003/diff/20001/components/translate/co...
File components/translate/core/browser/language_model.cc (right):

https://codereview.chromium.org/2649163003/diff/20001/components/translate/co...
components/translate/core/browser/language_model.cc:136: // Ignore all partial
removals and react only to "entire" history removal.
On 2017/01/24 15:35:34, msramek wrote:
> This might be worth documenting in the CBDRD unittest. Before you run the
> (base::Time(), base::Time::Max()) removal, please add another step where the
> time interval is finite and test that the model is unchanged.

Done.

https://codereview.chromium.org/2649163003/diff/60001/chrome/browser/browsing...
File
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc
(right):

https://codereview.chromium.org/2649163003/diff/60001/chrome/browser/browsing...
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc:1769:
EXPECT_THAT(language_model->GetLanguageFrequency("en"), FloatEq(0.75));
Furthermore, I've changed the ratio of the languages to catch more types of
potential changes to the model.

[msramek, 2017-01-25 11:54:59]

https://codereview.chromium.org/2649163003/diff/60001/chrome/browser/browsing...
File
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc
(right):

https://codereview.chromium.org/2649163003/diff/60001/chrome/browser/browsing...
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc:1769:
EXPECT_THAT(language_model->GetLanguageFrequency("en"), FloatEq(0.75));
On 2017/01/25 08:48:54, jkrcal wrote:
> Furthermore, I've changed the ratio of the languages to catch more types of
> potential changes to the model.

Acknowledged.

[groby-ooo-7-16, 2017-01-25 16:12:45]

translate LGTM w/ a tiny nit in tests

https://codereview.chromium.org/2649163003/diff/60001/components/translate/co...
File components/translate/core/browser/language_model.h (right):

https://codereview.chromium.org/2649163003/diff/60001/components/translate/co...
components/translate/core/browser/language_model.h:61: void
ClearHistory(base::Time begin, base::Time end);
Huh. I thought we had a TimeRange, but apparently only blink does...

https://codereview.chromium.org/2649163003/diff/60001/components/translate/co...
File components/translate/core/browser/language_model_unittest.cc (right):

https://codereview.chromium.org/2649163003/diff/60001/components/translate/co...
components/translate/core/browser/language_model_unittest.cc:140:
model.ClearHistory(base::Time::Now() - base::TimeDelta::FromHours(1),
Would you mind making this more than an hour? Because otherwise I'll always
worry what happens at DST switch :) (I know it won't break. Just the "time is
evil" part of my brain will be triggered ;)

[jkrcal, 2017-01-26 15:46:06]

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

[commit-bot: I haz the power, 2017-01-26 15:46:17]

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

[jkrcal, 2017-01-26 15:46:22]

Thanks!

https://codereview.chromium.org/2649163003/diff/60001/components/translate/co...
File components/translate/core/browser/language_model.h (right):

https://codereview.chromium.org/2649163003/diff/60001/components/translate/co...
components/translate/core/browser/language_model.h:61: void
ClearHistory(base::Time begin, base::Time end);
On 2017/01/25 16:12:45, groby wrote:
> Huh. I thought we had a TimeRange, but apparently only blink does...

Acknowledged.

https://codereview.chromium.org/2649163003/diff/60001/components/translate/co...
File components/translate/core/browser/language_model_unittest.cc (right):

https://codereview.chromium.org/2649163003/diff/60001/components/translate/co...
components/translate/core/browser/language_model_unittest.cc:140:
model.ClearHistory(base::Time::Now() - base::TimeDelta::FromHours(1),
On 2017/01/25 16:12:45, groby wrote:
> Would you mind making this more than an hour? Because otherwise I'll always
> worry what happens at DST switch :) (I know it won't break. Just the "time is
> evil" part of my brain will be triggered ;)
> 

Done. Thanks for pointing out! 

Actually, "1 hour" is spread all around the browsing_data_remover unit tests.
Maybe something to consider changing, Martin? ;)

[msramek, 2017-01-26 16:00:49]

https://codereview.chromium.org/2649163003/diff/60001/components/translate/co...
File components/translate/core/browser/language_model_unittest.cc (right):

https://codereview.chromium.org/2649163003/diff/60001/components/translate/co...
components/translate/core/browser/language_model_unittest.cc:140:
model.ClearHistory(base::Time::Now() - base::TimeDelta::FromHours(1),
On 2017/01/26 15:46:22, jkrcal wrote:
> On 2017/01/25 16:12:45, groby wrote:
> > Would you mind making this more than an hour? Because otherwise I'll always
> > worry what happens at DST switch :) (I know it won't break. Just the "time
is
> > evil" part of my brain will be triggered ;)
> > 
> 
> Done. Thanks for pointing out! 
> 
> Actually, "1 hour" is spread all around the browsing_data_remover unit tests.
> Maybe something to consider changing, Martin? ;) 

I'm pretty sure base::Time() is UTC. So base::TimeDelta::FromHours(1) is always
an hour ago. I would be more worried about Chrome travelling at lightspeed or
falling into a black hole than DST shifts :)

Even if it wasn't, we have to delete data using the same units with which they
were written, which is again usually base::Time().

Note that we do take DST shifts into account in places like the history page,
which cuts the entries into blocks from local midnight to local midnight - that
might be 23 or 25 hours sometimes.

[commit-bot: I haz the power, 2017-01-26 16:32:47]

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

[commit-bot: I haz the power, 2017-01-26 16:32:47]

Dry run: This issue passed the CQ dry run.

[jkrcal, 2017-01-26 16:34:11]

The CQ bit was checked by jkrcal@chromium.org

[jkrcal, 2017-01-26 16:34:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
msramek@chromium.org, groby@chromium.org
Link to the patchset: https://codereview.chromium.org/2649163003/#ps80001
(title: "Rachel's comments")

[commit-bot: I haz the power, 2017-01-26 16:34:46]

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-26 16:41:48]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485448451111110,
"parent_rev": "b43378b93feea2b566feaccb360051723e746b1a", "commit_rev":
"afb5fba70d41e1cc8971942d1d78cffa491933a0"}

[commit-bot: I haz the power, 2017-01-26 16:42:23]

Description was changed from

==========
[LanguageModel] Clear the model when clearing full history

Before this CL, the LanguageModel did not react in any way to user
clearing browsing data.

This CL adds wiping the model whenever the user clears the complete
browsing history.

BUG=683875
==========

to

==========
[LanguageModel] Clear the model when clearing full history

Before this CL, the LanguageModel did not react in any way to user
clearing browsing data.

This CL adds wiping the model whenever the user clears the complete
browsing history.

BUG=683875

Review-Url: https://codereview.chromium.org/2649163003
Cr-Commit-Position: refs/heads/master@{#446350}
Committed:
https://chromium.googlesource.com/chromium/src/+/afb5fba70d41e1cc8971942d1d78...
==========

[commit-bot: I haz the power, 2017-01-26 16:42:25]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/afb5fba70d41e1cc8971942d1d78...