Partially implement WorkerFetchContext and WorkerThreadableLoadingContext and add virtual tests

Partially implement WorkerFetchContext and WorkerThreadableLoadingContext and add virtual tests

BUG=443374
Review-Url: https://codereview.chromium.org/2880733002
Cr-Commit-Position: refs/heads/master@{#471649}
Committed: https://chromium.googlesource.com/chromium/src/+/496a826dff5e0b9c4a61a3a355f273191ec90c4e

[horo, 2017-05-12 08:17:06]

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

[horo, 2017-05-12 08:17:07]

kinuko@
Could you please review this?

[kinuko, 2017-05-12 08:41:45]

looking good!

https://codereview.chromium.org/2880733002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/2880733002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:396: //
TODO(horo): Currently we don't support the CORS preflie cache on worker
preflie -> preflight

https://codereview.chromium.org/2880733002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:776: //
TODO(horo): Currently we don't support the CORS preflie cache on worker
ditto

https://codereview.chromium.org/2880733002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/ThreadableLoadingContext.cpp (right):

https://codereview.chromium.org/2880733002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/ThreadableLoadingContext.cpp:98: String
errorMessage;
error_message

https://codereview.chromium.org/2880733002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/WorkerFetchContext.h (right):

https://codereview.chromium.org/2880733002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/WorkerFetchContext.h:66: explicit
WorkerFetchContext(WorkerGlobalScope&,
no need of explicit

https://codereview.chromium.org/2880733002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/loader/fetch/Resource.cpp (right):

https://codereview.chromium.org/2880733002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/loader/fetch/Resource.cpp:277:
->LoadingTaskRunner(),
It feels slightly weird why we don't use loading task runner for main thread
cases..

https://codereview.chromium.org/2880733002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/loader/fetch/ResourceRequest.h (right):

https://codereview.chromium.org/2880733002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/loader/fetch/ResourceRequest.h:322:
is_mojo_ipc_forced_ = is_mojo_ipc_forced;
nit: maybe call this method OverrideIPCType() and pass kMojo?

[horo, 2017-05-12 09:05:11]

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

[commit-bot: I haz the power, 2017-05-12 09:05:21]

Dry run: CQ is trying da patch.

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

[horo, 2017-05-12 09:06:02]

https://codereview.chromium.org/2880733002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/2880733002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:396: //
TODO(horo): Currently we don't support the CORS preflie cache on worker
On 2017/05/12 08:41:44, kinuko wrote:
> preflie -> preflight

Done.

https://codereview.chromium.org/2880733002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:776: //
TODO(horo): Currently we don't support the CORS preflie cache on worker
On 2017/05/12 08:41:45, kinuko wrote:
> ditto

Done.

https://codereview.chromium.org/2880733002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/ThreadableLoadingContext.cpp (right):

https://codereview.chromium.org/2880733002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/ThreadableLoadingContext.cpp:98: String
errorMessage;
On 2017/05/12 08:41:45, kinuko wrote:
> error_message

Done.

https://codereview.chromium.org/2880733002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/WorkerFetchContext.h (right):

https://codereview.chromium.org/2880733002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/WorkerFetchContext.h:66: explicit
WorkerFetchContext(WorkerGlobalScope&,
On 2017/05/12 08:41:45, kinuko wrote:
> no need of explicit

Done.

https://codereview.chromium.org/2880733002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/loader/fetch/Resource.cpp (right):

https://codereview.chromium.org/2880733002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/loader/fetch/Resource.cpp:277:
->LoadingTaskRunner(),
On 2017/05/12 08:41:45, kinuko wrote:
> It feels slightly weird why we don't use loading task runner for main thread
> cases..

I think so too...
But I don't want to change the existing behavior in this CL.

https://codereview.chromium.org/2880733002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/loader/fetch/ResourceRequest.h (right):

https://codereview.chromium.org/2880733002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/loader/fetch/ResourceRequest.h:322:
is_mojo_ipc_forced_ = is_mojo_ipc_forced;
On 2017/05/12 08:41:45, kinuko wrote:
> nit: maybe call this method OverrideIPCType() and pass kMojo?

Done.

[commit-bot: I haz the power, 2017-05-12 10:35:15]

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

[commit-bot: I haz the power, 2017-05-12 10:35:15]

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

[horo, 2017-05-12 14:39:15]

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

[commit-bot: I haz the power, 2017-05-12 14:39:34]

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-05-12 15:45:19]

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

[commit-bot: I haz the power, 2017-05-12 15:45:20]

Dry run: This issue passed the CQ dry run.

[kinuko, 2017-05-13 05:54:35]

Description was changed from

==========
Partially implement WorkerFetchContext and WorkerThreadableLoadingContext and
add virtual tests

BUG=443374
==========

to

==========
Partially implement WorkerFetchContext and WorkerThreadableLoadingContext and
add virtual tests

BUG=443374
==========

[kinuko, 2017-05-13 05:54:35]

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

[kinuko, 2017-05-13 05:54:55]

https://codereview.chromium.org/2880733002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/loader/fetch/Resource.cpp (right):

https://codereview.chromium.org/2880733002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/loader/fetch/Resource.cpp:277:
->LoadingTaskRunner(),
On 2017/05/12 09:06:02, horo wrote:
> On 2017/05/12 08:41:45, kinuko wrote:
> > It feels slightly weird why we don't use loading task runner for main thread
> > cases..
> 
> I think so too...
> But I don't want to change the existing behavior in this CL.

Yep, ok to leave it as is in this CL.

+yhirano@: do you know why this is using default task runner?

https://codereview.chromium.org/2880733002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/WorkerFetchContext.h (right):

https://codereview.chromium.org/2880733002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/WorkerFetchContext.h:38:
ContentSettingsClient* GetContentSettingsClient() const;
Wait... these need 'override' (or final), right?

[horo, 2017-05-13 08:14:34]

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

[horo, 2017-05-13 08:14:41]

https://codereview.chromium.org/2880733002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/WorkerFetchContext.h (right):

https://codereview.chromium.org/2880733002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/WorkerFetchContext.h:38:
ContentSettingsClient* GetContentSettingsClient() const;
On 2017/05/13 05:54:55, kinuko wrote:
> Wait... these need 'override' (or final), right?

Done.

[commit-bot: I haz the power, 2017-05-13 08:14:41]

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-05-13 11:47:04]

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

[commit-bot: I haz the power, 2017-05-13 11:47:05]

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

[kinuko, 2017-05-13 13:26:43]

lgtm

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

https://codereview.chromium.org/2880733002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ThreadableLoader.cpp:55: if
(worker_fetch_contrext) {
When this could return nullptr?

https://codereview.chromium.org/2880733002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ThreadableLoader.cpp:56: return
DocumentThreadableLoader::Create(
Can you add a TODO to DocumentThreadableLoader to note that it now works without
document and should be probably renamed?

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

https://codereview.chromium.org/2880733002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ThreadableLoadingContext.cpp:136: // For
now this is the only default implementation.
Please remove this comment.

[horo, 2017-05-13 17:03:18]

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

[commit-bot: I haz the power, 2017-05-13 17:03:33]

Dry run: CQ is trying da patch.

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

[horo, 2017-05-13 17:05:45]

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

https://codereview.chromium.org/2880733002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ThreadableLoader.cpp:55: if
(worker_fetch_contrext) {
On 2017/05/13 13:26:42, kinuko wrote:
> When this could return nullptr?

Ah, yes.
This should be DCHECK().

https://codereview.chromium.org/2880733002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ThreadableLoader.cpp:56: return
DocumentThreadableLoader::Create(
On 2017/05/13 13:26:42, kinuko wrote:
> Can you add a TODO to DocumentThreadableLoader to note that it now works
without
> document and should be probably renamed?

Done.

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

https://codereview.chromium.org/2880733002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ThreadableLoadingContext.cpp:136: // For
now this is the only default implementation.
On 2017/05/13 13:26:42, kinuko wrote:
> Please remove this comment.

Done.

[commit-bot: I haz the power, 2017-05-13 18:53:01]

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

[commit-bot: I haz the power, 2017-05-13 18:53:03]

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

[horo, 2017-05-15 00:03:50]

The CQ bit was checked by horo@chromium.org

[horo, 2017-05-15 00:03:51]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/2880733002/#ps60001
(title: "incorporated kinuko's comment")

[commit-bot: I haz the power, 2017-05-15 00:04:09]

CQ is trying da patch.

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

[yhirano, 2017-05-15 01:43:26]

https://codereview.chromium.org/2880733002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/loader/fetch/Resource.cpp (right):

https://codereview.chromium.org/2880733002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/loader/fetch/Resource.cpp:277:
->LoadingTaskRunner(),
On 2017/05/13 05:54:55, kinuko wrote:
> On 2017/05/12 09:06:02, horo wrote:
> > On 2017/05/12 08:41:45, kinuko wrote:
> > > It feels slightly weird why we don't use loading task runner for main
thread
> > > cases..
> > 
> > I think so too...
> > But I don't want to change the existing behavior in this CL.
> 
> Yep, ok to leave it as is in this CL.
> 
> +yhirano@: do you know why this is using default task runner?

No.

I also think this should be the loading task runner gotten from FetchContext,
though it's not accessible here.

[commit-bot: I haz the power, 2017-05-15 01:50:14]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494806630527610,
"parent_rev": "83bcead92eb1c2aec8bcb5e50deb6fc2440240f4", "commit_rev":
"496a826dff5e0b9c4a61a3a355f273191ec90c4e"}

[commit-bot: I haz the power, 2017-05-15 01:50:26]

Description was changed from

==========
Partially implement WorkerFetchContext and WorkerThreadableLoadingContext and
add virtual tests

BUG=443374
==========

to

==========
Partially implement WorkerFetchContext and WorkerThreadableLoadingContext and
add virtual tests

BUG=443374

Review-Url: https://codereview.chromium.org/2880733002
Cr-Commit-Position: refs/heads/master@{#471649}
Committed:
https://chromium.googlesource.com/chromium/src/+/496a826dff5e0b9c4a61a3a355f2...
==========

[commit-bot: I haz the power, 2017-05-15 01:50:27]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/496a826dff5e0b9c4a61a3a355f2...

[kinuko, 2017-05-15 01:52:20]

On 2017/05/15 01:43:26, yhirano wrote:
>
https://codereview.chromium.org/2880733002/diff/1/third_party/WebKit/Source/p...
> File third_party/WebKit/Source/platform/loader/fetch/Resource.cpp (right):
> 
>
https://codereview.chromium.org/2880733002/diff/1/third_party/WebKit/Source/p...
> third_party/WebKit/Source/platform/loader/fetch/Resource.cpp:277:
> ->LoadingTaskRunner(),
> On 2017/05/13 05:54:55, kinuko wrote:
> > On 2017/05/12 09:06:02, horo wrote:
> > > On 2017/05/12 08:41:45, kinuko wrote:
> > > > It feels slightly weird why we don't use loading task runner for main
> thread
> > > > cases..
> > > 
> > > I think so too...
> > > But I don't want to change the existing behavior in this CL.
> > 
> > Yep, ok to leave it as is in this CL.
> > 
> > +yhirano@: do you know why this is using default task runner?
> 
> No.
> 
> I also think this should be the loading task runner gotten from FetchContext,
> though it's not accessible here.

We can still get a default loading task runner from the renderer scheduler? 
(And yep, we'll need to figure out how we manage these
not-really-frame-associated Resource/MemoryCache activities)

horo-san: can we add a TODO comment here before we land this?

[kinuko, 2017-05-15 01:52:50]

On 2017/05/15 01:52:20, kinuko wrote:
> On 2017/05/15 01:43:26, yhirano wrote:
> >
>
https://codereview.chromium.org/2880733002/diff/1/third_party/WebKit/Source/p...
> > File third_party/WebKit/Source/platform/loader/fetch/Resource.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2880733002/diff/1/third_party/WebKit/Source/p...
> > third_party/WebKit/Source/platform/loader/fetch/Resource.cpp:277:
> > ->LoadingTaskRunner(),
> > On 2017/05/13 05:54:55, kinuko wrote:
> > > On 2017/05/12 09:06:02, horo wrote:
> > > > On 2017/05/12 08:41:45, kinuko wrote:
> > > > > It feels slightly weird why we don't use loading task runner for main
> > thread
> > > > > cases..
> > > > 
> > > > I think so too...
> > > > But I don't want to change the existing behavior in this CL.
> > > 
> > > Yep, ok to leave it as is in this CL.
> > > 
> > > +yhirano@: do you know why this is using default task runner?
> > 
> > No.
> > 
> > I also think this should be the loading task runner gotten from
FetchContext,
> > though it's not accessible here.
> 
> We can still get a default loading task runner from the renderer scheduler? 
> (And yep, we'll need to figure out how we manage these
> not-really-frame-associated Resource/MemoryCache activities)
> 
> horo-san: can we add a TODO comment here before we land this?

oops, it's landed just now.

[horo, 2017-05-15 03:26:55]

On 2017/05/15 01:52:50, kinuko wrote:
> On 2017/05/15 01:52:20, kinuko wrote:
> > On 2017/05/15 01:43:26, yhirano wrote:
> > >
> >
>
https://codereview.chromium.org/2880733002/diff/1/third_party/WebKit/Source/p...
> > > File third_party/WebKit/Source/platform/loader/fetch/Resource.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2880733002/diff/1/third_party/WebKit/Source/p...
> > > third_party/WebKit/Source/platform/loader/fetch/Resource.cpp:277:
> > > ->LoadingTaskRunner(),
> > > On 2017/05/13 05:54:55, kinuko wrote:
> > > > On 2017/05/12 09:06:02, horo wrote:
> > > > > On 2017/05/12 08:41:45, kinuko wrote:
> > > > > > It feels slightly weird why we don't use loading task runner for
main
> > > thread
> > > > > > cases..
> > > > > 
> > > > > I think so too...
> > > > > But I don't want to change the existing behavior in this CL.
> > > > 
> > > > Yep, ok to leave it as is in this CL.
> > > > 
> > > > +yhirano@: do you know why this is using default task runner?
> > > 
> > > No.
> > > 
> > > I also think this should be the loading task runner gotten from
> FetchContext,
> > > though it's not accessible here.
> > 
> > We can still get a default loading task runner from the renderer scheduler? 
> > (And yep, we'll need to figure out how we manage these
> > not-really-frame-associated Resource/MemoryCache activities)
> > 
> > horo-san: can we add a TODO comment here before we land this?
> 
> oops, it's landed just now.

Created https://codereview.chromium.org/2878343002.