Split the constructor of ThreadableLoader into two methods (ctor and start())

Split the constructor of ThreadableLoader into two methods (ctor and start())

Reasons:
- Basically, it's not good to do much work in a constructor.
- DocumentThreadableLoader may fail synchronously, and then
  m_client->didFail() will be called. The design that m_client is passed
  to the constructor means that the client should be ready for
  callbacks (didFail(), etc.) when calling the constructor. It's better
  if both didFail() call by network thread and the sync one are handled
  by single logic. However, in the sync callback case, the loader
  instance is not yet created. This difference gives us some unexpected
  complexity in the logic to be implemented in didFail().
- Splitting the constructor into a new trivial constructor and start()
  method allows finishing association between the loader and the client
  without worry about failure.

This CL simplifies EventSource code as m_requestInFlight is unnecessary.

start() takes a ResourceRequest as it's good to keep the current design
that we don't hold ResourceRequest in a member variable.

BUG=518855, 514547, 389326
Committed: https://crrev.com/f2a0d7de0ca23cb14148af488c18f8638a3976a8
Cr-Commit-Position: refs/heads/master@{#375537}

[hiroshige, 2015-07-28 11:35:37]

hiroshige@chromium.org changed reviewers:
+ hiroshige@chromium.org

[hiroshige, 2015-07-28 11:35:37]

Thanks for taking this!

Creating a separate crbug issue would be better, to track changes related to
this splitting and for documentation.

https://codereview.chromium.org/1264453002/diff/1/Source/core/loader/Threadab...
File Source/core/loader/ThreadableLoader.h (right):

https://codereview.chromium.org/1264453002/diff/1/Source/core/loader/Threadab...
Source/core/loader/ThreadableLoader.h:128: static PassRefPtr<ThreadableLoader>
create(ExecutionContext&, ThreadableLoaderClient*, const
ThreadableLoaderOptions&, const ResourceLoaderOptions&);
From the CL description:
> The design that m_client is passed
>   to the constructor means that the client should be ready for
>   callbacks (didFail(), etc.) when calling the constructor.
Then should we move the ThreadableLoaderClient* argument to start() to make
explcit that the client's callback is not called from the constructor?

https://codereview.chromium.org/1264453002/diff/1/Source/core/loader/Threadab...
Source/core/loader/ThreadableLoader.h:128: static PassRefPtr<ThreadableLoader>
create(ExecutionContext&, ThreadableLoaderClient*, const
ThreadableLoaderOptions&, const ResourceLoaderOptions&);
What is the reason for moving ResourceRequest& argument to start()? Please
describe it in the CL description.

https://codereview.chromium.org/1264453002/diff/1/Source/modules/fetch/FetchB...
File Source/modules/fetch/FetchBlobDataConsumerHandleTest.cpp (right):

https://codereview.chromium.org/1264453002/diff/1/Source/modules/fetch/FetchB...
Source/modules/fetch/FetchBlobDataConsumerHandleTest.cpp:117:
EXPECT_CALL(*loader, start(_));
saveArg<1>(&request) might fix the unit test failure.

[tyoshino (SeeGerritForStatus), 2015-07-28 12:21:51]

https://codereview.chromium.org/1264453002/diff/1/Source/core/loader/Threadab...
File Source/core/loader/ThreadableLoader.h (right):

https://codereview.chromium.org/1264453002/diff/1/Source/core/loader/Threadab...
Source/core/loader/ThreadableLoader.h:128: static PassRefPtr<ThreadableLoader>
create(ExecutionContext&, ThreadableLoaderClient*, const
ThreadableLoaderOptions&, const ResourceLoaderOptions&);
On 2015/07/28 11:35:37, hiroshige wrote:
> From the CL description:
> > The design that m_client is passed
> >   to the constructor means that the client should be ready for
> >   callbacks (didFail(), etc.) when calling the constructor.
> Then should we move the ThreadableLoaderClient* argument to start() to make
> explcit that the client's callback is not called from the constructor?

Ah, I see. It also works! Maybe we should move almost everything.

https://codereview.chromium.org/1264453002/diff/1/Source/core/loader/Threadab...
Source/core/loader/ThreadableLoader.h:128: static PassRefPtr<ThreadableLoader>
create(ExecutionContext&, ThreadableLoaderClient*, const
ThreadableLoaderOptions&, const ResourceLoaderOptions&);
On 2015/07/28 11:35:37, hiroshige wrote:
> What is the reason for moving ResourceRequest& argument to start()? Please
> describe it in the CL description.

ResourceRequest is not stored in a member variable in DocumentThreadableLoader.
Passing it on construction requires adding a new variable that is used only
between create() and start(). I wanted to avoid that.

https://codereview.chromium.org/1264453002/diff/1/Source/modules/fetch/FetchB...
File Source/modules/fetch/FetchBlobDataConsumerHandleTest.cpp (right):

https://codereview.chromium.org/1264453002/diff/1/Source/modules/fetch/FetchB...
Source/modules/fetch/FetchBlobDataConsumerHandleTest.cpp:117:
EXPECT_CALL(*loader, start(_));
On 2015/07/28 11:35:37, hiroshige wrote:
> saveArg<1>(&request) might fix the unit test failure.

Done.

[tyoshino (SeeGerritForStatus), 2015-07-28 12:22:35]

On 2015/07/28 11:35:37, hiroshige wrote:
> Thanks for taking this!
> 
> Creating a separate crbug issue would be better, to track changes related to
> this splitting and for documentation.

Adding 514547 sounds sufficient to you? A new one dedicated for this work?

[tyoshino (SeeGerritForStatus), 2015-07-28 12:36:55]

https://codereview.chromium.org/1264453002/diff/1/Source/core/loader/Threadab...
File Source/core/loader/ThreadableLoader.h (right):

https://codereview.chromium.org/1264453002/diff/1/Source/core/loader/Threadab...
Source/core/loader/ThreadableLoader.h:128: static PassRefPtr<ThreadableLoader>
create(ExecutionContext&, ThreadableLoaderClient*, const
ThreadableLoaderOptions&, const ResourceLoaderOptions&);
On 2015/07/28 12:21:51, tyoshino wrote:
> On 2015/07/28 11:35:37, hiroshige wrote:
> > From the CL description:
> > > The design that m_client is passed
> > >   to the constructor means that the client should be ready for
> > >   callbacks (didFail(), etc.) when calling the constructor.
> > Then should we move the ThreadableLoaderClient* argument to start() to make
> > explcit that the client's callback is not called from the constructor?
> 
> Ah, I see. It also works! Maybe we should move almost everything.

Hmm, but this needs much work. Can I postpone it?

https://codereview.chromium.org/1264453002/diff/1/Source/core/loader/Threadab...
Source/core/loader/ThreadableLoader.h:128: static PassRefPtr<ThreadableLoader>
create(ExecutionContext&, ThreadableLoaderClient*, const
ThreadableLoaderOptions&, const ResourceLoaderOptions&);
On 2015/07/28 12:21:50, tyoshino wrote:
> On 2015/07/28 11:35:37, hiroshige wrote:
> > What is the reason for moving ResourceRequest& argument to start()? Please
> > describe it in the CL description.
> 
> ResourceRequest is not stored in a member variable in
DocumentThreadableLoader.
> Passing it on construction requires adding a new variable that is used only
> between create() and start(). I wanted to avoid that.

Updated the CL description.

[tyoshino (SeeGerritForStatus), 2015-07-28 14:38:24]

Regarding the change on EventSource, I plan to merge it into
https://codereview.chromium.org/1262593004/.

[hiroshige, 2015-07-30 12:07:21]

Basically looks nice (except for EventSource changes about which I'm not yet so
sure).

> Adding 514547 sounds sufficient to you? A new one dedicated for this work?
Probably creating a new one will be better, because this effort contains actual
code changes (not just documentation as Issue 514547 states) and is specific for
loader creation (corresponding to #2 and #3 in Issue 514547).

https://codereview.chromium.org/1264453002/diff/1/Source/core/loader/Document...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/1264453002/diff/1/Source/core/loader/Document...
Source/core/loader/DocumentThreadableLoader.cpp:78: return loader.release();
Now can we merge the two lines above into one?

https://codereview.chromium.org/1264453002/diff/1/Source/core/loader/Document...
Source/core/loader/DocumentThreadableLoader.cpp:88: , m_sameOriginRequest(false)
optional: Should we add a comment in the header that |m_sameOriginRequest| is
not available on construction and only available after start()?

https://codereview.chromium.org/1264453002/diff/1/Source/core/loader/Document...
Source/core/loader/DocumentThreadableLoader.cpp:92: ,
m_requestContext(WebURLRequest::RequestContextUnspecified)
ditto.

https://codereview.chromium.org/1264453002/diff/1/Source/core/loader/Threadab...
File Source/core/loader/ThreadableLoader.h (right):

https://codereview.chromium.org/1264453002/diff/1/Source/core/loader/Threadab...
Source/core/loader/ThreadableLoader.h:128: static PassRefPtr<ThreadableLoader>
create(ExecutionContext&, ThreadableLoaderClient*, const
ThreadableLoaderOptions&, const ResourceLoaderOptions&);
On 2015/07/28 12:36:54, tyoshino wrote:
> Hmm, but this needs much work. Can I postpone it?
Yes.
Then please add a comment stating that ThreadableLoaderClient* methods are not
called in create(), but can be called synchronously in start().

https://codereview.chromium.org/1264453002/diff/1/Source/core/loader/WorkerTh...
File Source/core/loader/WorkerThreadableLoader.cpp (right):

https://codereview.chromium.org/1264453002/diff/1/Source/core/loader/WorkerTh...
Source/core/loader/WorkerThreadableLoader.cpp:157: if (!m_mainThreadLoader)
When can |m_mainThreadLoader| be null? Please add a comment.

https://codereview.chromium.org/1264453002/diff/1/Source/core/loader/WorkerTh...
File Source/core/loader/WorkerThreadableLoader.h (right):

https://codereview.chromium.org/1264453002/diff/1/Source/core/loader/WorkerTh...
Source/core/loader/WorkerThreadableLoader.h:108: void
mainThreadStart(PassOwnPtr<CrossThreadResourceRequestData>, const
ReferrerPolicy, const String&);
nit: add the name "outgoingReferrer" to the last argument?

https://codereview.chromium.org/1264453002/diff/1/Source/modules/fetch/FetchB...
File Source/modules/fetch/FetchBlobDataConsumerHandle.cpp (right):

https://codereview.chromium.org/1264453002/diff/1/Source/modules/fetch/FetchB...
Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:248: if (url.isEmpty()) {
Please call m_updater->update(createUnexpectedErrorDataConsumerHandle()); here.

https://codereview.chromium.org/1264453002/diff/1/Source/modules/fetch/FetchB...
Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:253: m_loader =
createLoader(executionContext, this);
Please add ASSERT(m_loader) here.

[tyoshino (SeeGerritForStatus), 2015-07-31 08:00:43]

https://codereview.chromium.org/1264453002/diff/1/Source/core/loader/Document...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/1264453002/diff/1/Source/core/loader/Document...
Source/core/loader/DocumentThreadableLoader.cpp:78: return loader.release();
On 2015/07/30 12:07:21, hiroshige wrote:
> Now can we merge the two lines above into one?

Done.

https://codereview.chromium.org/1264453002/diff/1/Source/core/loader/Document...
Source/core/loader/DocumentThreadableLoader.cpp:88: , m_sameOriginRequest(false)
On 2015/07/30 12:07:21, hiroshige wrote:
> optional: Should we add a comment in the header that |m_sameOriginRequest| is
> not available on construction and only available after start()?

Said that start() must always follow create().

https://codereview.chromium.org/1264453002/diff/1/Source/core/loader/Document...
Source/core/loader/DocumentThreadableLoader.cpp:92: ,
m_requestContext(WebURLRequest::RequestContextUnspecified)
On 2015/07/30 12:07:21, hiroshige wrote:
> ditto.

Done.

https://codereview.chromium.org/1264453002/diff/1/Source/core/loader/Threadab...
File Source/core/loader/ThreadableLoader.h (right):

https://codereview.chromium.org/1264453002/diff/1/Source/core/loader/Threadab...
Source/core/loader/ThreadableLoader.h:128: static PassRefPtr<ThreadableLoader>
create(ExecutionContext&, ThreadableLoaderClient*, const
ThreadableLoaderOptions&, const ResourceLoaderOptions&);
On 2015/07/30 12:07:21, hiroshige wrote:
> On 2015/07/28 12:36:54, tyoshino wrote:
> > Hmm, but this needs much work. Can I postpone it?
> Yes.
> Then please add a comment stating that ThreadableLoaderClient* methods are not
> called in create(), but can be called synchronously in start().
> 

Done.

https://codereview.chromium.org/1264453002/diff/1/Source/core/loader/WorkerTh...
File Source/core/loader/WorkerThreadableLoader.cpp (right):

https://codereview.chromium.org/1264453002/diff/1/Source/core/loader/WorkerTh...
Source/core/loader/WorkerThreadableLoader.cpp:157: if (!m_mainThreadLoader)
On 2015/07/30 12:07:21, hiroshige wrote:
> When can |m_mainThreadLoader| be null? Please add a comment.

Good point. As we require start() to be always called after create(), we can
make this ASSERT.

https://codereview.chromium.org/1264453002/diff/1/Source/core/loader/WorkerTh...
File Source/core/loader/WorkerThreadableLoader.h (right):

https://codereview.chromium.org/1264453002/diff/1/Source/core/loader/WorkerTh...
Source/core/loader/WorkerThreadableLoader.h:108: void
mainThreadStart(PassOwnPtr<CrossThreadResourceRequestData>, const
ReferrerPolicy, const String&);
On 2015/07/30 12:07:21, hiroshige wrote:
> nit: add the name "outgoingReferrer" to the last argument?

Done.

https://codereview.chromium.org/1264453002/diff/1/Source/modules/fetch/FetchB...
File Source/modules/fetch/FetchBlobDataConsumerHandle.cpp (right):

https://codereview.chromium.org/1264453002/diff/1/Source/modules/fetch/FetchB...
Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:248: if (url.isEmpty()) {
On 2015/07/30 12:07:21, hiroshige wrote:
> Please call m_updater->update(createUnexpectedErrorDataConsumerHandle());
here.

Good catch! Done

https://codereview.chromium.org/1264453002/diff/1/Source/modules/fetch/FetchB...
Source/modules/fetch/FetchBlobDataConsumerHandle.cpp:253: m_loader =
createLoader(executionContext, this);
On 2015/07/30 12:07:21, hiroshige wrote:
> Please add ASSERT(m_loader) here.

Done.

[tyoshino (SeeGerritForStatus), 2015-07-31 08:01:21]

On 2015/07/30 12:07:21, hiroshige wrote:
> Basically looks nice (except for EventSource changes about which I'm not yet
so
> sure).
> 
> > Adding 514547 sounds sufficient to you? A new one dedicated for this work?
> Probably creating a new one will be better, because this effort contains
actual
> code changes (not just documentation as Issue 514547 states) and is specific
for
> loader creation (corresponding to #2 and #3 in Issue 514547).

Created http://crbug.com/515800

[hiroshige, 2015-08-10 12:54:14]

This CL basically looks fine, but please rebase after related CLs (e.g.
https://codereview.chromium.org/1262593004/) are landed.

[hiroshige, 2015-08-10 15:00:05]

CL description:
- s/DocumentThreadableLoader/ThreadableLoader/g
- I created Issue 518855 for this; could you refer it instead of 515800?

https://codereview.chromium.org/1264453002/diff/40001/Source/core/loader/Thre...
File Source/core/loader/ThreadableLoader.h (right):

https://codereview.chromium.org/1264453002/diff/40001/Source/core/loader/Thre...
Source/core/loader/ThreadableLoader.h:129: // create() must always followed by
start() call.
Please mention that create() never returns null.

[hiroshige, 2015-08-11 09:54:42]

https://codereview.chromium.org/1264453002/diff/40001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/security/mixedContent/insecure-async-post-xhr-blocked.html
(right):

https://codereview.chromium.org/1264453002/diff/40001/LayoutTests/http/tests/...
LayoutTests/http/tests/security/mixedContent/insecure-async-post-xhr-blocked.html:15:
window.open("http://127.0.0.1:8000/security/mixedContent/resources/frame-with-insecure-async-xhr-post.html");
The description says "Opens a HTTPS window" but here we open HTTP window; should
this be window.open("https://127.0.0.1:8443/...")?

[tyoshino (SeeGerritForStatus), 2016-01-25 12:49:51]

Description was changed from

==========
Split the constructor of DocumentThreadableLoader into ctor and start() method

Reasons
- Basically, it's not good to do much work in a constructor.
- DocumentThreadableLoader may fail synchronously, and then
  m_client->didFail() will be called. The design that m_client is passed
  to the constructor means that the client should be ready for
  callbacks (didFail(), etc.) when calling the constructor. It's better
  if both didFail() call by network thread and the sync one are handled
  by single logic. However, in the sync callback case, the loader
  instance is not yet created. This difference gives us some unexpected
  complexity in the logic to be implemented in didFail().
- Splitting the constructor into a new trivial constructor and start()
  method allows finishing association between the loader and the client
  without worry about failure.

This CL simplifies EventSource code as m_requestInFlight is unnecessary.

start() takes a ResourceRequest as it's good to keep the current design
that we don't hold ResourceRequest in a member variable.

BUG=515800,514547,389326
==========

to

==========
Split the constructor of ThreadableLoader into ctor and start() method

Reasons
- Basically, it's not good to do much work in a constructor.
- DocumentThreadableLoader may fail synchronously, and then
  m_client->didFail() will be called. The design that m_client is passed
  to the constructor means that the client should be ready for
  callbacks (didFail(), etc.) when calling the constructor. It's better
  if both didFail() call by network thread and the sync one are handled
  by single logic. However, in the sync callback case, the loader
  instance is not yet created. This difference gives us some unexpected
  complexity in the logic to be implemented in didFail().
- Splitting the constructor into a new trivial constructor and start()
  method allows finishing association between the loader and the client
  without worry about failure.

This CL simplifies EventSource code as m_requestInFlight is unnecessary.

start() takes a ResourceRequest as it's good to keep the current design
that we don't hold ResourceRequest in a member variable.

BUG=518855,514547,389326
==========

[tyoshino (SeeGerritForStatus), 2016-01-25 12:57:51]

Description was changed from

==========
Split the constructor of ThreadableLoader into ctor and start() method

Reasons
- Basically, it's not good to do much work in a constructor.
- DocumentThreadableLoader may fail synchronously, and then
  m_client->didFail() will be called. The design that m_client is passed
  to the constructor means that the client should be ready for
  callbacks (didFail(), etc.) when calling the constructor. It's better
  if both didFail() call by network thread and the sync one are handled
  by single logic. However, in the sync callback case, the loader
  instance is not yet created. This difference gives us some unexpected
  complexity in the logic to be implemented in didFail().
- Splitting the constructor into a new trivial constructor and start()
  method allows finishing association between the loader and the client
  without worry about failure.

This CL simplifies EventSource code as m_requestInFlight is unnecessary.

start() takes a ResourceRequest as it's good to keep the current design
that we don't hold ResourceRequest in a member variable.

BUG=518855,514547,389326
==========

to

==========
Split the constructor of ThreadableLoader into two methods (ctor and start())

Reasons:
- Basically, it's not good to do much work in a constructor.
- DocumentThreadableLoader may fail synchronously, and then
  m_client->didFail() will be called. The design that m_client is passed
  to the constructor means that the client should be ready for
  callbacks (didFail(), etc.) when calling the constructor. It's better
  if both didFail() call by network thread and the sync one are handled
  by single logic. However, in the sync callback case, the loader
  instance is not yet created. This difference gives us some unexpected
  complexity in the logic to be implemented in didFail().
- Splitting the constructor into a new trivial constructor and start()
  method allows finishing association between the loader and the client
  without worry about failure.

This CL simplifies EventSource code as m_requestInFlight is unnecessary.

start() takes a ResourceRequest as it's good to keep the current design
that we don't hold ResourceRequest in a member variable.

BUG=518855,514547,389326
==========

[tyoshino (SeeGerritForStatus), 2016-01-25 13:45:56]

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

[commit-bot: I haz the power, 2016-01-25 13:46:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1264453002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1264453002/120001

[tyoshino (SeeGerritForStatus), 2016-01-25 13:49:09]

- Merged
- Rebased
- Fixed the CL description
  - Replaced DTL with TL in the subject
  - Replaced the bug number
- Added the comment
- Fixed the layout test
- Added Fetch API version of mixed content test
- Refined comments

Now the CQ dry run is running. If it's green when you look at this issue, please
review. Otherwise, don't. I'll fix the build issue before getting your review.

Sorry for long long delay in responding to your comments. Finally I got time to
rework on this...

[tyoshino (SeeGerritForStatus), 2016-01-25 13:49:42]

Description was changed from

==========
Split the constructor of ThreadableLoader into two methods (ctor and start())

Reasons:
- Basically, it's not good to do much work in a constructor.
- DocumentThreadableLoader may fail synchronously, and then
  m_client->didFail() will be called. The design that m_client is passed
  to the constructor means that the client should be ready for
  callbacks (didFail(), etc.) when calling the constructor. It's better
  if both didFail() call by network thread and the sync one are handled
  by single logic. However, in the sync callback case, the loader
  instance is not yet created. This difference gives us some unexpected
  complexity in the logic to be implemented in didFail().
- Splitting the constructor into a new trivial constructor and start()
  method allows finishing association between the loader and the client
  without worry about failure.

This CL simplifies EventSource code as m_requestInFlight is unnecessary.

start() takes a ResourceRequest as it's good to keep the current design
that we don't hold ResourceRequest in a member variable.

BUG=518855,514547,389326
==========

to

==========
Split the constructor of ThreadableLoader into two methods (ctor and start())

Reasons:
- Basically, it's not good to do much work in a constructor.
- DocumentThreadableLoader may fail synchronously, and then
  m_client->didFail() will be called. The design that m_client is passed
  to the constructor means that the client should be ready for
  callbacks (didFail(), etc.) when calling the constructor. It's better
  if both didFail() call by network thread and the sync one are handled
  by single logic. However, in the sync callback case, the loader
  instance is not yet created. This difference gives us some unexpected
  complexity in the logic to be implemented in didFail().
- Splitting the constructor into a new trivial constructor and start()
  method allows finishing association between the loader and the client
  without worry about failure.

This CL simplifies EventSource code as m_requestInFlight is unnecessary.

start() takes a ResourceRequest as it's good to keep the current design
that we don't hold ResourceRequest in a member variable.

BUG=518855,514547,389326
==========

[commit-bot: I haz the power, 2016-01-25 13:56:33]

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

[commit-bot: I haz the power, 2016-01-25 13:56:34]

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

[hiroshige, 2016-01-26 08:44:50]

Thanks for updates! Mostly nits.

https://codereview.chromium.org/1264453002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/security/mixedContent/insecure-async-post-xhr-blocked.html
(right):

https://codereview.chromium.org/1264453002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/mixedContent/insecure-async-post-xhr-blocked.html:9:
var test = async_test("Opens a HTTPS window that loads insecure data via XHR. 
We should trigger a mixed content callback and block the load.");
nit: two consecutive whitespaces in "... data via XHR.  We...".

https://codereview.chromium.org/1264453002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/security/mixedContent/insecure-fetch-blocked.html
(right):

https://codereview.chromium.org/1264453002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/mixedContent/insecure-fetch-blocked.html:9:
var test = async_test("Opens a HTTPS window that loads insecure data via Fetch
API.  We should trigger a mixed content callback and block the load.");
nit: ditto.

https://codereview.chromium.org/1264453002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/security/mixedContent/resources/frame-with-insecure-async-xhr-post.html
(right):

https://codereview.chromium.org/1264453002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/mixedContent/resources/frame-with-insecure-async-xhr-post.html:6:
}
Please add ';'.

https://codereview.chromium.org/1264453002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/ThreadableLoader.h (right):

https://codereview.chromium.org/1264453002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ThreadableLoader.h:145: // - not to ask
the users to handle fail in the constructor and other async
nit: s/ fail / failures / ?

https://codereview.chromium.org/1264453002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ThreadableLoader.h:154: //
|ThreadableLoaderClient| methods.
nit: I'm not sure about usage of |'s. Is there any guidelines how to use |'s?
Should we use |'s for function names with () and class/type names?
(I think I've got No for this question in a review comment but I cannot find the
CL.)

https://codereview.chromium.org/1264453002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h (right):

https://codereview.chromium.org/1264453002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h:94: void
start(const ResourceRequest&, const ReferrerPolicy, const String&);
nit: The third argument is |outgoingReferrer| and it's better not to omit the
name here.

https://codereview.chromium.org/1264453002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h:108: void
mainThreadStart(PassOwnPtr<CrossThreadResourceRequestData>, const
ReferrerPolicy, const String&);
nit: ditto.

https://codereview.chromium.org/1264453002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/EventSource.cpp (left):

https://codereview.chromium.org/1264453002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/EventSource.cpp:433: m_loader = nullptr;
Why do we remove |m_loader = nullptr;|? Is it safe?

https://codereview.chromium.org/1264453002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/FetchFormDataConsumerHandleTest.cpp
(right):

https://codereview.chromium.org/1264453002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/FetchFormDataConsumerHandleTest.cpp:59:
client->didReceiveResponse(0, ResourceResponse(), m_handle.release());
Please call didReceiveResponse() in NoopLoader::start(), since we'll move
ThreadableLoaderClient method calls from create() to start() in this CL.

[hiroshige, 2016-01-26 08:46:29]

Er, I found bots are red... (seems just simple compilation errors)

[tyoshino (SeeGerritForStatus), 2016-01-29 12:36:09]

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

[commit-bot: I haz the power, 2016-01-29 12:36:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1264453002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1264453002/160001

[tyoshino (SeeGerritForStatus), 2016-01-29 12:36:52]

Thanks!

https://codereview.chromium.org/1264453002/diff/40001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/security/mixedContent/insecure-async-post-xhr-blocked.html
(right):

https://codereview.chromium.org/1264453002/diff/40001/LayoutTests/http/tests/...
LayoutTests/http/tests/security/mixedContent/insecure-async-post-xhr-blocked.html:15:
window.open("http://127.0.0.1:8000/security/mixedContent/resources/frame-with-insecure-async-xhr-post.html");
On 2015/08/11 09:54:42, hiroshige wrote:
> The description says "Opens a HTTPS window" but here we open HTTP window;
should
> this be window.open("https://127.0.0.1:8443/...")?

Right! Fixed

https://codereview.chromium.org/1264453002/diff/40001/Source/core/loader/Thre...
File Source/core/loader/ThreadableLoader.h (right):

https://codereview.chromium.org/1264453002/diff/40001/Source/core/loader/Thre...
Source/core/loader/ThreadableLoader.h:129: // create() must always followed by
start() call.
On 2015/08/10 15:00:05, hiroshige wrote:
> Please mention that create() never returns null.

Done.

https://codereview.chromium.org/1264453002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/security/mixedContent/insecure-async-post-xhr-blocked.html
(right):

https://codereview.chromium.org/1264453002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/mixedContent/insecure-async-post-xhr-blocked.html:9:
var test = async_test("Opens a HTTPS window that loads insecure data via XHR. 
We should trigger a mixed content callback and block the load.");
On 2016/01/26 08:44:50, hiroshige wrote:
> nit: two consecutive whitespaces in "... data via XHR.  We...".

Done.

https://codereview.chromium.org/1264453002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/security/mixedContent/insecure-fetch-blocked.html
(right):

https://codereview.chromium.org/1264453002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/mixedContent/insecure-fetch-blocked.html:9:
var test = async_test("Opens a HTTPS window that loads insecure data via Fetch
API.  We should trigger a mixed content callback and block the load.");
On 2016/01/26 08:44:50, hiroshige wrote:
> nit: ditto.

Done.

https://codereview.chromium.org/1264453002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/security/mixedContent/resources/frame-with-insecure-async-xhr-post.html
(right):

https://codereview.chromium.org/1264453002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/mixedContent/resources/frame-with-insecure-async-xhr-post.html:6:
}
On 2016/01/26 08:44:50, hiroshige wrote:
> Please add ';'.

Done.

https://codereview.chromium.org/1264453002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/ThreadableLoader.h (right):

https://codereview.chromium.org/1264453002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ThreadableLoader.h:145: // - not to ask
the users to handle fail in the constructor and other async
On 2016/01/26 08:44:50, hiroshige wrote:
> nit: s/ fail / failures / ?

Right! Fixed.

https://codereview.chromium.org/1264453002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ThreadableLoader.h:154: //
|ThreadableLoaderClient| methods.
On 2016/01/26 08:44:50, hiroshige wrote:
> nit: I'm not sure about usage of |'s. Is there any guidelines how to use |'s?
> Should we use |'s for function names with () and class/type names?
> (I think I've got No for this question in a review comment but I cannot find
the
> CL.)

IIRC, we've never discussed this seriously and formally. As it's clear that
they're identifiers in the code as long as they have "()" or letters
capitalized, I'm ok with removing them. Fixed.

https://codereview.chromium.org/1264453002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h (right):

https://codereview.chromium.org/1264453002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h:94: void
start(const ResourceRequest&, const ReferrerPolicy, const String&);
On 2016/01/26 08:44:50, hiroshige wrote:
> nit: The third argument is |outgoingReferrer| and it's better not to omit the
> name here.

Done.

https://codereview.chromium.org/1264453002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h:108: void
mainThreadStart(PassOwnPtr<CrossThreadResourceRequestData>, const
ReferrerPolicy, const String&);
On 2016/01/26 08:44:50, hiroshige wrote:
> nit: ditto.

Done.

https://codereview.chromium.org/1264453002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/EventSource.cpp (left):

https://codereview.chromium.org/1264453002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/EventSource.cpp:433: m_loader = nullptr;
On 2016/01/26 08:44:50, hiroshige wrote:
> Why do we remove |m_loader = nullptr;|? Is it safe?

I've added this code to close().

https://codereview.chromium.org/1264453002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/FetchFormDataConsumerHandleTest.cpp
(right):

https://codereview.chromium.org/1264453002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/FetchFormDataConsumerHandleTest.cpp:59:
client->didReceiveResponse(0, ResourceResponse(), m_handle.release());
On 2016/01/26 08:44:50, hiroshige wrote:
> Please call didReceiveResponse() in NoopLoader::start(), since we'll move
> ThreadableLoaderClient method calls from create() to start() in this CL.

Oh, good point. Fixed.

[tyoshino (SeeGerritForStatus), 2016-01-29 12:37:56]

https://codereview.chromium.org/1264453002/diff/40001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/security/mixedContent/insecure-async-post-xhr-blocked.html
(right):

https://codereview.chromium.org/1264453002/diff/40001/LayoutTests/http/tests/...
LayoutTests/http/tests/security/mixedContent/insecure-async-post-xhr-blocked.html:15:
window.open("http://127.0.0.1:8000/security/mixedContent/resources/frame-with-insecure-async-xhr-post.html");
On 2016/01/29 12:36:52, tyoshino wrote:
> On 2015/08/11 09:54:42, hiroshige wrote:
> > The description says "Opens a HTTPS window" but here we open HTTP window;
> should
> > this be window.open("https://127.0.0.1:8443/...")?
> 
> Right! Fixed

Oh, this reply had to be published before.

https://codereview.chromium.org/1264453002/diff/40001/Source/core/loader/Thre...
File Source/core/loader/ThreadableLoader.h (right):

https://codereview.chromium.org/1264453002/diff/40001/Source/core/loader/Thre...
Source/core/loader/ThreadableLoader.h:129: // create() must always followed by
start() call.
On 2016/01/29 12:36:52, tyoshino wrote:
> On 2015/08/10 15:00:05, hiroshige wrote:
> > Please mention that create() never returns null.
> 
> Done.

This reply had to be published before.

[commit-bot: I haz the power, 2016-01-29 13:33:39]

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

[commit-bot: I haz the power, 2016-01-29 13:33:40]

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

[tyoshino (SeeGerritForStatus), 2016-01-29 13:40:32]

OK. CQ dry run failed. It's just because I need some replacement for
requestInflight flag. I'll do that next week.

[hiroshige, 2016-02-01 06:57:47]

https://codereview.chromium.org/1264453002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerScriptLoader.cpp (right):

https://codereview.chromium.org/1264453002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerScriptLoader.cpp:105: if (m_failed)
I've just found the code related to |m_failed| can be also cleaned up.
E.g. see the comments in WorkerScriptLoader::notifyError():
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

I think this should be done in a separate CL to keep this CL small and to make
debug easier in case when something wrong should happen.

[tyoshino (SeeGerritForStatus), 2016-02-01 12:48:16]

https://codereview.chromium.org/1264453002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerScriptLoader.cpp (right):

https://codereview.chromium.org/1264453002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerScriptLoader.cpp:105: if (m_failed)
On 2016/02/01 at 06:57:47, hiroshige wrote:
> I've just found the code related to |m_failed| can be also cleaned up.
> E.g. see the comments in WorkerScriptLoader::notifyError():
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> I think this should be done in a separate CL to keep this CL small and to make
debug easier in case when something wrong should happen.

OK. Will do in a separate CL.

[tyoshino (SeeGerritForStatus), 2016-02-01 12:48:41]

Rebased

Split MockThreadableLoader change into a separate CL:
https://codereview.chromium.org/1656673003

[hiroshige, 2016-02-05 07:09:28]

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

[commit-bot: I haz the power, 2016-02-05 07:09:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1264453002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1264453002/220001

[commit-bot: I haz the power, 2016-02-05 07:11:31]

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

[commit-bot: I haz the power, 2016-02-05 07:11:31]

Dry run: Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[hiroshige, 2016-02-05 08:11:54]

lgtm if bots turns green after rebasing after
https://codereview.chromium.org/1656673003/.

(I'm not sure whether we have to add more EXPECT_CALL() for
MockThreadableLoader::start or not, but it will be caught by the bots)

https://codereview.chromium.org/1264453002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/FetchFormDataConsumerHandleTest.cpp
(right):

https://codereview.chromium.org/1264453002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/FetchFormDataConsumerHandleTest.cpp:60:
EXPECT_CALL(*loader, start(_)).Times(1).WillOnce(InvokeWithoutArgs(this,
&LoaderFactory::handleDidReceiveResponse));
nit: |Times(1).WillOnce(...)| looks redundant. Is just |.WillOnce(...)|
sufficient?

[tyoshino (SeeGerritForStatus), 2016-02-09 12:21:43]

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

[commit-bot: I haz the power, 2016-02-09 12:22:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1264453002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1264453002/240001

[commit-bot: I haz the power, 2016-02-09 13:42:04]

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

[commit-bot: I haz the power, 2016-02-09 13:42:04]

Dry run: This issue passed the CQ dry run.

[tyoshino (SeeGerritForStatus), 2016-02-09 14:42:02]

https://codereview.chromium.org/1264453002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/FetchFormDataConsumerHandleTest.cpp
(right):

https://codereview.chromium.org/1264453002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/FetchFormDataConsumerHandleTest.cpp:60:
EXPECT_CALL(*loader, start(_)).Times(1).WillOnce(InvokeWithoutArgs(this,
&LoaderFactory::handleDidReceiveResponse));
On 2016/02/05 at 08:11:53, hiroshige wrote:
> nit: |Times(1).WillOnce(...)| looks redundant. Is just |.WillOnce(...)|
sufficient?

Yeah. We're using the StrictMock, so WillOnce() is sufficient. I thought that
it's insufficient for non strict mocks, but the NiceMock also fails when extra
calls happen.

[tyoshino (SeeGerritForStatus), 2016-02-10 12:01:14]

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

[commit-bot: I haz the power, 2016-02-10 12:03:08]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1264453002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1264453002/280001

[commit-bot: I haz the power, 2016-02-10 15:14:08]

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

[commit-bot: I haz the power, 2016-02-10 15:14:09]

Dry run: This issue passed the CQ dry run.

[tyoshino (SeeGerritForStatus), 2016-02-12 09:51:40]

tyoshino@chromium.org changed reviewers:
+ mkwst@chromium.org

[tyoshino (SeeGerritForStatus), 2016-02-12 09:52:50]

+mkwst

Hi Mike, sorry for long long delay. Finally this CL got chance to proceed. This
is needed to cleanly fix http://crbug.com/389326

Please take a look when you have time. Thanks.

[Mike West, 2016-02-15 13:08:20]

I think this looks pretty reasonable; LGTM, and thank you for the fix.

[tyoshino (SeeGerritForStatus), 2016-02-16 07:39:32]

The CQ bit was checked by tyoshino@chromium.org

[tyoshino (SeeGerritForStatus), 2016-02-16 07:39:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
hiroshige@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1264453002/#ps300001
(title: "Rebase")

[commit-bot: I haz the power, 2016-02-16 07:40:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1264453002/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1264453002/300001

[tyoshino (SeeGerritForStatus), 2016-02-16 07:53:49]

The CQ bit was checked by tyoshino@chromium.org

[tyoshino (SeeGerritForStatus), 2016-02-16 07:53:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
hiroshige@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1264453002/#ps320001
(title: "Removed pipes around non-variables in a comment")

[commit-bot: I haz the power, 2016-02-16 07:54:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1264453002/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1264453002/320001

[commit-bot: I haz the power, 2016-02-16 09:04:46]

Description was changed from

==========
Split the constructor of ThreadableLoader into two methods (ctor and start())

Reasons:
- Basically, it's not good to do much work in a constructor.
- DocumentThreadableLoader may fail synchronously, and then
  m_client->didFail() will be called. The design that m_client is passed
  to the constructor means that the client should be ready for
  callbacks (didFail(), etc.) when calling the constructor. It's better
  if both didFail() call by network thread and the sync one are handled
  by single logic. However, in the sync callback case, the loader
  instance is not yet created. This difference gives us some unexpected
  complexity in the logic to be implemented in didFail().
- Splitting the constructor into a new trivial constructor and start()
  method allows finishing association between the loader and the client
  without worry about failure.

This CL simplifies EventSource code as m_requestInFlight is unnecessary.

start() takes a ResourceRequest as it's good to keep the current design
that we don't hold ResourceRequest in a member variable.

BUG=518855,514547,389326
==========

to

==========
Split the constructor of ThreadableLoader into two methods (ctor and start())

Reasons:
- Basically, it's not good to do much work in a constructor.
- DocumentThreadableLoader may fail synchronously, and then
  m_client->didFail() will be called. The design that m_client is passed
  to the constructor means that the client should be ready for
  callbacks (didFail(), etc.) when calling the constructor. It's better
  if both didFail() call by network thread and the sync one are handled
  by single logic. However, in the sync callback case, the loader
  instance is not yet created. This difference gives us some unexpected
  complexity in the logic to be implemented in didFail().
- Splitting the constructor into a new trivial constructor and start()
  method allows finishing association between the loader and the client
  without worry about failure.

This CL simplifies EventSource code as m_requestInFlight is unnecessary.

start() takes a ResourceRequest as it's good to keep the current design
that we don't hold ResourceRequest in a member variable.

BUG=518855,514547,389326
==========

[commit-bot: I haz the power, 2016-02-16 09:04:47]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2016-02-16 22:51:52]

Description was changed from

==========
Split the constructor of ThreadableLoader into two methods (ctor and start())

Reasons:
- Basically, it's not good to do much work in a constructor.
- DocumentThreadableLoader may fail synchronously, and then
  m_client->didFail() will be called. The design that m_client is passed
  to the constructor means that the client should be ready for
  callbacks (didFail(), etc.) when calling the constructor. It's better
  if both didFail() call by network thread and the sync one are handled
  by single logic. However, in the sync callback case, the loader
  instance is not yet created. This difference gives us some unexpected
  complexity in the logic to be implemented in didFail().
- Splitting the constructor into a new trivial constructor and start()
  method allows finishing association between the loader and the client
  without worry about failure.

This CL simplifies EventSource code as m_requestInFlight is unnecessary.

start() takes a ResourceRequest as it's good to keep the current design
that we don't hold ResourceRequest in a member variable.

BUG=518855,514547,389326
==========

to

==========
Split the constructor of ThreadableLoader into two methods (ctor and start())

Reasons:
- Basically, it's not good to do much work in a constructor.
- DocumentThreadableLoader may fail synchronously, and then
  m_client->didFail() will be called. The design that m_client is passed
  to the constructor means that the client should be ready for
  callbacks (didFail(), etc.) when calling the constructor. It's better
  if both didFail() call by network thread and the sync one are handled
  by single logic. However, in the sync callback case, the loader
  instance is not yet created. This difference gives us some unexpected
  complexity in the logic to be implemented in didFail().
- Splitting the constructor into a new trivial constructor and start()
  method allows finishing association between the loader and the client
  without worry about failure.

This CL simplifies EventSource code as m_requestInFlight is unnecessary.

start() takes a ResourceRequest as it's good to keep the current design
that we don't hold ResourceRequest in a member variable.

BUG=518855,514547,389326

Committed: https://crrev.com/f2a0d7de0ca23cb14148af488c18f8638a3976a8
Cr-Commit-Position: refs/heads/master@{#375537}
==========

[commit-bot: I haz the power, 2016-02-16 22:51:53]

Patchset 17 (id:??) landed as
https://crrev.com/f2a0d7de0ca23cb14148af488c18f8638a3976a8
Cr-Commit-Position: refs/heads/master@{#375537}