Attach mixed content status to resource requests when sent to devtools

Attach mixed content status to resource requests when sent to devtools

This CL computes the mixed content status (blockable, optionally
blockable, or not mixed) of resource requests before they are sent to
DevTools via the InspectorResourceAgent. The ResourceRequests's mixed
content status gets sent to DevTools as part of the Request parameter to
the requestWillBeSent event. The mixed content status will be used to
display mixed resources in the Security panel.

BUG=518065
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200936

[estark, 2015-08-14 22:50:20]

estark@chromium.org changed reviewers:
+ mkwst@chromium.org, pfeldman@chromium.org

[estark, 2015-08-14 22:50:21]

pfeldman: could you please review the devtools changes in this CL? That would be
InspectorResourceAgent.cpp and protocol.json. This is also my first attempt at
writing an inspector-protocol LayoutTest, so it would be great if you could take
a look at that.

mkwst: could you please review, um, the rest? The meat of it is mostly in
MixedContentChecker.cpp and ResourceRequest.h, I think. The rest is mostly just
removing const from a bunch of places where ResourceRequests are passed, which
is kind of a bummer. Instead of mutating the ResourceRequest, we could return
the information about what type of mixed content it is from
FrameFetchContext::canRequest() and plumb it directly to devtools instead of
passing it on the ResourceRequest... but I'm not sure if that's any better.

Thanks!

Oh, also, this is Part 2 of the plan described in
https://docs.google.com/a/google.com/document/d/1Jpw4Fa6jFNalf_Kvu5dqN_fT9Xp6...
(Google-only doc).

[Mike West, 2015-08-17 15:36:45]

I agree that dropping const is the better of two evils (the other being to pipe
the new value all the way through the return path for the request).

This looks like a reasonable approach, just one thing about that new enum:

https://codereview.chromium.org/1299493003/diff/1/Source/core/loader/MixedCon...
File Source/core/loader/MixedContentChecker.h (right):

https://codereview.chromium.org/1299493003/diff/1/Source/core/loader/MixedCon...
Source/core/loader/MixedContentChecker.h:58: static bool
shouldBlockFetch(LocalFrame*, ResourceRequest*, WebURLRequest::RequestContext,
WebURLRequest::FrameType, const KURL&, ReportingStatus = SendReport);
You can probably make this variant private, as I think callsites are going to
use the one on #60 or #65, right?

https://codereview.chromium.org/1299493003/diff/1/Source/platform/network/Res...
File Source/platform/network/ResourceRequest.h (right):

