[LoaderTest] Add unit tests for Document/WorkerThreadableLoader

[Loader] Add unit tests for Document/WorkerThreadableLoader

This CL tests how ThreadableLoaderClient methods are called from
DocumentThreadableLoader and WorkerThreadableLoader, including the cases
where ThreadableLoader is canceled or destructed in the
ThreadableLoaderClient method calls.

This CL ignores a presubmit warning:
    "You should probably be using one of the FrameTestHelpers::(re)load*
    functions instead of serveAsynchronousMockedRequests()"
in ThreadableLoaderTest.cpp, because this is not a frame loading test.

BUG=532364, 515850
Committed: https://crrev.com/c273884cebc427b90d7b989c1210ebb0ffeebfe3
Cr-Commit-Position: refs/heads/master@{#382156}

[hiroshige, 2015-08-05 16:00:26]

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

[hiroshige, 2015-08-05 16:00:27]

PTAL.

[tyoshino (SeeGerritForStatus), 2015-08-06 05:17:53]

Please wrap the CL description to 80 col or less before commit.
https://www.chromium.org/developers/contributing-code#TOC-Change-List-Descrip...

[hiroshige, 2015-08-11 08:09:13]

Done > wrap.

Also I added tests for WorkerThreadableLoader in Patch Set 6.

[hiroshige, 2015-09-15 13:38:34]

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

[commit-bot: I haz the power, 2015-09-15 13:38:46]

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

[commit-bot: I haz the power, 2015-09-15 14:55:15]

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

[commit-bot: I haz the power, 2015-09-15 14:55:15]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[tyoshino (SeeGerritForStatus), 2016-02-18 09:30:13]

Thanks for writing the tests, and sorry for long delay on review and refactoring
work.

Overall, the tests look good. Please rebase and see if they still work with the
refactored DTL when you have time.

[hiroshige, 2016-02-22 22:48:48]

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

[commit-bot: I haz the power, 2016-02-22 22:50:37]

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

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

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

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

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hiroshige, 2016-02-23 02:21:06]

Description was changed from

==========
[Loader] Add unit tests for Document/WorkerThreadableLoader

This CL tests how ThreadableLoaderClient methods are called from
DocumentThreadableLoader and WorkerThreadableLoader, including the cases
where ThreadableLoader is canceled or destructed in the
ThreadableLoaderClient method calls.

This CL also adds a gtest pretty printer for ResourceError.

This CL ignores two presubmit errors:
Ignoring:
  "Streams are highly discouraged.  [readability/streams]"
  in PlatformTestPrinters.cpp.
Because:
  std::ostream is required for gtest pretty printing.

Ignoring:
  "You should be using FrameTestHelpers::pumpPendingRequests() instead of
   serveAsynchronousMockedRequests()" in ThreadableLoaderTestBase.cpp
Because:
  This is not a frame loading test.

BUG=515850, 514547, 532364

Depending on:
Blink-side:
  https://codereview.chromium.org/1264453002/
==========

to

==========
[Loader] Add unit tests for Document/WorkerThreadableLoader

This CL tests how ThreadableLoaderClient methods are called from
DocumentThreadableLoader and WorkerThreadableLoader, including the cases
where ThreadableLoader is canceled or destructed in the
ThreadableLoaderClient method calls.

This CL ignores a presubmit warning:
  "You should be using FrameTestHelpers::pumpPendingRequests() instead of
   serveAsynchronousMockedRequests()" in ThreadableLoaderTestBase.cpp
Because:
  This is not a frame loading test.

BUG=515850, 514547, 532364
==========

[hiroshige, 2016-02-23 02:21:15]

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

[commit-bot: I haz the power, 2016-02-23 02:21:24]

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

[commit-bot: I haz the power, 2016-02-23 03:12:47]

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

[commit-bot: I haz the power, 2016-02-23 03:12:48]

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

[hiroshige, 2016-02-23 18:36:24]

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

[hiroshige, 2016-02-23 18:37:42]

Description was changed from

==========
[Loader] Add unit tests for Document/WorkerThreadableLoader

This CL tests how ThreadableLoaderClient methods are called from
DocumentThreadableLoader and WorkerThreadableLoader, including the cases
where ThreadableLoader is canceled or destructed in the
ThreadableLoaderClient method calls.

