Use per-frame TaskRunner instead of thread's default in DataConsumerHandle

Use per-frame TaskRunner instead of thread's default in DataConsumerHandle

This CL adds an WebTaskRunner parameter to DataConsumerHandle::obtainReader
and removes access to the thread's default task runner in favor to
use per-frame task runner.

BUG=624696

[tzik, 2016-07-26 10:41:10]

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

[commit-bot: I haz the power, 2016-07-26 10:41:30]

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, 2016-07-26 10:44:13]

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

[commit-bot: I haz the power, 2016-07-26 10:44:14]

Dry run: 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-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[tzik, 2016-07-27 09:16:03]

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

[commit-bot: I haz the power, 2016-07-27 09:16:21]

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

[tzik, 2016-07-27 09:42:29]

Description was changed from

==========
Use per-frame TaskRunner instead of thread's default in DataConsumerHandle

WIP

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

to

==========
Use per-frame TaskRunner instead of thread's default in DataConsumerHandle

This CL adds an WebTaskRunner parameter to DataConsumerHandle::obtainReader
and removes access to the thread's default task runner in favor to
use per-frame task runner.

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

[tzik, 2016-07-27 09:43:09]

tzik@chromium.org changed reviewers:
+ haraken@chromium.org

[tzik, 2016-07-27 09:44:08]

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

[tzik, 2016-07-27 09:45:17]

tzik@chromium.org changed reviewers:
+ skyostil@chromium.org

[tzik, 2016-07-27 09:46:56]

Description was changed from

==========
Use per-frame TaskRunner instead of thread's default in DataConsumerHandle

This CL adds an WebTaskRunner parameter to DataConsumerHandle::obtainReader
and removes access to the thread's default task runner in favor to
use per-frame task runner.

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

to

==========
Use per-frame TaskRunner instead of thread's default in DataConsumerHandle

This CL adds an WebTaskRunner parameter to DataConsumerHandle::obtainReader
and removes access to the thread's default task runner in favor to
use per-frame task runner.

BUG=624696
==========

[tzik, 2016-07-27 09:55:35]

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

[tzik, 2016-07-27 09:55:36]

PTAL to:
yhirano: modules/fetch stuff,
haraken: overall,
skyostil: //component/scheduler,
kinuko: //content/child, if you have time.

[commit-bot: I haz the power, 2016-07-27 10:20:17]

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

[commit-bot: I haz the power, 2016-07-27 10:20:18]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-07-27 11:05:25]

Thanks for the CL!

Hmm, it looks like this CL is passing task runners a bit too much. We might want
to consider a way to decrease # of task runner parameters. If a given object has
a way to get its associated Document, Frame, ScriptState etc, we don't need to
pass in task runners to that object.

https://codereview.chromium.org/2177243002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp
(right):

https://codereview.chromium.org/2177243002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp:246:
m_readerTaskRunner->postTask(BLINK_FROM_HERE, WTF::bind(&ReadingContext::notify,
PassRefPtr<ReadingContext>(this)));

For example, you can avoid adding m_readerTaskRunner by using
TaskRunnerHelper::getUnthrottledTaskRunner(m_scriptState.get()).

[tzik, 2016-07-27 14:22:11]

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

[commit-bot: I haz the power, 2016-07-27 14:22:23]

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, 2016-07-27 14:25:26]

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

[commit-bot: I haz the power, 2016-07-27 14:25:27]

Dry run: This issue passed the CQ dry run.

[tzik, 2016-07-27 14:59:41]

https://codereview.chromium.org/2177243002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp
(right):

https://codereview.chromium.org/2177243002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp:246:
m_readerTaskRunner->postTask(BLINK_FROM_HERE, WTF::bind(&ReadingContext::notify,
PassRefPtr<ReadingContext>(this)));
On 2016/07/27 11:05:25, haraken wrote:
> 
> For example, you can avoid adding m_readerTaskRunner by using
> TaskRunnerHelper::getUnthrottledTaskRunner(m_scriptState.get()).

Done.
Frame/Document/ScriptState seem not available in other use cases.

[tzik, 2016-07-27 14:59:43]

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

[commit-bot: I haz the power, 2016-07-27 15:00:08]

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

[haraken, 2016-07-27 15:42:14]