https://codereview.chromium.org/1299493003/diff/1/Source/platform/network/Res...
Source/platform/network/ResourceRequest.h:67: enum MixedContentStatus {
1. Why not use `MixedContentChecker::ContentType` instead of creating a new
enum?

2. I'd prefer to align with the spec's "blockable" and "optionally blockable"
language.

[estark, 2015-08-17 17:20:12]

https://codereview.chromium.org/1299493003/diff/1/Source/core/loader/MixedCon...
File Source/core/loader/MixedContentChecker.h (right):

https://codereview.chromium.org/1299493003/diff/1/Source/core/loader/MixedCon...
Source/core/loader/MixedContentChecker.h:58: static bool
shouldBlockFetch(LocalFrame*, ResourceRequest*, WebURLRequest::RequestContext,
WebURLRequest::FrameType, const KURL&, ReportingStatus = SendReport);
On 2015/08/17 15:36:45, Mike West (traveling) wrote:
> You can probably make this variant private, as I think callsites are going to
> use the one on #60 or #65, right?

Done.

https://codereview.chromium.org/1299493003/diff/1/Source/platform/network/Res...
File Source/platform/network/ResourceRequest.h (right):

https://codereview.chromium.org/1299493003/diff/1/Source/platform/network/Res...
Source/platform/network/ResourceRequest.h:67: enum MixedContentStatus {
On 2015/08/17 15:36:45, Mike West (traveling) wrote:
> 1. Why not use `MixedContentChecker::ContentType` instead of creating a new
> enum?
> 
> 2. I'd prefer to align with the spec's "blockable" and "optionally blockable"
> language.

Ah, I didn't realize that blockable/optionally blockable was spec language;
thought it was an internal MixedContentChecker thingy. Ok, so I moved
ContextType to be ResourceRequest::ContextType instead of
MixedContentChecker::ContextType, so that ResourceRequest can use it -- does
that sound okay?

There is a small additional wrinkle that ContextType doesn't allow a way to say
that a given request is NOT mixed content. So I added
ResourceRequest::isMixedContent() to distinguish, but that feels a little clunky
to me. (For example, not sure what to initialize ResourceRequest::contextType()
to if isMixedContent() is false.) Another alternative would be to add a
ContextTypeNotMixedContent value to the ContextType enum. What do you think?

https://codereview.chromium.org/1299493003/diff/20001/Source/core/inspector/I...
File Source/core/inspector/InspectorResourceAgent.cpp (right):

https://codereview.chromium.org/1299493003/diff/20001/Source/core/inspector/I...
Source/core/inspector/InspectorResourceAgent.cpp:214: return
TypeBuilder::Network::Request::MixedContentStatus::Active;
Mike, do you think we should stick with the blockable/optionally-blockable
language everywhere, namely in the devtools protocol as well?

I'm sort of torn -- it would be nice if we used the same language everywhere,
but we already use "displayed/ran insecure content" and "passive/active mixed
content" in lots of other places, so I'm not sure.

[Mike West, 2015-08-17 17:32:27]

On 2015/08/17 at 17:20:12, estark wrote:
>
https://codereview.chromium.org/1299493003/diff/1/Source/core/loader/MixedCon...
> File Source/core/loader/MixedContentChecker.h (right):
> 
>
https://codereview.chromium.org/1299493003/diff/1/Source/core/loader/MixedCon...
> Source/core/loader/MixedContentChecker.h:58: static bool
shouldBlockFetch(LocalFrame*, ResourceRequest*, WebURLRequest::RequestContext,
WebURLRequest::FrameType, const KURL&, ReportingStatus = SendReport);
> On 2015/08/17 15:36:45, Mike West (traveling) wrote:
> > You can probably make this variant private, as I think callsites are going
to
> > use the one on #60 or #65, right?
> 
> Done.
> 
>
https://codereview.chromium.org/1299493003/diff/1/Source/platform/network/Res...
> File Source/platform/network/ResourceRequest.h (right):
> 
>
https://codereview.chromium.org/1299493003/diff/1/Source/platform/network/Res...
> Source/platform/network/ResourceRequest.h:67: enum MixedContentStatus {
> On 2015/08/17 15:36:45, Mike West (traveling) wrote:
> > 1. Why not use `MixedContentChecker::ContentType` instead of creating a new
> > enum?
> > 
> > 2. I'd prefer to align with the spec's "blockable" and "optionally
blockable"
> > language.
> 
> Ah, I didn't realize that blockable/optionally blockable was spec language;
thought it was an internal MixedContentChecker thingy. Ok, so I moved
ContextType to be ResourceRequest::ContextType instead of
MixedContentChecker::ContextType, so that ResourceRequest can use it -- does
that sound okay?
> 
> There is a small additional wrinkle that ContextType doesn't allow a way to
say that a given request is NOT mixed content. So I added
ResourceRequest::isMixedContent() to distinguish, but that feels a little clunky
to me. (For example, not sure what to initialize ResourceRequest::contextType()
to if isMixedContent() is false.) Another alternative would be to add a
ContextTypeNotMixedContent value to the ContextType enum. What do you think?

Sounds totally reasonable to me. I much prefer that to `isMixedContent()`.

>
https://codereview.chromium.org/1299493003/diff/20001/Source/core/inspector/I...
> File Source/core/inspector/InspectorResourceAgent.cpp (right):
> 
>
https://codereview.chromium.org/1299493003/diff/20001/Source/core/inspector/I...
> Source/core/inspector/InspectorResourceAgent.cpp:214: return
TypeBuilder::Network::Request::MixedContentStatus::Active;
> Mike, do you think we should stick with the blockable/optionally-blockable
language everywhere, namely in the devtools protocol as well?
> 
> I'm sort of torn -- it would be nice if we used the same language everywhere,
but we already use "displayed/ran insecure content" and "passive/active mixed
content" in lots of other places, so I'm not sure.

We dropped "passive" and "active" from the spec because it was a bit limiting.
Basically, we want to block more and more types of content as usage allows. Look
at fonts, for instance: it's hard to argue that they're "active", but it's
pretty clear that we want to block them in a secure context. We invented
"blockable" and "optionally-blockable" to better document the intent of the
categories. I'd prefer to use that nomenclature throughout the code.

[estark, 2015-08-17 17:55:35]

Thanks, Mike. Changed isMixedContent() to ContextTypeNotMixedContent, and
changed the language in the devtools protocol enum.

On 2015/08/17 17:32:27, Mike West (traveling) wrote:
> On 2015/08/17 at 17:20:12, estark wrote:
> >
>
https://codereview.chromium.org/1299493003/diff/1/Source/core/loader/MixedCon...
> > File Source/core/loader/MixedContentChecker.h (right):
> > 
> >
>
https://codereview.chromium.org/1299493003/diff/1/Source/core/loader/MixedCon...
> > Source/core/loader/MixedContentChecker.h:58: static bool
> shouldBlockFetch(LocalFrame*, ResourceRequest*, WebURLRequest::RequestContext,
> WebURLRequest::FrameType, const KURL&, ReportingStatus = SendReport);
> > On 2015/08/17 15:36:45, Mike West (traveling) wrote:
> > > You can probably make this variant private, as I think callsites are going
> to
> > > use the one on #60 or #65, right?
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1299493003/diff/1/Source/platform/network/Res...
> > File Source/platform/network/ResourceRequest.h (right):
> > 
> >
>
https://codereview.chromium.org/1299493003/diff/1/Source/platform/network/Res...
> > Source/platform/network/ResourceRequest.h:67: enum MixedContentStatus {
> > On 2015/08/17 15:36:45, Mike West (traveling) wrote:
> > > 1. Why not use `MixedContentChecker::ContentType` instead of creating a
new
> > > enum?
> > > 
> > > 2. I'd prefer to align with the spec's "blockable" and "optionally
> blockable"
> > > language.
> > 
> > Ah, I didn't realize that blockable/optionally blockable was spec language;
> thought it was an internal MixedContentChecker thingy. Ok, so I moved
> ContextType to be ResourceRequest::ContextType instead of
> MixedContentChecker::ContextType, so that ResourceRequest can use it -- does
> that sound okay?
> > 
> > There is a small additional wrinkle that ContextType doesn't allow a way to
> say that a given request is NOT mixed content. So I added
> ResourceRequest::isMixedContent() to distinguish, but that feels a little
clunky
> to me. (For example, not sure what to initialize
ResourceRequest::contextType()
> to if isMixedContent() is false.) Another alternative would be to add a
> ContextTypeNotMixedContent value to the ContextType enum. What do you think?
> 
> Sounds totally reasonable to me. I much prefer that to `isMixedContent()`.
> 
> >
>
https://codereview.chromium.org/1299493003/diff/20001/Source/core/inspector/I...
> > File Source/core/inspector/InspectorResourceAgent.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1299493003/diff/20001/Source/core/inspector/I...
> > Source/core/inspector/InspectorResourceAgent.cpp:214: return
> TypeBuilder::Network::Request::MixedContentStatus::Active;
> > Mike, do you think we should stick with the blockable/optionally-blockable
> language everywhere, namely in the devtools protocol as well?
> > 
> > I'm sort of torn -- it would be nice if we used the same language
everywhere,
> but we already use "displayed/ran insecure content" and "passive/active mixed
> content" in lots of other places, so I'm not sure.
> 
> We dropped "passive" and "active" from the spec because it was a bit limiting.
> Basically, we want to block more and more types of content as usage allows.
Look
> at fonts, for instance: it's hard to argue that they're "active", but it's
> pretty clear that we want to block them in a secure context. We invented
> "blockable" and "optionally-blockable" to better document the intent of the
> categories. I'd prefer to use that nomenclature throughout the code.

[Mike West, 2015-08-17 19:43:42]

LGTM. Land it once Pavel is happy. :)

[pfeldman, 2015-08-17 20:32:52]

https://codereview.chromium.org/1299493003/diff/40001/Source/core/inspector/I...
File Source/core/inspector/InspectorResourceAgent.cpp (right):

https://codereview.chromium.org/1299493003/diff/40001/Source/core/inspector/I...
Source/core/inspector/InspectorResourceAgent.cpp:207:
TypeBuilder::Network::Request::MixedContentType::Enum getMixedContentType(const
ResourceRequest& request)
no get prefixes in Blink please.

https://codereview.chromium.org/1299493003/diff/40001/Source/core/loader/Mixe...
File Source/core/loader/MixedContentChecker.cpp (right):

https://codereview.chromium.org/1299493003/diff/40001/Source/core/loader/Mixe...
Source/core/loader/MixedContentChecker.cpp:301:
request->setContextType(ResourceRequest::ContextTypeNotMixedContent);
Sorry for not taking a look at it earlier. You should not plumb it as mutable
all the way to here to set this bit. Is returning enum an option?

[estark, 2015-08-17 20:36:22]

https://codereview.chromium.org/1299493003/diff/40001/Source/core/loader/Mixe...
File Source/core/loader/MixedContentChecker.cpp (right):

https://codereview.chromium.org/1299493003/diff/40001/Source/core/loader/Mixe...
Source/core/loader/MixedContentChecker.cpp:301:
request->setContextType(ResourceRequest::ContextTypeNotMixedContent);
On 2015/08/17 20:32:52, pfeldman wrote:
> Sorry for not taking a look at it earlier. You should not plumb it as mutable
> all the way to here to set this bit. Is returning enum an option?

Mike preferred setting it here on the request. If we return an enum, it would
have to be returned not just from here but also from
FrameFetchContext::canRequest().

[pfeldman, 2015-08-17 21:17:48]

> Mike preferred setting it here on the request. If we return an enum, it would
> have to be returned not just from here but also from
> FrameFetchContext::canRequest().

That would be definitely more reasonable than plumbing a non-const reference
into a query method and mutating it there.

[pfeldman, 2015-08-17 21:21:03]

Also, why would canRequest behavior change? Could you point to exactly what
information you need to plumb to where?

[estark, 2015-08-17 21:32:51]

On 2015/08/17 21:21:03, pfeldman wrote:
> Also, why would canRequest behavior change? Could you point to exactly what
> information you need to plumb to where?

The mixed content status needs to be associated with a Network.Request object
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
So I am trying to send it along with the requestWillBeSent event
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).

This event gets triggered from Resource::load() which gets called at
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...,
after FrameFetchContext::canRequest().

Mike, are you willing to reconsider the enum approach?

[Mike West, 2015-08-17 21:36:50]

On 2015/08/17 at 21:32:51, estark wrote:
> On 2015/08/17 21:21:03, pfeldman wrote:
> > Also, why would canRequest behavior change? Could you point to exactly what
> > information you need to plumb to where?
> 
> The mixed content status needs to be associated with a Network.Request object
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
So I am trying to send it along with the requestWillBeSent event
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
> 
> This event gets triggered from Resource::load() which gets called at
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...,
after FrameFetchContext::canRequest().
> 
> Mike, are you willing to reconsider the enum approach?

If Pavel has strong feelings about it, I'm happy to make you do a bunch of
busywork. :) It seems less clear than simply poking the bit onto the Request
while doing the checks, but perhaps I'm overestimating the annoyance of piping
the extra value around.

[pfeldman, 2015-08-17 21:55:37]

> If Pavel has strong feelings about it, I'm happy to make you do a bunch of
> busywork. :) It seems less clear than simply poking the bit onto the Request
> while doing the checks, but perhaps I'm overestimating the annoyance of piping
> the extra value around.