This CL ignores a presubmit warning:
  "You should be using FrameTestHelpers::pumpPendingRequests() instead of
   serveAsynchronousMockedRequests()" in ThreadableLoaderTestBase.cpp
Because:
  This is not a frame loading test.

BUG=515850, 514547, 532364
==========

to

==========
[Loader] Add unit tests for Document/WorkerThreadableLoader

This CL tests how ThreadableLoaderClient methods are called from
DocumentThreadableLoader and WorkerThreadableLoader, including the cases
where ThreadableLoader is canceled or destructed in the
ThreadableLoaderClient method calls.

This CL ignores a presubmit warning:
    "You should probably be using one of the FrameTestHelpers::(re)load*
    functions instead of serveAsynchronousMockedRequests()"
in ThreadableLoaderTest.cpp, because this is not a frame loading test.

BUG=515850, 514547, 532364
==========

[commit-bot: I haz the power, 2016-02-23 18:46:25]

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

[commit-bot: I haz the power, 2016-02-23 20:21:42]

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

[commit-bot: I haz the power, 2016-02-23 20:21:43]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[hiroshige, 2016-02-23 20:54:49]

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

[commit-bot: I haz the power, 2016-02-23 20:57:55]

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

[commit-bot: I haz the power, 2016-02-23 23:29:54]

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

[commit-bot: I haz the power, 2016-02-23 23:29:55]

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

[hiroshige, 2016-02-24 20:46:11]

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

[commit-bot: I haz the power, 2016-02-24 20:49:15]

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

[commit-bot: I haz the power, 2016-02-24 22:00:16]

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

[commit-bot: I haz the power, 2016-02-24 22:00:17]

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

[hiroshige, 2016-02-24 22:07:01]

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

[commit-bot: I haz the power, 2016-02-24 22:07:43]

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

[hiroshige, 2016-02-24 23:47:11]

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

[commit-bot: I haz the power, 2016-02-24 23:49:01]

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

[commit-bot: I haz the power, 2016-02-25 00:16:36]

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

[commit-bot: I haz the power, 2016-02-25 00:16:37]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[hiroshige, 2016-02-26 20:33:50]

Patchset #30 (id:560001) has been deleted

[hiroshige, 2016-02-26 21:40:55]

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

[commit-bot: I haz the power, 2016-02-26 21:41:53]

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

[hiroshige, 2016-02-26 22:34:13]

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

[commit-bot: I haz the power, 2016-02-26 22:36:08]

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

[hiroshige, 2016-02-26 23:06:10]

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

[commit-bot: I haz the power, 2016-02-26 23:09:50]

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

[hiroshige, 2016-02-27 00:09:15]

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

[hiroshige, 2016-02-27 00:10:09]

The CQ bit was unchecked by hiroshige@chromium.org

[hiroshige, 2016-02-27 00:13:49]

Patchset #25 (id:460001) has been deleted

[hiroshige, 2016-02-27 00:14:09]

Patchset #23 (id:440001) has been deleted

[hiroshige, 2016-02-27 00:14:26]

Patchset #23 (id:440002) has been deleted

[hiroshige, 2016-02-27 00:14:47]

Patchset #22 (id:420001) has been deleted

[hiroshige, 2016-02-27 00:15:27]

Patchset #28 (id:620001) has been deleted

[hiroshige, 2016-02-27 00:15:48]

Patchset #29 (id:660001) has been deleted

[hiroshige, 2016-02-27 00:16:02]

Patchset #27 (id:600001) has been deleted

[hiroshige, 2016-02-27 00:16:13]

Patchset #24 (id:520001) has been deleted

[hiroshige, 2016-02-27 00:16:47]

Patchset #23 (id:500001) has been deleted

[hiroshige, 2016-02-27 00:17:06]

Patchset #23 (id:540001) has been deleted

[hiroshige, 2016-02-27 00:17:21]

Patchset #23 (id:580001) has been deleted

[hiroshige, 2016-02-27 00:17:39]

Patchset #24 (id:680001) has been deleted

[hiroshige, 2016-02-27 00:32:25]

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

[commit-bot: I haz the power, 2016-02-27 00:33:27]

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

