[Mojo-Loading] Use independent URLLoaderClient

[Mojo-Loading] Use independent URLLoaderClient

With this CL, various content/ classes start using independent URLLoaderClient
and DownloadedTempFile instead of associated interfaces.

As the blob creation became a sync operation, no message ordering guarantee
is needed for DownloadedTempFile. According to the test results, it seems
the guarantee is not needed for URLLoaderClient, too.

This CL also sets the appropriate task runner (i.e., the loading task runner)
to the URLLoaderClientImpl, which was blocked by the channel-associated interfaces.

BUG=603396
Review-Url: https://codereview.chromium.org/2715423003
Cr-Commit-Position: refs/heads/master@{#453832}
Committed: https://chromium.googlesource.com/chromium/src/+/6f57b50b31b18fdfbd8cabf7a6709cd7f809f181

[yhirano, 2017-02-28 03:22:21]

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

[commit-bot: I haz the power, 2017-02-28 03:22:55]

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-02-28 03:50:57]

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

[commit-bot: I haz the power, 2017-02-28 03:50:58]

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

[yhirano, 2017-02-28 03:53:20]

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

[commit-bot: I haz the power, 2017-02-28 03:53:51]

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-02-28 05:34:16]

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

[commit-bot: I haz the power, 2017-02-28 05:34:16]

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

[yhirano, 2017-02-28 08:42:36]

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

[commit-bot: I haz the power, 2017-02-28 08:42:47]

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-02-28 08:53:21]

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

[commit-bot: I haz the power, 2017-02-28 08:53:22]

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

[yhirano, 2017-02-28 09:04:01]

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

[commit-bot: I haz the power, 2017-02-28 09:04:13]

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

[yhirano, 2017-02-28 09:20:32]

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

[commit-bot: I haz the power, 2017-02-28 09:20:43]

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

[yhirano, 2017-02-28 10:26:28]

Description was changed from

==========
[wip] make url-loader-client independent

BUG=603396
==========

to

==========
[Mojo-Loading] Use independent URLLoaderClient

With this CL, various content/ classes start using independent URLLoaderClient
and DownloadedTempFile instead of associated interfaces.

As the blob creation became a sync operation, no message ordering guarantee
is needed for DownloadedTempFile. Based on the tests, it seems the guarantee
is not needed for URLLoaderClient, too.

This CL also sets the appropriate task runner (i.e., the loading task runner)
to the URLLoaderClientImpl, which is blocked by the channel-associated
interfaces.

BUG=603396
==========

[yhirano, 2017-02-28 10:26:28]

yhirano@chromium.org changed reviewers:
+ kinuko@chromium.org, tzik@chromium.org, yzshen@chromium.org

[yhirano, 2017-02-28 10:27:13]

Description was changed from

==========
[Mojo-Loading] Use independent URLLoaderClient

With this CL, various content/ classes start using independent URLLoaderClient
and DownloadedTempFile instead of associated interfaces.

As the blob creation became a sync operation, no message ordering guarantee
is needed for DownloadedTempFile. Based on the tests, it seems the guarantee
is not needed for URLLoaderClient, too.

This CL also sets the appropriate task runner (i.e., the loading task runner)
to the URLLoaderClientImpl, which is blocked by the channel-associated
interfaces.

BUG=603396
==========

to

==========
[Mojo-Loading] Use independent URLLoaderClient

With this CL, various content/ classes start using independent URLLoaderClient
and DownloadedTempFile instead of associated interfaces.

As the blob creation became a sync operation, no message ordering guarantee
is needed for DownloadedTempFile. According to the test results, it seems
the guarantee is not needed for URLLoaderClient, too.

This CL also sets the appropriate task runner (i.e., the loading task runner)
to the URLLoaderClientImpl, which is blocked by the channel-associated
interfaces.

BUG=603396
==========

[yhirano, 2017-02-28 10:27:25]

Description was changed from

==========
[Mojo-Loading] Use independent URLLoaderClient

With this CL, various content/ classes start using independent URLLoaderClient
and DownloadedTempFile instead of associated interfaces.

As the blob creation became a sync operation, no message ordering guarantee
is needed for DownloadedTempFile. According to the test results, it seems
the guarantee is not needed for URLLoaderClient, too.

