Supporting changes in preparation of browser side mixed content checking.

Supporting changes in preparation of browser side mixed content checking.

This CL adds some support ahead of time for the upcoming one that will partially
move mixed content checks to the browser (https://crrev.com/1905033002). These
changes are:
- Split mixed content Blink public code into two different files to separate
  constants (WebMixedContentContextType) from functions (WebMixedContent).
- The renderer now sends back to the browser information about the
  WebMixedContentContextType for navigation requests so that the browser won't
  need to execute Blink code (what is not acceptable). There's quite some
  piping involved in making this happen and some tests needed updates to avoid
  DCHECKs to be hit. For browser side requests it is set to match the
  RequestContextType used (blink::WebMixedContentContextType::Blockable to
  content::REQUEST_CONTEXT_TYPE_LOCATION).
- Moved mixed content code that was previously made public back into
  MixedContentChecker where it used to be as it doesn't need to be public
  anymore.

BUG=576270
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2625633002
Cr-Commit-Position: refs/heads/master@{#442963}
Committed: https://chromium.googlesource.com/chromium/src/+/709f33fc760a622f45e96214c9cd90b3b2021485

[carlosk, 2017-01-10 06:32:35]

Description was changed from

==========
Supporting changes in preparation of browser side mixed content checking.

This CL adds some support ahead of time for the upcoming one that will partially
move mixed content checks to the browser (https://crrev.com/1905033002). These
changes are:
- Split mixed content Blink public code into two different files to separate
  constants (WebMixedContentContextType) from functions (WebMixedContent).
- The renderer now sends back to the browser information about the
  WebMixedContentContextType for navigation requests so that the browser won't
  need to execute Blink code (what is not acceptable). There's quite some
  piping involved in making this happen and some tests needed updates to avoid
  DCHECKs to be hit. For browser side requests it is set to match the
  RequestContextType used (blink::WebMixedContentContextType::Blockable to
  content::REQUEST_CONTEXT_TYPE_LOCATION).
- Moved mixed content code that was previously made public back into
  MixedContentChecker where it used to be as it doesn't need to be public
  anymore.

BUG=576270
==========

to

==========
Supporting changes in preparation of browser side mixed content checking.

This CL adds some support ahead of time for the upcoming one that will partially
move mixed content checks to the browser (https://crrev.com/1905033002). These
changes are:
- Split mixed content Blink public code into two different files to separate
  constants (WebMixedContentContextType) from functions (WebMixedContent).
- The renderer now sends back to the browser information about the
  WebMixedContentContextType for navigation requests so that the browser won't
  need to execute Blink code (what is not acceptable). There's quite some
  piping involved in making this happen and some tests needed updates to avoid
  DCHECKs to be hit. For browser side requests it is set to match the
  RequestContextType used (blink::WebMixedContentContextType::Blockable to
  content::REQUEST_CONTEXT_TYPE_LOCATION).
- Moved mixed content code that was previously made public back into
  MixedContentChecker where it used to be as it doesn't need to be public
  anymore.

BUG=576270
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[carlosk, 2017-01-10 06:32:48]

The CQ bit was checked by carlosk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 06:33:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[carlosk, 2017-01-10 06:37:28]

carlosk@chromium.org changed reviewers:
+ jam@chromium.org

[carlosk, 2017-01-10 06:37:29]

jam@: PTAL.

https://codereview.chromium.org/2625633002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebMixedContentContextType.h (right):

https://codereview.chromium.org/2625633002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebMixedContentContextType.h:37: enum class
WebMixedContentContextType {
Anyone with an opinion on renaming WebMixedContentContextType to
WebMixedContentType?

I have in mind that there was a reason for the "context" snippet here but I
can't remember what it was.

Also: I might rename this file to WebMixedContentConstants to make it more
generic so that it could contain other related constants.

[carlosk, 2017-01-10 07:03:34]

The CQ bit was checked by carlosk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 07:03:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2017-01-10 07:08:04]

lgtm

I'm not an owner in webkit so please pick another reviewer for that

https://codereview.chromium.org/2625633002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/2625633002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.h:33: namespace blink {
nit: curious why you're forward declaring instead of including the header?

https://codereview.chromium.org/2625633002/diff/1/content/browser/loader/navi...
File content/browser/loader/navigation_resource_throttle.h (right):

https://codereview.chromium.org/2625633002/diff/1/content/browser/loader/navi...
content/browser/loader/navigation_resource_throttle.h:16: enum class
WebMixedContentContextType;
ditto

https://codereview.chromium.org/2625633002/diff/1/content/common/navigation_p...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/2625633002/diff/1/content/common/navigation_p...
content/common/navigation_params.h:29: enum class WebMixedContentContextType;
ditto

[jam, 2017-01-10 07:09:18]

On 2017/01/10 07:08:04, jam wrote:
> lgtm
> 
> I'm not an owner in webkit so please pick another reviewer for that
> 
>
https://codereview.chromium.org/2625633002/diff/1/content/browser/frame_host/...
> File content/browser/frame_host/navigation_handle_impl.h (right):
> 
>
https://codereview.chromium.org/2625633002/diff/1/content/browser/frame_host/...
> content/browser/frame_host/navigation_handle_impl.h:33: namespace blink {
> nit: curious why you're forward declaring instead of including the header?
> 
>
https://codereview.chromium.org/2625633002/diff/1/content/browser/loader/navi...
> File content/browser/loader/navigation_resource_throttle.h (right):
> 
>
https://codereview.chromium.org/2625633002/diff/1/content/browser/loader/navi...
> content/browser/loader/navigation_resource_throttle.h:16: enum class
> WebMixedContentContextType;
> ditto
> 
>
https://codereview.chromium.org/2625633002/diff/1/content/common/navigation_p...
> File content/common/navigation_params.h (right):
> 
>
https://codereview.chromium.org/2625633002/diff/1/content/common/navigation_p...
> content/common/navigation_params.h:29: enum class WebMixedContentContextType;
> ditto

ah ignore all my comments since your second patchset already did that :)

[carlosk, 2017-01-10 07:16:14]

On 2017/01/10 07:09:18, jam wrote:
> On 2017/01/10 07:08:04, jam wrote:
> > lgtm
> > 
> > I'm not an owner in webkit so please pick another reviewer for that

You might not be an owner but you sure are an OWNER. :)
I'll get someone else now.

>
https://codereview.chromium.org/2625633002/diff/1/content/browser/frame_host/...
> > File content/browser/frame_host/navigation_handle_impl.h (right):
> > 
> >
>
https://codereview.chromium.org/2625633002/diff/1/content/browser/frame_host/...
> > content/browser/frame_host/navigation_handle_impl.h:33: namespace blink {
> > nit: curious why you're forward declaring instead of including the header?
> > 
> >
>
https://codereview.chromium.org/2625633002/diff/1/content/browser/loader/navi...
> > File content/browser/loader/navigation_resource_throttle.h (right):
> > 
> >
>
https://codereview.chromium.org/2625633002/diff/1/content/browser/loader/navi...
> > content/browser/loader/navigation_resource_throttle.h:16: enum class
> > WebMixedContentContextType;
> > ditto
> > 
> >
>
https://codereview.chromium.org/2625633002/diff/1/content/common/navigation_p...
> > File content/common/navigation_params.h (right):
> > 
> >
>
https://codereview.chromium.org/2625633002/diff/1/content/common/navigation_p...
> > content/common/navigation_params.h:29: enum class
WebMixedContentContextType;
> > ditto
> 
> ah ignore all my comments since your second patchset already did that :)

Ignored.

[carlosk, 2017-01-10 07:21:12]

carlosk@chromium.org changed reviewers:
+ jochen@chromium.org, nasko@chromium.org

[carlosk, 2017-01-10 07:21:13]

jochen@: PTAL at third_party/WebKit/*. Please also mind my comment on patch set
#1.

nasko@: PTAL at the two IPC files under content/common.

[commit-bot: I haz the power, 2017-01-10 08:42:58]

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

[commit-bot: I haz the power, 2017-01-10 08:42:59]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2017-01-10 09:47:03]

jochen@chromium.org changed reviewers:
+ mkwst@chromium.org

[jochen (gone - plz use gerrit), 2017-01-10 09:47:03]

deferring to Mike for WebKit

[jochen (gone - plz use gerrit), 2017-01-10 15:32:12]

lgtm

[nasko, 2017-01-10 17:11:36]

https://codereview.chromium.org/2625633002/diff/20001/content/common/DEPS
File content/common/DEPS (right):

https://codereview.chromium.org/2625633002/diff/20001/content/common/DEPS#new...
content/common/DEPS:41:
"+third_party/WebKit/public/platform/modules/permissions/WebPermissionType.h",
Why did the ordering of these change?

https://codereview.chromium.org/2625633002/diff/20001/content/common/resource...
File content/common/resource_messages.h (right):

https://codereview.chromium.org/2625633002/diff/20001/content/common/resource...
content/common/resource_messages.h:140:
blink::WebMixedContentContextType::ShouldBeBlockable)
Why not use a "last" type value? If someone adds a new value to the enum, this
will likely break, if not updated. It is a lot less error-prone to use the
"last" pattern.

[carlosk, 2017-01-10 21:46:18]

Thanks.

https://codereview.chromium.org/2625633002/diff/20001/content/common/DEPS
File content/common/DEPS (right):

https://codereview.chromium.org/2625633002/diff/20001/content/common/DEPS#new...
content/common/DEPS:41:
"+third_party/WebKit/public/platform/modules/permissions/WebPermissionType.h",
On 2017/01/10 17:11:36, nasko wrote:
> Why did the ordering of these change?

When I add new entries I generally select a lot/all lines of the list I'm
changing and sort it using my editor according to the applicable rules
(case-sensitive in this case). So I am considering this an ordering "fix".

https://codereview.chromium.org/2625633002/diff/20001/content/common/resource...
File content/common/resource_messages.h (right):

https://codereview.chromium.org/2625633002/diff/20001/content/common/resource...
content/common/resource_messages.h:140:
blink::WebMixedContentContextType::ShouldBeBlockable)
On 2017/01/10 17:11:36, nasko wrote:
> Why not use a "last" type value? If someone adds a new value to the enum, this
> will likely break, if not updated. It is a lot less error-prone to use the
> "last" pattern.

Done.

[nasko, 2017-01-10 23:28:43]

IPC LGTM

[carlosk, 2017-01-10 23:33:22]

The CQ bit was checked by carlosk@chromium.org

[carlosk, 2017-01-10 23:33:23]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2625633002/#ps40001
(title: "Address code review comments.")

[commit-bot: I haz the power, 2017-01-10 23:34:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 02:59:40]

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

[commit-bot: I haz the power, 2017-01-11 02:59:42]

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

[Mike West, 2017-01-11 12:58:15]

//third_party/WebKit LGTM. :)

[carlosk, 2017-01-11 18:26:24]

The CQ bit was checked by carlosk@chromium.org

[commit-bot: I haz the power, 2017-01-11 18:27:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 18:44:19]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484159184073880,
"parent_rev": "b9b03f98739ab86f5d2f4f608a9770e6049542ae", "commit_rev":
"709f33fc760a622f45e96214c9cd90b3b2021485"}

[commit-bot: I haz the power, 2017-01-11 18:44:53]

Description was changed from

==========
Supporting changes in preparation of browser side mixed content checking.

This CL adds some support ahead of time for the upcoming one that will partially
move mixed content checks to the browser (https://crrev.com/1905033002). These
changes are:
- Split mixed content Blink public code into two different files to separate
  constants (WebMixedContentContextType) from functions (WebMixedContent).
- The renderer now sends back to the browser information about the
  WebMixedContentContextType for navigation requests so that the browser won't
  need to execute Blink code (what is not acceptable). There's quite some
  piping involved in making this happen and some tests needed updates to avoid
  DCHECKs to be hit. For browser side requests it is set to match the
  RequestContextType used (blink::WebMixedContentContextType::Blockable to
  content::REQUEST_CONTEXT_TYPE_LOCATION).
- Moved mixed content code that was previously made public back into
  MixedContentChecker where it used to be as it doesn't need to be public
  anymore.

BUG=576270
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Supporting changes in preparation of browser side mixed content checking.

This CL adds some support ahead of time for the upcoming one that will partially
move mixed content checks to the browser (https://crrev.com/1905033002). These
changes are:
- Split mixed content Blink public code into two different files to separate
  constants (WebMixedContentContextType) from functions (WebMixedContent).
- The renderer now sends back to the browser information about the
  WebMixedContentContextType for navigation requests so that the browser won't
  need to execute Blink code (what is not acceptable). There's quite some
  piping involved in making this happen and some tests needed updates to avoid
  DCHECKs to be hit. For browser side requests it is set to match the
  RequestContextType used (blink::WebMixedContentContextType::Blockable to
  content::REQUEST_CONTEXT_TYPE_LOCATION).
- Moved mixed content code that was previously made public back into
  MixedContentChecker where it used to be as it doesn't need to be public
  anymore.

BUG=576270
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2625633002
Cr-Commit-Position: refs/heads/master@{#442963}
Committed:
https://chromium.googlesource.com/chromium/src/+/709f33fc760a622f45e96214c9cd...
==========

[commit-bot: I haz the power, 2017-01-11 18:44:55]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/709f33fc760a622f45e96214c9cd...