Add ELEMENT_TYPE_WEBSOCKET to subresource filter

Add ELEMENT_TYPE_WEBSOCKET to subresource filter

This also hooks up websocket filtering to various
WebDocumentSubresourceFilter mocks and fakes, and adds a simple
layout test.

BUG=609747
Review-Url: https://codereview.chromium.org/2724903004
Cr-Commit-Position: refs/heads/master@{#455339}
Committed: https://chromium.googlesource.com/chromium/src/+/8e17d5d11241dbe2c7c525af0de14aa691019eff

[Charlie Harrison, 2017-03-02 21:01:14]

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

[commit-bot: I haz the power, 2017-03-02 21:02:30]

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

[Charlie Harrison, 2017-03-02 21:05:27]

csharrison@chromium.org changed reviewers:
+ pkalinnikov@chromium.org

[Charlie Harrison, 2017-03-02 21:05:27]

Pavel, PTAL. I chose to just add another method to the WebDSF for a few reasons:
1. Websocket connections are pretty different from normal fetch requests, so it
makes sense for the API to separate them.
2. It feels wrong to duplicate the element type proto for such a small reason.

[commit-bot: I haz the power, 2017-03-02 21:05:48]

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

[commit-bot: I haz the power, 2017-03-02 21:05:49]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Charlie Harrison, 2017-03-02 21:12:35]

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

[commit-bot: I haz the power, 2017-03-02 21:12:59]

Dry run: 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-03-02 23:21:38]

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

[commit-bot: I haz the power, 2017-03-02 23:21:39]

Dry run: This issue passed the CQ dry run.

[pkalinnikov, 2017-03-03 10:27:21]

https://codereview.chromium.org/2724903004/diff/20001/components/subresource_...
File
components/subresource_filter/content/renderer/web_document_subresource_filter_impl.h
(right):

https://codereview.chromium.org/2724903004/diff/20001/components/subresource_...
components/subresource_filter/content/renderer/web_document_subresource_filter_impl.h:39:
blink::WebDocumentSubresourceFilter::LoadPolicy getLoadPolicy(
Just realized that there is not need in blink::WDSF:: prefix here and below,
because it's inherited. Can you please fix this?

https://codereview.chromium.org/2724903004/diff/20001/components/subresource_...
components/subresource_filter/content/renderer/web_document_subresource_filter_impl.h:47:
blink::WebDocumentSubresourceFilter::LoadPolicy getLoadPolicyForElementType(
optional: Could we call this just getLoadPolicyImpl?

https://codereview.chromium.org/2724903004/diff/20001/components/subresource_...
components/subresource_filter/content/renderer/web_document_subresource_filter_impl.h:49:
proto::ElementType element_type);
nit: Please add a blank line here.

https://codereview.chromium.org/2724903004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/websocket/subresource-filter-disallows.html
(right):

https://codereview.chromium.org/2724903004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/websocket/subresource-filter-disallows.html:7:
console.log("YO");
Is this line necessary?

https://codereview.chromium.org/2724903004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/WebDocumentSubresourceFilterTest.cpp
(right):

https://codereview.chromium.org/2724903004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebDocumentSubresourceFilterTest.cpp:40:
LoadPolicy getLoadPolicyForWebSocketConnect(const WebURL& resourceUrl) {
nit: Add 'override'.

https://codereview.chromium.org/2724903004/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebDocumentSubresourceFilter.h (right):

https://codereview.chromium.org/2724903004/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebDocumentSubresourceFilter.h:16: enum
LoadPolicy { Allow, Disallow, WouldDisallow };
Can you please add a blank line, while you are here?

https://codereview.chromium.org/2724903004/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebDocumentSubresourceFilter.h:21: const
WebURL& resourceUrl) = 0;
optional: Do you feel 'resource' is a good word for a WebSocket connection? How
about 'websocketUrl' or just 'url'?

[Charlie Harrison, 2017-03-03 13:41:00]

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

[commit-bot: I haz the power, 2017-03-03 13:41:11]

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

[Charlie Harrison, 2017-03-03 13:41:30]

Thanks!

https://codereview.chromium.org/2724903004/diff/20001/components/subresource_...
File
components/subresource_filter/content/renderer/web_document_subresource_filter_impl.h
(right):

https://codereview.chromium.org/2724903004/diff/20001/components/subresource_...
components/subresource_filter/content/renderer/web_document_subresource_filter_impl.h:39:
blink::WebDocumentSubresourceFilter::LoadPolicy getLoadPolicy(
On 2017/03/03 10:27:20, pkalinnikov wrote:
> Just realized that there is not need in blink::WDSF:: prefix here and below,
> because it's inherited. Can you please fix this?

Done.

https://codereview.chromium.org/2724903004/diff/20001/components/subresource_...
components/subresource_filter/content/renderer/web_document_subresource_filter_impl.h:47:
blink::WebDocumentSubresourceFilter::LoadPolicy getLoadPolicyForElementType(
On 2017/03/03 10:27:20, pkalinnikov wrote:
> optional: Could we call this just getLoadPolicyImpl?

Done.

https://codereview.chromium.org/2724903004/diff/20001/components/subresource_...
components/subresource_filter/content/renderer/web_document_subresource_filter_impl.h:49:
proto::ElementType element_type);
On 2017/03/03 10:27:20, pkalinnikov wrote:
> nit: Please add a blank line here.

Done.

https://codereview.chromium.org/2724903004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/websocket/subresource-filter-disallows.html
(right):

https://codereview.chromium.org/2724903004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/websocket/subresource-filter-disallows.html:7:
console.log("YO");
On 2017/03/03 10:27:20, pkalinnikov wrote:
> Is this line necessary?

How embarrassing! Removed. Usually presubmits catch this but I guess not in
layout tests!

https://codereview.chromium.org/2724903004/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebDocumentSubresourceFilter.h (right):

https://codereview.chromium.org/2724903004/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebDocumentSubresourceFilter.h:16: enum
LoadPolicy { Allow, Disallow, WouldDisallow };
On 2017/03/03 10:27:21, pkalinnikov wrote:
> Can you please add a blank line, while you are here?

Done.

https://codereview.chromium.org/2724903004/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebDocumentSubresourceFilter.h:21: const
WebURL& resourceUrl) = 0;
On 2017/03/03 10:27:20, pkalinnikov wrote:
> optional: Do you feel 'resource' is a good word for a WebSocket connection?
How
> about 'websocketUrl' or just 'url'?

I changed to "url" everywhere. (This means for Blink headers it is omitted, due
to blink style).

[pkalinnikov, 2017-03-03 13:48:52]

LGTM, thanks!

[commit-bot: I haz the power, 2017-03-03 15:31:41]

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

[commit-bot: I haz the power, 2017-03-03 15:31:42]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2017-03-03 15:51:57]

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

