Remove the unused OffDomainInclusionDetector

Remove the unused OffDomainInclusionDetector

BUG=606023
Committed: https://crrev.com/14968931960567f7985f97f28322d8e82f6497a8
Cr-Commit-Position: refs/heads/master@{#393914}

[Joe Mason, 2016-05-11 16:50:50]

joenotcharles@chromium.org changed reviewers:
+ gab@chromium.org, nparker@chromium.org

[Joe Mason, 2016-05-11 16:50:51]

PTAL (nparker for code, gab for histograms)

[Nathan Parker, 2016-05-11 17:34:28]

lgtm

[gab, 2016-05-11 17:48:25]

lgtm, thanks

[gab, 2016-05-11 17:52:05]

On 2016/05/11 17:48:25, gab wrote:
> lgtm, thanks

Actually hold on, one more thing, https://codereview.chromium.org/836963003 had
introduced MatchInclusionWhitelistUrl() and looks like
OffDomainInclusionDetector was still the only caller so it could be removed too.

[Joe Mason, 2016-05-11 19:59:19]

On 2016/05/11 17:52:05, gab wrote:
> On 2016/05/11 17:48:25, gab wrote:
> > lgtm, thanks
> 
> Actually hold on, one more thing, https://codereview.chromium.org/836963003
had
> introduced MatchInclusionWhitelistUrl() and looks like
> OffDomainInclusionDetector was still the only caller so it could be removed
too.

Done. Going further to get rid of the actual implementation of the whitelist
goes down a rabbit-hole that I don't have time for right now, so I just added a
TODO.

[Joe Mason, 2016-05-11 19:59:42]

On 2016/05/11 19:59:19, Joe Mason wrote:
> On 2016/05/11 17:52:05, gab wrote:
> > On 2016/05/11 17:48:25, gab wrote:
> > > lgtm, thanks
> > 
> > Actually hold on, one more thing, https://codereview.chromium.org/836963003
> had
> > introduced MatchInclusionWhitelistUrl() and looks like
> > OffDomainInclusionDetector was still the only caller so it could be removed
> too.
> 
> Done. Going further to get rid of the actual implementation of the whitelist
> goes down a rabbit-hole that I don't have time for right now, so I just added
a
> TODO.

...and will leave the bug open since there's more to remove.

[Joe Mason, 2016-05-15 16:44:59]

This should be the whole thing. PTAL again.

https://codereview.chromium.org/1970823002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_database.h (right):

https://codereview.chromium.org/1970823002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_database.h:446: obsolete_INCLUSION,
Wasn't sure if it's safe to renumber this enum so I didn't remove this value.

[gab, 2016-05-16 13:27:58]

https://codereview.chromium.org/1970823002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

https://codereview.chromium.org/1970823002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_database.cc:710: // TODO(davidben):
Remove this after April 15, 2016.
Remove this while you're here?

https://codereview.chromium.org/1970823002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_database.cc:1662: const bool r5 =
true; // Obsolete: inclusion_whitelist_store_
Remove this one and re-label other r#'s as they are merely runtime local
variables.

https://codereview.chromium.org/1970823002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_database.h (right):

https://codereview.chromium.org/1970823002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_database.h:446: obsolete_INCLUSION,
On 2016/05/15 16:44:59, Joe Mason wrote:
> Wasn't sure if it's safe to renumber this enum so I didn't remove this value.

This enum is only used at runtime (not stored to disk or histogram) so it's safe
to remove this value.

Note: the only time it's not safe to remove a value and implicitly re-label
values following it is when it's used in histograms or stored to disk (in which
case best practice is to declare explicit values and leave it commented out for
reference on deprecation, e.g. as you did for ListType::INCLUSIONWHITELIST).

[Joe Mason, 2016-05-16 14:18:52]

https://codereview.chromium.org/1970823002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

https://codereview.chromium.org/1970823002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_database.cc:710: // TODO(davidben):
Remove this after April 15, 2016.
On 2016/05/16 13:27:58, gab wrote:
> Remove this while you're here?

Prefer to do this in a followup since it's not logically part of this change.

https://codereview.chromium.org/1970823002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_database.cc:1662: const bool r5 =
true; // Obsolete: inclusion_whitelist_store_
On 2016/05/16 13:27:58, gab wrote:
> Remove this one and re-label other r#'s as they are merely runtime local
> variables.

Acknowledged.

https://codereview.chromium.org/1970823002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_database.h (right):