On 2016/07/27 14:59:41, tzik wrote:
>
https://codereview.chromium.org/2177243002/diff/80001/third_party/WebKit/Sour...
> File
> third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp
> (right):
> 
>
https://codereview.chromium.org/2177243002/diff/80001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp:246:
> m_readerTaskRunner->postTask(BLINK_FROM_HERE,
WTF::bind(&ReadingContext::notify,
> PassRefPtr<ReadingContext>(this)));
> On 2016/07/27 11:05:25, haraken wrote:
> > 
> > For example, you can avoid adding m_readerTaskRunner by using
> > TaskRunnerHelper::getUnthrottledTaskRunner(m_scriptState.get()).
> 
> Done.
> Frame/Document/ScriptState seem not available in other use cases.

Sorry, I believe I should have clarified this earlier...

Another important goal of the per-frame scheduler project is to make more Blink
code be aware of Frame (e.g., we've introduced FrameBramer). So we prefer
passing around LocalFrame/Document/ScriptState than task runners. I apologize I
didn't mention this before you spend efforts on creating a large CL :/

In this particular CL, I think we can pass around LocalFrame or implement
WebDataConsumerHandle::Client::frame(). I want to ask yhirano@'s thoughts as
well.

It might be better to wait for Sami's comment and confirm that we're going in a
right direction.

[commit-bot: I haz the power, 2016-07-27 16:29:23]

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

[commit-bot: I haz the power, 2016-07-27 16:29:24]

Dry run: This issue passed the CQ dry run.

[tzik, 2016-07-28 06:53:10]

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

[commit-bot: I haz the power, 2016-07-28 06:53:28]

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

[tzik, 2016-07-28 07:15:05]

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

[commit-bot: I haz the power, 2016-07-28 07:15:12]

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

[tzik, 2016-07-28 07:17:19]

On 2016/07/27 15:42:14, haraken wrote:
> On 2016/07/27 14:59:41, tzik wrote:
> >
>
https://codereview.chromium.org/2177243002/diff/80001/third_party/WebKit/Sour...
> > File
> > third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp
> > (right):
> > 
> >
>
https://codereview.chromium.org/2177243002/diff/80001/third_party/WebKit/Sour...
> >
>
third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp:246:
> > m_readerTaskRunner->postTask(BLINK_FROM_HERE,
> WTF::bind(&ReadingContext::notify,
> > PassRefPtr<ReadingContext>(this)));
> > On 2016/07/27 11:05:25, haraken wrote:
> > > 
> > > For example, you can avoid adding m_readerTaskRunner by using
> > > TaskRunnerHelper::getUnthrottledTaskRunner(m_scriptState.get()).
> > 
> > Done.
> > Frame/Document/ScriptState seem not available in other use cases.
> 
> Sorry, I believe I should have clarified this earlier...
> 
> Another important goal of the per-frame scheduler project is to make more
Blink
> code be aware of Frame (e.g., we've introduced FrameBramer). So we prefer
> passing around LocalFrame/Document/ScriptState than task runners. I apologize
I
> didn't mention this before you spend efforts on creating a large CL :/
> 
> In this particular CL, I think we can pass around LocalFrame or implement
> WebDataConsumerHandle::Client::frame(). I want to ask yhirano@'s thoughts as
> well.
> 
> It might be better to wait for Sami's comment and confirm that we're going in
a
> right direction.

Updated the CL to add WebDataConsumerHandle::Client::getTaskRunner(), and
reverted obtainReader() part.
Since the interface is implemented in content/ too, TaskRunner looks better to
me to expose. WDYT?

[commit-bot: I haz the power, 2016-07-28 07:36:25]

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

[commit-bot: I haz the power, 2016-07-28 07:36:26]

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

[tzik, 2016-07-28 08:11:05]

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

[commit-bot: I haz the power, 2016-07-28 08:11:27]

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, 2016-07-28 08:45:44]

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

[commit-bot: I haz the power, 2016-07-28 08:45:45]

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

[tzik, 2016-07-28 09:13:09]

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

[commit-bot: I haz the power, 2016-07-28 09:13:28]

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

[haraken, 2016-07-28 09:19:30]

Thanks, this looks much nicer!

https://codereview.chromium.org/2177243002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/CompositeDataConsumerHandle.cpp
(right):

https://codereview.chromium.org/2177243002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/CompositeDataConsumerHandle.cpp:150:
std::unique_ptr<WebTaskRunner> m_readerTaskRunner;

Instead of holding a task runner, can we probably use m_client->getTaskRunner()?

https://codereview.chromium.org/2177243002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/DataConsumerHandleTestUtil.cpp
(right):

https://codereview.chromium.org/2177243002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/DataConsumerHandleTestUtil.cpp:315:
return Platform::current()->currentThread()->getWebTaskRunner()->clone();

Do you need clone()?

https://codereview.chromium.org/2177243002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/DataConsumerHandleTestUtil.h
(right):

https://codereview.chromium.org/2177243002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/DataConsumerHandleTestUtil.h:438:
std::unique_ptr<WebTaskRunner> m_readerTaskRunner;

Ditto.

https://codereview.chromium.org/2177243002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/DataConsumerTee.cpp (right):

https://codereview.chromium.org/2177243002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/DataConsumerTee.cpp:252:
std::unique_ptr<WebTaskRunner> m_readerTaskRunner;

Instead of holding a task runner, can we probably use m_client->getTaskRunner()?

[commit-bot: I haz the power, 2016-07-28 09:51:07]

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

[commit-bot: I haz the power, 2016-07-28 09:51:08]

Dry run: 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...)