[Charlie Harrison, 2017-03-03 15:51:58]

mkwst: Would you review Blink and //content for OWNERS bits? It's a pretty
mechanical CL.

[Mike West, 2017-03-06 12:44:21]

LGTM.

[Charlie Harrison, 2017-03-07 22:36:39]

The CQ bit was checked by csharrison@chromium.org

[commit-bot: I haz the power, 2017-03-07 22:37:41]

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-03-07 22:42:02]

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

[commit-bot: I haz the power, 2017-03-07 22:42:03]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Charlie Harrison, 2017-03-07 23:04:51]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2017-03-07 23:04:52]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
pkalinnikov@chromium.org
Link to the patchset: https://codereview.chromium.org/2724903004/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2017-03-07 23:05:42]

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-03-08 01:57:35]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488927891995320,
"parent_rev": "eabc10e7801155a68e5b787b61a92c735ea8f876", "commit_rev":
"8e17d5d11241dbe2c7c525af0de14aa691019eff"}

[commit-bot: I haz the power, 2017-03-08 01:58:46]

Description was changed from

==========
Add ELEMENT_TYPE_WEBSOCKET to subresource filter

This also hooks up websocket filtering to various
WebDocumentSubresourceFilter mocks and fakes, and adds a simple
layout test.

BUG=609747
==========

to

==========
Add ELEMENT_TYPE_WEBSOCKET to subresource filter

This also hooks up websocket filtering to various
WebDocumentSubresourceFilter mocks and fakes, and adds a simple
layout test.

BUG=609747

Review-Url: https://codereview.chromium.org/2724903004
Cr-Commit-Position: refs/heads/master@{#455339}
Committed:
https://chromium.googlesource.com/chromium/src/+/8e17d5d11241dbe2c7c525af0de1...
==========

[commit-bot: I haz the power, 2017-03-08 01:58:48]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/8e17d5d11241dbe2c7c525af0de1...