Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
878413002:
Wire the scheduler up in TestBlinkWebUnitTestSupport (Closed)
|
Created:
5 years, 11 months ago by alex clarke (OOO till 29th) Modified:
5 years, 10 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, rmcilroy_google, picksi1, jochen (gone - plz use gerrit) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWire the scheduler up in TestBlinkWebUnitTestSupport
This is needed to make the webkit_unit_tests pass when the HTML parseing
tasks are posted on the loading task queue. Required for patch
https://codereview.chromium.org/876623002
BUG=391005
Committed: https://crrev.com/85df08a96469963c49b0903c40eddb09f4f80d35
Cr-Commit-Position: refs/heads/master@{#314002}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Sami's idea #
Total comments: 2
Patch Set 3 : Fix componant builds #Patch Set 4 : Fix windows build? #Patch Set 5 : Use Pass #Patch Set 6 : Lazily create the WebSchedulerImpl and RendererScheduler if needed #
Messages
Total messages: 34 (11 generated)
alexclarke@chromium.org changed reviewers: + jochen@chromium.org, skyostil@chromium.org
Turns out I need to wire the scheduler up in TestBlinkWebUnitTestSupport before I can submit https://codereview.chromium.org/876623002
https://codereview.chromium.org/878413002/diff/1/content/test/test_blink_web_... File content/test/test_blink_web_unit_test_support.h (right): https://codereview.chromium.org/878413002/diff/1/content/test/test_blink_web_... content/test/test_blink_web_unit_test_support.h:37: explicit TestBlinkWebUnitTestSupport(RendererScheduler* renderer_scheduler); Could you make this a scoped_ptr so the ownership model is self-documenting?
https://codereview.chromium.org/878413002/diff/1/content/test/test_blink_web_... File content/test/test_blink_web_unit_test_support.h (right): https://codereview.chromium.org/878413002/diff/1/content/test/test_blink_web_... content/test/test_blink_web_unit_test_support.h:37: explicit TestBlinkWebUnitTestSupport(RendererScheduler* renderer_scheduler); On 2015/01/28 12:16:27, Sami (OoO) wrote: > Could you make this a scoped_ptr so the ownership model is self-documenting? Done.
Thanks, non-owner lgtm.
alexclarke@chromium.org changed reviewers: + sievers@chromium.org - jochen@chromium.org
+sievers@ Jochen looks snowed under with reviews, would you mind taking a look? Thanks!
lgtm https://codereview.chromium.org/878413002/diff/20001/content/test/test_blink_... File content/test/test_blink_web_unit_test_support.cc (right): https://codereview.chromium.org/878413002/diff/20001/content/test/test_blink_... content/test/test_blink_web_unit_test_support.cc:52: renderer_scheduler_(renderer_scheduler.release()), nit: drop '.release()', should be able to construct scoped_ptr from scoped_ptr.
https://codereview.chromium.org/878413002/diff/20001/content/test/test_blink_... File content/test/test_blink_web_unit_test_support.cc (right): https://codereview.chromium.org/878413002/diff/20001/content/test/test_blink_... content/test/test_blink_web_unit_test_support.cc:52: renderer_scheduler_(renderer_scheduler.release()), On 2015/01/28 18:35:22, sievers wrote: > nit: drop '.release()', should be able to construct scoped_ptr from scoped_ptr. Unfortunately I get a compile error if I try that: error: field of type 'scoped_ptr<content::RendererScheduler>' has private copy constructor
The CQ bit was checked by alexclarke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/878413002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by alexclarke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/878413002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by alexclarke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/878413002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
On 2015/01/29 11:29:19, alexclarke1 wrote: > https://codereview.chromium.org/878413002/diff/20001/content/test/test_blink_... > File content/test/test_blink_web_unit_test_support.cc (right): > > https://codereview.chromium.org/878413002/diff/20001/content/test/test_blink_... > content/test/test_blink_web_unit_test_support.cc:52: > renderer_scheduler_(renderer_scheduler.release()), > On 2015/01/28 18:35:22, sievers wrote: > > nit: drop '.release()', should be able to construct scoped_ptr from > scoped_ptr. > > Unfortunately I get a compile error if I try that: > > error: field of type 'scoped_ptr<content::RendererScheduler>' has private copy > constructor Pass() then :)
On 2015/01/29 16:25:19, sievers wrote: > On 2015/01/29 11:29:19, alexclarke1 wrote: > > > https://codereview.chromium.org/878413002/diff/20001/content/test/test_blink_... > > File content/test/test_blink_web_unit_test_support.cc (right): > > > > > https://codereview.chromium.org/878413002/diff/20001/content/test/test_blink_... > > content/test/test_blink_web_unit_test_support.cc:52: > > renderer_scheduler_(renderer_scheduler.release()), > > On 2015/01/28 18:35:22, sievers wrote: > > > nit: drop '.release()', should be able to construct scoped_ptr from > > scoped_ptr. > > > > Unfortunately I get a compile error if I try that: > > > > error: field of type 'scoped_ptr<content::RendererScheduler>' has private copy > > constructor > > Pass() then :) Done. Could you also look at patch https://codereview.chromium.org/875633003? Looks like I have to export WebSchedulerImpl for componant builds and that means blink::WebScheduler needs exporting too.
On 2015/01/29 18:24:40, alexclarke1 wrote: > On 2015/01/29 16:25:19, sievers wrote: > > On 2015/01/29 11:29:19, alexclarke1 wrote: > > > > > > https://codereview.chromium.org/878413002/diff/20001/content/test/test_blink_... > > > File content/test/test_blink_web_unit_test_support.cc (right): > > > > > > > > > https://codereview.chromium.org/878413002/diff/20001/content/test/test_blink_... > > > content/test/test_blink_web_unit_test_support.cc:52: > > > renderer_scheduler_(renderer_scheduler.release()), > > > On 2015/01/28 18:35:22, sievers wrote: > > > > nit: drop '.release()', should be able to construct scoped_ptr from > > > scoped_ptr. > > > > > > Unfortunately I get a compile error if I try that: > > > > > > error: field of type 'scoped_ptr<content::RendererScheduler>' has private > copy > > > constructor > > > > Pass() then :) > > Done. Could you also look at patch https://codereview.chromium.org/875633003 > Looks like I have to export WebSchedulerImpl for componant builds and that means > blink::WebScheduler needs exporting too. Looks like I'll need to add a blink owner too for that :)
On 2015/01/29 18:24:40, alexclarke1 wrote: > On 2015/01/29 16:25:19, sievers wrote: > > On 2015/01/29 11:29:19, alexclarke1 wrote: > > > > > > https://codereview.chromium.org/878413002/diff/20001/content/test/test_blink_... > > > File content/test/test_blink_web_unit_test_support.cc (right): > > > > > > > > > https://codereview.chromium.org/878413002/diff/20001/content/test/test_blink_... > > > content/test/test_blink_web_unit_test_support.cc:52: > > > renderer_scheduler_(renderer_scheduler.release()), > > > On 2015/01/28 18:35:22, sievers wrote: > > > > nit: drop '.release()', should be able to construct scoped_ptr from > > > scoped_ptr. > > > > > > Unfortunately I get a compile error if I try that: > > > > > > error: field of type 'scoped_ptr<content::RendererScheduler>' has private > copy > > > constructor > > > > Pass() then :) > > Done. Could you also look at patch https://codereview.chromium.org/875633003 > Looks like I have to export WebSchedulerImpl for componant builds and that means > blink::WebScheduler needs exporting too. Yes, unfortunately you do, and only for the unittests (where the reference is in a different component while in the real app content/renderer things are in the same component).
The CQ bit was checked by alexclarke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/878413002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
PTAL The TestBlinkWebUnitTestSupport is used in a wide variety of tests and eagerly creating a scheduler (plus message loop) broke expectations for some of the _ng tests. Instead I'm now lazily creating the WebScheduler. This is slightly non-ideal since the default_task_runner used in the blink::TestEnvironment tests will be the be message loop's default proxy rather than the scheduler's default task runner (tasks may get executed in an unexpected order with respect to anything going through the scheduler).
The CQ bit was checked by alexclarke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/878413002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/85df08a96469963c49b0903c40eddb09f4f80d35 Cr-Commit-Position: refs/heads/master@{#314002} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