This CL also sets the appropriate task runner (i.e., the loading task runner)
to the URLLoaderClientImpl, which is blocked by the channel-associated
interfaces.

BUG=603396
==========

to

==========
[Mojo-Loading] Use independent URLLoaderClient

With this CL, various content/ classes start using independent URLLoaderClient
and DownloadedTempFile instead of associated interfaces.

As the blob creation became a sync operation, no message ordering guarantee
is needed for DownloadedTempFile. According to the test results, it seems
the guarantee is not needed for URLLoaderClient, too.

This CL also sets the appropriate task runner (i.e., the loading task runner)
to the URLLoaderClientImpl, which was blocked by the channel-associated
interfaces.

BUG=603396
==========

[yhirano, 2017-02-28 10:30:48]

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

[yhirano, 2017-02-28 11:15:25]

PTAL

+tzik@ for the overall change.
+yzshen@ as a mojo expert.
+dcheng@ for mojom files.
+kinuko@ for OWNER review.

Please see https://codereview.chromium.org/2553153002#ps600001 which includes
this change for mojo-loading enabled test results. The webkit_tests failures are
not introduced by this change (see
https://codereview.chromium.org/2553153002/#ps420001).

[commit-bot: I haz the power, 2017-02-28 11:18:25]

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

[commit-bot: I haz the power, 2017-02-28 11:18:26]

Dry run: This issue passed the CQ dry run.

[kinuko, 2017-02-28 13:08:28]

Woohoo!  lgtm

[tzik, 2017-02-28 13:40:26]

LGTM!!!

[tzik, 2017-02-28 14:00:36]

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

[commit-bot: I haz the power, 2017-02-28 14:00:53]

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-02-28 14:04:07]

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

[commit-bot: I haz the power, 2017-02-28 14:04:09]

Dry run: This issue passed the CQ dry run.

[yzshen1, 2017-02-28 17:56:33]

LGTM

https://codereview.chromium.org/2715423003/diff/70001/content/browser/loader/...
File content/browser/loader/downloaded_temp_file_impl.h (right):

https://codereview.chromium.org/2715423003/diff/70001/content/browser/loader/...
content/browser/loader/downloaded_temp_file_impl.h:21: // interface, and returns
the interface ptr info to be passed to the client.
nit: please remove "info"

[yhirano, 2017-03-01 00:53:23]

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

[yhirano, 2017-03-01 00:53:43]

https://codereview.chromium.org/2715423003/diff/70001/content/browser/loader/...
File content/browser/loader/downloaded_temp_file_impl.h (right):

https://codereview.chromium.org/2715423003/diff/70001/content/browser/loader/...
content/browser/loader/downloaded_temp_file_impl.h:21: // interface, and returns
the interface ptr info to be passed to the client.
On 2017/02/28 17:56:33, yzshen1 wrote:
> nit: please remove "info"

Done.

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

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

[dcheng, 2017-03-01 01:29:13]

https://codereview.chromium.org/2715423003/diff/90001/content/child/resource_...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/2715423003/diff/90001/content/child/resource_...
content/child/resource_dispatcher.cc:420: request_info->download_to_file &&
!it->second->url_loader;
I think a comment here would be useful: I'm not actually sure why we need to
check that we have a bound URLLoader here.

https://codereview.chromium.org/2715423003/diff/90001/content/child/url_loade...
File content/child/url_loader_client_impl.cc (right):

https://codereview.chromium.org/2715423003/diff/90001/content/child/url_loade...
content/child/url_loader_client_impl.cc:32: binding_.Bind(client_ptr,
task_runner_.get());
Nit: no .get()

[yhirano, 2017-03-01 01:36:30]

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

[commit-bot: I haz the power, 2017-03-01 01:37:05]

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

[yhirano, 2017-03-01 01:39:20]

https://codereview.chromium.org/2715423003/diff/90001/content/child/resource_...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/2715423003/diff/90001/content/child/resource_...
content/child/resource_dispatcher.cc:420: request_info->download_to_file &&
!it->second->url_loader;
On 2017/03/01 01:29:13, dcheng wrote:
> I think a comment here would be useful: I'm not actually sure why we need to
> check that we have a bound URLLoader here.

