Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1646583002:
[Reland] Per WebViewScheduler virtual time (Closed)
|
Created:
4 years, 11 months ago by alex clarke (OOO till 29th) Modified:
4 years, 10 months ago CC:
blink-reviews, blink-reviews-api_chromium.org, blundell+watchlist_chromium.org, chromium-reviews, darin-cc_chromium.org, dglazkov+blink, droger+watchlist_chromium.org, jam, kinuko+watch, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, scheduler-bugs_chromium.org, sdefresne+watchlist_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPer WebViewScheduler virtual time.
This patch introduces a new per WebViewScheduler virtual time and
wires it up into the BlinkTImers. NOTE there will be follow on
patches where various other parts of blink will also use virtual time.
For determinism some parts of blink will need to use the VirtualTime
although new code should generally use the original non-virtual APIs.
For more details see:
https://docs.google.com/document/d/1y9KDT_ZEzT7pBeY6uzVt1dgKlwc1OB_vY4NZO1zBQmo/edit#
BUG=546953
Committed: https://crrev.com/0d2d2840ad6d7f149569eb42332c8c8083401234
Cr-Commit-Position: refs/heads/master@{#377094}
Committed: https://crrev.com/c6f9c49b8221a2002ba771a3f108514a6c506ff0
Cr-Commit-Position: refs/heads/master@{#377287}
Patch Set 1 #Patch Set 2 : Narrow down the deps change to test only #Patch Set 3 : Rebase + add some new tests #Patch Set 4 : Try and fix gn build #Patch Set 5 : Some more tests #Patch Set 6 : Try to fix gn + another test #Patch Set 7 : Remove content/child dep #Patch Set 8 : rebased #Patch Set 9 : WebViewSchedulerImpl to unregister the virtual_time_domain_ in the destructor #
Total comments: 22
Patch Set 10 : Sami's comments #Patch Set 11 : Mirror changes in the other patch #Patch Set 12 : Rebased #Patch Set 13 : Fix compile #Patch Set 14 : Try to fix the broken tests #
Total comments: 21
Patch Set 15 : Wire up virtual time via WebThread instead of Platform #Patch Set 16 : Rebase + fix timer.h assert #Patch Set 17 : Fix some broken tests #Patch Set 18 : Fix test lekage in ScrollableAreaTest #Patch Set 19 : Rebase #Patch Set 20 : Remove some unneeded changes #
Total comments: 4
Patch Set 21 : Remove assert #Patch Set 22 : As discussed offline route virtual time in via WebTaskRunner #Patch Set 23 : ran glt cl format #Patch Set 24 : Rebased #
Total comments: 2
Patch Set 25 : Fix WebTaskRunnerImpl::monotonicallyIncreasingVirtualTimeSeconds #Patch Set 26 : Keep the setAllowVirtualTimeToAdvance comment in step with scheduler code changes #Patch Set 27 : Rebased #
Total comments: 24
Patch Set 28 : Move the browser test to blink #Patch Set 29 : Fix include paths #Patch Set 30 : Try to fix test crash #Patch Set 31 : Rebased #
Total comments: 4
Patch Set 32 : Remove runTasksForPeriodInMilliseconds #Patch Set 33 : Deflake VirtualTimeTest. #Patch Set 34 : WebTaskRunnerImpl to cope with null TimeDomains #Patch Set 35 : Fix compile #Messages
Total messages: 166 (83 generated)
The CQ bit was checked by alexclarke@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/1646583002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1646583002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by alexclarke@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/1646583002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1646583002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_compile_dbg_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_...)
The CQ bit was checked by alexclarke@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/1646583002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1646583002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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 alexclarke@chromium.org to run a CQ dry run
alexclarke@chromium.org changed reviewers: + skyostil@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1646583002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1646583002/80001
Description was changed from ========== WIP: PerWebViewScheduler virtual time BUG= ========== to ========== Per WebViewScheduler virtual time. This patch removes the plumbing for the old VirtualTime in favor of a new per WebViewScheduler virtual time. BUG=510398 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Per WebViewScheduler virtual time. This patch removes the plumbing for the old VirtualTime in favor of a new per WebViewScheduler virtual time. BUG=510398 ========== to ========== Per WebViewScheduler virtual time. This patch removes the plumbing for the old VirtualTime in favor of a new per WebViewScheduler virtual time. For determinism some parts of blink will need to use the VirtualTime although the real time should be the default for new code. BUG=510398 ==========
Description was changed from ========== Per WebViewScheduler virtual time. This patch removes the plumbing for the old VirtualTime in favor of a new per WebViewScheduler virtual time. For determinism some parts of blink will need to use the VirtualTime although the real time should be the default for new code. BUG=510398 ========== to ========== Per WebViewScheduler virtual time. This patch removes the plumbing for the old VirtualTime in favor of a new per WebViewScheduler virtual time. For determinism some parts of blink will need to use the VirtualTime although the real time should be the default for new code. BUG=510398 ==========
The CQ bit was checked by alexclarke@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/1646583002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1646583002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) 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_...)
The CQ bit was checked by alexclarke@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/1646583002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1646583002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by alexclarke@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/1646583002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1646583002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by alexclarke@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/1646583002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1646583002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Looks pretty good. The bug link seems wrong though? If you want to make this a bit smaller, you could land the lower level changes separately. https://codereview.chromium.org/1646583002/diff/160001/components/scheduler/b... File components/scheduler/base/task_queue_manager.cc (right): https://codereview.chromium.org/1646583002/diff/160001/components/scheduler/b... components/scheduler/base/task_queue_manager.cc:284: current_selected_task_queue_ = prev_selected_task_queue; Should we guard against prev_selected_task_queue getting deleted by this task? https://codereview.chromium.org/1646583002/diff/160001/components/scheduler/b... File components/scheduler/base/task_queue_manager.h (right): https://codereview.chromium.org/1646583002/diff/160001/components/scheduler/b... components/scheduler/base/task_queue_manager.h:129: internal::TaskQueueImpl* current_selected_task_queue() const { Should this be a regular TaskQueue instead of the internal one? How about currently_executing_task_queue() to make it clear when this is valid? https://codereview.chromium.org/1646583002/diff/160001/components/scheduler/c... File components/scheduler/child/scheduler_helper.h (right): https://codereview.chromium.org/1646583002/diff/160001/components/scheduler/c... components/scheduler/child/scheduler_helper.h:100: TaskQueue* CurrentSelectedTaskQueue() const; Similarly CurrentlyExecutingTaskQueue? https://codereview.chromium.org/1646583002/diff/160001/components/scheduler/r... File components/scheduler/renderer/auto_advancing_virtual_time_domain.h (right): https://codereview.chromium.org/1646583002/diff/160001/components/scheduler/r... components/scheduler/renderer/auto_advancing_virtual_time_domain.h:13: class SCHEDULER_EXPORT AutoAdvancingVirtualTimeDomain Could you add a short description about what this class does? https://codereview.chromium.org/1646583002/diff/160001/components/scheduler/r... components/scheduler/renderer/auto_advancing_virtual_time_domain.h:24: // Contols whether or not virtual time is allowed to advance, when the typo: Controls https://codereview.chromium.org/1646583002/diff/160001/components/scheduler/r... File components/scheduler/renderer/web_frame_scheduler_impl.cc (right): https://codereview.chromium.org/1646583002/diff/160001/components/scheduler/r... components/scheduler/renderer/web_frame_scheduler_impl.cc:22: virtual_time_domain_(nullptr), If a new frame gets created under a WebViewScheduler which uses virtual time, it should probably get virtual time automatically too. Maybe a pull model would work better for that? (That is we'd ask WebViewScheduler for our time domain & pump policy.) https://codereview.chromium.org/1646583002/diff/160001/components/scheduler/r... components/scheduler/renderer/web_frame_scheduler_impl.cc:121: DCHECK(virtual_time_domain_) instead to make sure everyone enables virtual time before trying to use it. https://codereview.chromium.org/1646583002/diff/160001/components/scheduler/r... File components/scheduler/renderer/web_frame_scheduler_impl.h (right): https://codereview.chromium.org/1646583002/diff/160001/components/scheduler/r... components/scheduler/renderer/web_frame_scheduler_impl.h:42: void UseVirtualTime(AutoAdvancingVirtualTimeDomain* virtual_time_domain); nit: SetVirtualTimeDomain? https://codereview.chromium.org/1646583002/diff/160001/components/scheduler/r... File components/scheduler/renderer/web_view_scheduler_impl.cc (right): https://codereview.chromium.org/1646583002/diff/160001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl.cc:111: if (virtual_time_domain_.get()) { Should we DCHECK(virtual_time_domain_) instead? If someone is calling this without enabling virtual time first, we probably shouldn't fail silently. https://codereview.chromium.org/1646583002/diff/160001/components/scheduler/r... File components/scheduler/renderer/web_view_scheduler_impl.h (right): https://codereview.chromium.org/1646583002/diff/160001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl.h:42: void useVirtualTime() override; Now that this is WebView-specific, I guess it gives another reason for us to get rid of the default task queues. Any idea how many systems are still using those? https://codereview.chromium.org/1646583002/diff/160001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebViewScheduler.h (right): https://codereview.chromium.org/1646583002/diff/160001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebViewScheduler.h:42: virtual void setAllowVirtualTimeToAdvance(bool) = 0; Mention that useVirtualTime() must be called before this function.
PTAL https://codereview.chromium.org/1646583002/diff/160001/components/scheduler/b... File components/scheduler/base/task_queue_manager.cc (right): https://codereview.chromium.org/1646583002/diff/160001/components/scheduler/b... components/scheduler/base/task_queue_manager.cc:284: current_selected_task_queue_ = prev_selected_task_queue; On 2016/02/01 11:15:06, Sami wrote: > Should we guard against prev_selected_task_queue getting deleted by this task? Done. https://codereview.chromium.org/1646583002/diff/160001/components/scheduler/b... File components/scheduler/base/task_queue_manager.h (right): https://codereview.chromium.org/1646583002/diff/160001/components/scheduler/b... components/scheduler/base/task_queue_manager.h:129: internal::TaskQueueImpl* current_selected_task_queue() const { On 2016/02/01 11:15:06, Sami wrote: > Should this be a regular TaskQueue instead of the internal one? > > How about currently_executing_task_queue() to make it clear when this is valid? Done. https://codereview.chromium.org/1646583002/diff/160001/components/scheduler/c... File components/scheduler/child/scheduler_helper.h (right): https://codereview.chromium.org/1646583002/diff/160001/components/scheduler/c... components/scheduler/child/scheduler_helper.h:100: TaskQueue* CurrentSelectedTaskQueue() const; On 2016/02/01 11:15:07, Sami wrote: > Similarly CurrentlyExecutingTaskQueue? Done. https://codereview.chromium.org/1646583002/diff/160001/components/scheduler/r... File components/scheduler/renderer/auto_advancing_virtual_time_domain.h (right): https://codereview.chromium.org/1646583002/diff/160001/components/scheduler/r... components/scheduler/renderer/auto_advancing_virtual_time_domain.h:13: class SCHEDULER_EXPORT AutoAdvancingVirtualTimeDomain On 2016/02/01 11:15:07, Sami wrote: > Could you add a short description about what this class does? Done. https://codereview.chromium.org/1646583002/diff/160001/components/scheduler/r... components/scheduler/renderer/auto_advancing_virtual_time_domain.h:24: // Contols whether or not virtual time is allowed to advance, when the On 2016/02/01 11:15:07, Sami wrote: > typo: Controls Done. https://codereview.chromium.org/1646583002/diff/160001/components/scheduler/r... File components/scheduler/renderer/web_frame_scheduler_impl.cc (right): https://codereview.chromium.org/1646583002/diff/160001/components/scheduler/r... components/scheduler/renderer/web_frame_scheduler_impl.cc:22: virtual_time_domain_(nullptr), On 2016/02/01 11:15:07, Sami wrote: > If a new frame gets created under a WebViewScheduler which uses virtual time, it > should probably get virtual time automatically too. Maybe a pull model would > work better for that? (That is we'd ask WebViewScheduler for our time domain & > pump policy.) I think the pull vs push is cleaner, done. https://codereview.chromium.org/1646583002/diff/160001/components/scheduler/r... components/scheduler/renderer/web_frame_scheduler_impl.cc:121: On 2016/02/01 11:15:07, Sami wrote: > DCHECK(virtual_time_domain_) instead to make sure everyone enables virtual time > before trying to use it. Why do we need to enforce that? We can make it do the right thing regardless. https://codereview.chromium.org/1646583002/diff/160001/components/scheduler/r... File components/scheduler/renderer/web_frame_scheduler_impl.h (right): https://codereview.chromium.org/1646583002/diff/160001/components/scheduler/r... components/scheduler/renderer/web_frame_scheduler_impl.h:42: void UseVirtualTime(AutoAdvancingVirtualTimeDomain* virtual_time_domain); On 2016/02/01 11:15:07, Sami wrote: > nit: SetVirtualTimeDomain? Done. https://codereview.chromium.org/1646583002/diff/160001/components/scheduler/r... File components/scheduler/renderer/web_view_scheduler_impl.cc (right): https://codereview.chromium.org/1646583002/diff/160001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl.cc:111: if (virtual_time_domain_.get()) { On 2016/02/01 11:15:07, Sami wrote: > Should we DCHECK(virtual_time_domain_) instead? If someone is calling this > without enabling virtual time first, we probably shouldn't fail silently. But it doesn't fail :) See line 92 https://codereview.chromium.org/1646583002/diff/160001/components/scheduler/r... File components/scheduler/renderer/web_view_scheduler_impl.h (right): https://codereview.chromium.org/1646583002/diff/160001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl.h:42: void useVirtualTime() override; On 2016/02/01 11:15:07, Sami wrote: > Now that this is WebView-specific, I guess it gives another reason for us to get > rid of the default task queues. Any idea how many systems are still using those? Quite a few :( One problem is the order in which classes are constructed in blink. The frame is quite late IIRC. https://codereview.chromium.org/1646583002/diff/160001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebViewScheduler.h (right): https://codereview.chromium.org/1646583002/diff/160001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebViewScheduler.h:42: virtual void setAllowVirtualTimeToAdvance(bool) = 0; On 2016/02/01 11:15:07, Sami wrote: > Mention that useVirtualTime() must be called before this function. But it can be used in any order.
Description was changed from ========== Per WebViewScheduler virtual time. This patch removes the plumbing for the old VirtualTime in favor of a new per WebViewScheduler virtual time. For determinism some parts of blink will need to use the VirtualTime although the real time should be the default for new code. BUG=510398 ========== to ========== Per WebViewScheduler virtual time. This patch removes the plumbing for the old VirtualTime in favor of a new per WebViewScheduler virtual time. For determinism some parts of blink will need to use the VirtualTime although the real time should be the default for new code. BUG=546953 ==========
Description was changed from ========== Per WebViewScheduler virtual time. This patch removes the plumbing for the old VirtualTime in favor of a new per WebViewScheduler virtual time. For determinism some parts of blink will need to use the VirtualTime although the real time should be the default for new code. BUG=546953 ========== to ========== Per WebViewScheduler virtual time. This patch introduces a new per WebViewScheduler virtual time and wires it up into the BlinkTImers. NOTE there will be follow on patches where various other parts of blink will also use virtual time. For determinism some parts of blink will need to use the VirtualTime although the real time should be the default for new code. BUG=546953 ==========
The CQ bit was checked by alexclarke@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/1646583002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1646583002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by alexclarke@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/1646583002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1646583002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Patchset #13 (id:240001) has been deleted
The CQ bit was checked by alexclarke@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/1646583002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1646583002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by alexclarke@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/1646583002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1646583002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
I think this overall approach of having two sets of time entrypoints should work for us. Mind opening a dedicated bug for virtual time and the follow-up moving of subsystems to use it? We could also link the virtual time design doc there. https://codereview.chromium.org/1646583002/diff/280001/components/scheduler/r... File components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc (right): https://codereview.chromium.org/1646583002/diff/280001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:64: "window.timerFn = function(delay, value){" nit: missing space before { https://codereview.chromium.org/1646583002/diff/280001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:65: " setTimeout(function(){ window.run_order.push(value);}, delay);" nit: missing space before { Same comment for all the other tests too. https://codereview.chromium.org/1646583002/diff/280001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:83: // Normally the above JS would take 100 hours to run, but thanks to timer nit: The JS runs pretty much instantly but the timer callbacks will take 100h to fire. https://codereview.chromium.org/1646583002/diff/280001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:86: EXPECT_LT(elapsed_time, base::TimeDelta::FromSeconds(1)); One second may be a little too optimistic for Android :P https://codereview.chromium.org/1646583002/diff/280001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:107: // If virtual time is not supplied to TimeBase then the setInterval won't Did you mean blink::TimerBase? https://codereview.chromium.org/1646583002/diff/280001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:124: ProcessPendingMessages(); If the thread gets descheduled, we might end up running some of the timers, right? Then again if we set the timeout really high we won't be testing anything here. I wonder if there's a way to make this more reliable? https://codereview.chromium.org/1646583002/diff/280001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:159: Should we have a test that combines throttling and virtual time? A test with a subframe might be a good sanity check too. https://codereview.chromium.org/1646583002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/Timer.cpp (right): https://codereview.chromium.org/1646583002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/Timer.cpp:145: return Platform::current()->monotonicallyIncreasingVirtualTimeSeconds(); This is going to get called fairly often so it might make sense to cache the TLS lookup. https://codereview.chromium.org/1646583002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/Timer.h (right): https://codereview.chromium.org/1646583002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/Timer.h:69: double now = time(); Sorry to bikeshed but I like the descriptiveness of the old name. How about something like monotonicallyIncreasingTimeForTimers or timerMonotonicallyIncreasingTime?
The CQ bit was checked by alexclarke@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/1646583002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1646583002/300001
Description was changed from ========== Per WebViewScheduler virtual time. This patch introduces a new per WebViewScheduler virtual time and wires it up into the BlinkTImers. NOTE there will be follow on patches where various other parts of blink will also use virtual time. For determinism some parts of blink will need to use the VirtualTime although the real time should be the default for new code. BUG=546953 ========== to ========== Per WebViewScheduler virtual time. This patch introduces a new per WebViewScheduler virtual time and wires it up into the BlinkTImers. NOTE there will be follow on patches where various other parts of blink will also use virtual time. For determinism some parts of blink will need to use the VirtualTime although new code should generally use the original non-virtual APIs. BUG=546953 ==========
PTAL https://codereview.chromium.org/1646583002/diff/280001/components/scheduler/r... File components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc (right): https://codereview.chromium.org/1646583002/diff/280001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:64: "window.timerFn = function(delay, value){" On 2016/02/03 18:03:44, Sami wrote: > nit: missing space before { Done. https://codereview.chromium.org/1646583002/diff/280001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:65: " setTimeout(function(){ window.run_order.push(value);}, delay);" On 2016/02/03 18:03:44, Sami wrote: > nit: missing space before { > > Same comment for all the other tests too. Done. https://codereview.chromium.org/1646583002/diff/280001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:83: // Normally the above JS would take 100 hours to run, but thanks to timer On 2016/02/03 18:03:44, Sami wrote: > nit: The JS runs pretty much instantly but the timer callbacks will take 100h to > fire. Done. https://codereview.chromium.org/1646583002/diff/280001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:107: // If virtual time is not supplied to TimeBase then the setInterval won't On 2016/02/03 18:03:44, Sami wrote: > Did you mean blink::TimerBase? Done. https://codereview.chromium.org/1646583002/diff/280001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:124: ProcessPendingMessages(); On 2016/02/03 18:03:44, Sami wrote: > If the thread gets descheduled, we might end up running some of the timers, > right? Then again if we set the timeout really high we won't be testing anything > here. I wonder if there's a way to make this more reliable? As discussed offline I think we're OK because if virtual time was broken, then DOMTimersFireInExpectedOrder would break too. https://codereview.chromium.org/1646583002/diff/280001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:159: On 2016/02/03 18:03:44, Sami wrote: > Should we have a test that combines throttling and virtual time? A test with a > subframe might be a good sanity check too. We have a unit test RepeatingTimer_PageInBackground_MeansNothingForFirtualTime which checks what happens here (the two do not co-exist). Do we need to repeat the test here? https://codereview.chromium.org/1646583002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/Timer.cpp (right): https://codereview.chromium.org/1646583002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/Timer.cpp:145: return Platform::current()->monotonicallyIncreasingVirtualTimeSeconds(); On 2016/02/03 18:03:44, Sami wrote: > This is going to get called fairly often so it might make sense to cache the TLS > lookup. Done. https://codereview.chromium.org/1646583002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/Timer.h (right): https://codereview.chromium.org/1646583002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/Timer.h:69: double now = time(); On 2016/02/03 18:03:44, Sami wrote: > Sorry to bikeshed but I like the descriptiveness of the old name. How about > something like monotonicallyIncreasingTimeForTimers or > timerMonotonicallyIncreasingTime? I agree.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by alexclarke@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/1646583002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1646583002/320001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by alexclarke@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/1646583002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1646583002/340001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by alexclarke@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/1646583002/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1646583002/380001
The CQ bit was checked by alexclarke@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/1646583002/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1646583002/400001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Forgot to hit send earlier. https://codereview.chromium.org/1646583002/diff/280001/components/scheduler/r... File components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc (right): https://codereview.chromium.org/1646583002/diff/280001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:86: EXPECT_LT(elapsed_time, base::TimeDelta::FromSeconds(1)); On 2016/02/03 18:03:44, Sami wrote: > One second may be a little too optimistic for Android :P Should we bump this to 10s to increase the flakiness margin a bit? Anything smaller than an hour should do, right? https://codereview.chromium.org/1646583002/diff/280001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:159: On 2016/02/04 15:00:09, alexclarke1 wrote: > On 2016/02/03 18:03:44, Sami wrote: > > Should we have a test that combines throttling and virtual time? A test with a > > subframe might be a good sanity check too. > > We have a unit test RepeatingTimer_PageInBackground_MeansNothingForFirtualTime > which checks what happens here (the two do not co-exist). > > Do we need to repeat the test here? Ah okay, that should be sufficient. https://codereview.chromium.org/1646583002/diff/400001/components/scheduler/r... File components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc (right): https://codereview.chromium.org/1646583002/diff/400001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:61: " setTimeout(function() { window.run_order.push(value);}, delay);" nit: Missing space before the } too. Same below in other tests. https://codereview.chromium.org/1646583002/diff/400001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:103: // If virtual time is not supplied to blink::TimeBase then the setInterval typo: TimerBase
PTAL https://codereview.chromium.org/1646583002/diff/280001/components/scheduler/r... File components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc (right): https://codereview.chromium.org/1646583002/diff/280001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:86: EXPECT_LT(elapsed_time, base::TimeDelta::FromSeconds(1)); On 2016/02/05 13:54:34, Sami wrote: > On 2016/02/03 18:03:44, Sami wrote: > > One second may be a little too optimistic for Android :P > > Should we bump this to 10s to increase the flakiness margin a bit? Anything > smaller than an hour should do, right? Done. https://codereview.chromium.org/1646583002/diff/280001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:159: On 2016/02/05 13:54:34, Sami wrote: > On 2016/02/04 15:00:09, alexclarke1 wrote: > > On 2016/02/03 18:03:44, Sami wrote: > > > Should we have a test that combines throttling and virtual time? A test with > a > > > subframe might be a good sanity check too. > > > > We have a unit test RepeatingTimer_PageInBackground_MeansNothingForFirtualTime > > > which checks what happens here (the two do not co-exist). > > > > Do we need to repeat the test here? > > Ah okay, that should be sufficient. Acknowledged. https://codereview.chromium.org/1646583002/diff/400001/components/scheduler/r... File components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc (right): https://codereview.chromium.org/1646583002/diff/400001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:61: " setTimeout(function() { window.run_order.push(value);}, delay);" On 2016/02/05 13:54:34, Sami wrote: > nit: Missing space before the } too. Same below in other tests. Done. https://codereview.chromium.org/1646583002/diff/400001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:103: // If virtual time is not supplied to blink::TimeBase then the setInterval On 2016/02/05 13:54:34, Sami wrote: > typo: TimerBase Done.
PTAL https://codereview.chromium.org/1646583002/diff/280001/components/scheduler/r... File components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc (right): https://codereview.chromium.org/1646583002/diff/280001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:86: EXPECT_LT(elapsed_time, base::TimeDelta::FromSeconds(1)); On 2016/02/05 13:54:34, Sami wrote: > On 2016/02/03 18:03:44, Sami wrote: > > One second may be a little too optimistic for Android :P > > Should we bump this to 10s to increase the flakiness margin a bit? Anything > smaller than an hour should do, right? Done. https://codereview.chromium.org/1646583002/diff/280001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:159: On 2016/02/05 13:54:34, Sami wrote: > On 2016/02/04 15:00:09, alexclarke1 wrote: > > On 2016/02/03 18:03:44, Sami wrote: > > > Should we have a test that combines throttling and virtual time? A test with > a > > > subframe might be a good sanity check too. > > > > We have a unit test RepeatingTimer_PageInBackground_MeansNothingForFirtualTime > > > which checks what happens here (the two do not co-exist). > > > > Do we need to repeat the test here? > > Ah okay, that should be sufficient. Acknowledged. https://codereview.chromium.org/1646583002/diff/400001/components/scheduler/r... File components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc (right): https://codereview.chromium.org/1646583002/diff/400001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:61: " setTimeout(function() { window.run_order.push(value);}, delay);" On 2016/02/05 13:54:34, Sami wrote: > nit: Missing space before the } too. Same below in other tests. Done. https://codereview.chromium.org/1646583002/diff/400001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:103: // If virtual time is not supplied to blink::TimeBase then the setInterval On 2016/02/05 13:54:34, Sami wrote: > typo: TimerBase Done.
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
PING
PING :)
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1646583002/480001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1646583002/480001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Thanks, lgtm. https://codereview.chromium.org/1646583002/diff/480001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebTaskRunner.h (right): https://codereview.chromium.org/1646583002/diff/480001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebTaskRunner.h:47: // This may represent either the real time, or a virtual time depending on These two functions no longer depend on whether we're running a virtual time task or not, right? Looks like both comments need updating.
The CQ bit was checked by alexclarke@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/1646583002/500001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1646583002/500001
alexclarke@chromium.org changed reviewers: + esprehn@chromium.org
+Elliot Could you please look at the blink parts of this patch? We have a spreadsheet showing the various places in blink that do and don't need virtual time. The ones that do need it will (most likely) need to get the time via the WebTaskRunner. http://go/headless_chrome_virtualtime https://codereview.chromium.org/1646583002/diff/480001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebTaskRunner.h (right): https://codereview.chromium.org/1646583002/diff/480001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebTaskRunner.h:47: // This may represent either the real time, or a virtual time depending on On 2016/02/09 21:00:28, Sami wrote: > These two functions no longer depend on whether we're running a virtual time > task or not, right? Looks like both comments need updating. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Per WebViewScheduler virtual time. This patch introduces a new per WebViewScheduler virtual time and wires it up into the BlinkTImers. NOTE there will be follow on patches where various other parts of blink will also use virtual time. For determinism some parts of blink will need to use the VirtualTime although new code should generally use the original non-virtual APIs. BUG=546953 ========== to ========== Per WebViewScheduler virtual time. This patch introduces a new per WebViewScheduler virtual time and wires it up into the BlinkTImers. NOTE there will be follow on patches where various other parts of blink will also use virtual time. For determinism some parts of blink will need to use the VirtualTime although new code should generally use the original non-virtual APIs. BUG=546953 ==========
The CQ bit was checked by alexclarke@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/1646583002/500001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1646583002/500001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Per WebViewScheduler virtual time. This patch introduces a new per WebViewScheduler virtual time and wires it up into the BlinkTImers. NOTE there will be follow on patches where various other parts of blink will also use virtual time. For determinism some parts of blink will need to use the VirtualTime although new code should generally use the original non-virtual APIs. BUG=546953 ========== to ========== Per WebViewScheduler virtual time. This patch introduces a new per WebViewScheduler virtual time and wires it up into the BlinkTImers. NOTE there will be follow on patches where various other parts of blink will also use virtual time. For determinism some parts of blink will need to use the VirtualTime although new code should generally use the original non-virtual APIs. See: https://docs.google.com/document/d/1y9KDT_ZEzT7pBeY6uzVt1dgKlwc1OB_vY4NZO1zBQ... BUG=546953 ==========
Description was changed from ========== Per WebViewScheduler virtual time. This patch introduces a new per WebViewScheduler virtual time and wires it up into the BlinkTImers. NOTE there will be follow on patches where various other parts of blink will also use virtual time. For determinism some parts of blink will need to use the VirtualTime although new code should generally use the original non-virtual APIs. See: https://docs.google.com/document/d/1y9KDT_ZEzT7pBeY6uzVt1dgKlwc1OB_vY4NZO1zBQ... BUG=546953 ========== to ========== Per WebViewScheduler virtual time. This patch introduces a new per WebViewScheduler virtual time and wires it up into the BlinkTImers. NOTE there will be follow on patches where various other parts of blink will also use virtual time. For determinism some parts of blink will need to use the VirtualTime although new code should generally use the original non-virtual APIs. For more details see: https://docs.google.com/document/d/1y9KDT_ZEzT7pBeY6uzVt1dgKlwc1OB_vY4NZO1zBQ... BUG=546953 ==========
The CQ bit was checked by alexclarke@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/1646583002/520001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1646583002/520001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
PING :)
I guess this is okay, but I have to admit I think this test being out in content is busted. Why isn't that test inside blink? https://codereview.chromium.org/1646583002/diff/540001/components/scheduler/r... File components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc (right): https://codereview.chromium.org/1646583002/diff/540001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:19: // http://crbug.com/187500 is this really true? https://code.google.com/p/chromium/issues/detail?id=187500#c8 claims they work most of the time now. https://codereview.chromium.org/1646583002/diff/540001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:43: bool MAYBE_WebViewSchedulerBrowserTest::ExecuteJavaScriptAndReturnStringValue( why not return a std::string instead and just empty check it? Out params seem kind of silly here. https://codereview.chromium.org/1646583002/diff/540001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:44: const base::string16& script, I think you want to use std::string https://codereview.chromium.org/1646583002/diff/540001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:47: v8::Local<v8::Value> result = GetMainFrame()->executeScriptAndReturnValue( the docs claim this function is deprecated and you should use requestExecuteScriptAndReturnValue instead. https://codereview.chromium.org/1646583002/diff/540001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:48: blink::WebScriptSource(script)); WebString::fromUTF8 ? https://codereview.chromium.org/1646583002/diff/540001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:59: "window.run_order = [];" var, no need to write window. all over here https://codereview.chromium.org/1646583002/diff/540001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:75: base::ASCIIToUTF16("window.run_order.join(', ')"), &result)); ditto, also just pass a const char* ? No need for all this inflation ceremony. https://codereview.chromium.org/1646583002/diff/540001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:124: base::ASCIIToUTF16("window.run_order.join(', ')"), &result)); I think your test wants to return window.run_order.join(', ').length instead. instead of this bool and string check, always return a real value https://codereview.chromium.org/1646583002/diff/540001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:132: EXPECT_EQ("c, b, a", result); EXPECT_EQ("c, b, a", ExecuteJavaScriptAndReturnStringValue( "window.run_order.join(', ')" )); https://codereview.chromium.org/1646583002/diff/540001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:138: "window.timerFn = function(delay, value) {" function scheduleTimer, also with the other ones https://codereview.chromium.org/1646583002/diff/540001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/ScrollableAreaTest.cpp (right): https://codereview.chromium.org/1646583002/diff/540001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ScrollableAreaTest.cpp:182: Heap::collectAllGarbage(); woah... can we do this in a separate change? https://codereview.chromium.org/1646583002/diff/540001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/testing/TestingPlatformSupport.cpp (right): https://codereview.chromium.org/1646583002/diff/540001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/TestingPlatformSupport.cpp:141: return 0.0; this makes all timers in these tests busted since the time is not actually monotonically increasing?
I guess this is okay, but I have to admit I think this test being out in content is busted. Why isn't that test inside blink? https://codereview.chromium.org/1646583002/diff/540001/components/scheduler/r... File components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc (right): https://codereview.chromium.org/1646583002/diff/540001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:19: // http://crbug.com/187500 is this really true? https://code.google.com/p/chromium/issues/detail?id=187500#c8 claims they work most of the time now. https://codereview.chromium.org/1646583002/diff/540001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:43: bool MAYBE_WebViewSchedulerBrowserTest::ExecuteJavaScriptAndReturnStringValue( why not return a std::string instead and just empty check it? Out params seem kind of silly here. https://codereview.chromium.org/1646583002/diff/540001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:44: const base::string16& script, I think you want to use std::string https://codereview.chromium.org/1646583002/diff/540001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:47: v8::Local<v8::Value> result = GetMainFrame()->executeScriptAndReturnValue( the docs claim this function is deprecated and you should use requestExecuteScriptAndReturnValue instead. https://codereview.chromium.org/1646583002/diff/540001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:48: blink::WebScriptSource(script)); WebString::fromUTF8 ? https://codereview.chromium.org/1646583002/diff/540001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:59: "window.run_order = [];" var, no need to write window. all over here https://codereview.chromium.org/1646583002/diff/540001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:75: base::ASCIIToUTF16("window.run_order.join(', ')"), &result)); ditto, also just pass a const char* ? No need for all this inflation ceremony. https://codereview.chromium.org/1646583002/diff/540001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:124: base::ASCIIToUTF16("window.run_order.join(', ')"), &result)); I think your test wants to return window.run_order.join(', ').length instead. instead of this bool and string check, always return a real value https://codereview.chromium.org/1646583002/diff/540001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:132: EXPECT_EQ("c, b, a", result); EXPECT_EQ("c, b, a", ExecuteJavaScriptAndReturnStringValue( "window.run_order.join(', ')" )); https://codereview.chromium.org/1646583002/diff/540001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:138: "window.timerFn = function(delay, value) {" function scheduleTimer, also with the other ones https://codereview.chromium.org/1646583002/diff/540001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/ScrollableAreaTest.cpp (right): https://codereview.chromium.org/1646583002/diff/540001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ScrollableAreaTest.cpp:182: Heap::collectAllGarbage(); woah... can we do this in a separate change? https://codereview.chromium.org/1646583002/diff/540001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/testing/TestingPlatformSupport.cpp (right): https://codereview.chromium.org/1646583002/diff/540001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/TestingPlatformSupport.cpp:141: return 0.0; this makes all timers in these tests busted since the time is not actually monotonically increasing?
The CQ bit was checked by alexclarke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/1646583002/#ps560001 (title: "Move the browser test to blink")
The CQ bit was unchecked by alexclarke@chromium.org
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
The CQ bit was unchecked by alexclarke@chromium.org
The CQ bit was checked by alexclarke@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/1646583002/560001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1646583002/560001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
The CQ bit was checked by alexclarke@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/1646583002/580001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1646583002/580001
PTAL https://codereview.chromium.org/1646583002/diff/540001/components/scheduler/r... File components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc (right): https://codereview.chromium.org/1646583002/diff/540001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:19: // http://crbug.com/187500 On 2016/02/18 03:22:59, esprehn wrote: > is this really true? > > https://code.google.com/p/chromium/issues/detail?id=187500#c8 > > claims they work most of the time now. Lets find out :) It would be nice if it does work on Android. https://codereview.chromium.org/1646583002/diff/540001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:43: bool MAYBE_WebViewSchedulerBrowserTest::ExecuteJavaScriptAndReturnStringValue( On 2016/02/18 03:23:00, esprehn wrote: > why not return a std::string instead and just empty check it? > > Out params seem kind of silly here. Done. https://codereview.chromium.org/1646583002/diff/540001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:44: const base::string16& script, On 2016/02/18 03:22:59, esprehn wrote: > I think you want to use std::string Done. https://codereview.chromium.org/1646583002/diff/540001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:47: v8::Local<v8::Value> result = GetMainFrame()->executeScriptAndReturnValue( On 2016/02/18 03:22:59, esprehn wrote: > the docs claim this function is deprecated and you should use > requestExecuteScriptAndReturnValue instead. Done. https://codereview.chromium.org/1646583002/diff/540001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:48: blink::WebScriptSource(script)); On 2016/02/18 03:22:59, esprehn wrote: > WebString::fromUTF8 ? Done. https://codereview.chromium.org/1646583002/diff/540001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:59: "window.run_order = [];" On 2016/02/18 03:22:59, esprehn wrote: > var, > > no need to write window. all over here Done. https://codereview.chromium.org/1646583002/diff/540001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:75: base::ASCIIToUTF16("window.run_order.join(', ')"), &result)); On 2016/02/18 03:22:59, esprehn wrote: > ditto, also just pass a const char* ? No need for all this inflation ceremony. Done. https://codereview.chromium.org/1646583002/diff/540001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:124: base::ASCIIToUTF16("window.run_order.join(', ')"), &result)); On 2016/02/18 03:22:59, esprehn wrote: > I think your test wants to return window.run_order.join(', ').length instead. > > instead of this bool and string check, always return a real value Acknowledged. https://codereview.chromium.org/1646583002/diff/540001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:132: EXPECT_EQ("c, b, a", result); On 2016/02/18 03:22:59, esprehn wrote: > EXPECT_EQ("c, b, a", ExecuteJavaScriptAndReturnStringValue( > "window.run_order.join(', ')" > )); Done. https://codereview.chromium.org/1646583002/diff/540001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_browsertest.cc:138: "window.timerFn = function(delay, value) {" On 2016/02/18 03:22:59, esprehn wrote: > function scheduleTimer, also with the other ones Done. https://codereview.chromium.org/1646583002/diff/540001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/ScrollableAreaTest.cpp (right): https://codereview.chromium.org/1646583002/diff/540001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ScrollableAreaTest.cpp:182: Heap::collectAllGarbage(); On 2016/02/18 03:23:00, esprehn wrote: > woah... can we do this in a separate change? Done. https://codereview.chromium.org/1646583002/diff/540001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/testing/TestingPlatformSupport.cpp (right): https://codereview.chromium.org/1646583002/diff/540001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/TestingPlatformSupport.cpp:141: return 0.0; On 2016/02/18 03:23:00, esprehn wrote: > this makes all timers in these tests busted since the time is not actually > monotonically increasing? I think they're already busted since this particular mock class doesn't support delayed tasks. Adding some ASSERT_NOT_REACHED(); seems like the right thing to do here in case some future test code ends up calling these.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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 alexclarke@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/1646583002/600001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1646583002/600001
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_...)
Patchset #31 (id:620001) has been deleted
The CQ bit was checked by alexclarke@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/1646583002/640001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1646583002/640001
Hi Elliott, do you have any more feedback or is it now OK to land this?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PING
Lgtm w/ comments addressed. I have to admit I'm not very comfortable with that test though, it seems like it's going to be flaky. https://codereview.chromium.org/1646583002/diff/640001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/Timer.h (right): https://codereview.chromium.org/1646583002/diff/640001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/Timer.h:91: { Does this really need to be inline in the header? https://codereview.chromium.org/1646583002/diff/640001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/testing/UnitTestHelpers.cpp (right): https://codereview.chromium.org/1646583002/diff/640001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/UnitTestHelpers.cpp:59: Platform::current()->currentThread()->taskRunner()->postDelayedTask(BLINK_FROM_HERE, new QuitTask, runTimeMilliseconds); This is scary, what keeps this from being flaky?
The CQ bit was checked by alexclarke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org, esprehn@chromium.org Link to the patchset: https://codereview.chromium.org/1646583002/#ps660001 (title: "Remove runTasksForPeriodInMilliseconds")
Thanks all! https://codereview.chromium.org/1646583002/diff/640001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/Timer.h (right): https://codereview.chromium.org/1646583002/diff/640001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/Timer.h:91: { On 2016/02/23 10:30:11, esprehn wrote: > Does this really need to be inline in the header? I guess it doesn't really matter if it's defined in the cpp file. Done. https://codereview.chromium.org/1646583002/diff/640001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/testing/UnitTestHelpers.cpp (right): https://codereview.chromium.org/1646583002/diff/640001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/UnitTestHelpers.cpp:59: Platform::current()->currentThread()->taskRunner()->postDelayedTask(BLINK_FROM_HERE, new QuitTask, runTimeMilliseconds); On 2016/02/23 10:30:11, esprehn wrote: > This is scary, what keeps this from being flaky? Turns out we don't actually need this for VirtualTimeTest.DOMTimersFireInExpectedOrder. The other test which used this api (the non-virtual time case) isn't strictly needed since there's a lot of test coverage elsewhere, so I've dropped it along with runTasksForPeriodInMilliseconds.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1646583002/660001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1646583002/660001
The CQ bit was unchecked by commit-bot@chromium.org
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 alexclarke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1646583002/660001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1646583002/660001
Message was sent while issue was closed.
Description was changed from ========== Per WebViewScheduler virtual time. This patch introduces a new per WebViewScheduler virtual time and wires it up into the BlinkTImers. NOTE there will be follow on patches where various other parts of blink will also use virtual time. For determinism some parts of blink will need to use the VirtualTime although new code should generally use the original non-virtual APIs. For more details see: https://docs.google.com/document/d/1y9KDT_ZEzT7pBeY6uzVt1dgKlwc1OB_vY4NZO1zBQ... BUG=546953 ========== to ========== Per WebViewScheduler virtual time. This patch introduces a new per WebViewScheduler virtual time and wires it up into the BlinkTImers. NOTE there will be follow on patches where various other parts of blink will also use virtual time. For determinism some parts of blink will need to use the VirtualTime although new code should generally use the original non-virtual APIs. For more details see: https://docs.google.com/document/d/1y9KDT_ZEzT7pBeY6uzVt1dgKlwc1OB_vY4NZO1zBQ... BUG=546953 ==========
Message was sent while issue was closed.
Committed patchset #32 (id:660001)
Message was sent while issue was closed.
Description was changed from ========== Per WebViewScheduler virtual time. This patch introduces a new per WebViewScheduler virtual time and wires it up into the BlinkTImers. NOTE there will be follow on patches where various other parts of blink will also use virtual time. For determinism some parts of blink will need to use the VirtualTime although new code should generally use the original non-virtual APIs. For more details see: https://docs.google.com/document/d/1y9KDT_ZEzT7pBeY6uzVt1dgKlwc1OB_vY4NZO1zBQ... BUG=546953 ========== to ========== Per WebViewScheduler virtual time. This patch introduces a new per WebViewScheduler virtual time and wires it up into the BlinkTImers. NOTE there will be follow on patches where various other parts of blink will also use virtual time. For determinism some parts of blink will need to use the VirtualTime although new code should generally use the original non-virtual APIs. For more details see: https://docs.google.com/document/d/1y9KDT_ZEzT7pBeY6uzVt1dgKlwc1OB_vY4NZO1zBQ... BUG=546953 Committed: https://crrev.com/0d2d2840ad6d7f149569eb42332c8c8083401234 Cr-Commit-Position: refs/heads/master@{#377094} ==========
Message was sent while issue was closed.
Patchset 32 (id:??) landed as https://crrev.com/0d2d2840ad6d7f149569eb42332c8c8083401234 Cr-Commit-Position: refs/heads/master@{#377094}
Message was sent while issue was closed.
A revert of this CL (patchset #32 id:660001) has been created in https://codereview.chromium.org/1727103002/ by shrike@chromium.org. The reason for reverting is: Regression: Mac ASan 64 Tests (1) Suspecting this cl for cause of Mac ASan regression, from list of Sheriff-o-matic suggested cls ( https://chromium.googlesource.com/chromium/src/+log/83534ab036f32a6aa7417e64f... ). https://build.chromium.org/p/chromium.memory/builders/Mac%20ASan%2064%20Tests... Build #12287 - https://build.chromium.org/p/chromium.memory/builders/Mac%20ASan%2064%20Tests... [ RUN ] VirtualTimeTest.AllowVirtualTimeToAdvance ../../third_party/WebKit/Source/web/tests/VirtualTimeTest.cpp:103: Failure Value of: ExecuteJavaScript("run_order.join(', ')") Actual: "c, b" Expected: "" Which is: 0x11200a6e0 [ FAILED ] VirtualTimeTest.AllowVirtualTimeToAdvance (60 ms) [2777/2778] VirtualTimeTest.AllowVirtualTimeToAdvance (60 ms) [ RUN ] All/ParameterizedWebFrameTest.DeviceScaleFactorUsesDefaultWithoutViewportTag/0 [ OK ] All/ParameterizedWebFrameTest.DeviceScaleFactorUsesDefaultWithoutViewportTag/0 (65 ms) [2778/2778] All/ParameterizedWebFrameTest.DeviceScaleFactorUsesDefaultWithoutViewportTag/0 (65 ms) Retrying 1 test (retry #2) [ RUN ] VirtualTimeTest.AllowVirtualTimeToAdvance ../../third_party/WebKit/Source/web/tests/VirtualTimeTest.cpp:103: Failure Value of: ExecuteJavaScript("run_order.join(', ')") Actual: "c, b" Expected: "" Which is: 0x10d8e66e0 [ FAILED ] VirtualTimeTest.AllowVirtualTimeToAdvance (62 ms) [2779/2779] VirtualTimeTest.AllowVirtualTimeToAdvance (62 ms) Retrying 1 test (retry #3) [ RUN ] VirtualTimeTest.AllowVirtualTimeToAdvance ../../third_party/WebKit/Source/web/tests/VirtualTimeTest.cpp:103: Failure Value of: ExecuteJavaScript("run_order.join(', ')") Actual: "c, b" Expected: "" Which is: 0x1159e26e0 [ FAILED ] VirtualTimeTest.AllowVirtualTimeToAdvance (61 ms) .
Message was sent while issue was closed.
Description was changed from ========== Per WebViewScheduler virtual time. This patch introduces a new per WebViewScheduler virtual time and wires it up into the BlinkTImers. NOTE there will be follow on patches where various other parts of blink will also use virtual time. For determinism some parts of blink will need to use the VirtualTime although new code should generally use the original non-virtual APIs. For more details see: https://docs.google.com/document/d/1y9KDT_ZEzT7pBeY6uzVt1dgKlwc1OB_vY4NZO1zBQ... BUG=546953 Committed: https://crrev.com/0d2d2840ad6d7f149569eb42332c8c8083401234 Cr-Commit-Position: refs/heads/master@{#377094} ========== to ========== Per WebViewScheduler virtual time. This patch introduces a new per WebViewScheduler virtual time and wires it up into the BlinkTImers. NOTE there will be follow on patches where various other parts of blink will also use virtual time. For determinism some parts of blink will need to use the VirtualTime although new code should generally use the original non-virtual APIs. For more details see: https://docs.google.com/document/d/1y9KDT_ZEzT7pBeY6uzVt1dgKlwc1OB_vY4NZO1zBQ... BUG=546953 Committed: https://crrev.com/0d2d2840ad6d7f149569eb42332c8c8083401234 Cr-Commit-Position: refs/heads/master@{#377094} ==========
Message was sent while issue was closed.
The test flake was caused because VirtualTimeTest::ExecuteJavaScript calls testing::runPendingTasks but the tests had not been updated to reflect that change. I've fixed that and they shouldn't flake now.
The CQ bit was checked by alexclarke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org, skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/1646583002/#ps680001 (title: "Deflake VirtualTimeTest.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1646583002/680001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1646583002/680001
The CQ bit was unchecked by alexclarke@chromium.org
The CQ bit was checked by alexclarke@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/1646583002/700001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1646583002/700001
lgtm to reland.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by alexclarke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org, skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/1646583002/#ps720001 (title: "Fix compile")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1646583002/720001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1646583002/720001
Message was sent while issue was closed.
Description was changed from ========== Per WebViewScheduler virtual time. This patch introduces a new per WebViewScheduler virtual time and wires it up into the BlinkTImers. NOTE there will be follow on patches where various other parts of blink will also use virtual time. For determinism some parts of blink will need to use the VirtualTime although new code should generally use the original non-virtual APIs. For more details see: https://docs.google.com/document/d/1y9KDT_ZEzT7pBeY6uzVt1dgKlwc1OB_vY4NZO1zBQ... BUG=546953 Committed: https://crrev.com/0d2d2840ad6d7f149569eb42332c8c8083401234 Cr-Commit-Position: refs/heads/master@{#377094} ========== to ========== Per WebViewScheduler virtual time. This patch introduces a new per WebViewScheduler virtual time and wires it up into the BlinkTImers. NOTE there will be follow on patches where various other parts of blink will also use virtual time. For determinism some parts of blink will need to use the VirtualTime although new code should generally use the original non-virtual APIs. For more details see: https://docs.google.com/document/d/1y9KDT_ZEzT7pBeY6uzVt1dgKlwc1OB_vY4NZO1zBQ... BUG=546953 Committed: https://crrev.com/0d2d2840ad6d7f149569eb42332c8c8083401234 Cr-Commit-Position: refs/heads/master@{#377094} ==========
Message was sent while issue was closed.
Committed patchset #35 (id:720001)
Message was sent while issue was closed.
Description was changed from ========== Per WebViewScheduler virtual time. This patch introduces a new per WebViewScheduler virtual time and wires it up into the BlinkTImers. NOTE there will be follow on patches where various other parts of blink will also use virtual time. For determinism some parts of blink will need to use the VirtualTime although new code should generally use the original non-virtual APIs. For more details see: https://docs.google.com/document/d/1y9KDT_ZEzT7pBeY6uzVt1dgKlwc1OB_vY4NZO1zBQ... BUG=546953 Committed: https://crrev.com/0d2d2840ad6d7f149569eb42332c8c8083401234 Cr-Commit-Position: refs/heads/master@{#377094} ========== to ========== Per WebViewScheduler virtual time. This patch introduces a new per WebViewScheduler virtual time and wires it up into the BlinkTImers. NOTE there will be follow on patches where various other parts of blink will also use virtual time. For determinism some parts of blink will need to use the VirtualTime although new code should generally use the original non-virtual APIs. For more details see: https://docs.google.com/document/d/1y9KDT_ZEzT7pBeY6uzVt1dgKlwc1OB_vY4NZO1zBQ... BUG=546953 Committed: https://crrev.com/0d2d2840ad6d7f149569eb42332c8c8083401234 Cr-Commit-Position: refs/heads/master@{#377094} Committed: https://crrev.com/c6f9c49b8221a2002ba771a3f108514a6c506ff0 Cr-Commit-Position: refs/heads/master@{#377287} ==========
Message was sent while issue was closed.
Patchset 35 (id:??) landed as https://crrev.com/c6f9c49b8221a2002ba771a3f108514a6c506ff0 Cr-Commit-Position: refs/heads/master@{#377287} |