I might be underestimating the scope of necessary changes, but changing the
const ref into mutable and setting the bit in a can* method sounds like a major
design violation. That's why I am wondering what exactly needs to be plumbed
where.

[estark, 2015-08-17 21:58:08]

On 2015/08/17 21:55:37, pfeldman wrote:
> > If Pavel has strong feelings about it, I'm happy to make you do a bunch of
> > busywork. :) It seems less clear than simply poking the bit onto the Request
> > while doing the checks, but perhaps I'm overestimating the annoyance of
piping
> > the extra value around.
> 
> I might be underestimating the scope of necessary changes, but changing the
> const ref into mutable and setting the bit in a can* method sounds like a
major
> design violation. That's why I am wondering what exactly needs to be plumbed
> where.

FWIW, I don't think either approach is that much work. I suspect they're about
the same amount of work, since they both require changing the same APIs.

Pavel, please see my previous comment about plumbing the mixed content result
into the requestWillBeSent event to see if you think there is a better way than
returning an enum or struct from FrameFetchContext::canRequest.

[pfeldman, 2015-08-17 22:17:35]

Can we add something as simple as say:

MixedContentChecker::contextTypeFromContextForInspector(frame, request,
requestContext)
{
    LocalFrame* mixedFrame = inWhichFrameIsContentMixed(frame,
request.frameType(), request.url());
    if (!mixedFrame) {
        if (request)
             return ResourceRequest::ContextTypeNotMixedContent;
    }
    if (frameType == WebURLRequest::FrameTypeNested &&
!SchemeRegistry::shouldTreatURLSchemeAsCORSEnabled(request.url().protocol()))
        return ResourceRequest::ContextTypeOptionallyBlockable;
    return contextTypeFromContext(requestContext, mixedFrame);
}

