Introduce ThreadableLoadingContext: make threaded loading code one step away from Document

Introduce ThreadableLoadingContext: make threaded loading code one step away from Document.

For now it's just a wrapper of Document.

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

[kinuko, 2017-02-24 08:22:09]

Description was changed from

==========
Make Document one step away from threaded loading code

* Introduce LoadingContext as an abstract concept for top-level loading context
* For now it's just a wrapper of Document

BUG=695279, 538751

patch from issue 2706293011 at patchset 1 (http://crrev.com/2706293011#ps1)
==========

to

==========
Make Document one step away from threaded loading code

* Introduce LoadingContext as an abstract concept for top-level loading context
* For now it's just a wrapper of Document

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

[kinuko, 2017-02-24 08:22:15]

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

[commit-bot: I haz the power, 2017-02-24 08:22:26]

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

[kinuko, 2017-02-24 08:23:23]

Description was changed from

==========
Make Document one step away from threaded loading code

* Introduce LoadingContext as an abstract concept for top-level loading context
* For now it's just a wrapper of Document

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

to

==========
Introduce LoadingContext as an abstraction for top-level loading context

... to make threaded loading code one step away from raw Document & Frame
classes.  For now it's just a wrapper of Document.

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

[kinuko, 2017-02-24 08:24:20]

Description was changed from

==========
Introduce LoadingContext as an abstraction for top-level loading context

... to make threaded loading code one step away from raw Document & Frame
classes.  For now it's just a wrapper of Document.

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

to

==========
Introduce LoadingContext: make threaded loading code one step away from
Document.

For now it's just a wrapper of Document.

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

[commit-bot: I haz the power, 2017-02-24 10:13:27]

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

[commit-bot: I haz the power, 2017-02-24 10:13:28]

Dry run: This issue passed the CQ dry run.

[kinuko, 2017-02-24 10:39:43]

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

[kinuko, 2017-02-24 10:40:27]

What do you think about this change?

[tyoshino (SeeGerritForStatus), 2017-02-24 10:50:58]

On 2017/02/24 10:40:27, kinuko wrote:
> What do you think about this change?

looks promising! as you were questioning, it might be better to align the naming
with the ThreadableLoader though ThreadableLoader itself has been waiting for
better name. I can live with LoadingContext.

[kinuko, 2017-02-27 04:25:20]

On 2017/02/24 10:50:58, tyoshino wrote:
> On 2017/02/24 10:40:27, kinuko wrote:
> > What do you think about this change?
> 
> looks promising! as you were questioning, it might be better to align the
naming
> with the ThreadableLoader though ThreadableLoader itself has been waiting for
> better name. I can live with LoadingContext.

ThreadableLoadingContext?  This context itself isn't supposed to be threadable
(to be used only on one thread), which is making me feel a bit ambiguous about
it.  (No strong preference otherwise though)

I also want to create a corresponding, minimum FetchContext which doesn't
require Frame/Document, it'd be nice if we could use the same prefix.

[horo, 2017-02-27 04:49:20]

looks good.

BTW do you have any plan for off-main thread WebSocket?

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

https://codereview.chromium.org/2715803004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.h:89:
DocumentThreadableLoader(LoadingContext&,
Why do you use reference instead of pointer?

[kinuko, 2017-02-27 04:55:25]

Thanks,

On 2017/02/27 04:49:20, horo wrote:
> looks good.
> 
> BTW do you have any plan for off-main thread WebSocket?

I don't, but yhirano/tyoshino/ricea may? :)

I think it's the matter of prioritization, now that we mojofied WebSocket it
should be doable but I don't have a clear view off-hand about how much it'd
cost.

[kinuko, 2017-02-27 04:55:35]

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

https://codereview.chromium.org/2715803004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.h:89:
DocumentThreadableLoader(LoadingContext&,
On 2017/02/27 04:49:20, horo wrote:
> Why do you use reference instead of pointer?

Just to indicate this should never be null.

[tyoshino (SeeGerritForStatus), 2017-02-27 05:20:51]

On 2017/02/27 04:25:20, kinuko wrote:
> On 2017/02/24 10:50:58, tyoshino wrote:
> > On 2017/02/24 10:40:27, kinuko wrote:
> > > What do you think about this change?
> > 
> > looks promising! as you were questioning, it might be better to align the
> naming
> > with the ThreadableLoader though ThreadableLoader itself has been waiting
for
> > better name. I can live with LoadingContext.
> 
> ThreadableLoadingContext?  This context itself isn't supposed to be threadable
> (to be used only on one thread), which is making me feel a bit ambiguous about
> it.  (No strong preference otherwise though)

Yes, I just explained the concern I came up with (we have FetchContext which is
for ResourceFetcher, ResourceFetcher has ResourceLoader, LoadingContext is ...
for ThreadableLoader, wow), but ThreadableLoaderContext is not viable
considering due to e.g. such an imprecation as you said. So, don't mind so much,
sorry!

> 
> I also want to create a corresponding, minimum FetchContext which doesn't
> require Frame/Document, it'd be nice if we could use the same prefix.

Could you please elaborate? What's the name in your mind for the
Frame/Document-free FetchContext?

[horo, 2017-02-27 05:57:28]

lgtm

[kinuko, 2017-02-27 06:10:19]

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

[kinuko, 2017-02-27 06:12:38]

+japhet@ for owner review, also to see if he has any opinion.

On 2017/02/27 05:20:51, tyoshino wrote:
> On 2017/02/27 04:25:20, kinuko wrote:
> > On 2017/02/24 10:50:58, tyoshino wrote:
> > > On 2017/02/24 10:40:27, kinuko wrote:
> > > > What do you think about this change?
> > > 
> > > looks promising! as you were questioning, it might be better to align the
> > naming
> > > with the ThreadableLoader though ThreadableLoader itself has been waiting
> for
> > > better name. I can live with LoadingContext.
> > 
> > ThreadableLoadingContext?  This context itself isn't supposed to be
threadable
> > (to be used only on one thread), which is making me feel a bit ambiguous
about
> > it.  (No strong preference otherwise though)
> 
> Yes, I just explained the concern I came up with (we have FetchContext which
is
> for ResourceFetcher, ResourceFetcher has ResourceLoader, LoadingContext is ...
> for ThreadableLoader, wow), but ThreadableLoaderContext is not viable
> considering due to e.g. such an imprecation as you said. So, don't mind so
much,
> sorry!

No, this is a very valid concern. I just can't come up with a good name.  I can
start with ThreadableLoaderContext to start with, to avoid polluting the name
space.

> > I also want to create a corresponding, minimum FetchContext which doesn't
> > require Frame/Document, it'd be nice if we could use the same prefix.
> 
> Could you please elaborate? What's the name in your mind for the
> Frame/Document-free FetchContext?

No idea for now.  BasicFetchContext...? meh :( or I could also start with
ThreadableFetchContext, which may not be that bad...

[yhirano, 2017-02-28 07:06:34]

Currently DocumentThreadableLoader is associated with a non-null Document. You
are changing it, right? When the associated Document is null after this change?

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

https://codereview.chromium.org/2715803004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:784:
LocalFrame* frame = document() ? document()->frame() : nullptr;
Hmm, the comment says |frame| must not be null, but you are changing the
assumption, right? Then perhaps removing the comment and DCHECK is good...

[kinuko, 2017-02-28 13:45:36]

On 2017/02/28 07:06:34, yhirano (slow) wrote:
> Currently DocumentThreadableLoader is associated with a non-null Document. You
> are changing it, right? When the associated Document is null after this
change?

Right. This change itself doesn't change the assumption though.  (I could also
keep the DCHECKs / non-null assumptions until we really change the behavior)

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

https://codereview.chromium.org/2715803004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:784:
LocalFrame* frame = document() ? document()->frame() : nullptr;
On 2017/02/28 07:06:34, yhirano (slow) wrote:
> Hmm, the comment says |frame| must not be null, but you are changing the
> assumption, right? Then perhaps removing the comment and DCHECK is good...

Done.  (Will close crbug.com/578849, though we haven't really identified the
cause of the original problem...)

[tyoshino (SeeGerritForStatus), 2017-02-28 13:47:57]

On 2017/02/27 06:12:38, kinuko wrote:
> No, this is a very valid concern. I just can't come up with a good name.  I
can
> start with ThreadableLoaderContext to start with, to avoid polluting the name
> space.

Oops, sorry. I was misunderstanding that you were objecting to it.

ThreadableLoaderContext sounds ok.

> No idea for now.  BasicFetchContext...? meh :( or I could also start with
> ThreadableFetchContext, which may not be that bad...

Ah ... ok.

[tyoshino (SeeGerritForStatus), 2017-02-28 13:49:28]

On 2017/02/27 04:55:25, kinuko wrote:
> Thanks,
> 
> On 2017/02/27 04:49:20, horo wrote:
> > looks good.
> > 
> > BTW do you have any plan for off-main thread WebSocket?
> 
> I don't, but yhirano/tyoshino/ricea may? :)

We haven't started any work yet.

> I think it's the matter of prioritization, now that we mojofied WebSocket it
> should be doable but I don't have a clear view off-hand about how much it'd
> cost.

Yes...

[Nate Chapin, 2017-02-28 20:47:18]

LGTM with a couple nits.

Is the hope that this refactoring will be helpful for enabling off-main-thread
loading someday? :)

https://codereview.chromium.org/2715803004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/2715803004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:148:
DocumentThreadableLoader* DocumentThreadableLoader::create(
It's a bit confusing that this create() version and loadResourceSynchronusly()
take a Document&, instead of all creators taking a LoadingContext&. Would it be
too much of a pain to change them all?

https://codereview.chromium.org/2715803004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.h
(right):

https://codereview.chromium.org/2715803004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.h:143: //
This may return non-null.
s/non-null/null/ ?

[yhirano, 2017-03-01 03:41:34]

lgtm

https://codereview.chromium.org/2715803004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/LoadingContext.cpp (right):

https://codereview.chromium.org/2715803004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/LoadingContext.cpp:13: class
DocumentLoadingContext : public LoadingContext {
+final

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

https://codereview.chromium.org/2715803004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/LoadingContext.h:27: : public
GarbageCollectedFinalized<LoadingContext> {
"Finalized" is not needed at this moment.

[kinuko, 2017-03-01 15:07:51]

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

[commit-bot: I haz the power, 2017-03-01 15:08:13]

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

[kinuko, 2017-03-01 15:08:29]

Thanks!

Renamed to ThreadableLoadingContext.

https://codereview.chromium.org/2715803004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/2715803004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:148:
DocumentThreadableLoader* DocumentThreadableLoader::create(
On 2017/02/28 20:47:18, Nate Chapin wrote:
> It's a bit confusing that this create() version and loadResourceSynchronusly()
> take a Document&, instead of all creators taking a LoadingContext&. Would it
be
> too much of a pain to change them all?

Agreed, done.

https://codereview.chromium.org/2715803004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/LoadingContext.cpp (right):

https://codereview.chromium.org/2715803004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/LoadingContext.cpp:13: class
DocumentLoadingContext : public LoadingContext {
On 2017/03/01 03:41:34, yhirano (slow) wrote:
> +final

Done.

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

https://codereview.chromium.org/2715803004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/LoadingContext.h:27: : public
GarbageCollectedFinalized<LoadingContext> {
On 2017/03/01 03:41:34, yhirano (slow) wrote:
> "Finalized" is not needed at this moment.

Done.

https://codereview.chromium.org/2715803004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.h
(right):

https://codereview.chromium.org/2715803004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.h:143: //
This may return non-null.
On 2017/02/28 20:47:18, Nate Chapin wrote:
> s/non-null/null/ ?

D'oh. Done.

[kinuko, 2017-03-01 15:08:40]

Description was changed from

==========
Introduce LoadingContext: make threaded loading code one step away from
Document.

For now it's just a wrapper of Document.

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

to

==========
Introduce ThreadableLoadingContext: make threaded loading code one step away
from Document.

For now it's just a wrapper of Document.

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

[commit-bot: I haz the power, 2017-03-01 15:20:27]

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

[commit-bot: I haz the power, 2017-03-01 15:20:28]

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

[kinuko, 2017-03-01 15:28:15]

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

[commit-bot: I haz the power, 2017-03-01 15:28:35]

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-01 15:45:53]

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

[commit-bot: I haz the power, 2017-03-01 15:45:54]

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

[kinuko, 2017-03-02 00:32:46]

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

[commit-bot: I haz the power, 2017-03-02 00:33:34]

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

[kinuko, 2017-03-02 01:49:35]

The CQ bit was unchecked by kinuko@chromium.org

[kinuko, 2017-03-02 01:49:38]

The CQ bit was checked by kinuko@chromium.org

[kinuko, 2017-03-02 01:49:38]

The patchset sent to the CQ was uploaded after l-g-t-m from horo@chromium.org,
japhet@chromium.org, yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/2715803004/#ps100001
(title: ".")

[kinuko, 2017-03-02 01:49:42]

The CQ bit was checked by kinuko@chromium.org

[commit-bot: I haz the power, 2017-03-02 01:50:31]

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 04:56:59]

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

[commit-bot: I haz the power, 2017-03-02 04:57:01]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[kinuko, 2017-03-02 05:02:29]

The CQ bit was checked by kinuko@chromium.org

[commit-bot: I haz the power, 2017-03-02 05:04:07]

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 07:01:34]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1488430949495100,
"parent_rev": "9cd1712465e607972bae01b8d3aaadcec75bd106", "commit_rev":
"1a7f9ee8e1a0d5944b59e4474c1344b8bf7acbf8"}

[commit-bot: I haz the power, 2017-03-02 07:04:28]

Description was changed from

==========
Introduce ThreadableLoadingContext: make threaded loading code one step away
from Document.

For now it's just a wrapper of Document.

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

to

==========
Introduce ThreadableLoadingContext: make threaded loading code one step away
from Document.

For now it's just a wrapper of Document.

BUG=695279, 538751

Review-Url: https://codereview.chromium.org/2715803004
Cr-Commit-Position: refs/heads/master@{#454198}
Committed:
https://chromium.googlesource.com/chromium/src/+/1a7f9ee8e1a0d5944b59e4474c13...
==========

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

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/1a7f9ee8e1a0d5944b59e4474c13...