[commit-bot: I haz the power, 2016-02-27 02:40:22]

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

[commit-bot: I haz the power, 2016-02-27 02:40:23]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2016-02-29 16:33:28]

PTAL.

Test failures on Mac was solved by https://codereview.chromium.org/1743783002/.
Test failures on asan bots was solved by using test helpers of WorkerThreadTest.

[tyoshino (SeeGerritForStatus), 2016-03-02 07:57:59]

https://codereview.chromium.org/1257343003/diff/740001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp (left):

https://codereview.chromium.org/1257343003/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp:11: #include
"core/workers/WorkerThreadStartupData.h"
WorkerThreadStartupData is used in the code that remains in this file even after
this CL.

https://codereview.chromium.org/1257343003/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp:17: #include
"testing/gmock/include/gmock/gmock.h"
EXPECT_CALL is used in the code that remains in this file even after this CL.

https://codereview.chromium.org/1257343003/diff/740001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerThreadTestHelper.h (right):

https://codereview.chromium.org/1257343003/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThreadTestHelper.h:13: #include
"testing/gmock/include/gmock/gmock.h"
add PassOwnPtr.h
add Heap.h
add WorkerLoaderProxy.h
add Forward.h
add CurrentTime.h
add SecurityOrigin.h
add WebThreadSupportingGC.h
add v8.h

forward declare KURL

[tyoshino (SeeGerritForStatus), 2016-03-03 14:43:20]

almost lg

nice coverage!

https://codereview.chromium.org/1257343003/diff/740001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp (right):

https://codereview.chromium.org/1257343003/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp:10: #include
"core/loader/ThreadableLoaderClient.h"
also include ThreadableLoader.h

https://codereview.chromium.org/1257343003/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp:27: #include
"testing/gtest/include/gtest/gtest.h"
Add

PassOwnPtr.h
WorkerLoaderProxy.h
OwnPtr.h
RefPtr.h
IntSize.h
CrossThreadCopier.h
Assertions.h
gmock.h
ContentSecurityPolicy.h
ContentSecurityPolicyParsers.h
Vector.h
V8CacheOptions.h
WebThreadSupportingGC.h

forward declare SecurityOrigin

https://codereview.chromium.org/1257343003/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp:283:
ASSERT(m_workerThread->workerGlobalScope()->isWorkerGlobalScope());
OK, but wonder what kind of mistake we want to catch by this.

https://codereview.chromium.org/1257343003/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp:457:
callCheckpoint(1);
is this checkpoint for ensuring that the following expected calls happen after
createLoader() completes?

https://codereview.chromium.org/1257343003/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp:516:
TEST_P(ThreadableLoaderTest, CancelIndidReceiveData)
did -> Did

https://codereview.chromium.org/1257343003/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp:533:
TEST_P(ThreadableLoaderTest, ClearIndidReceiveData)
did -> Did

https://codereview.chromium.org/1257343003/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp:559:
EXPECT_CALL(*client(), didReceiveResourceTiming(_)).Times(AtMost(1));
please explain why the expectation is AtMost(1)

[hiroshige, 2016-03-03 22:47:26]

Patchset #27 (id:760001) has been deleted

[hiroshige, 2016-03-04 01:52:15]

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

[commit-bot: I haz the power, 2016-03-04 01:52:46]

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

[commit-bot: I haz the power, 2016-03-04 03:28:18]

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

[commit-bot: I haz the power, 2016-03-04 03:28:20]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2016-03-08 23:40:58]

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

[hiroshige, 2016-03-08 23:40:59]

+japhet@, could you take a look?

https://codereview.chromium.org/1257343003/diff/740001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp (right):

https://codereview.chromium.org/1257343003/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp:10: #include
"core/loader/ThreadableLoaderClient.h"
On 2016/03/03 14:43:19, tyoshino wrote:
> also include ThreadableLoader.h

It is included in Line 5.

https://codereview.chromium.org/1257343003/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp:27: #include
"testing/gtest/include/gtest/gtest.h"
Added:
> PassOwnPtr.h
> WorkerLoaderProxy.h
> OwnPtr.h
> RefPtr.h
> IntSize.h
> Assertions.h
> gmock.h