and call it from InspectorResourceAgent::willSendRequest ?

[estark, 2015-08-17 22:20:36]

On 2015/08/17 22:17:35, pfeldman wrote:
> Can we add something as simple as say:
> 
> MixedContentChecker::contextTypeFromContextForInspector(frame, request,
> requestContext)
> {
>     LocalFrame* mixedFrame = inWhichFrameIsContentMixed(frame,
> request.frameType(), request.url());
>     if (!mixedFrame) {
>         if (request)
>              return ResourceRequest::ContextTypeNotMixedContent;
>     }
>     if (frameType == WebURLRequest::FrameTypeNested &&
> !SchemeRegistry::shouldTreatURLSchemeAsCORSEnabled(request.url().protocol()))
>         return ResourceRequest::ContextTypeOptionallyBlockable;
>     return contextTypeFromContext(requestContext, mixedFrame);
> }
> 
> and call it from InspectorResourceAgent::willSendRequest ?

Does InspectorResourceAgent know from which frame the request was sent? It looks
like FrameFetchContext does something a little tricky to pick the frame to use
to check mixed content:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[pfeldman, 2015-08-17 22:25:17]

It should. Annotate frame of willSendRequest as [Keep] in
InspectorInstrumentation.idl and add it as a param in resource agent.

[estark, 2015-08-17 22:31:17]