https://codereview.chromium.org/1970823002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_database.h:446: obsolete_INCLUSION,
On 2016/05/16 13:27:58, gab wrote:
> On 2016/05/15 16:44:59, Joe Mason wrote:
> > Wasn't sure if it's safe to renumber this enum so I didn't remove this
value.
> 
> This enum is only used at runtime (not stored to disk or histogram) so it's
safe
> to remove this value.
> 
> Note: the only time it's not safe to remove a value and implicitly re-label
> values following it is when it's used in histograms or stored to disk (in
which
> case best practice is to declare explicit values and leave it commented out
for
> reference on deprecation, e.g. as you did for ListType::INCLUSIONWHITELIST).

Acknowledged. Couldn't tell from looking at the code whether it was serialized
or not.

[Nathan Parker, 2016-05-16 16:40:10]

lgtm

with some nits

https://codereview.chromium.org/1970823002/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_database_unittest.cc (right):

https://codereview.chromium.org/1970823002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_database_unittest.cc:446:
chunks.push_back(AddChunkFullHashValue(8,
Go ahead and renumber these, so they're easier to verify visually.

https://codereview.chromium.org/1970823002/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_service.h (right):

https://codereview.chromium.org/1970823002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service.h:60: #if
defined(FULL_SAFE_BROWSING)
Delete this line as well, and the ifdef, since it isn't used in this file.

[Joe Mason, 2016-05-16 19:34:54]

The CQ bit was checked by joenotcharles@chromium.org

[Joe Mason, 2016-05-16 19:34:55]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org,
nparker@chromium.org
Link to the patchset: https://codereview.chromium.org/1970823002/#ps80001
(title: "Fix nits")

[Joe Mason, 2016-05-16 19:35:12]

https://codereview.chromium.org/1970823002/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_database_unittest.cc (right):

https://codereview.chromium.org/1970823002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_database_unittest.cc:446:
chunks.push_back(AddChunkFullHashValue(8,
On 2016/05/16 16:40:10, Nathan Parker wrote:
> Go ahead and renumber these, so they're easier to verify visually.

Done.

https://codereview.chromium.org/1970823002/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_service.h (right):

https://codereview.chromium.org/1970823002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service.h:60: #if
defined(FULL_SAFE_BROWSING)
On 2016/05/16 16:40:10, Nathan Parker wrote:
> Delete this line as well, and the ifdef, since it isn't used in this file.

Done.

[commit-bot: I haz the power, 2016-05-16 19:35:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1970823002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1970823002/80001

[commit-bot: I haz the power, 2016-05-16 19:45:34]

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

[commit-bot: I haz the power, 2016-05-16 19:45:36]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Joe Mason, 2016-05-16 20:03:37]

joenotcharles@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Joe Mason, 2016-05-16 20:03:37]

+asvitkine to review histograms.xml (I guess gab wasn't a valid owner after
all.)

[Alexei Svitkine (slow), 2016-05-16 20:41:19]

lgtm

[Joe Mason, 2016-05-16 20:41:53]

The CQ bit was checked by joenotcharles@chromium.org

[commit-bot: I haz the power, 2016-05-16 20:42:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1970823002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1970823002/80001

[gab, 2016-05-16 20:43:52]

lgtm, should also notify the safe browsing team that they can stop serving this
list (or maybe we want to wait until M52 hits stable? guess it doesn't matter if
we're not using the data from M50/M51 either)

https://codereview.chromium.org/1970823002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

https://codereview.chromium.org/1970823002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_database.cc:710: // TODO(davidben):
Remove this after April 15, 2016.
On 2016/05/16 14:18:52, Joe Mason wrote:
> On 2016/05/16 13:27:58, gab wrote:
> > Remove this while you're here?
> 
> Prefer to do this in a followup since it's not logically part of this change.

As you wish.

[commit-bot: I haz the power, 2016-05-16 21:07:03]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-16 21:08:41]

Description was changed from

==========
Remove the unused OffDomainInclusionDetector

BUG=606023
==========

to

==========
Remove the unused OffDomainInclusionDetector

BUG=606023

Committed: https://crrev.com/14968931960567f7985f97f28322d8e82f6497a8
Cr-Commit-Position: refs/heads/master@{#393914}
==========

[commit-bot: I haz the power, 2016-05-16 21:08:43]

Patchset 5 (id:??) landed as
https://crrev.com/14968931960567f7985f97f28322d8e82f6497a8
Cr-Commit-Position: refs/heads/master@{#393914}