Remove obsolete histogram values from SSL.InsecureContent

Remove obsolete histogram values from SSL.InsecureContent

Mixed content histogram values that depend on the URL of the page have
since been rendered obsolete by Rappor, and they complicate mixed
content checking with OOPIFs. This CL deprecates the histogram values
that break down mixed content by Google property but preserves the basic
counters for displaying or running mixed content.

BUG=486936
Committed: https://crrev.com/163c47aac94491b32d1e1f3a049d4fb67a01855e
Cr-Commit-Position: refs/heads/master@{#369851}

[estark, 2016-01-14 19:14:21]

estark@chromium.org changed reviewers:
+ alexmos@chromium.org

[estark, 2016-01-14 19:14:21]

Hey Alex -- after learning that most of the SSL.InsecureContent values are
obsolete, I think it makes sense to change tack here and just remove them, which
will simplify the plumbing for OOPIF mixed content checking. So this CL could
replace https://codereview.chromium.org/1550233002/, and I've rebased
https://codereview.chromium.org/1550723003/ on top of this one.

[estark, 2016-01-14 20:59:41]

https://codereview.chromium.org/1583813003/diff/40001/chrome/renderer/content...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/1583813003/diff/40001/chrome/renderer/content...
chrome/renderer/content_settings_observer.cc:457: Send(new
ChromeViewHostMsg_DidBlockDisplayingInsecureContent(routing_id()));
alexmos: in a different CL you had asked this:

> Just curious, how does this still work once you change the frame on which this
> is called to always be the LocalFrame?  Does this go through the correct
> WebContents on the browser side regardless of which of its frames it's routed
> through?

I don't understand this question and so now I'm worried that I'm very confused
about something. :) I thought all the frames in a page will go to the same
WebContents, so it doesn't matter which frame this messages goes through. Does
that make any sense?

[alexmos, 2016-01-14 22:23:01]

https://codereview.chromium.org/1583813003/diff/40001/chrome/renderer/content...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/1583813003/diff/40001/chrome/renderer/content...
chrome/renderer/content_settings_observer.cc:457: Send(new
ChromeViewHostMsg_DidBlockDisplayingInsecureContent(routing_id()));
On 2016/01/14 20:59:41, estark wrote:
> alexmos: in a different CL you had asked this:
> 
> > Just curious, how does this still work once you change the frame on which
this
> > is called to always be the LocalFrame?  Does this go through the correct
> > WebContents on the browser side regardless of which of its frames it's
routed
> > through?
> 
> I don't understand this question and so now I'm worried that I'm very confused
> about something. :) I thought all the frames in a page will go to the same
> WebContents, so it doesn't matter which frame this messages goes through. Does
> that make any sense?

Sorry, I didn't phrase that question correctly. :)  Your understanding is
correct; since the message was routed through a different frame on the same
page, I just wanted to double-check that the receiver didn't care about which
frame sent the message, i.e., that it was a page-level message handled somewhere
in WebContents which didn't use the receiving RenderFrameHost.  (I.e.,
WebContentsObserver has two versions of OnMessageReceived, one with the
RenderFrameHost and one without.  InfoBarService, which handles this IPC,
overrides the one without the RFH, so indeed it doesn't care about the frame
that sent it.)

[alexmos, 2016-01-14 23:04:32]

Awesome, I'm very happy that we can do this.  LGTM!

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

https://codereview.chromium.org/1583813003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:67723: +  <int value="1"
label="(deprecated) Displayed by *.google.com"/>
nit: I don't know anything about histogram conventions, but looking at this file
it seems a bit more common to put "(deprecated)" at the end of the label text
rather than beginning.  Probably fine either way, though.

[estark, 2016-01-14 23:13:16]

estark@chromium.org changed reviewers:
+ jochen@chromium.org, mkwst@chromium.org, mpearson@chromium.org,
torne@chromium.org

[estark, 2016-01-14 23:13:16]

Thanks Alex!

mpearson: can you please review histograms.xml?
mkwst: can you please review Blink changes and components/test_runner?
torne: can you please review android_webview/?
jochen: can you please review chrome/renderer/content_settings_observer.*?

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

https://codereview.chromium.org/1583813003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:67723: +  <int value="1"
label="(deprecated) Displayed by *.google.com"/>
On 2016/01/14 23:04:32, alexmos wrote:
> nit: I don't know anything about histogram conventions, but looking at this
file
> it seems a bit more common to put "(deprecated)" at the end of the label text
> rather than beginning.  Probably fine either way, though.

