Add foundation for trimming the AffiliationDatabase.

Add foundation for trimming the AffiliationDatabase.

Add support for trimming no longer needed data from the AffiliationDatabase's cache SQL database; and for wiping the database if affiliated matches are disabled.

BUG=479507
Committed: https://crrev.com/0dd8900d56f66a279e69bec2a39ec11238287b85
Cr-Commit-Position: refs/heads/master@{#330101}

[engedy, 2015-04-22 19:39:01]

engedy@chromium.org changed reviewers:
+ mkwst@chromium.org

[engedy, 2015-04-22 19:45:01]

engedy@chromium.org changed reviewers:
+ vasilii@chromium.org
- mkwst@chromium.org

[engedy, 2015-04-22 19:45:02]

@Vasilii, could you please take a look?

[vasilii, 2015-04-23 13:16:35]

I think that the data behind AffiliationBackend can be qualified as a cache.
Therefore, if I clear with REMOVE_CACHE then the entire AffiliationDatabase or
everything downloaded between |delete_begin_| and |delete_end_| should be wiped
out.
If I clear the passwords then touching AffiliationDatabase seems optional.
The current implementation is cleaning the internal stale records. It can be
done at any time without any harm. In the context of BrowsingDataRemover I'd
like to do the hard reset.

https://codereview.chromium.org/1072413003/diff/1/components/password_manager...
File components/password_manager/core/browser/affiliation_backend.cc (right):

https://codereview.chromium.org/1072413003/diff/1/components/password_manager...
components/password_manager/core/browser/affiliation_backend.cc:100: // Discard
all equivalence classes except those that contain >= 1 facet for
... contain at least one facet ....

https://codereview.chromium.org/1072413003/diff/1/components/password_manager...
components/password_manager/core/browser/affiliation_backend.cc:106: for (const
auto& facet_uri : affiliation.facets) {
const FacetURI&

https://codereview.chromium.org/1072413003/diff/1/components/password_manager...
components/password_manager/core/browser/affiliation_backend.cc:116:
cache_->DeleteAffiliationsForFacet(affiliation.facets[0]);
Should you delete FacetManager?

https://codereview.chromium.org/1072413003/diff/1/components/password_manager...
File components/password_manager/core/browser/affiliation_service.cc (right):

https://codereview.chromium.org/1072413003/diff/1/components/password_manager...
components/password_manager/core/browser/affiliation_service.cc:95:
base::SingleThreadTaskRunner* backend_task_runner) {
AffiliationService should know the correct target thread better than the caller.

https://codereview.chromium.org/1072413003/diff/1/components/password_manager...
File components/password_manager/core/browser/affiliation_service.h (right):

https://codereview.chromium.org/1072413003/diff/1/components/password_manager...
components/password_manager/core/browser/affiliation_service.h:146: // at
|db_path|, and all auxiliary files. The database must be closed before
So where is it ensured that the database is closed?

https://codereview.chromium.org/1072413003/diff/1/components/password_manager...
File components/password_manager/core/browser/password_store.cc (right):

https://codereview.chromium.org/1072413003/diff/1/components/password_manager...
components/password_manager/core/browser/password_store.cc:115: if
(affiliated_match_helper_)
Should it be a DCHECK()?

[engedy, 2015-04-23 13:50:25]

On 2015/04/23 13:16:35, vasilii wrote:
> I think that the data behind AffiliationBackend can be qualified as a cache.
> Therefore, if I clear with REMOVE_CACHE then the entire AffiliationDatabase or
> everything downloaded between |delete_begin_| and |delete_end_| should be
wiped
> out.
> If I clear the passwords then touching AffiliationDatabase seems optional.
> The current implementation is cleaning the internal stale records. It can be
> done at any time without any harm. In the context of BrowsingDataRemover I'd
> like to do the hard reset.

Would you rather we did that even if most of the data would be fetched again
immediately afterwards?

[vasilii, 2015-04-23 14:10:49]

On 2015/04/23 13:50:25, engedy wrote:
> On 2015/04/23 13:16:35, vasilii wrote:
> > I think that the data behind AffiliationBackend can be qualified as a cache.
> > Therefore, if I clear with REMOVE_CACHE then the entire AffiliationDatabase
or
> > everything downloaded between |delete_begin_| and |delete_end_| should be
> wiped
> > out.
> > If I clear the passwords then touching AffiliationDatabase seems optional.
> > The current implementation is cleaning the internal stale records. It can be
> > done at any time without any harm. In the context of BrowsingDataRemover I'd
> > like to do the hard reset.
> 
> Would you rather we did that even if most of the data would be fetched again
> immediately afterwards?

Why immediately if I don't log in to a site?
Remember the use case of Chrome remote sign-out. We delete all browsing data and
passwords. However, in the current implementation AffiliationDatabase still
contains personal URLs and android app's ids afterwards.

[engedy, 2015-05-13 14:58:48]

engedy@chromium.org changed reviewers:
+ mkwst@chromium.org
- vasilii@chromium.org

[engedy, 2015-05-13 14:59:11]

Mike, as Vasilii is OOO, could you please take a look at this?

[Mike West, 2015-05-13 15:20:06]

This looks pretty reasonable, but I'd like to see how you deal with Vasilii's
comments, and how you deal with the ideals I noted in the browsing data remover
code before landing it.

I'll be online tomorrow and Friday.

https://codereview.chromium.org/1072413003/diff/1/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/1072413003/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover.cc:1022:
TrimAffiliationCache();
Ideally, we'd wait on this as well.

https://codereview.chromium.org/1072413003/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover.cc:1028:
PasswordStoreFactory::TrimOrDeleteAffiliationCache(profile_);
Ideally, we'd have tests showing that this method is called if REMOVE_CACHE or
REMOVE_PASSWORDS is set.

[engedy, 2015-05-15 15:03:24]

Mike, I have removed the controversial parts and will address them in a
follow-up CL once everyone is back from vacation. PTAL.

[Mike West, 2015-05-15 15:12:05]

The diff LGTM. It's a start, I suppose.

[engedy, 2015-05-15 15:14:48]

On 2015/05/15 15:12:05, Mike West (holiday in DE) wrote:
> The diff LGTM. It's a start, I suppose.

Yes, that's all it is. :-)

I am planning to wire it into the BrowsingDataRemover next week, once everybody
is back from OOO and we can decide what behavior we want exactly; and how to
address edge cases.

[engedy, 2015-05-15 15:14:57]

The CQ bit was checked by engedy@chromium.org

[commit-bot: I haz the power, 2015-05-15 15:15:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1072413003/20001

[commit-bot: I haz the power, 2015-05-15 16:05:04]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-05-15 16:05:54]

Patchset 2 (id:??) landed as
https://crrev.com/0dd8900d56f66a279e69bec2a39ec11238287b85
Cr-Commit-Position: refs/heads/master@{#330101}