I moved the related code to WorkerThreadTestHelper.h and add #include of the
following there:
> ContentSecurityPolicy.h
> ContentSecurityPolicyParsers.h
> Vector.h
> V8CacheOptions.h
> WebThreadSupportingGC.h

> CrossThreadCopier.h
Should we add CrossThreadCopier.h?
In general, I include ThreadSafeFunctional.h for threadSafeBind() but not
CrossThreadCopier.

> forward declare SecurityOrigin
Added include to here and to WorkerThreadTestHelper.h, because just a forward
declaration of SecurityOrigin is not sufficient.

https://codereview.chromium.org/1257343003/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp:283:
ASSERT(m_workerThread->workerGlobalScope()->isWorkerGlobalScope());
On 2016/03/03 14:43:20, tyoshino wrote:
> OK, but wonder what kind of mistake we want to catch by this.

I think I added this ASSERT() from
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
i.e. to ensure ThreadableLoader::create() creates WorkerThreadableLoader.

https://codereview.chromium.org/1257343003/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp:457:
callCheckpoint(1);
On 2016/03/03 14:43:19, tyoshino wrote:
> is this checkpoint for ensuring that the following expected calls happen after
> createLoader() completes?

Yes, i.e. not synchronously in createLoader().

https://codereview.chromium.org/1257343003/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp:516:
TEST_P(ThreadableLoaderTest, CancelIndidReceiveData)
On 2016/03/03 14:43:19, tyoshino wrote:
> did -> Did

Done.

https://codereview.chromium.org/1257343003/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp:533:
TEST_P(ThreadableLoaderTest, ClearIndidReceiveData)
On 2016/03/03 14:43:19, tyoshino wrote:
> did -> Did

Done.

https://codereview.chromium.org/1257343003/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp:559:
EXPECT_CALL(*client(), didReceiveResourceTiming(_)).Times(AtMost(1));
On 2016/03/03 14:43:20, tyoshino wrote:
> please explain why the expectation is AtMost(1)

Because it is called in DocumentThreadableLoader case but not
WorkerThreadableLoader case.

ksakamoto@, you added this callback in
https://codereview.chromium.org/1184403003.
Is this expected to be called also in workers?
If so, I'll fix it and update the tests.

https://codereview.chromium.org/1257343003/diff/740001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp (left):

https://codereview.chromium.org/1257343003/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp:11: #include
"core/workers/WorkerThreadStartupData.h"
On 2016/03/02 07:57:59, tyoshino wrote:
> WorkerThreadStartupData is used in the code that remains in this file even
after
> this CL.

I moved the code to WorkerThreadTestHelper.h so WorkerThreadStartupData.h is no
longer needed here.

https://codereview.chromium.org/1257343003/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp:17: #include
"testing/gmock/include/gmock/gmock.h"
On 2016/03/02 07:57:59, tyoshino wrote:
> EXPECT_CALL is used in the code that remains in this file even after this CL.

Done.

https://codereview.chromium.org/1257343003/diff/740001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerThreadTestHelper.h (right):

https://codereview.chromium.org/1257343003/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThreadTestHelper.h:13: #include
"testing/gmock/include/gmock/gmock.h"
On 2016/03/02 07:57:59, tyoshino wrote:
> add PassOwnPtr.h
> add WorkerLoaderProxy.h
> add SecurityOrigin.h
> add WebThreadSupportingGC.h
> add CurrentTime.h
> add Forward.h
> add v8.h
Added.

> add Heap.h
Handle.h?

> forward declare KURL
Added #include because I added KURL constructor call here.

[tyoshino (SeeGerritForStatus), 2016-03-09 06:26:12]

lgtm

https://codereview.chromium.org/1257343003/diff/740001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp (right):

https://codereview.chromium.org/1257343003/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp:10: #include
"core/loader/ThreadableLoaderClient.h"
On 2016/03/08 at 23:40:59, hiroshige wrote:
> On 2016/03/03 14:43:19, tyoshino wrote:
> > also include ThreadableLoader.h
> 
> It is included in Line 5.

ahh, sorry.

