Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1130433002:
Adding task runner/ message loop to tests that use IsolateHolder. (Closed)
DescriptionAdding task runner/ message loop to tests that use IsolateHolder.
For adding v8 heap memory dump provider, the gin::IsolateHolder needs
to have a task runner at initialization. The tests which don't
initiallize task runner when initialization are changed in this CL to
have either the message loop initializaed earlier or a dummy task
runner. Refer crrev.com/1088683003 for the dump provider.
BUG=476013
Committed: https://crrev.com/54d19441b064ae06604c6a26c3863a091a8bb8bb
Cr-Commit-Position: refs/heads/master@{#328531}
Patch Set 1 #Patch Set 2 : #
Total comments: 20
Patch Set 3 : Addressed comments. #
Total comments: 1
Patch Set 4 : Removing comment. #Patch Set 5 : Fixing mac build. #
Messages
Total messages: 23 (4 generated)
ssid@chromium.org changed reviewers: + primiano@chromium.org, skyostil@chromium.org
PTAL, fixing the tests.
Thanks, some comments below. https://codereview.chromium.org/1130433002/diff/20001/content/test/test_blink... File content/test/test_blink_web_unit_test_support.cc (right): https://codereview.chromium.org/1130433002/diff/20001/content/test/test_blink... content/test/test_blink_web_unit_test_support.cc:10: #include "base/message_loop/message_loop.h" Do you need this for anything? https://codereview.chromium.org/1130433002/diff/20001/content/test/test_blink... content/test/test_blink_web_unit_test_support.cc:67: // Always returns true to avoid triggering DCHECKs. Could you make this do the right thing instead? Some call sites change their behavior based on what this method returns. https://codereview.chromium.org/1130433002/diff/20001/content/test/test_blink... content/test/test_blink_web_unit_test_support.cc:93: scoped_ptr<base::ThreadTaskRunnerHandle> handle; nit: |handle| is a little too nonspecific for a variable name I think. https://codereview.chromium.org/1130433002/diff/20001/content/test/test_blink... content/test/test_blink_web_unit_test_support.cc:103: // loop which is not known upfront and each test suite that uses thread nit: "posted to this handle" => "posted to this task runner" The final sentence is a little hard to read. How about: The message loop is not created before this initialization because some tests need specific kinds of message loops, and their types are not known upfront. Some tests also create their own thread bundles and message loops, and doing the same in TestBlinkWebUnitTestSupport would introduce a conflict. https://codereview.chromium.org/1130433002/diff/20001/content/test/test_blink... content/test/test_blink_web_unit_test_support.cc:111: if (dummy_task_runner.get()) { There's no need to reset the task explicitly is there? Please mention in the comment though that this task runner will only exist within the scope of this function. https://codereview.chromium.org/1130433002/diff/20001/gin/modules/module_regi... File gin/modules/module_registry_unittest.cc (left): https://codereview.chromium.org/1130433002/diff/20001/gin/modules/module_regi... gin/modules/module_registry_unittest.cc:28: base::MessageLoop message_loop; Why did we need to remove this? https://codereview.chromium.org/1130433002/diff/20001/gin/modules/timer_unitt... File gin/modules/timer_unittest.cc (right): https://codereview.chromium.org/1130433002/diff/20001/gin/modules/timer_unitt... gin/modules/timer_unittest.cc:68: base::MessageLoop::current()->PostDelayedTask( Please use ThreadTaskRunnerHandle::Get() instead of MessageLoop::current() (the Post*Task functions on MessageLoop are deprecated).
Just some another couple of comments on top of Sami's ones. https://codereview.chromium.org/1130433002/diff/20001/content/test/test_blink... File content/test/test_blink_web_unit_test_support.cc (right): https://codereview.chromium.org/1130433002/diff/20001/content/test/test_blink... content/test/test_blink_web_unit_test_support.cc:111: if (dummy_task_runner.get()) { Nit: just if (dummy_task_runner), scoped_refptr is already boolean-testable. On 2015/05/05 13:55:54, Sami wrote: > There's no need to reset the task explicitly is there? UH? I think he actually needs to reset the TLS slot (by resetting dummy_task_runner_handle) and to destroy the dummy_task_runner itself, right? https://codereview.chromium.org/1130433002/diff/20001/gin/modules/timer_unitt... File gin/modules/timer_unittest.cc (right): https://codereview.chromium.org/1130433002/diff/20001/gin/modules/timer_unitt... gin/modules/timer_unittest.cc:68: base::MessageLoop::current()->PostDelayedTask( On 2015/05/05 13:55:54, Sami wrote: > Please use ThreadTaskRunnerHandle::Get() instead of MessageLoop::current() (the > Post*Task functions on MessageLoop are deprecated). Is this going to use the DummyTaskRunner? If so, isn't this going to hit the DCHECK?
https://codereview.chromium.org/1130433002/diff/20001/content/test/test_blink... File content/test/test_blink_web_unit_test_support.cc (right): https://codereview.chromium.org/1130433002/diff/20001/content/test/test_blink... content/test/test_blink_web_unit_test_support.cc:111: if (dummy_task_runner.get()) { On 2015/05/05 14:26:51, Primiano Tucci wrote: > Nit: just if (dummy_task_runner), scoped_refptr is already boolean-testable. > > On 2015/05/05 13:55:54, Sami wrote: > > There's no need to reset the task explicitly is there? > UH? I think he actually needs to reset the TLS slot (by resetting > dummy_task_runner_handle) and to destroy the dummy_task_runner itself, right? I meant the destructors of both variables would automatically do that when they go out of scope of this function. I don't see any harm in leaving the task runner around for a few more lines of initialization. https://codereview.chromium.org/1130433002/diff/20001/gin/modules/timer_unitt... File gin/modules/timer_unittest.cc (right): https://codereview.chromium.org/1130433002/diff/20001/gin/modules/timer_unitt... gin/modules/timer_unittest.cc:68: base::MessageLoop::current()->PostDelayedTask( On 2015/05/05 14:26:51, Primiano Tucci wrote: > On 2015/05/05 13:55:54, Sami wrote: > > Please use ThreadTaskRunnerHandle::Get() instead of MessageLoop::current() > (the > > Post*Task functions on MessageLoop are deprecated). > > Is this going to use the DummyTaskRunner? If so, isn't this going to hit the > DCHECK? Actually not, because you can't have both a MessageLoop and a DummyTaskRunner active at the same time (they both DCHECK that no-one else is using the same TLS handle).
https://codereview.chromium.org/1130433002/diff/20001/gin/test/v8_test.h File gin/test/v8_test.h (right): https://codereview.chromium.org/1130433002/diff/20001/gin/test/v8_test.h#newc... gin/test/v8_test.h:32: base::MessageLoop message_loop; Please move this first since I'm guess this should be constructed first and destroyed last. Also add _.
Made changes, PTAL. https://codereview.chromium.org/1130433002/diff/20001/content/test/test_blink... File content/test/test_blink_web_unit_test_support.cc (right): https://codereview.chromium.org/1130433002/diff/20001/content/test/test_blink... content/test/test_blink_web_unit_test_support.cc:10: #include "base/message_loop/message_loop.h" On 2015/05/05 13:55:54, Sami wrote: > Do you need this for anything? Done. https://codereview.chromium.org/1130433002/diff/20001/content/test/test_blink... content/test/test_blink_web_unit_test_support.cc:67: // Always returns true to avoid triggering DCHECKs. On 2015/05/05 13:55:54, Sami wrote: > Could you make this do the right thing instead? Some call sites change their > behavior based on what this method returns. Done. https://codereview.chromium.org/1130433002/diff/20001/content/test/test_blink... content/test/test_blink_web_unit_test_support.cc:93: scoped_ptr<base::ThreadTaskRunnerHandle> handle; On 2015/05/05 13:55:54, Sami wrote: > nit: |handle| is a little too nonspecific for a variable name I think. Done. https://codereview.chromium.org/1130433002/diff/20001/content/test/test_blink... content/test/test_blink_web_unit_test_support.cc:103: // loop which is not known upfront and each test suite that uses thread On 2015/05/05 13:55:54, Sami wrote: > nit: "posted to this handle" => "posted to this task runner" > > The final sentence is a little hard to read. How about: > > The message loop is not created before this initialization because some tests > need specific kinds of message loops, and their types are not known upfront. > Some tests also create their own thread bundles and message loops, and doing the > same in TestBlinkWebUnitTestSupport would introduce a conflict. Done. https://codereview.chromium.org/1130433002/diff/20001/content/test/test_blink... content/test/test_blink_web_unit_test_support.cc:111: if (dummy_task_runner.get()) { On 2015/05/05 14:35:46, Sami wrote: > On 2015/05/05 14:26:51, Primiano Tucci wrote: > > Nit: just if (dummy_task_runner), scoped_refptr is already boolean-testable. > > > > On 2015/05/05 13:55:54, Sami wrote: > > > There's no need to reset the task explicitly is there? > > UH? I think he actually needs to reset the TLS slot (by resetting > > dummy_task_runner_handle) and to destroy the dummy_task_runner itself, right? > > I meant the destructors of both variables would automatically do that when they > go out of scope of this function. I don't see any harm in leaving the task > runner around for a few more lines of initialization. Done. https://codereview.chromium.org/1130433002/diff/20001/gin/modules/module_regi... File gin/modules/module_registry_unittest.cc (left): https://codereview.chromium.org/1130433002/diff/20001/gin/modules/module_regi... gin/modules/module_registry_unittest.cc:28: base::MessageLoop message_loop; On 2015/05/05 13:55:54, Sami wrote: > Why did we need to remove this? This is removed from here because we already initialize message loop in v8/include/v8.h : V8Test, which is using this class. https://codereview.chromium.org/1130433002/diff/20001/gin/modules/timer_unitt... File gin/modules/timer_unittest.cc (right): https://codereview.chromium.org/1130433002/diff/20001/gin/modules/timer_unitt... gin/modules/timer_unittest.cc:68: base::MessageLoop::current()->PostDelayedTask( On 2015/05/05 14:35:46, Sami wrote: > On 2015/05/05 14:26:51, Primiano Tucci wrote: > > On 2015/05/05 13:55:54, Sami wrote: > > > Please use ThreadTaskRunnerHandle::Get() instead of MessageLoop::current() > > (the > > > Post*Task functions on MessageLoop are deprecated). > > > > Is this going to use the DummyTaskRunner? If so, isn't this going to hit the > > DCHECK? > > Actually not, because you can't have both a MessageLoop and a DummyTaskRunner > active at the same time (they both DCHECK that no-one else is using the same TLS > handle). No, this change here is not related to the dummy task runner added. The TestBlinkWebUnitTestSupport does not use this test suite. This is part of gin_unittests, which uses real message loop. I added in same CL since both are fixing tests and doing similar changes. https://codereview.chromium.org/1130433002/diff/20001/gin/test/v8_test.h File gin/test/v8_test.h (right): https://codereview.chromium.org/1130433002/diff/20001/gin/test/v8_test.h#newc... gin/test/v8_test.h:32: base::MessageLoop message_loop; On 2015/05/05 15:10:26, Sami wrote: > Please move this first since I'm guess this should be constructed first and > destroyed last. Also add _. Done.
primiano@chromium.org changed reviewers: + jochen@chromium.org
Non-owner LGTM. +jochen
Thanks, lgtm with a nit. https://codereview.chromium.org/1130433002/diff/40001/content/test/test_blink... File content/test/test_blink_web_unit_test_support.cc (right): https://codereview.chromium.org/1130433002/diff/40001/content/test/test_blink... content/test/test_blink_web_unit_test_support.cc:67: // Always returns true to avoid triggering DCHECKs. This comment isn't accurate anymore.
lgtm
The CQ bit was checked by ssid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/1130433002/#ps60001 (title: "Removing comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1130433002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/54d19441b064ae06604c6a26c3863a091a8bb8bb Cr-Commit-Position: refs/heads/master@{#328531}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/1129863002/ by jochen@chromium.org. The reason for reverting is: speculative revert, gin unittests doesn't compile on mac builders anymore.
Message was sent while issue was closed.
On 2015/05/06 15:09:06, jochen wrote: > A revert of this CL (patchset #4 id:60001) has been created in > https://codereview.chromium.org/1129863002/ by mailto:jochen@chromium.org. > > The reason for reverting is: speculative revert, gin unittests doesn't compile > on mac builders anymore. Yeah, base/message_loop/message_loop.h ends up including base/time/time.h -> (MAC) CoreFoundation/CoreFoundation.h -> CFBase.h which typedefs Boolean = unsigned char, which conflicts with v8::Boolean. The easy resolution seems to be moving the "using v8::Boolean" in converter_unittest.cc inside "namespace gin {" The truth is that base/time/time.h should not leak that Boolean typedef (and there are a bunch of other collisions), but is not clear to me how to prevent that. (or well, I have something in mind, but is even more gross, in essence the same trick we did here: https://code.google.com/p/chromium/codesearch#chromium/src/breakpad/src/commo...)
On 2015/05/06 17:17:22, Primiano Tucci wrote: > On 2015/05/06 15:09:06, jochen wrote: > > A revert of this CL (patchset #4 id:60001) has been created in > > https://codereview.chromium.org/1129863002/ by mailto:jochen@chromium.org. > > > > The reason for reverting is: speculative revert, gin unittests doesn't compile > > on mac builders anymore. > > Yeah, base/message_loop/message_loop.h ends up including base/time/time.h -> > (MAC) CoreFoundation/CoreFoundation.h -> CFBase.h which typedefs Boolean = > unsigned char, which conflicts with v8::Boolean. > > The easy resolution seems to be moving the "using v8::Boolean" in > converter_unittest.cc inside "namespace gin {" > The truth is that base/time/time.h should not leak that Boolean typedef (and > there are a bunch of other collisions), but is not clear to me how to prevent > that. > (or well, I have something in mind, but is even more gross, in essence the same > trick we did here: > https://code.google.com/p/chromium/codesearch#chromium/src/breakpad/src/commo...) Moved the using statements, PTAL.
> Moved the using statements, PTAL. We don't re-use the same CL issue for multiple commits. Please upload a new CL entitled Reland of "Adding task runner/ message loop to tests that use IsolateHolder." Reason of reland: fixed failing unittests. <original message here> In the new CL, it's also desirable that you create two patchsets. The first one should be identical to the one that got reverted (i.e. Patchset 4 of this CL), and the second one should have the extra changes for the fix (i.e. Patchset 5 of this CL), so that it's easy for reviewers to see your extra changes and stamp.
On 2015/05/07 08:22:16, Primiano Tucci wrote: > > Moved the using statements, PTAL. > > We don't re-use the same CL issue for multiple commits. > Please upload a new CL entitled > > Reland of "Adding task runner/ message loop to tests that use IsolateHolder." > > Reason of reland: fixed failing unittests. > <original message here> > > In the new CL, it's also desirable that you create two patchsets. The first one > should be identical to the one that got reverted (i.e. Patchset 4 of this CL), > and the second one should have the extra changes for the fix (i.e. Patchset 5 of > this CL), so that it's easy for reviewers to see your extra changes and stamp. I added new CL. https://codereview.chromium.org/1129873002/ Sami told me I could use the same cl, so I reopened this.
Message was sent while issue was closed.
> Sami told me I could use the same cl, so I reopened this. There was some prior discussion on chromium-dev in [1] and [2] The short version is that is not forbidden to recycle the CL, but might be tricky to get it right. Starting a new CL is IMHO, conceptually, simpler. [1] https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/reusin... [2] https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/reusin...
Message was sent while issue was closed.
On 2015/05/07 09:21:28, Primiano Tucci wrote: > > Sami told me I could use the same cl, so I reopened this. > > There was some prior discussion on chromium-dev in [1] and [2] > > The short version is that is not forbidden to recycle the CL, but might be > tricky to get it right. Starting a new CL is IMHO, conceptually, simpler. > > [1] > https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/reusin... > [2] > https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/reusin... My feeling is that reusing CLs is better because it shows you the history of a patch more easily. But, as was also apparent in the linked thread, people have different opinions and both ways are acceptable. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