On 2015/08/17 22:25:17, pfeldman wrote:
> It should. Annotate frame of willSendRequest as [Keep] in
> InspectorInstrumentation.idl and add it as a param in resource agent.

Sorry, I don't understand. The Frame that is passed to
MixedContentChecker::shouldBlockFetch() in FrameFetchContext::canRequest() is
not necessarily the frame that gets sent to the willSendRequest event. We'd have
to duplicate the logic at
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
that walks up the frame tree before calling
MixedContentChecker::shouldBlockFetch().

We can duplicate that in InspectorResourceAgent, but to me it feels a little
weird to duplicate so much logic and that we are likely to drift out of sync at
some point.

[pfeldman, 2015-08-17 22:59:25]

> We can duplicate that in InspectorResourceAgent, but to me it feels a little
> weird to duplicate so much logic and that we are likely to drift out of sync
at
> some point.

You mean going to the parent in case of loading the main resource? That seems
pretty straightforward and hard to diverge from. It is anyways way better than
mutating in the getter. You can also extract effectiveFrame into a method and
reuse it.

[estark, 2015-08-17 23:04:15]

On 2015/08/17 22:59:25, pfeldman wrote:
> > We can duplicate that in InspectorResourceAgent, but to me it feels a little
> > weird to duplicate so much logic and that we are likely to drift out of sync
> at
> > some point.
> 
> You mean going to the parent in case of loading the main resource? That seems
> pretty straightforward and hard to diverge from. It is anyways way better than
> mutating in the getter. You can also extract effectiveFrame into a method and
> reuse it.