https://codereview.chromium.org/1257343003/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp:27: #include
"testing/gtest/include/gtest/gtest.h"
On 2016/03/08 at 23:40:59, hiroshige wrote:
> Added:
> > PassOwnPtr.h
> > WorkerLoaderProxy.h
> > OwnPtr.h
> > RefPtr.h
> > IntSize.h
> > Assertions.h
> > gmock.h
> 
> I moved the related code to WorkerThreadTestHelper.h and add #include of the
following there:

looks good

> > ContentSecurityPolicy.h
> > ContentSecurityPolicyParsers.h
> > Vector.h
> > V8CacheOptions.h
> > WebThreadSupportingGC.h
> 
> > CrossThreadCopier.h
> Should we add CrossThreadCopier.h?
> In general, I include ThreadSafeFunctional.h for threadSafeBind() but not
CrossThreadCopier.
> 

I suggested that since this file is using AllowCrossThreadAccess() directly.

> > forward declare SecurityOrigin
> Added include to here and to WorkerThreadTestHelper.h, because just a forward
declaration of SecurityOrigin is not sufficient.

ok

https://codereview.chromium.org/1257343003/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp:283:
ASSERT(m_workerThread->workerGlobalScope()->isWorkerGlobalScope());
On 2016/03/08 23:40:59, hiroshige wrote:
> On 2016/03/03 14:43:20, tyoshino wrote:
> > OK, but wonder what kind of mistake we want to catch by this.
> 
> I think I added this ASSERT() from
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> i.e. to ensure ThreadableLoader::create() creates WorkerThreadableLoader.

Ah, I see. I understood the motivation. But this would still look strange for
readers of this unittest without the knowledge of ThreadableLoader::create()
implementation.

Could you please add a comment like:

// ThreadableLoader::create() determines whether it should create
// a DocumentThreadableLoader or WorkerThreadableLoader based on
// isWorkerGlobalScope(). Ensure that we're passing an execution
// context that satisfies this requirement.

https://codereview.chromium.org/1257343003/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp:559:
EXPECT_CALL(*client(), didReceiveResourceTiming(_)).Times(AtMost(1));
On 2016/03/08 23:40:59, hiroshige wrote:
> On 2016/03/03 14:43:20, tyoshino wrote:
> > please explain why the expectation is AtMost(1)
> 
> Because it is called in DocumentThreadableLoader case but not
> WorkerThreadableLoader case.
> 
> ksakamoto@, you added this callback in
> https://codereview.chromium.org/1184403003.
> Is this expected to be called also in workers?
> If so, I'll fix it and update the tests.

Thanks. Please add a short comment explaining that after receiving reply from
ksakamoto@.

https://codereview.chromium.org/1257343003/diff/740001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerThreadTestHelper.h (right):

https://codereview.chromium.org/1257343003/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThreadTestHelper.h:13: #include
"testing/gmock/include/gmock/gmock.h"
On 2016/03/08 23:40:59, hiroshige wrote:
> On 2016/03/02 07:57:59, tyoshino wrote:
> > add PassOwnPtr.h
> > add WorkerLoaderProxy.h
> > add SecurityOrigin.h
> > add WebThreadSupportingGC.h
> > add CurrentTime.h
> > add Forward.h
> > add v8.h
> Added.
> 
> > add Heap.h
> Handle.h?
> 

Right, sorry!

> > forward declare KURL
> Added #include because I added KURL constructor call here.

[hiroshige, 2016-03-09 19:14:27]

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

[hiroshige, 2016-03-09 19:14:28]

Oh, I forgot adding +ksakamoto@ as a reviewer.

ksakamoto@, could you take a look at comments about didReceiveResourceTiming()?
https://codereview.chromium.org/1257343003/diff/740001/third_party/WebKit/Sou...

[Kunihiko Sakamoto, 2016-03-10 02:17:31]

https://codereview.chromium.org/1257343003/diff/740001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp (right):

https://codereview.chromium.org/1257343003/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp:559:
EXPECT_CALL(*client(), didReceiveResourceTiming(_)).Times(AtMost(1));
hiroshige@ is correct, WorkerThreadableLoader doesn't call this.
(This callback is for ResourceTimingInfo plumbing from main thread to
WorkerThreadableLoader.)

[hiroshige, 2016-03-10 03:20:09]

https://codereview.chromium.org/1257343003/diff/740001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp (right):

