Implement CanRequest in BaseFetchContext

Implement CanRequest in BaseFetchContext

BUG=695279, 538751
Review-Url: https://codereview.chromium.org/2823213002
Cr-Commit-Position: refs/heads/master@{#466610}
Committed: https://chromium.googlesource.com/chromium/src/+/c9dd99240fda60e9daca1f9b87c20a92b10de38a

[kinuko, 2017-04-18 12:28:47]

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

[commit-bot: I haz the power, 2017-04-18 12:29:03]

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-04-18 12:36:37]

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

[commit-bot: I haz the power, 2017-04-18 12:36:38]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[kinuko, 2017-04-18 16:05:22]

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

[commit-bot: I haz the power, 2017-04-18 16:05:43]

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-04-18 16:57:34]

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

[commit-bot: I haz the power, 2017-04-18 16:57:34]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[kinuko, 2017-04-19 06:56:05]

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

[commit-bot: I haz the power, 2017-04-19 06:56:22]

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-04-19 08:24:16]

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

[commit-bot: I haz the power, 2017-04-19 08:24:18]

Dry run: This issue passed the CQ dry run.

[kinuko, 2017-04-19 08:57:12]

Description was changed from

==========
BaseFetchContext::CanRequestInternal

BUG=
==========

to

==========
Implement CanRequest in BaseFetchContext

BUG=695279
==========

[kinuko, 2017-04-19 08:57:13]

kinuko@chromium.org changed reviewers:
+ horo@chromium.org, japhet@chromium.org

[kinuko, 2017-04-19 08:58:26]

Description was changed from

==========
Implement CanRequest in BaseFetchContext

BUG=695279
==========

to

==========
Implement CanRequest in BaseFetchContext

BUG=695279, 538751
==========

[kinuko, 2017-04-19 09:00:13]

Might look a bit silly, but let me hear what you think.