Ah, you're right, the one example that I happened to look at had it at the
beginning. Done.

[estark, 2016-01-14 23:14:03]

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

[estark, 2016-01-14 23:14:03]

Also +lgarron because he owns the histogram for which I'm deprecating values.

[Mark P, 2016-01-14 23:18:21]

histograms.xml lgtm

https://codereview.chromium.org/1583813003/diff/40002/chrome/renderer/content...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/1583813003/diff/40002/chrome/renderer/content...
chrome/renderer/content_settings_observer.cc:47: enum {
Do you want to put
// deprecated
comments here for the appropriate enums?

[Mike West, 2016-01-15 08:29:31]

LGTM % question. If we can remove the argument from the "ran insecure content"
bits as well, that would be lovely. If not, the current code looks fine.

https://codereview.chromium.org/1583813003/diff/40002/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/MixedContentChecker.cpp (right):

https://codereview.chromium.org/1583813003/diff/40002/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:344: allowed =
shouldAskEmbedder && client->allowRunningInsecureContent(settings &&
settings->allowRunningOfInsecureContent(), securityOrigin, url);
What about the |securityOrigin| here? I thought these were passed in for the
same purpose.

https://codereview.chromium.org/1583813003/diff/40002/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:346:
client->didRunInsecureContent(securityOrigin, url);
Ditto.

[jochen (gone - plz use gerrit), 2016-01-15 14:48:51]

lgtm

[Torne, 2016-01-15 16:32:14]

android_webview LGTM

[Torne, 2016-01-15 16:32:15]

android_webview LGTM

[estark, 2016-01-15 18:42:32]

Thanks, everyone.

https://codereview.chromium.org/1583813003/diff/40002/chrome/renderer/content...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/1583813003/diff/40002/chrome/renderer/content...
chrome/renderer/content_settings_observer.cc:47: enum {
On 2016/01/14 23:18:20, Mark P wrote:
> Do you want to put
> // deprecated
> comments here for the appropriate enums?

Done.

https://codereview.chromium.org/1583813003/diff/40002/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/MixedContentChecker.cpp (right):

https://codereview.chromium.org/1583813003/diff/40002/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:344: allowed =
shouldAskEmbedder && client->allowRunningInsecureContent(settings &&
settings->allowRunningOfInsecureContent(), securityOrigin, url);
On 2016/01/15 08:29:31, Mike West wrote:
> What about the |securityOrigin| here? I thought these were passed in for the
> same purpose.

Unfortunately, this gets passed into DidBlockContentType(), which sends it as
the |details| argument to ChromeViewHostMsg_ContentBlocked, which records the
origin with Rappor. So I think this argument has to stay as long as we care
about those Rappor metrics.

[estark, 2016-01-15 18:42:37]

The CQ bit was checked by estark@chromium.org

[estark, 2016-01-15 18:42:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
alexmos@chromium.org, jochen@chromium.org, torne@chromium.org,
mpearson@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1583813003/#ps70001
(title: "mpearson comment")

[commit-bot: I haz the power, 2016-01-15 18:43:21]

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

[commit-bot: I haz the power, 2016-01-15 18:48:28]

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

[commit-bot: I haz the power, 2016-01-15 18:48:29]

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_...)

[estark, 2016-01-15 20:41:56]

The CQ bit was checked by estark@chromium.org

[commit-bot: I haz the power, 2016-01-15 20:42:20]

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

[commit-bot: I haz the power, 2016-01-15 21:41:10]

Committed patchset #5 (id:70001)

[commit-bot: I haz the power, 2016-01-15 21:42:18]

Description was changed from

==========
Remove obsolete histogram values from SSL.InsecureContent

Mixed content histogram values that depend on the URL of the page have
since been rendered obsolete by Rappor, and they complicate mixed
content checking with OOPIFs. This CL deprecates the histogram values
that break down mixed content by Google property but preserves the basic
counters for displaying or running mixed content.

BUG=486936
==========

to

==========
Remove obsolete histogram values from SSL.InsecureContent

Mixed content histogram values that depend on the URL of the page have
since been rendered obsolete by Rappor, and they complicate mixed
content checking with OOPIFs. This CL deprecates the histogram values
that break down mixed content by Google property but preserves the basic
counters for displaying or running mixed content.

BUG=486936

Committed: https://crrev.com/163c47aac94491b32d1e1f3a049d4fb67a01855e
Cr-Commit-Position: refs/heads/master@{#369851}
==========

[commit-bot: I haz the power, 2016-01-15 21:42:19]

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