Switch Sdch3.AdvertisedWithSecureScheme histogram to use target_url.SchemeIsCryptographic().

Switch Sdch3.AdvertisedWithSecureScheme histogram to use target_url.SchemeIsCryptographic().

SchemeIsSecure() is deprecated. Replacing it with SchemeIsCryptographic() in this case is safe because the check was effectively only checking for HTTP vs. HTTPS. (The UMA histogram values are even called "HTTP" and HTTPS"):

- Sdch3.AdvertisedWithSecureScheme is recorded in SdchManager::GetDictionarySet(),
- which is only called from URLRequestHttpJob::AddExtraHeaders(),
- which is only used for HTTP and HTTPS requests.

BUG=362214
Committed: https://crrev.com/ee4c0b0c531417177164e91d3a987ab2946c6792
Cr-Commit-Position: refs/heads/master@{#343801}

[lgarron, 2015-08-14 20:44:51]

lgarron@chromium.org changed reviewers:
+ rdsmith@chromium.org

[lgarron, 2015-08-14 20:44:52]

Welcome to the removal of the last remaining use of SchemeIsSecure()!

rdsmith@: You reviewed the introduction of this histogram[1]; would you mind
reviewing this?

[1] https://codereview.chromium.org/1137753006

[Randy Smith (Not in Mondays), 2015-08-17 16:18:17]

rdsmith@chromium.org changed reviewers:
+ ellyjones@chromium.org

[Randy Smith (Not in Mondays), 2015-08-17 16:18:17]

So I'd suggest revving the histogram name and marking the old one obsolete if
there's any functional difference between IsSecureScheme() and
SchemeIsCryptographic() (which I suspect there is, otherwise you wouldn't be
bothering with the change:-}).  That'll mean that any UMA statistics that
straddle versions that do and don't have this change won't be lumping apples and
oranges in the same bin.

Also, Elly, could you glance at this?  I think of your security foo as stronger
than mine.  It's a one line change--I'm just pulling you in for conceptual level
awareness/gotchas.

[Elly Fong-Jones, 2015-08-17 18:17:34]

On 2015/08/17 16:18:17, rdsmith wrote:
> So I'd suggest revving the histogram name and marking the old one obsolete if
> there's any functional difference between IsSecureScheme() and
> SchemeIsCryptographic() (which I suspect there is, otherwise you wouldn't be
> bothering with the change:-}).  That'll mean that any UMA statistics that
> straddle versions that do and don't have this change won't be lumping apples
and
> oranges in the same bin.
> 
> Also, Elly, could you glance at this?  I think of your security foo as
stronger
> than mine.  It's a one line change--I'm just pulling you in for conceptual
level
> awareness/gotchas.

Sure. SchemeIsCryptographic is the correct thing to use here; we only care about
whether SDCH is being advertised over a TLS connection specifically, not about
whether it's being advertised to a "secure scheme", which includes weird stuff
like chrome:// and such that we would never advertise SDCH to anyway.

I don't think we need to rev the histogram; this is just making the intent of
the code clearer. There's no way we advertised to chrome:// and similar before.

[Randy Smith (Not in Mondays), 2015-08-17 18:43:33]

Ok, thanks!  LGTM from me based on Elly's review; you can ignore my suggestion.

[lgarron, 2015-08-17 18:45:05]

On 2015/08/17 at 18:43:33, rdsmith wrote:
> Ok, thanks!  LGTM from me based on Elly's review; you can ignore my
suggestion.

Cool. Thanks for confirming the behaviour, Elly. :-)

[lgarron, 2015-08-17 18:45:06]

The CQ bit was checked by lgarron@chromium.org

[commit-bot: I haz the power, 2015-08-17 18:46:35]

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

[commit-bot: I haz the power, 2015-08-17 21:37:07]

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

[commit-bot: I haz the power, 2015-08-17 21:37:08]

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

[lgarron, 2015-08-17 21:39:05]

The CQ bit was checked by lgarron@chromium.org

[commit-bot: I haz the power, 2015-08-17 21:40:10]

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

[commit-bot: I haz the power, 2015-08-18 00:37:50]

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

[commit-bot: I haz the power, 2015-08-18 00:37:51]

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

[lgarron, 2015-08-18 01:05:26]

The CQ bit was checked by lgarron@chromium.org

[commit-bot: I haz the power, 2015-08-18 01:06:35]

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

[commit-bot: I haz the power, 2015-08-18 05:10:11]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-08-18 05:11:19]

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