Remove PrerenderLocalPredictor, part 2.

Remove PrerenderLocalPredictor, part 2.

This removes the side-effect-free SafeBrowsing whitelist.

BUG=471807
Committed: https://crrev.com/b67d3e3c0491a4b3c613295a098eacc3c6b28b7c
Cr-Commit-Position: refs/heads/master@{#325740}

[davidben, 2015-04-08 21:44:40]

davidben@chromium.org changed reviewers:
+ shess@chromium.org

[davidben, 2015-04-08 21:44:41]

This depends on https://codereview.chromium.org/1077433002/

[Scott Hess - ex-Googler, 2015-04-08 22:32:27]

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

https://codereview.chromium.org/1073633002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_database_unittest.cc:448:
chunks.push_back(AddChunkHashedIpValue(9, "::ffff:192.168.1.0", 120));
I don't think you need to renumber these.  Unless you do.  If not renumbering
works, I'd stick with that just to avoid changing other list tests.  It's not
like we're going to run out of chunk ids.

https://codereview.chromium.org/1073633002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1073633002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:33137: +  </obsolete>
AFAICT, the practice for marking the field trial obsolete in SafeBrowsingLists
would be to edit the LABEL.

[davidben, 2015-04-09 21:54:38]

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

https://codereview.chromium.org/1073633002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_database_unittest.cc:448:
chunks.push_back(AddChunkHashedIpValue(9, "::ffff:192.168.1.0", 120));
On 2015/04/08 22:32:26, Scott Hess wrote:
> I don't think you need to renumber these.  Unless you do.  If not renumbering
> works, I'd stick with that just to avoid changing other list tests.  It's not
> like we're going to run out of chunk ids.

Done. (The index into lists has to change, but I kept the chunk IDs the same.)

https://codereview.chromium.org/1073633002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1073633002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:33137: +  </obsolete>
On 2015/04/08 22:32:26, Scott Hess wrote:
> AFAICT, the practice for marking the field trial obsolete in SafeBrowsingLists
> would be to edit the LABEL.

I'm not sure I follow. What do you mean by LABEL?

[Scott Hess - ex-Googler, 2015-04-09 23:17:19]

https://codereview.chromium.org/1073633002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1073633002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:33137: +  </obsolete>
On 2015/04/09 21:54:38, David Benjamin wrote:
> On 2015/04/08 22:32:26, Scott Hess wrote:
> > AFAICT, the practice for marking the field trial obsolete in
SafeBrowsingLists
> > would be to edit the LABEL.
> 
> I'm not sure I follow. What do you mean by LABEL?

Sorry, I'm away from my main desk, there should be a histogram_suffixes item for
SafeBrowsingLists.  The one for side-effect-free-whitelist should change so the
LABEL includes "obsolete" in the text.  If there is no label, add one for the
appropriate item (I think the rule is that the label is used for display in
preference to the name, but the name is used if there is no label).  Look around
the area, there are examples with an "obsolete" inline like that.

[davidben, 2015-04-09 23:41:22]

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

[davidben, 2015-04-09 23:41:23]

+asvitkine for histograms.xml

https://codereview.chromium.org/1073633002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1073633002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:33137: +  </obsolete>
On 2015/04/09 23:17:19, Scott Hess wrote:
> On 2015/04/09 21:54:38, David Benjamin wrote:
> > On 2015/04/08 22:32:26, Scott Hess wrote:
> > > AFAICT, the practice for marking the field trial obsolete in
> SafeBrowsingLists
> > > would be to edit the LABEL.
> > 
> > I'm not sure I follow. What do you mean by LABEL?
> 
> Sorry, I'm away from my main desk, there should be a histogram_suffixes item
for
> SafeBrowsingLists.  The one for side-effect-free-whitelist should change so
the
> LABEL includes "obsolete" in the text.  If there is no label, add one for the
> appropriate item (I think the rule is that the label is used for display in
> preference to the name, but the name is used if there is no label).  Look
around
> the area, there are examples with an "obsolete" inline like that.

Gotcha. I think actually I need to obsolete both this one (which isn't generated
by suffixes) and the SideEffectFreeWhitelist suffix.

It seems you can actually obsolete individual suffixes with the <obsolete> tag.
I've done that since it'll participate in the UI right, but if you prefer
changing the label, I can stick with that.

[Scott Hess - ex-Googler, 2015-04-09 23:52:31]

LGTM

https://codereview.chromium.org/1073633002/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1073633002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:66943: +  </suffix>
Oh, if this works, I'm absolutely fine with it.  From reading around it looked
to me like you could only obsolete the entire group.

[Alexei Svitkine (slow), 2015-04-10 15:03:38]

lgtm % comment

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

https://codereview.chromium.org/1073633002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_database.cc:726: // from when it was
previously enabled.
Should this have a TODO() to clean up this code in some number of releases?

[Scott Hess - ex-Googler, 2015-04-10 15:51:45]

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

https://codereview.chromium.org/1073633002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_database.cc:726: // from when it was
previously enabled.
On 2015/04/10 15:03:37, Alexei Svitkine wrote:
> Should this have a TODO() to clean up this code in some number of releases?

I'd say after a specific date, but wouldn't worry too much about whether anyone
bothers (given the I/O load in this region, this operation really only costs the
code to support it).

I once would have been able to tell you how quickly things churn based on the
version-tracking histograms (for stores and prefix sets), but I no longer
remember.  I think around two release cycles was where I started to wonder if
the numbers were reflecting organic restarts versus weird edge cases, like
restarts from freeze-dried VM images, or profiles with broken storage that
wasn't writable.  So if this said something like "Delete after April 15, 2016",
I think you'd be more than safe.

[davidben, 2015-04-13 19:48:18]

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

https://codereview.chromium.org/1073633002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_database.cc:726: // from when it was
previously enabled.
On 2015/04/10 15:51:45, Scott Hess wrote:
> On 2015/04/10 15:03:37, Alexei Svitkine wrote:
> > Should this have a TODO() to clean up this code in some number of releases?
> 
> I'd say after a specific date, but wouldn't worry too much about whether
anyone
> bothers (given the I/O load in this region, this operation really only costs
the
> code to support it).
> 
> I once would have been able to tell you how quickly things churn based on the
> version-tracking histograms (for stores and prefix sets), but I no longer
> remember.  I think around two release cycles was where I started to wonder if
> the numbers were reflecting organic restarts versus weird edge cases, like
> restarts from freeze-dried VM images, or profiles with broken storage that
> wasn't writable.  So if this said something like "Delete after April 15,
2016",
> I think you'd be more than safe.

Done.

[davidben, 2015-04-17 19:52:38]

The CQ bit was checked by davidben@chromium.org

[davidben, 2015-04-17 19:52:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, shess@chromium.org
Link to the patchset: https://codereview.chromium.org/1073633002/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2015-04-17 19:53:09]

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

[commit-bot: I haz the power, 2015-04-18 00:07:22]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-04-18 00:08:16]

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