https://codereview.chromium.org/1257343003/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp:559:
EXPECT_CALL(*client(), didReceiveResourceTiming(_)).Times(AtMost(1));
On 2016/03/10 02:17:31, Kunihiko Sakamoto wrote:
> hiroshige@ is correct, WorkerThreadableLoader doesn't call this.
> (This callback is for ResourceTimingInfo plumbing from main thread to
> WorkerThreadableLoader.)

Thanks! Then I'll make the expectation more strict, i.e.
didReceiveResourceTiming() is called once in the case of DocumenThreadableLoader
only, and add a comment.

[hiroshige, 2016-03-11 19:51:34]

Patchset #29 (id:820001) has been deleted

[hiroshige, 2016-03-11 19:52:14]

https://codereview.chromium.org/1257343003/diff/740001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp (right):

https://codereview.chromium.org/1257343003/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp:283:
ASSERT(m_workerThread->workerGlobalScope()->isWorkerGlobalScope());
On 2016/03/09 06:26:12, tyoshino wrote:
> On 2016/03/08 23:40:59, hiroshige wrote:
> > On 2016/03/03 14:43:20, tyoshino wrote:
> > > OK, but wonder what kind of mistake we want to catch by this.
> > 
> > I think I added this ASSERT() from
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > i.e. to ensure ThreadableLoader::create() creates WorkerThreadableLoader.
> 
> Ah, I see. I understood the motivation. But this would still look strange for
> readers of this unittest without the knowledge of ThreadableLoader::create()
> implementation.
> 
> Could you please add a comment like:
> 
> // ThreadableLoader::create() determines whether it should create
> // a DocumentThreadableLoader or WorkerThreadableLoader based on
> // isWorkerGlobalScope(). Ensure that we're passing an execution
> // context that satisfies this requirement.

Done.

https://codereview.chromium.org/1257343003/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp:559:
EXPECT_CALL(*client(), didReceiveResourceTiming(_)).Times(AtMost(1));
On 2016/03/10 03:20:08, hiroshige wrote:
> On 2016/03/10 02:17:31, Kunihiko Sakamoto wrote:
> > hiroshige@ is correct, WorkerThreadableLoader doesn't call this.
> > (This callback is for ResourceTimingInfo plumbing from main thread to
> > WorkerThreadableLoader.)
> 
> Thanks! Then I'll make the expectation more strict, i.e.
> didReceiveResourceTiming() is called once in the case of
DocumenThreadableLoader
> only, and add a comment.

Done.

[hiroshige, 2016-03-11 19:56:37]

Description was changed from

==========
[Loader] Add unit tests for Document/WorkerThreadableLoader

This CL tests how ThreadableLoaderClient methods are called from
DocumentThreadableLoader and WorkerThreadableLoader, including the cases
where ThreadableLoader is canceled or destructed in the
ThreadableLoaderClient method calls.

This CL ignores a presubmit warning:
    "You should probably be using one of the FrameTestHelpers::(re)load*
    functions instead of serveAsynchronousMockedRequests()"
in ThreadableLoaderTest.cpp, because this is not a frame loading test.

BUG=515850, 514547, 532364
==========

to

==========
[Loader] Add unit tests for Document/WorkerThreadableLoader

This CL tests how ThreadableLoaderClient methods are called from
DocumentThreadableLoader and WorkerThreadableLoader, including the cases
where ThreadableLoader is canceled or destructed in the
ThreadableLoaderClient method calls.

This CL ignores a presubmit warning:
    "You should probably be using one of the FrameTestHelpers::(re)load*
    functions instead of serveAsynchronousMockedRequests()"
in ThreadableLoaderTest.cpp, because this is not a frame loading test.

BUG=532364, 515850
==========

[hiroshige, 2016-03-11 22:45:10]

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

[commit-bot: I haz the power, 2016-03-11 22:45:42]

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

[commit-bot: I haz the power, 2016-03-11 22:48:48]

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

[commit-bot: I haz the power, 2016-03-11 22:48:50]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hiroshige, 2016-03-11 23:21:47]

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

[commit-bot: I haz the power, 2016-03-11 23:22:20]

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

[commit-bot: I haz the power, 2016-03-12 01:12:11]

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

[commit-bot: I haz the power, 2016-03-12 01:12:13]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2016-03-17 18:48:16]

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