Done.

https://codereview.chromium.org/2715423003/diff/90001/content/child/url_loade...
File content/child/url_loader_client_impl.cc (right):

https://codereview.chromium.org/2715423003/diff/90001/content/child/url_loade...
content/child/url_loader_client_impl.cc:32: binding_.Bind(client_ptr,
task_runner_.get());
On 2017/03/01 01:29:13, dcheng wrote:
> Nit: no .get()

Done.

[dcheng, 2017-03-01 01:42:24]

lgtm

[dcheng, 2017-03-01 01:44:25]

(Forgot to send comments)

https://codereview.chromium.org/2715423003/diff/110001/content/child/resource...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/2715423003/diff/110001/content/child/resource...
content/child/resource_dispatcher.cc:420: // release it here.
I think it would be good if this comment also mentioned why it might be null
here: does that mean we were racing on ipc messages, or we never bound the
client, or something else?

[yhirano, 2017-03-01 01:50:44]

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

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

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

[yhirano, 2017-03-01 01:51:47]

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

[commit-bot: I haz the power, 2017-03-01 01:52:18]

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

[yhirano, 2017-03-01 01:53:04]

https://codereview.chromium.org/2715423003/diff/110001/content/child/resource...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/2715423003/diff/110001/content/child/resource...
content/child/resource_dispatcher.cc:420: // release it here.
On 2017/03/01 01:44:24, dcheng wrote:
> I think it would be good if this comment also mentioned why it might be null
> here: does that mean we were racing on ipc messages, or we never bound the
> client, or something else?

Fixed. Is it clear now?

[dcheng, 2017-03-01 01:57:49]

lgtm

https://codereview.chromium.org/2715423003/diff/150001/content/child/resource...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/2715423003/diff/150001/content/child/resource...
content/child/resource_dispatcher.cc:420: // are using Chrome IPC), we should
release it here.
Ah I see. Thanks!

[yhirano, 2017-03-01 02:03:15]

The CQ bit was unchecked by yhirano@chromium.org

[yhirano, 2017-03-01 02:03:17]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2017-03-01 02:03:17]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
yzshen@chromium.org, tzik@chromium.org
Link to the patchset: https://codereview.chromium.org/2715423003/#ps150001
(title: "fix")

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

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 03:44:29]

CQ is committing da patch.

Bot data: {"patchset_id": 150001, "attempt_start_ts": 1488333797271980,
"parent_rev": "44776c7ba1d01c4228149422661e6dec978b4005", "commit_rev":
"6f57b50b31b18fdfbd8cabf7a6709cd7f809f181"}

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

Description was changed from

==========
[Mojo-Loading] Use independent URLLoaderClient

With this CL, various content/ classes start using independent URLLoaderClient
and DownloadedTempFile instead of associated interfaces.

As the blob creation became a sync operation, no message ordering guarantee
is needed for DownloadedTempFile. According to the test results, it seems
the guarantee is not needed for URLLoaderClient, too.

This CL also sets the appropriate task runner (i.e., the loading task runner)
to the URLLoaderClientImpl, which was blocked by the channel-associated
interfaces.

BUG=603396
==========

to

==========
[Mojo-Loading] Use independent URLLoaderClient

With this CL, various content/ classes start using independent URLLoaderClient
and DownloadedTempFile instead of associated interfaces.

As the blob creation became a sync operation, no message ordering guarantee
is needed for DownloadedTempFile. According to the test results, it seems
the guarantee is not needed for URLLoaderClient, too.

This CL also sets the appropriate task runner (i.e., the loading task runner)
to the URLLoaderClientImpl, which was blocked by the channel-associated
interfaces.

BUG=603396

Review-Url: https://codereview.chromium.org/2715423003
Cr-Commit-Position: refs/heads/master@{#453832}
Committed:
https://chromium.googlesource.com/chromium/src/+/6f57b50b31b18fdfbd8cabf7a670...
==========

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

Committed patchset #9 (id:150001) as
https://chromium.googlesource.com/chromium/src/+/6f57b50b31b18fdfbd8cabf7a670...