Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(286)

Issue 2617403005: Move HISTORY and PASSWORDS from BrowsingDataRemover to the delegate (Closed)

Created:
3 years, 11 months ago by msramek
Modified:
3 years, 11 months ago
Reviewers:
Mike West
CC:
chromium-reviews, markusheintz_, msramek+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Move HISTORY and PASSWORDS from BrowsingDataRemover to the delegate In https://codereview.chromium.org/2554413002/, BrowsingDataRemover was split into content-related and embedder-specific datatypes (BrowsingDataRemover[Impl] and [Chrome]BrowsingDataRemoverDelegate, respectively). The split was done according to where the relevant data storage backends resided. This CL intends to make the split more precise by looking at it more semantically. 1. While SSLHostStateDelegate lives in content/, it's state is deleted as a part of the HISTORY datatype, which is not a content/ concept. Therefore, this was moved to ChromeBrowsingDataRemoverDelegate. 2. The DCHECK verifying that all UI entrypoints to history deletion were disabled if !|may_delete_history| was also moved to ChromeBrowsingDataRemoverDelegate, as UI is also not a content/ concept. 3. The deletion of auth cache is done in BrowsingDataRemoverDelegate for both the COOKIES and PASSWORDS datatypes. However, PASSWORDS are not a content/ concept. Therefore, BrowsingDataRemoverImpl now deletes the auth cache for COOKIES, and ChromeBrowsingDataRemoverDelegate for PASSWORDS. This is currently done by the duplication of the code, as some duplication is necessary anyway, and only about ~10 lines of code are duplicated unnecessarily; this seems to be too few to justify creating a new file in content/public to be shared between content/ and chrome/. TODO: This is the second duplication between the two classes. If this keeps happenning, consider putting the shared functionality to content/public anyway. BUG=668114 Review-Url: https://codereview.chromium.org/2617403005 Cr-Commit-Position: refs/heads/master@{#443233} Committed: https://chromium.googlesource.com/chromium/src/+/8733eb80974a809a19c16535bfd39c867845acab

Patch Set 1 #

Patch Set 2 : Rebase. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+50 lines, -21 lines) Patch
M chrome/browser/browsing_data/browsing_data_remover_impl.cc View 3 chunks +1 line, -21 lines 0 comments Download
M chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc View 1 7 chunks +48 lines, -0 lines 0 comments Download

Messages

Total messages: 17 (11 generated)
msramek
Hi Mike! Please have a look! This is a little cleanup needed to split the ...
3 years, 11 months ago (2017-01-10 15:24:25 UTC) #7
msramek
Hmm, regarding point #3 in the CL description, I now realized that there might be ...
3 years, 11 months ago (2017-01-10 16:26:44 UTC) #10
Mike West
This CL LGTM, but reducing future duplication sounds like a reasonable goal. I don't think ...
3 years, 11 months ago (2017-01-12 12:56:18 UTC) #11
msramek
On 2017/01/12 12:56:18, Mike West (sloooooow) wrote: > This CL LGTM, but reducing future duplication ...
3 years, 11 months ago (2017-01-12 14:03:21 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2617403005/40001
3 years, 11 months ago (2017-01-12 14:03:45 UTC) #14
commit-bot: I haz the power
3 years, 11 months ago (2017-01-12 15:15:44 UTC) #17
Message was sent while issue was closed.
Committed patchset #2 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/8733eb80974a809a19c16535bfd3...

Powered by Google App Engine
This is Rietveld 408576698