[tzik, 2016-07-28 13:13:27]

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

[commit-bot: I haz the power, 2016-07-28 13:13:32]

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

[tzik, 2016-07-28 13:43:18]

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

[commit-bot: I haz the power, 2016-07-28 13:43:37]

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

[Sami, 2016-07-28 14:28:12]

On 2016/07/27 15:42:14, haraken wrote:
> Sorry, I believe I should have clarified this earlier...
> 
> Another important goal of the per-frame scheduler project is to make more
Blink
> code be aware of Frame (e.g., we've introduced FrameBramer). So we prefer
> passing around LocalFrame/Document/ScriptState than task runners. I apologize
I
> didn't mention this before you spend efforts on creating a large CL :/
> 
> In this particular CL, I think we can pass around LocalFrame or implement
> WebDataConsumerHandle::Client::frame(). I want to ask yhirano@'s thoughts as
> well.
> 
> It might be better to wait for Sami's comment and confirm that we're going in
a
> right direction.

I think if we can make a piece of code use the right (per-frame) task runner,
then it doesn't really matter whether it was passed in via a
Document/LocalFrame/etc. or just a plain task runner. It's really up to each
module whether it makes sense to deal with frames (high level) or task runners
(low level). In general I would lean toward task runners unless you also need
something else from the document/frame too.

One thought about getBaseTaskRunner(): I'm working on moving the Blink scheduler
into Blink[1], which should make it possible for a WebTaskRunner to directly
expose the underlying base::TaskRunner. While getBaseTaskRunner() is sort of a
dangerous API (how do you know you have the right kind of WebTaskRunner?), it is
compatible with the way I'm going in my patch so it's probably better than
adding another task runner wrapper.

[1] https://codereview.chromium.org/2118903002/

[tzik, 2016-07-28 14:40:19]

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

[commit-bot: I haz the power, 2016-07-28 14:40:41]

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

[tzik, 2016-07-28 14:45:34]

https://codereview.chromium.org/2177243002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/CompositeDataConsumerHandle.cpp
(right):

https://codereview.chromium.org/2177243002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/CompositeDataConsumerHandle.cpp:150:
std::unique_ptr<WebTaskRunner> m_readerTaskRunner;
On 2016/07/28 09:19:30, haraken wrote:
> 
> Instead of holding a task runner, can we probably use
m_client->getTaskRunner()?

Since m_client is nullable and we need to use it even when it's null in this
case, I think we can't remove this.

https://codereview.chromium.org/2177243002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/DataConsumerHandleTestUtil.cpp
(right):

https://codereview.chromium.org/2177243002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/DataConsumerHandleTestUtil.cpp:315:
return Platform::current()->currentThread()->getWebTaskRunner()->clone();
On 2016/07/28 09:19:30, haraken wrote:
> 
> Do you need clone()?

This is no longer used. Let me remove it.

https://codereview.chromium.org/2177243002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/DataConsumerHandleTestUtil.h
(right):

https://codereview.chromium.org/2177243002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/DataConsumerHandleTestUtil.h:438:
std::unique_ptr<WebTaskRunner> m_readerTaskRunner;
On 2016/07/28 09:19:30, haraken wrote:
> 
> Ditto.

This task runner seems to be used to identify attached reader, where each reader
has its own thread.

https://codereview.chromium.org/2177243002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/DataConsumerTee.cpp (right):

https://codereview.chromium.org/2177243002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/DataConsumerTee.cpp:252:
std::unique_ptr<WebTaskRunner> m_readerTaskRunner;
On 2016/07/28 09:19:30, haraken wrote:
> 
> Instead of holding a task runner, can we probably use
m_client->getTaskRunner()?

Done.

[haraken, 2016-07-28 15:16:55]

On 2016/07/28 14:28:12, Sami wrote:
> On 2016/07/27 15:42:14, haraken wrote:
> > Sorry, I believe I should have clarified this earlier...
> > 
> > Another important goal of the per-frame scheduler project is to make more
> Blink
> > code be aware of Frame (e.g., we've introduced FrameBramer). So we prefer
> > passing around LocalFrame/Document/ScriptState than task runners. I
apologize
> I
> > didn't mention this before you spend efforts on creating a large CL :/
> > 
> > In this particular CL, I think we can pass around LocalFrame or implement
> > WebDataConsumerHandle::Client::frame(). I want to ask yhirano@'s thoughts as
> > well.
> > 
> > It might be better to wait for Sami's comment and confirm that we're going
in
> a
> > right direction.
> 
> I think if we can make a piece of code use the right (per-frame) task runner,
> then it doesn't really matter whether it was passed in via a
> Document/LocalFrame/etc. or just a plain task runner. It's really up to each
> module whether it makes sense to deal with frames (high level) or task runners
> (low level). In general I would lean toward task runners unless you also need
> something else from the document/frame too.

I'd prefer passing Document/Frame/ScriptState (high-level things) than task
runners (low-level things) since the high-level things are more generic and
convenient. In Blink, it sometimes happens that we pass too many low-level
things which can be replaced with one high-level thing. (e.g., that's why we
introduced ScriptState, which encapsulates all information about script
execution.)

> 
> One thought about getBaseTaskRunner(): I'm working on moving the Blink
scheduler
> into Blink[1], which should make it possible for a WebTaskRunner to directly
> expose the underlying base::TaskRunner. While getBaseTaskRunner() is sort of a
> dangerous API (how do you know you have the right kind of WebTaskRunner?), it
is
> compatible with the way I'm going in my patch so it's probably better than
> adding another task runner wrapper.
> 
> [1] https://codereview.chromium.org/2118903002/

The migration of scheduler/ makes things a lot easier :) Do you think you can
land it in a reasonable timeline?

[kinuko, 2016-07-28 15:41:15]

On 2016/07/28 15:16:55, haraken wrote:
> On 2016/07/28 14:28:12, Sami wrote:
> > On 2016/07/27 15:42:14, haraken wrote:
> > > Sorry, I believe I should have clarified this earlier...
> > > 
> > > Another important goal of the per-frame scheduler project is to make more
> > Blink
> > > code be aware of Frame (e.g., we've introduced FrameBramer). So we prefer
> > > passing around LocalFrame/Document/ScriptState than task runners. I
> apologize
> > I
> > > didn't mention this before you spend efforts on creating a large CL :/
> > > 
> > > In this particular CL, I think we can pass around LocalFrame or implement
> > > WebDataConsumerHandle::Client::frame(). I want to ask yhirano@'s thoughts
as
> > > well.
> > > 
> > > It might be better to wait for Sami's comment and confirm that we're going
> in
> > a
> > > right direction.
> > 
> > I think if we can make a piece of code use the right (per-frame) task
runner,
> > then it doesn't really matter whether it was passed in via a
> > Document/LocalFrame/etc. or just a plain task runner. It's really up to each
> > module whether it makes sense to deal with frames (high level) or task
runners
> > (low level). In general I would lean toward task runners unless you also
need
> > something else from the document/frame too.
> 
> I'd prefer passing Document/Frame/ScriptState (high-level things) than task
> runners (low-level things) since the high-level things are more generic and
> convenient. In Blink, it sometimes happens that we pass too many low-level
> things which can be replaced with one high-level thing. (e.g., that's why we
> introduced ScriptState, which encapsulates all information about script
> execution.)

I don't oppose to this idea in general but WebDataConsumerHandle is exposed via
public/platform and its design seems to be intended to be lower-level /
platform-layer API, I think exposing task runner is more appropriate here
(otherwise we'll need more redesign).  If we really start to see the problem
like you mention here (i.e. ended up passing too many low-level things) we could
revisit it again?

> > 
> > One thought about getBaseTaskRunner(): I'm working on moving the Blink
> scheduler
> > into Blink[1], which should make it possible for a WebTaskRunner to directly
> > expose the underlying base::TaskRunner. While getBaseTaskRunner() is sort of
a
> > dangerous API (how do you know you have the right kind of WebTaskRunner?),
it
> is
> > compatible with the way I'm going in my patch so it's probably better than
> > adding another task runner wrapper.
> > 
> > [1] https://codereview.chromium.org/2118903002/
> 
> The migration of scheduler/ makes things a lot easier :) Do you think you can
> land it in a reasonable timeline?

[kinuko, 2016-07-28 15:41:31]

https://codereview.chromium.org/2177243002/diff/260001/content/child/shared_m...
File content/child/shared_memory_data_consumer_handle.cc (right):

https://codereview.chromium.org/2177243002/diff/260001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle.cc:128:
scheduler::WebTaskRunnerImpl::GetBaseTaskRunner(*task_runner);
Per Sami's comment this would be a tentative solution but this code makes me
feel uneasy :(  There doesn't seem enough guarantee that this is going to be
always safe.

Does it make sense to instead use WebTaskRunner for notification_task_runner_
for now?  (For null-client case could we use
Platform::current()->currentThread()->getWebTaskRunner()?)

Or I think we need some comment at least to explicitly note that this should be
safe and will be gone soon.

(Or if we're landing the scheduler migration patch first we won't need this
hack, I assume)

https://codereview.chromium.org/2177243002/diff/260001/content/child/web_data...
File content/child/web_data_consumer_handle_impl.cc (right):

https://codereview.chromium.org/2177243002/diff/260001/content/child/web_data...
content/child/web_data_consumer_handle_impl.cc:32: return
base::ThreadTaskRunnerHandle::Get();
ditto.

[haraken, 2016-07-28 16:02:17]

On 2016/07/28 15:41:15, kinuko wrote:
> On 2016/07/28 15:16:55, haraken wrote:
> > On 2016/07/28 14:28:12, Sami wrote:
> > > On 2016/07/27 15:42:14, haraken wrote:
> > > > Sorry, I believe I should have clarified this earlier...
> > > > 
> > > > Another important goal of the per-frame scheduler project is to make
more
> > > Blink
> > > > code be aware of Frame (e.g., we've introduced FrameBramer). So we
prefer
> > > > passing around LocalFrame/Document/ScriptState than task runners. I
> > apologize
> > > I
> > > > didn't mention this before you spend efforts on creating a large CL :/
> > > > 
> > > > In this particular CL, I think we can pass around LocalFrame or
implement
> > > > WebDataConsumerHandle::Client::frame(). I want to ask yhirano@'s
thoughts
> as
> > > > well.
> > > > 
> > > > It might be better to wait for Sami's comment and confirm that we're
going
> > in
> > > a
> > > > right direction.
> > > 
> > > I think if we can make a piece of code use the right (per-frame) task
> runner,
> > > then it doesn't really matter whether it was passed in via a
> > > Document/LocalFrame/etc. or just a plain task runner. It's really up to
each
> > > module whether it makes sense to deal with frames (high level) or task
> runners
> > > (low level). In general I would lean toward task runners unless you also
> need
> > > something else from the document/frame too.
> > 
> > I'd prefer passing Document/Frame/ScriptState (high-level things) than task
> > runners (low-level things) since the high-level things are more generic and
> > convenient. In Blink, it sometimes happens that we pass too many low-level
> > things which can be replaced with one high-level thing. (e.g., that's why we
> > introduced ScriptState, which encapsulates all information about script
> > execution.)
> 
> I don't oppose to this idea in general but WebDataConsumerHandle is exposed
via
> public/platform and its design seems to be intended to be lower-level /
> platform-layer API, I think exposing task runner is more appropriate here
> (otherwise we'll need more redesign).  If we really start to see the problem
> like you mention here (i.e. ended up passing too many low-level things) we
could
> revisit it again?

Just to clarify, I'm fine with PS14 since that is not unnecessarily passing
around task runners (client->taskRunner() provides a proper task runner). PS14
looks good to me except the GetBaseTaskRunner issue.

My point is just that if we have to pass around task runners in many Blink
classes like PS5, I want to pass Document/Frame/ScriptState instead.

[commit-bot: I haz the power, 2016-07-28 16:20:38]

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

[commit-bot: I haz the power, 2016-07-28 16:20:39]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-07-28 16:25:06]

As I talked offline, I'm deprecating most of WebDataConsumerHandle
implementations (crbug.com/610195) and am not happy to add much code.

I have a question: XHR onreadystatechange is executed as a loading task while
fetch(url).then(r => r.text()) is resolved on the ScriptState's task runner with
this CL. Is it consistent?

https://codereview.chromium.org/2177243002/diff/260001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebDataConsumerHandle.h (right):

https://codereview.chromium.org/2177243002/diff/260001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebDataConsumerHandle.h:58: virtual
WebTaskRunner* getTaskRunner() = 0;
Please write comments. What is this? Is it allowed to return null?

[Sami, 2016-07-28 17:10:08]

On 2016/07/28 15:16:55, haraken wrote:
> The migration of scheduler/ makes things a lot easier :) Do you think you can
> land it in a reasonable timeline?

Working on it :)

[yhirano, 2016-07-29 02:30:38]

On 2016/07/28 07:17:19, tzik wrote:
> On 2016/07/27 15:42:14, haraken wrote:
> > On 2016/07/27 14:59:41, tzik wrote:
> > >
> >
>
https://codereview.chromium.org/2177243002/diff/80001/third_party/WebKit/Sour...
> > > File
> > >
third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2177243002/diff/80001/third_party/WebKit/Sour...
> > >
> >
>
third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp:246:
> > > m_readerTaskRunner->postTask(BLINK_FROM_HERE,
> > WTF::bind(&ReadingContext::notify,
> > > PassRefPtr<ReadingContext>(this)));
> > > On 2016/07/27 11:05:25, haraken wrote:
> > > > 
> > > > For example, you can avoid adding m_readerTaskRunner by using
> > > > TaskRunnerHelper::getUnthrottledTaskRunner(m_scriptState.get()).
> > > 
> > > Done.
> > > Frame/Document/ScriptState seem not available in other use cases.
> > 
> > Sorry, I believe I should have clarified this earlier...
> > 
> > Another important goal of the per-frame scheduler project is to make more
> Blink
> > code be aware of Frame (e.g., we've introduced FrameBramer). So we prefer
> > passing around LocalFrame/Document/ScriptState than task runners. I
apologize
> I
> > didn't mention this before you spend efforts on creating a large CL :/
> > 
> > In this particular CL, I think we can pass around LocalFrame or implement
> > WebDataConsumerHandle::Client::frame(). I want to ask yhirano@'s thoughts as
> > well.
> > 
> > It might be better to wait for Sami's comment and confirm that we're going
in
> a
> > right direction.
> 
> Updated the CL to add WebDataConsumerHandle::Client::getTaskRunner(), and
> reverted obtainReader() part.
> Since the interface is implemented in content/ too, TaskRunner looks better to
> me to expose. WDYT?

Can you tell me why having Client::getTaskRunner is better than passing a task
runner to obtainReader? I guess Client::getTaskRunner needs to be called from
the client's thread, and that means most if not all implementations who needs
the task runner calls it in their obtainReader implementation.
Client::getTaskRunner looks overly general to me.

[haraken, 2016-07-29 08:49:41]

On 2016/07/29 02:30:38, yhirano wrote:
> On 2016/07/28 07:17:19, tzik wrote:
> > On 2016/07/27 15:42:14, haraken wrote:
> > > On 2016/07/27 14:59:41, tzik wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2177243002/diff/80001/third_party/WebKit/Sour...
> > > > File
> > > >
> third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp
> > > > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2177243002/diff/80001/third_party/WebKit/Sour...
> > > >
> > >
> >
>
third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp:246:
> > > > m_readerTaskRunner->postTask(BLINK_FROM_HERE,
> > > WTF::bind(&ReadingContext::notify,
> > > > PassRefPtr<ReadingContext>(this)));
> > > > On 2016/07/27 11:05:25, haraken wrote:
> > > > > 
> > > > > For example, you can avoid adding m_readerTaskRunner by using
> > > > > TaskRunnerHelper::getUnthrottledTaskRunner(m_scriptState.get()).
> > > > 
> > > > Done.
> > > > Frame/Document/ScriptState seem not available in other use cases.
> > > 
> > > Sorry, I believe I should have clarified this earlier...
> > > 
> > > Another important goal of the per-frame scheduler project is to make more
> > Blink
> > > code be aware of Frame (e.g., we've introduced FrameBramer). So we prefer
> > > passing around LocalFrame/Document/ScriptState than task runners. I
> apologize
> > I
> > > didn't mention this before you spend efforts on creating a large CL :/
> > > 
> > > In this particular CL, I think we can pass around LocalFrame or implement
> > > WebDataConsumerHandle::Client::frame(). I want to ask yhirano@'s thoughts
as
> > > well.
> > > 
> > > It might be better to wait for Sami's comment and confirm that we're going
> in
> > a
> > > right direction.
> > 
> > Updated the CL to add WebDataConsumerHandle::Client::getTaskRunner(), and
> > reverted obtainReader() part.
> > Since the interface is implemented in content/ too, TaskRunner looks better
to
> > me to expose. WDYT?
> 
> Can you tell me why having Client::getTaskRunner is better than passing a task
> runner to obtainReader? I guess Client::getTaskRunner needs to be called from
> the client's thread, and that means most if not all implementations who needs
> the task runner calls it in their obtainReader implementation.
> Client::getTaskRunner looks overly general to me.

My point is just that I want to avoid passing low-level things like task runners
in many Blink classes. Also the lifetime management of the task runner is a bit
tricky because we need to clone it.

BTW, on what thread does the obtainReader run? I was thinking that it runs on
the main thread, so Client::getTaskRunner() would make sense.

- If it runs on only the main thread, Client::getTaskRunner(), Client::frame()
or passing frame would work.

- If it runs on a non-main thread, I think it's better to make the thread
understand the task runner when the thread is constructed (c.f., kinuko-san's
CL: https://codereview.chromium.org/2163983004/) instead of passing the task
runner every time obtainReader is requested.

[yhirano, 2016-07-29 09:08:41]

On 2016/07/29 08:49:41, haraken wrote:
> On 2016/07/29 02:30:38, yhirano wrote:
> > On 2016/07/28 07:17:19, tzik wrote:
> > > On 2016/07/27 15:42:14, haraken wrote:
> > > > On 2016/07/27 14:59:41, tzik wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2177243002/diff/80001/third_party/WebKit/Sour...
> > > > > File
> > > > >
> > third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp
> > > > > (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2177243002/diff/80001/third_party/WebKit/Sour...
> > > > >
> > > >
> > >
> >
>
third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp:246:
> > > > > m_readerTaskRunner->postTask(BLINK_FROM_HERE,
> > > > WTF::bind(&ReadingContext::notify,
> > > > > PassRefPtr<ReadingContext>(this)));
> > > > > On 2016/07/27 11:05:25, haraken wrote:
> > > > > > 
> > > > > > For example, you can avoid adding m_readerTaskRunner by using
> > > > > > TaskRunnerHelper::getUnthrottledTaskRunner(m_scriptState.get()).
> > > > > 
> > > > > Done.
> > > > > Frame/Document/ScriptState seem not available in other use cases.
> > > > 
> > > > Sorry, I believe I should have clarified this earlier...
> > > > 
> > > > Another important goal of the per-frame scheduler project is to make
more
> > > Blink
> > > > code be aware of Frame (e.g., we've introduced FrameBramer). So we
prefer
> > > > passing around LocalFrame/Document/ScriptState than task runners. I
> > apologize
> > > I
> > > > didn't mention this before you spend efforts on creating a large CL :/
> > > > 
> > > > In this particular CL, I think we can pass around LocalFrame or
implement
> > > > WebDataConsumerHandle::Client::frame(). I want to ask yhirano@'s
thoughts
> as
> > > > well.
> > > > 
> > > > It might be better to wait for Sami's comment and confirm that we're
going
> > in
> > > a
> > > > right direction.
> > > 
> > > Updated the CL to add WebDataConsumerHandle::Client::getTaskRunner(), and
> > > reverted obtainReader() part.
> > > Since the interface is implemented in content/ too, TaskRunner looks
better
> to
> > > me to expose. WDYT?
> > 
> > Can you tell me why having Client::getTaskRunner is better than passing a
task
> > runner to obtainReader? I guess Client::getTaskRunner needs to be called
from
> > the client's thread, and that means most if not all implementations who
needs
> > the task runner calls it in their obtainReader implementation.
> > Client::getTaskRunner looks overly general to me.
> 
> My point is just that I want to avoid passing low-level things like task
runners
> in many Blink classes. Also the lifetime management of the task runner is a
bit
> tricky because we need to clone it.
> 
> BTW, on what thread does the obtainReader run? I was thinking that it runs on
> the main thread, so Client::getTaskRunner() would make sense.
> 
> - If it runs on only the main thread, Client::getTaskRunner(), Client::frame()
> or passing frame would work.
> 
> - If it runs on a non-main thread, I think it's better to make the thread
> understand the task runner when the thread is constructed (c.f., kinuko-san's
> CL: https://codereview.chromium.org/2163983004/) instead of passing the task
> runner every time obtainReader is requested.

It's used on the main thread and worker threads.
WebDataConsumerHandle can be used in a separate thread from which it is created,
and that makes implementations harder to read, harder to modify. Again, I am
fixing it by replacing them with a thread-bound interface.

[haraken, 2016-07-29 09:34:52]

On 2016/07/29 09:08:41, yhirano wrote:
> On 2016/07/29 08:49:41, haraken wrote:
> > On 2016/07/29 02:30:38, yhirano wrote:
> > > On 2016/07/28 07:17:19, tzik wrote:
> > > > On 2016/07/27 15:42:14, haraken wrote:
> > > > > On 2016/07/27 14:59:41, tzik wrote:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2177243002/diff/80001/third_party/WebKit/Sour...
> > > > > > File
> > > > > >
> > >
third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp
> > > > > > (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2177243002/diff/80001/third_party/WebKit/Sour...
> > > > > >
> > > > >
> > > >
> > >
> >
>
third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp:246:
> > > > > > m_readerTaskRunner->postTask(BLINK_FROM_HERE,
> > > > > WTF::bind(&ReadingContext::notify,
> > > > > > PassRefPtr<ReadingContext>(this)));
> > > > > > On 2016/07/27 11:05:25, haraken wrote:
> > > > > > > 
> > > > > > > For example, you can avoid adding m_readerTaskRunner by using
> > > > > > > TaskRunnerHelper::getUnthrottledTaskRunner(m_scriptState.get()).
> > > > > > 
> > > > > > Done.
> > > > > > Frame/Document/ScriptState seem not available in other use cases.
> > > > > 
> > > > > Sorry, I believe I should have clarified this earlier...
> > > > > 
> > > > > Another important goal of the per-frame scheduler project is to make
> more
> > > > Blink
> > > > > code be aware of Frame (e.g., we've introduced FrameBramer). So we
> prefer
> > > > > passing around LocalFrame/Document/ScriptState than task runners. I
> > > apologize
> > > > I
> > > > > didn't mention this before you spend efforts on creating a large CL :/
> > > > > 
> > > > > In this particular CL, I think we can pass around LocalFrame or
> implement
> > > > > WebDataConsumerHandle::Client::frame(). I want to ask yhirano@'s
> thoughts
> > as
> > > > > well.
> > > > > 
> > > > > It might be better to wait for Sami's comment and confirm that we're
> going
> > > in
> > > > a
> > > > > right direction.
> > > > 
> > > > Updated the CL to add WebDataConsumerHandle::Client::getTaskRunner(),
and
> > > > reverted obtainReader() part.
> > > > Since the interface is implemented in content/ too, TaskRunner looks
> better
> > to
> > > > me to expose. WDYT?
> > > 
> > > Can you tell me why having Client::getTaskRunner is better than passing a
> task
> > > runner to obtainReader? I guess Client::getTaskRunner needs to be called
> from
> > > the client's thread, and that means most if not all implementations who
> needs
> > > the task runner calls it in their obtainReader implementation.
> > > Client::getTaskRunner looks overly general to me.
> > 
> > My point is just that I want to avoid passing low-level things like task
> runners
> > in many Blink classes. Also the lifetime management of the task runner is a
> bit
> > tricky because we need to clone it.
> > 
> > BTW, on what thread does the obtainReader run? I was thinking that it runs
on
> > the main thread, so Client::getTaskRunner() would make sense.
> > 
> > - If it runs on only the main thread, Client::getTaskRunner(),
Client::frame()
> > or passing frame would work.
> > 
> > - If it runs on a non-main thread, I think it's better to make the thread
> > understand the task runner when the thread is constructed (c.f.,
kinuko-san's
> > CL: https://codereview.chromium.org/2163983004/) instead of passing the task
> > runner every time obtainReader is requested.
> 
> It's used on the main thread and worker threads.
> WebDataConsumerHandle can be used in a separate thread from which it is
created,
> and that makes implementations harder to read, harder to modify. Again, I am
> fixing it by replacing them with a thread-bound interface.

Yeah, this looks like a good way to go. Then the thread-bound interface can use
a thread-bound helper class that returns the task runners of that thread (<-- we
need to implement the helper class by extending kinuko-san's CL).