|
|
Created:
5 years, 4 months ago by hiroshige Modified:
4 years, 9 months ago CC:
blink-reviews, gavinp+loader_chromium.org, Nate Chapin, tyoshino+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[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}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : Adding more tests. #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : Add WorkerThreadableLoaderTest #Patch Set 7 : Revert ThreadableLoaderClientWrapper.h changes #Patch Set 8 : Fix test failure in WorkerThreadableLoaderTest.ClearInDidFailInStart #Patch Set 9 : Rebase. #Patch Set 10 : Rebase. #Patch Set 11 : Rebase. #Patch Set 12 : Rebase. #Patch Set 13 : Add more tests - temp. #Patch Set 14 : Fix rebase error. #Patch Set 15 : Add more tests on ManualRedirect. #Patch Set 16 : Add checks of URLs. #Patch Set 17 : Preparing to rebase... #Patch Set 18 : Rebase. #Patch Set 19 : Rebase. #Patch Set 20 : Bot failure fix. #Patch Set 21 : Set LoadTiming #Patch Set 22 : Rebase #Patch Set 23 : #Patch Set 24 : Rebase. #Patch Set 25 : Rebase. #Patch Set 26 : Rebase, #
Total comments: 29
Patch Set 27 : Rebase, Reflect comments, move more common code to Helper.h #Patch Set 28 : Rebase. #Patch Set 29 : Reflect comments #Patch Set 30 : Rebase. #Patch Set 31 : #Patch Set 32 : Rebase. #Patch Set 33 : Rebase #Dependent Patchsets: Messages
Total messages: 109 (60 generated)
hiroshige@chromium.org changed reviewers: + tyoshino@chromium.org
PTAL.
Please wrap the CL description to 80 col or less before commit. https://www.chromium.org/developers/contributing-code#TOC-Change-List-Descrip...
Done > wrap. Also I added tests for WorkerThreadableLoader in Patch Set 6.
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
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.
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
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 ==========
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
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 ==========
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
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
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
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
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...)
Patchset #30 (id:560001) has been deleted
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
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
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
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
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
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
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
The CQ bit was unchecked by hiroshige@chromium.org
Patchset #25 (id:460001) has been deleted
Patchset #23 (id:440001) has been deleted
Patchset #23 (id:440002) has been deleted
Patchset #22 (id:420001) has been deleted
Patchset #28 (id:620001) has been deleted
Patchset #29 (id:660001) has been deleted
Patchset #27 (id:600001) has been deleted
Patchset #24 (id:520001) has been deleted
Patchset #23 (id:500001) has been deleted
Patchset #23 (id:540001) has been deleted
Patchset #23 (id:580001) has been deleted
Patchset #24 (id:680001) has been deleted
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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.
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
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)
Patchset #27 (id:760001) has been deleted
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
hiroshige@chromium.org changed reviewers: + japhet@chromium.org
+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.
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@chromium.org changed reviewers: + ksakamoto@chromium.org
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...
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.)
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.
Patchset #29 (id:820001) has been deleted
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.
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 ==========
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
hiroshige@chromium.org changed reviewers: + kinuko@chromium.org
japhet@, could you take a look? +kinuko@ for moving WorkerThreadTest-related code.
lgtm
lgtm
The CQ bit was checked by hiroshige@chromium.org
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.")
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
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by hiroshige@chromium.org
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.")
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
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by hiroshige@chromium.org
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")
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
Message was sent while issue was closed.
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 ==========
Message was sent while issue was closed.
Committed patchset #33 (id:910001)
Message was sent while issue was closed.
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} ==========
Message was sent while issue was closed.
Patchset 33 (id:??) landed as https://crrev.com/c273884cebc427b90d7b989c1210ebb0ffeebfe3 Cr-Commit-Position: refs/heads/master@{#382156} |