I just mean that in general, to me, returning an enum seems much less likely to
result in bugs than doing a second mixed content check. But, you're the OWNER
and this is probably a matter of taste, so I'll go with
MixedContentChecker::contextTypeFromContextForInspector() unless Mike objects.

[pfeldman, 2015-08-17 23:07:04]

> I just mean that in general, to me, returning an enum seems much less likely
to
> result in bugs than doing a second mixed content check. But, you're the OWNER
> and this is probably a matter of taste, so I'll go with
> MixedContentChecker::contextTypeFromContextForInspector() unless Mike objects.

This clearly is not a matter of taste. The can* method is used for a different
purpose, it should not return enum and same can* method should not mutate
things. Since there is a way to get this information externally,  neither of
these arguable solutions is necessary.

[estark, 2015-08-17 23:51:03]

Patchset #4 (id:60001) has been deleted

[estark, 2015-08-18 00:02:11]

On 2015/08/17 23:07:04, pfeldman wrote:
> > I just mean that in general, to me, returning an enum seems much less likely
> to
> > result in bugs than doing a second mixed content check. But, you're the
OWNER
> > and this is probably a matter of taste, so I'll go with
> > MixedContentChecker::contextTypeFromContextForInspector() unless Mike
objects.
> 
> This clearly is not a matter of taste. The can* method is used for a different
> purpose, it should not return enum and same can* method should not mutate
> things. Since there is a way to get this information externally,  neither of
> these arguable solutions is necessary.

That seems reasonable, though, independent of this CL,
FrameFetchContext::canRequest() doesn't really live up to this standard: it
already has a number of side effects and mutates state on the browser-side (such
as sending CSP reports and changing the lock icon). Maybe that's a different
matter than mutating its arguments, though.

PTAL at patchset #4 which does the
MixedContentChecker::contextTypeForInspector() approach.

[estark, 2015-08-19 15:32:00]

friendly ping

[pfeldman, 2015-08-19 17:10:19]

The code looks good, the test could be improved though.

https://codereview.chromium.org/1299493003/diff/80001/LayoutTests/http/tests/...
File LayoutTests/http/tests/inspector-protocol/request-mixed-content-status.html
(right):

https://codereview.chromium.org/1299493003/diff/80001/LayoutTests/http/tests/...
LayoutTests/http/tests/inspector-protocol/request-mixed-content-status.html:53:
if (req.url.indexOf("check-mixed-content-status") === -1)
You should dump into the log more aggressively, like dump mixedContentType
instead of checking its value.