(Context: I want to implement one more FetchContext to make the worker's shadow
page go away for the cases when/where horo's code path's not taken and want to
remove dup'ed code as much as possible)

[horo, 2017-04-19 11:38:39]

https://codereview.chromium.org/2823213002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/BaseFetchContext.h (right):

https://codereview.chromium.org/2823213002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/BaseFetchContext.h:64: virtual void
CountUsage(UseCounter::Feature) const = 0;
Why don't you directly call CountUsage(execution_context_, feature)?

https://codereview.chromium.org/2823213002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/BaseFetchContext.h:65: virtual void
CountDeprecation(UseCounter::Feature) const = 0;
same as CountUsage.

[kinuko, 2017-04-20 02:33:22]

https://codereview.chromium.org/2823213002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/BaseFetchContext.h (right):

https://codereview.chromium.org/2823213002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/BaseFetchContext.h:64: virtual void
CountUsage(UseCounter::Feature) const = 0;
On 2017/04/19 11:38:39, horo wrote:
> Why don't you directly call CountUsage(execution_context_, feature)?

This one's a bit tricky, GetFrame()->GetDocument() and execution_context_ is not
always same (one is not set while the other is set in some cases).  We could
code like that in default impl but may still need override in FrameFetchContext.

[horo, 2017-04-20 06:00:48]

lgtm with nits

https://codereview.chromium.org/2823213002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/BaseFetchContext.cpp (right):

https://codereview.chromium.org/2823213002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/BaseFetchContext.cpp:203: if
(!GetContentSettingsClient()->AllowScriptFromSource(
nit: We should check GetContentSettingsClient()

https://codereview.chromium.org/2823213002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/BaseFetchContext.h (right):

https://codereview.chromium.org/2823213002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/BaseFetchContext.h:64: virtual void
CountUsage(UseCounter::Feature) const = 0;
On 2017/04/20 02:33:22, kinuko wrote:
> On 2017/04/19 11:38:39, horo wrote:
> > Why don't you directly call CountUsage(execution_context_, feature)?
> 
> This one's a bit tricky, GetFrame()->GetDocument() and execution_context_ is
not
> always same (one is not set while the other is set in some cases).  We could
> code like that in default impl but may still need override in
FrameFetchContext.

Ah, I got it.

https://codereview.chromium.org/2823213002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/BaseFetchContextTest.cpp (right):

https://codereview.chromium.org/2823213002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/BaseFetchContextTest.cpp:44: 
nit: destructor.

[kinuko, 2017-04-21 09:42:03]

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

[commit-bot: I haz the power, 2017-04-21 09:42:19]

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

[kinuko, 2017-04-21 09:42:56]

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

[kinuko, 2017-04-21 09:45:26]

kinuko@chromium.org changed reviewers:
- mkwst@chromium.org

[kinuko, 2017-04-21 09:45:27]

Thanks, updated.

Rebased and also moved some tests from FrameFetchContextTest to
BaseFetchContextTest.

Nate: What do you think about this change?

Also cc-ing mkwst@ as he probably knows best about the code this CL's moving
around

https://codereview.chromium.org/2823213002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/BaseFetchContext.cpp (right):

https://codereview.chromium.org/2823213002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/BaseFetchContext.cpp:203: if
(!GetContentSettingsClient()->AllowScriptFromSource(
On 2017/04/20 06:00:48, horo wrote:
> nit: We should check GetContentSettingsClient()

Done.

https://codereview.chromium.org/2823213002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/BaseFetchContextTest.cpp (right):

https://codereview.chromium.org/2823213002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/BaseFetchContextTest.cpp:44: 
On 2017/04/20 06:00:48, horo wrote:
> nit: destructor.

Done.

[commit-bot: I haz the power, 2017-04-21 11:44:41]

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

[commit-bot: I haz the power, 2017-04-21 11:44:42]

Dry run: This issue passed the CQ dry run.

[Nate Chapin, 2017-04-21 18:33:07]

LGTM with a couple nits.

https://codereview.chromium.org/2823213002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/BaseFetchContext.cpp (right):

https://codereview.chromium.org/2823213002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/BaseFetchContext.cpp:190:
ReportLocalLoadFailed(url);
Maybe add a TODO to make ReportLocalLoadFiled live on ExecutionContext? There's
nothing frame-specific about it.

https://codereview.chromium.org/2823213002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/BaseFetchContext.h (right):

https://codereview.chromium.org/2823213002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/BaseFetchContext.h:81: void
PrintAccessDeniedMessage(const KURL&) const;
Nit: I think this can be private now.

https://codereview.chromium.org/2823213002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/BaseFetchContextTest.cpp (right):

https://codereview.chromium.org/2823213002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/BaseFetchContextTest.cpp:77:
execution_context_ = Document::Create();
Why is this change necessary?

https://codereview.chromium.org/2823213002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/SubresourceFilter.h (right):

https://codereview.chromium.org/2823213002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/SubresourceFilter.h:19: class
DocumentLoader;
Drop this if DocumentLoader.h is included?

[kinuko, 2017-04-24 06:05:27]

Thanks! Updated.

https://codereview.chromium.org/2823213002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/BaseFetchContext.cpp (right):

https://codereview.chromium.org/2823213002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/BaseFetchContext.cpp:190:
ReportLocalLoadFailed(url);
On 2017/04/21 18:33:07, Nate Chapin wrote:
> Maybe add a TODO to make ReportLocalLoadFiled live on ExecutionContext?
There's
> nothing frame-specific about it.

Yeah looks like. Done.

https://codereview.chromium.org/2823213002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/BaseFetchContextTest.cpp (right):

https://codereview.chromium.org/2823213002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/BaseFetchContextTest.cpp:77:
execution_context_ = Document::Create();
On 2017/04/21 18:33:07, Nate Chapin wrote:
> Why is this change necessary?

NullExecutionContext doesn't have ContentSecurityPolicy. I've reverted this line
but instead added some support code to test CSP with NullExecutionContext.

https://codereview.chromium.org/2823213002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/SubresourceFilter.h (right):

https://codereview.chromium.org/2823213002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/SubresourceFilter.h:19: class
DocumentLoader;
On 2017/04/21 18:33:07, Nate Chapin wrote:
> Drop this if DocumentLoader.h is included?

Done.

[kinuko, 2017-04-24 06:05:35]

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

[kinuko, 2017-04-24 06:05:46]

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

[commit-bot: I haz the power, 2017-04-24 06:05:50]

Dry run: CQ is trying da patch.

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

[kinuko, 2017-04-24 06:08:26]

On 2017/04/24 06:05:27, kinuko wrote:
>
https://codereview.chromium.org/2823213002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/loader/BaseFetchContextTest.cpp:77:
> execution_context_ = Document::Create();
> On 2017/04/21 18:33:07, Nate Chapin wrote:
> > Why is this change necessary?
> 
> NullExecutionContext doesn't have ContentSecurityPolicy. I've reverted this
line
> but instead added some support code to test CSP with NullExecutionContext.

Looks like the current CSP code doesn't expect non-Document execution context.

Mike: I removed some "GetDocument() && GetDocument()->GetFrame()" checks in the
ContentSecurityPolicy.cpp code to make very basic things work for non-document
cases.  I believe these changes are okay (given that we have TODO's like
"TODO(mkwst): Support POSTing violation reports from a Worker.") but let me know
otherwise, I'm happy to make follow-up changes (or can even revert this one). 
Thanks!

[kinuko, 2017-04-24 06:08:41]

On 2017/04/24 06:08:26, kinuko wrote:
> On 2017/04/24 06:05:27, kinuko wrote:
> >
>
https://codereview.chromium.org/2823213002/diff/60001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/loader/BaseFetchContextTest.cpp:77:
> > execution_context_ = Document::Create();
> > On 2017/04/21 18:33:07, Nate Chapin wrote:
> > > Why is this change necessary?
> > 
> > NullExecutionContext doesn't have ContentSecurityPolicy. I've reverted this
> line
> > but instead added some support code to test CSP with NullExecutionContext.
> 
> Looks like the current CSP code doesn't expect non-Document execution context.
> 
> Mike: I removed some "GetDocument() && GetDocument()->GetFrame()" checks in
the

s/removed/moved/

> ContentSecurityPolicy.cpp code to make very basic things work for non-document
> cases.  I believe these changes are okay (given that we have TODO's like
> "TODO(mkwst): Support POSTing violation reports from a Worker.") but let me
know
> otherwise, I'm happy to make follow-up changes (or can even revert this one). 
> Thanks!

[kinuko, 2017-04-24 06:53:31]

The CQ bit was unchecked by kinuko@chromium.org

[kinuko, 2017-04-24 06:53:40]

The CQ bit was checked by kinuko@chromium.org

[kinuko, 2017-04-24 06:53:40]

The patchset sent to the CQ was uploaded after l-g-t-m from horo@chromium.org,
japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/2823213002/#ps80001
(title: "make it work with non-document")

[commit-bot: I haz the power, 2017-04-24 06:53:54]

CQ is trying da patch.

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

[Mike West, 2017-04-24 07:19:41]

CSP bits LGTM, thanks!

[commit-bot: I haz the power, 2017-04-24 09:30:39]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493016820511430,
"parent_rev": "9a51ef8f83c3ebd1e7c04ec66e35cc191e9870fe", "commit_rev":
"c9dd99240fda60e9daca1f9b87c20a92b10de38a"}

[commit-bot: I haz the power, 2017-04-24 09:30:53]

Description was changed from

==========
Implement CanRequest in BaseFetchContext

BUG=695279, 538751
==========

to

==========
Implement CanRequest in BaseFetchContext

BUG=695279, 538751

Review-Url: https://codereview.chromium.org/2823213002
Cr-Commit-Position: refs/heads/master@{#466610}
Committed:
https://chromium.googlesource.com/chromium/src/+/c9dd99240fda60e9daca1f9b87c2...
==========

[commit-bot: I haz the power, 2017-04-24 09:30:55]

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