[hiroshige, 2016-03-17 18:48:17]

japhet@, could you take a look?

+kinuko@ for moving WorkerThreadTest-related code.

[Nate Chapin, 2016-03-17 22:43:58]

lgtm

[kinuko, 2016-03-18 08:54:30]

lgtm

[hiroshige, 2016-03-18 17:17:48]

The CQ bit was checked by hiroshige@chromium.org

[hiroshige, 2016-03-18 17:17:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
tyoshino@chromium.org
Link to the patchset: https://codereview.chromium.org/1257343003/#ps850001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-03-18 17:18:06]

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

[commit-bot: I haz the power, 2016-03-18 17:19:47]

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

[commit-bot: I haz the power, 2016-03-18 17:19:49]

Try jobs failed on following builders:
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[hiroshige, 2016-03-18 22:05:20]

The CQ bit was checked by hiroshige@chromium.org

[hiroshige, 2016-03-18 22:05:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
tyoshino@chromium.org, kinuko@chromium.org, japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/1257343003/#ps890001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-03-18 22:05:50]

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

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

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

[commit-bot: I haz the power, 2016-03-18 22:09:05]

Try jobs failed on following builders:
  ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hiroshige, 2016-03-18 22:34:55]

The CQ bit was checked by hiroshige@chromium.org

[hiroshige, 2016-03-18 22:34:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
tyoshino@chromium.org, kinuko@chromium.org, japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/1257343003/#ps910001
(title: "Rebase")

[commit-bot: I haz the power, 2016-03-18 22:35:28]

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

[commit-bot: I haz the power, 2016-03-19 00:57:20]

Description was changed from

==========
[Loader] Add unit tests for Document/WorkerThreadableLoader

This CL tests how ThreadableLoaderClient methods are called from
DocumentThreadableLoader and WorkerThreadableLoader, including the cases
where ThreadableLoader is canceled or destructed in the
ThreadableLoaderClient method calls.

This CL ignores a presubmit warning:
    "You should probably be using one of the FrameTestHelpers::(re)load*
    functions instead of serveAsynchronousMockedRequests()"
in ThreadableLoaderTest.cpp, because this is not a frame loading test.

BUG=532364, 515850
==========

to

==========
[Loader] Add unit tests for Document/WorkerThreadableLoader

This CL tests how ThreadableLoaderClient methods are called from
DocumentThreadableLoader and WorkerThreadableLoader, including the cases
where ThreadableLoader is canceled or destructed in the
ThreadableLoaderClient method calls.

This CL ignores a presubmit warning:
    "You should probably be using one of the FrameTestHelpers::(re)load*
    functions instead of serveAsynchronousMockedRequests()"
in ThreadableLoaderTest.cpp, because this is not a frame loading test.

BUG=532364, 515850
==========

[commit-bot: I haz the power, 2016-03-19 00:57:22]

Committed patchset #33 (id:910001)

[commit-bot: I haz the power, 2016-03-19 00:58:20]

Description was changed from

==========
[Loader] Add unit tests for Document/WorkerThreadableLoader

This CL tests how ThreadableLoaderClient methods are called from
DocumentThreadableLoader and WorkerThreadableLoader, including the cases
where ThreadableLoader is canceled or destructed in the
ThreadableLoaderClient method calls.

This CL ignores a presubmit warning:
    "You should probably be using one of the FrameTestHelpers::(re)load*
    functions instead of serveAsynchronousMockedRequests()"
in ThreadableLoaderTest.cpp, because this is not a frame loading test.

BUG=532364, 515850
==========

to

==========
[Loader] Add unit tests for Document/WorkerThreadableLoader

This CL tests how ThreadableLoaderClient methods are called from
DocumentThreadableLoader and WorkerThreadableLoader, including the cases
where ThreadableLoader is canceled or destructed in the
ThreadableLoaderClient method calls.

This CL ignores a presubmit warning:
    "You should probably be using one of the FrameTestHelpers::(re)load*
    functions instead of serveAsynchronousMockedRequests()"
in ThreadableLoaderTest.cpp, because this is not a frame loading test.

BUG=532364, 515850

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

[commit-bot: I haz the power, 2016-03-19 00:58:22]

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