https://codereview.chromium.org/1299493003/diff/80001/LayoutTests/http/tests/...
LayoutTests/http/tests/inspector-protocol/request-mixed-content-status.html:65:
} else if (expectedMixedContentType == "blockable") {
So this is a small state machine that iterates over the scenarios? You should
run them in the Runtime.evaluate callbacks instead of checking the content type.

[estark, 2015-08-19 19:07:04]

https://codereview.chromium.org/1299493003/diff/80001/LayoutTests/http/tests/...
File LayoutTests/http/tests/inspector-protocol/request-mixed-content-status.html
(right):

https://codereview.chromium.org/1299493003/diff/80001/LayoutTests/http/tests/...
LayoutTests/http/tests/inspector-protocol/request-mixed-content-status.html:53:
if (req.url.indexOf("check-mixed-content-status") === -1)
On 2015/08/19 17:10:19, pfeldman wrote:
> You should dump into the log more aggressively, like dump mixedContentType
> instead of checking its value.

Done.

https://codereview.chromium.org/1299493003/diff/80001/LayoutTests/http/tests/...
LayoutTests/http/tests/inspector-protocol/request-mixed-content-status.html:65:
} else if (expectedMixedContentType == "blockable") {
On 2015/08/19 17:10:19, pfeldman wrote:
> So this is a small state machine that iterates over the scenarios? You should
> run them in the Runtime.evaluate callbacks instead of checking the content
type.

Hmm, I wasn't quite able to get this to work, because the Runtime.evaluate
callback runs before the corresponding onRequestWillBeSent() events fire. I
think we need to track *something* in onRequestWillBeSent() to be able to detect
when the test is over and call InspectorTest.notifyComplete(). There was also an
additional wrinkle that I couldn't figure out how to access
get_host_info().HTTPS_ORIGIN inside a Runtime.evaluate callback.

What I did instead was fire off the three scenarios (noMixedContent(),
blockableMixedContent(), optionallyBlockableMixedContent()) one after the other,
and count the number of requests in onRequestWillBeSent() to know when the test
is done. I'm not sure if we can assume that the requests will always come in the
same order, though. If not, I think we do have to use onRequestWillBeSent() to
trigger the next scenario, since otherwise we won't be able to guarantee that
the requests get sent in the expected order. What do you think?

[pfeldman, 2015-08-19 19:09:35]

Ok, we can follow up on the test when you are on site, mind adding // FIXME
there?

[pfeldman, 2015-08-19 19:09:41]

lgtm

[estark, 2015-08-19 20:13:29]

On 2015/08/19 19:09:35, pfeldman wrote:
> Ok, we can follow up on the test when you are on site, mind adding // FIXME
> there?

Thanks, done.

Mike, do you want to take another look before I land this?

[Mike West, 2015-08-19 22:55:55]

On 2015/08/19 at 20:13:29, estark wrote:
> On 2015/08/19 19:09:35, pfeldman wrote:
> > Ok, we can follow up on the test when you are on site, mind adding // FIXME
> > there?
> 
> Thanks, done.
> 
> Mike, do you want to take another look before I land this?

I was about to rubberstamp it, but then I accidentally looked at
MixedContentChecker, and I don't understand. I'll grab you in a minute since
you're sitting across the room from me. :)

[Mike West, 2015-08-19 22:59:17]

On 2015/08/19 at 22:55:55, Mike West (traveling) wrote:
> On 2015/08/19 at 20:13:29, estark wrote:
> > On 2015/08/19 19:09:35, pfeldman wrote:
> > > Ok, we can follow up on the test when you are on site, mind adding //
FIXME
> > > there?
> > 
> > Thanks, done.
> > 
> > Mike, do you want to take another look before I land this?
> 
> I was about to rubberstamp it, but then I accidentally looked at
MixedContentChecker, and I don't understand. I'll grab you in a minute since
you're sitting across the room from me. :)

Also: MixedContentChecker::contextTypeForInspector seems ripe for unit testing.

[estark, 2015-08-20 05:05:23]

Discussed with Mike in person to clear up the confusion, and added a
MixedContentChecker::contextTypeForInspector() unit test now that I know about
DummyPageLoader (thanks Mike!). PTAL again?

[pfeldman, 2015-08-20 18:30:59]

devtools still lgtm, deferring the test review to Mike.

[Mike West, 2015-08-20 20:07:45]

LGTM, thanks!

[estark, 2015-08-20 20:12:14]

The CQ bit was checked by estark@chromium.org

[commit-bot: I haz the power, 2015-08-20 20:12:29]

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

[estark, 2015-08-20 20:30:56]

The CQ bit was unchecked by estark@chromium.org

[estark, 2015-08-20 20:31:07]

The CQ bit was checked by estark@chromium.org

[commit-bot: I haz the power, 2015-08-20 20:31:34]

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

[commit-bot: I haz the power, 2015-08-20 21:20:46]

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

[commit-bot: I haz the power, 2015-08-20 21:20:47]

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, 2015-08-20 21:31:12]

The CQ bit was checked by estark@chromium.org

[commit-bot: I haz the power, 2015-08-20 21:31:46]

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

[commit-bot: I haz the power, 2015-08-20 22:43:27]

Committed patchset #9 (id:180001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200936