Move the new WebUI's code to read the data off the IO thread as it's generally from the memory-mapp…

Move the new WebUI's code to read the data off the IO thread as it's generally from the memory-mapped resource file.

To avoid extra thread hops, I call (and bind) URLLoaderClient on the thread where we do the reads. I've found it easier to read to remove the URLLoaderImpl and just pass the required data to static methods.

BUG=717714
Review-Url: https://codereview.chromium.org/2867083002
Cr-Commit-Position: refs/heads/master@{#470082}
Committed: https://chromium.googlesource.com/chromium/src/+/165aa1cd8fb624a2f222c2049edd5b1b48597b63

[jam, 2017-05-08 16:38:55]

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

[commit-bot: I haz the power, 2017-05-08 16:39:44]

Dry run: CQ is trying da patch.

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

[jam, 2017-05-08 16:42:13]

Description was changed from

==========
Move the new WebUI's code to read the data off the IO thread as it's generally
from the memory-mapped resource file.

Since we have a lot of thread hops now, I've removed the URLLoaderImpl class and
instead passed the necessary data along. To avoid extra thread hops, I call (and
bind) URLLoaderClient on the thread where we do the reads.

BUG=717714
==========

to

==========
Move the new WebUI's code to read the data off the IO thread as it's generally
from the memory-mapped resource file.

To avoid extra thread hops, I call (and bind) URLLoaderClient on the thread
where we do the reads. I've found it easier to read to remove the URLLoaderImpl
and just pass the required data to static methods.

BUG=717714
==========

[commit-bot: I haz the power, 2017-05-08 16:58:43]

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

[commit-bot: I haz the power, 2017-05-08 16:58:44]

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

[jam, 2017-05-08 17:45:56]

jam@chromium.org changed reviewers:
+ yzshen@chromium.org

[jam, 2017-05-08 17:45:57]

hey Yuzhu, this addresses the comment Matt pointed out in the previous cl. I've
switched to binding URLLoaderClient only on the disk IO thread, to avoid the
expensive unbind/bind calls.

[yzshen1, 2017-05-08 18:07:04]

LGTM

https://codereview.chromium.org/2867083002/diff/1/content/browser/webui/web_u...
File content/browser/webui/web_ui_url_loader_factory.cc (right):

https://codereview.chromium.org/2867083002/diff/1/content/browser/webui/web_u...
content/browser/webui/web_ui_url_loader_factory.cc:49:
mojo::InterfacePtrInfo<mojom::URLLoaderClient> client_info,
fyi: you could use the alias mojom::URLLoaderClientPtrInfo which is shorter.

[jam, 2017-05-08 18:18:53]

https://codereview.chromium.org/2867083002/diff/1/content/browser/webui/web_u...
File content/browser/webui/web_ui_url_loader_factory.cc (right):

https://codereview.chromium.org/2867083002/diff/1/content/browser/webui/web_u...
content/browser/webui/web_ui_url_loader_factory.cc:49:
mojo::InterfacePtrInfo<mojom::URLLoaderClient> client_info,
On 2017/05/08 18:07:04, yzshen1 wrote:
> fyi: you could use the alias mojom::URLLoaderClientPtrInfo which is shorter.

Done.

[jam, 2017-05-08 18:19:52]

The CQ bit was checked by jam@chromium.org

[jam, 2017-05-08 18:19:52]

The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org
Link to the patchset: https://codereview.chromium.org/2867083002/#ps20001
(title: "review comments and a TODO")

[commit-bot: I haz the power, 2017-05-08 18:20:28]

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-08 19:49:01]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1494267592414130,
"parent_rev": "8d745a7f38b559d75efe5b04a79f5fecd89a2e22", "commit_rev":
"165aa1cd8fb624a2f222c2049edd5b1b48597b63"}

[commit-bot: I haz the power, 2017-05-08 19:49:10]

Description was changed from

==========
Move the new WebUI's code to read the data off the IO thread as it's generally
from the memory-mapped resource file.

To avoid extra thread hops, I call (and bind) URLLoaderClient on the thread
where we do the reads. I've found it easier to read to remove the URLLoaderImpl
and just pass the required data to static methods.

BUG=717714
==========

to

==========
Move the new WebUI's code to read the data off the IO thread as it's generally
from the memory-mapped resource file.

To avoid extra thread hops, I call (and bind) URLLoaderClient on the thread
where we do the reads. I've found it easier to read to remove the URLLoaderImpl
and just pass the required data to static methods.

BUG=717714

Review-Url: https://codereview.chromium.org/2867083002
Cr-Commit-Position: refs/heads/master@{#470082}
Committed:
https://chromium.googlesource.com/chromium/src/+/165aa1cd8fb624a2f222c2049edd...
==========

[commit-bot: I haz the power, 2017-05-08 19:49:11]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/165aa1cd8fb624a2f222c2049edd...

[groby-ooo-7-16, 2017-05-15 18:31:45]

On 2017/05/08 19:49:11, commit-bot: I haz the power wrote:
> Committed patchset #2 (id:20001) as
>
https://chromium.googlesource.com/chromium/src/+/165aa1cd8fb624a2f222c2049edd...

The read is deliberately ON the IO thread - even though it's a memory-mapped
file, it often blocks on page-in, leading to jankiness on WebUI - see
https://codereview.chromium.org/2268653002

Does mojo code run on a separate worker thread, or is this moving us back to the
UI thread?

[jam, 2017-05-15 19:00:11]

On 2017/05/15 18:31:45, groby wrote:
> On 2017/05/08 19:49:11, commit-bot: I haz the power wrote:
> > Committed patchset #2 (id:20001) as
> >
>
https://chromium.googlesource.com/chromium/src/+/165aa1cd8fb624a2f222c2049edd...
> 
> The read is deliberately ON the IO thread - even though it's a memory-mapped
> file, it often blocks on page-in, leading to jankiness on WebUI - see
> https://codereview.chromium.org/2268653002
> 
> Does mojo code run on a separate worker thread, or is this moving us back to
the
> UI thread?

This change moves it off BrowserThread::IO (the non-blocking IO), and onto the
(blocking) IO threads. this is to match the existing behavior.