Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2109843003:
Adds enableVirtualTime and setVirtualTimePolicy To Emulation domain. (Closed)
|
Created:
4 years, 5 months ago by alex clarke (OOO till 29th) Modified:
4 years, 4 months ago CC:
chromium-reviews, tyoshino+watch_chromium.org, kinuko+watch, Yoav Weiss, lushnikov+blink_chromium.org, blink-reviews-api_chromium.org, pfeldman+blink_chromium.org, dglazkov+blink, caseq+blink_chromium.org, apavlov+blink_chromium.org, gavinp+loader_chromium.org, devtools-reviews_chromium.org, blink-reviews, loading-reviews+fetch_chromium.org, sergeyv+blink_chromium.org, Nate Chapin, scheduler-bugs_chromium.org, loading-reviews_chromium.org, kozyatinskiy+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds enableVirtualTime and setVirtualTimePolicy To Emulation domain.
This allows virtual time to be controlled via DevTools protocol.
For more details about virtual time see here: https://docs.google.com/document/d/1y9KDT_ZEzT7pBeY6uzVt1dgKlwc1OB_vY4NZO1zBQmo/edit
BUG=546953
Committed: https://crrev.com/9599120a7c4d3532a0aaeed18b81ed6e9892381c
Cr-Commit-Position: refs/heads/master@{#403943}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Added some tests #Patch Set 3 : Use a WeakPtr to solve WebViewSchduler <= ResourceLoader lifetime issue #Patch Set 4 : Addressing Sami's comments #Patch Set 5 : Fix test crashes #Patch Set 6 : Fix test crash #
Total comments: 10
Patch Set 7 : Fix layering issue #
Total comments: 6
Patch Set 8 : Addressing Sami's comments #
Total comments: 2
Patch Set 9 : Rebase + address nit #
Total comments: 10
Patch Set 10 : Changes for dgozman #Patch Set 11 : Remove frame iteration #Messages
Total messages: 88 (35 generated)
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/v2/patch-status/codereview.chromium.or...
I'd like to write some sort of test for this but I'm not sure what would be appropriate given it's in blink. Any ideas?
Description was changed from ========== Adds enableVirtualTime and setVirtualTimePolicy To Emulation domain. This allows virtual time to be controlled via DevTools protocol. BUG=546953 ========== to ========== Adds enableVirtualTime and setVirtualTimePolicy To Emulation domain. This allows virtual time to be controlled via DevTools protocol. For more details about virtual time see here: https://docs.google.com/document/d/1y9KDT_ZEzT7pBeY6uzVt1dgKlwc1OB_vY4NZO1zBQ... BUG=546953 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2109843003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2109843003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/inspector/browser_protocol.json:771: "name": "setVirtualTimePolicy", Would renderer suspension also fall under this? Also wondering about explicit BeginFrame control. https://codereview.chromium.org/2109843003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/WebViewScheduler.cpp (right): https://codereview.chromium.org/2109843003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/WebViewScheduler.cpp:29: m_pendingResourceLoadCount--; DCHECK >= 0? https://codereview.chromium.org/2109843003/diff/1/third_party/WebKit/public/p... File third_party/WebKit/public/platform/WebViewScheduler.h (right): https://codereview.chromium.org/2109843003/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/WebViewScheduler.h:53: void incrementPendingResorceLoadCount(); typo: Resource
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
alexclarke@chromium.org changed reviewers: + pfeldman@chromium.org
PTAL +Pavel https://codereview.chromium.org/2109843003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2109843003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/inspector/browser_protocol.json:771: "name": "setVirtualTimePolicy", On 2016/06/29 16:22:27, Sami wrote: > Would renderer suspension also fall under this? Also wondering about explicit > BeginFrame control. Potentially yes. I think we need to prototype that to be sure however. https://codereview.chromium.org/2109843003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/WebViewScheduler.cpp (right): https://codereview.chromium.org/2109843003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/WebViewScheduler.cpp:29: m_pendingResourceLoadCount--; On 2016/06/29 16:22:27, Sami wrote: > DCHECK >= 0? Done. https://codereview.chromium.org/2109843003/diff/1/third_party/WebKit/public/p... File third_party/WebKit/public/platform/WebViewScheduler.h (right): https://codereview.chromium.org/2109843003/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/WebViewScheduler.h:53: void incrementPendingResorceLoadCount(); On 2016/06/29 16:22:27, Sami wrote: > typo: Resource Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.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 to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #6 (id:100001) has been deleted
Patchset #5 (id:80001) 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/v2/patch-status/codereview.chromium.or...
alexclarke@chromium.org changed reviewers: + dgozman@chromium.org
Looks like Pavel is OOO. Dimitri would you mind taking a look?
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I only took a close look at inspector-related part. Please tell if you need something else from me. https://codereview.chromium.org/2109843003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/FetchContext.h (right): https://codereview.chromium.org/2109843003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/FetchContext.h:117: virtual WebViewScheduler* webViewScheduler() const { return nullptr; } I find it strange to have WebViewScheduler mentioned in FetchContenxt. Isn't this a layering violation? https://codereview.chromium.org/2109843003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2109843003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:767: "description": "Turns on virtualTime.", More explanation about virtualTime won't hurt here. https://codereview.chromium.org/2109843003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:768: "handlers": ["renderer"] nit: "renderer" is a default value, so you can just omit "handlers" property. https://codereview.chromium.org/2109843003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:775: "enum": [ Let's declare enum in "types" section, with description of each value. They are quite different. https://codereview.chromium.org/2109843003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/WebViewScheduler.cpp (right): https://codereview.chromium.org/2109843003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/WebViewScheduler.cpp:67: if (policy == advance) { This string->enum conversion should happen in InspectorEmulationAgent. There should already be constants generated by inspector-protocol, e.g. protocol::Emulation::VirtualTimePolicyEnum::Advance.
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/v2/patch-status/codereview.chromium.or...
Thanks I've addressed your feebback. Does this look better now? https://codereview.chromium.org/2109843003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/FetchContext.h (right): https://codereview.chromium.org/2109843003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/FetchContext.h:117: virtual WebViewScheduler* webViewScheduler() const { return nullptr; } On 2016/06/30 17:32:08, dgozman wrote: > I find it strange to have WebViewScheduler mentioned in FetchContenxt. Isn't > this a layering violation? I think this is a good point and it seems it's not necessary to add this here. The FrameFetchContext looks a better place to tell the WebViewScheduler about pending fetches. https://codereview.chromium.org/2109843003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2109843003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:767: "description": "Turns on virtualTime.", On 2016/06/30 17:32:08, dgozman wrote: > More explanation about virtualTime won't hurt here. Done. https://codereview.chromium.org/2109843003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:768: "handlers": ["renderer"] On 2016/06/30 17:32:08, dgozman wrote: > nit: "renderer" is a default value, so you can just omit "handlers" property. Done. https://codereview.chromium.org/2109843003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:775: "enum": [ On 2016/06/30 17:32:08, dgozman wrote: > Let's declare enum in "types" section, with description of each value. They are > quite different. Done. https://codereview.chromium.org/2109843003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/WebViewScheduler.cpp (right): https://codereview.chromium.org/2109843003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/WebViewScheduler.cpp:67: if (policy == advance) { On 2016/06/30 17:32:08, dgozman wrote: > This string->enum conversion should happen in InspectorEmulationAgent. > There should already be constants generated by inspector-protocol, e.g. > protocol::Emulation::VirtualTimePolicyEnum::Advance. Ah I didn't know about those. This is nicer now, thanks!
Thanks I've addressed your feebback. Does this look better now? https://codereview.chromium.org/2109843003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/FetchContext.h (right): https://codereview.chromium.org/2109843003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/FetchContext.h:117: virtual WebViewScheduler* webViewScheduler() const { return nullptr; } On 2016/06/30 17:32:08, dgozman wrote: > I find it strange to have WebViewScheduler mentioned in FetchContenxt. Isn't > this a layering violation? I think this is a good point and it seems it's not necessary to add this here. The FrameFetchContext looks a better place to tell the WebViewScheduler about pending fetches. https://codereview.chromium.org/2109843003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2109843003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:767: "description": "Turns on virtualTime.", On 2016/06/30 17:32:08, dgozman wrote: > More explanation about virtualTime won't hurt here. Done. https://codereview.chromium.org/2109843003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:768: "handlers": ["renderer"] On 2016/06/30 17:32:08, dgozman wrote: > nit: "renderer" is a default value, so you can just omit "handlers" property. Done. https://codereview.chromium.org/2109843003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:775: "enum": [ On 2016/06/30 17:32:08, dgozman wrote: > Let's declare enum in "types" section, with description of each value. They are > quite different. Done. https://codereview.chromium.org/2109843003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/WebViewScheduler.cpp (right): https://codereview.chromium.org/2109843003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/WebViewScheduler.cpp:67: if (policy == advance) { On 2016/06/30 17:32:08, dgozman wrote: > This string->enum conversion should happen in InspectorEmulationAgent. > There should already be constants generated by inspector-protocol, e.g. > protocol::Emulation::VirtualTimePolicyEnum::Advance. Ah I didn't know about those. This is nicer now, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
https://codereview.chromium.org/2109843003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/WebViewScheduler.cpp (right): https://codereview.chromium.org/2109843003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/WebViewScheduler.cpp:9: WebViewScheduler::WebViewScheduler() Let's avoid forking WebViewScheduler and WebViewSchedulerImpl like this and instead implement this in the latter. We're in the about move WebViewSchedulerImpl to Blink soon anyway so I think that will simplify the transition. https://codereview.chromium.org/2109843003/diff/160001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebViewScheduler.h (right): https://codereview.chromium.org/2109843003/diff/160001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebViewScheduler.h:52: void incrementPendingResourceLoadCount(); Could you add these on the WebFrameScheduler instead? Internally they could just call into WebViewSchedulerImpl, but it feels like keeping the frame association might be useful.
https://codereview.chromium.org/2109843003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2109843003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:776: "name": "enableVirtualTime", this should either be setVirtualTimeEnabled(bool) or should not exist at all and policy should have an additional "disabled" value. Sorry, only looked at this file.
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
PTAL https://codereview.chromium.org/2109843003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2109843003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:776: "name": "enableVirtualTime", On 2016/07/01 15:14:13, pfeldman_slow wrote: > this should either be setVirtualTimeEnabled(bool) or should not exist at all and > policy should have an additional "disabled" value. Sorry, only looked at this > file. I know where you're coming from but I'm not sure it makes sense to disable virtual time. The virtual time base may get far ahead of real time which means posted timers could get "stranded in the future". https://codereview.chromium.org/2109843003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/WebViewScheduler.cpp (right): https://codereview.chromium.org/2109843003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/WebViewScheduler.cpp:9: WebViewScheduler::WebViewScheduler() On 2016/07/01 14:46:30, Sami wrote: > Let's avoid forking WebViewScheduler and WebViewSchedulerImpl like this and > instead implement this in the latter. We're in the about move > WebViewSchedulerImpl to Blink soon anyway so I think that will simplify the > transition. Done. https://codereview.chromium.org/2109843003/diff/160001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebViewScheduler.h (right): https://codereview.chromium.org/2109843003/diff/160001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebViewScheduler.h:52: void incrementPendingResourceLoadCount(); On 2016/07/01 14:46:30, Sami wrote: > Could you add these on the WebFrameScheduler instead? Internally they could just > call into WebViewSchedulerImpl, but it feels like keeping the frame association > might be useful. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm. https://codereview.chromium.org/2109843003/diff/180001/components/scheduler/r... File components/scheduler/renderer/web_view_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/2109843003/diff/180001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_unittest.cc:340: blink::WebViewScheduler::VirtualTimePolicy::PAUSE); nit: no need for the namespace prefix with the 'using' (below too).
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
https://codereview.chromium.org/2109843003/diff/180001/components/scheduler/r... File components/scheduler/renderer/web_view_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/2109843003/diff/180001/components/scheduler/r... components/scheduler/renderer/web_view_scheduler_impl_unittest.cc:340: blink::WebViewScheduler::VirtualTimePolicy::PAUSE); On 2016/07/05 10:26:55, Sami wrote: > nit: no need for the namespace prefix with the 'using' (below too). Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sorry for delay, got holidays here. https://codereview.chromium.org/2109843003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2109843003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:682: "description": "For a description of the virtual time policies see: https://docs.google.com/document/d/1y9KDT_ZEzT7pBeY6uzVt1dgKlwc1OB_vY4NZO1zBQmo" We prefer to not include links here, since it's published for end-users. Can we copy descriptions from the last table in the document? https://codereview.chromium.org/2109843003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:776: "name": "enableVirtualTime", Why can't we disable it? https://codereview.chromium.org/2109843003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:781: "name": "setVirtualTimePolicy", Should we add "disabled" to the list of policies and have just a single method? Client will have to enable and immediately set the policy anyway. Let's make any policy automatically enable virtual time? https://codereview.chromium.org/2109843003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/InspectorEmulationAgent.cpp (right): https://codereview.chromium.org/2109843003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/web/InspectorEmulationAgent.cpp:98: for (WebLocalFrame* frame = m_webLocalFrameImpl; frame; frame = frame->traverseNextLocal(false)) { We have InspectedFrames class, which allows to iterate over frames being inspected by this agent. Let's pass it in constructor and use here.
https://codereview.chromium.org/2109843003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2109843003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:682: "description": "For a description of the virtual time policies see: https://docs.google.com/document/d/1y9KDT_ZEzT7pBeY6uzVt1dgKlwc1OB_vY4NZO1zBQmo" On 2016/07/05 15:30:35, dgozman wrote: > We prefer to not include links here, since it's published for end-users. Can we > copy descriptions from the last table in the document? Done. https://codereview.chromium.org/2109843003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:776: "name": "enableVirtualTime", On 2016/07/05 15:30:35, dgozman wrote: > Why can't we disable it? We could but I'm not sure it's a good idea to allow it. The reason being the virtual time base could get far ahead of real time and if we disable virtual time that would leave any timers posted "stranded in the future". https://codereview.chromium.org/2109843003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:781: "name": "setVirtualTimePolicy", On 2016/07/05 15:30:35, dgozman wrote: > Should we add "disabled" See above. to the list of policies and have just a single method? > Client will have to enable and immediately set the policy anyway. Let's make any > policy automatically enable virtual time? Sure we can reduce this to a single method. https://codereview.chromium.org/2109843003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/InspectorEmulationAgent.cpp (right): https://codereview.chromium.org/2109843003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/web/InspectorEmulationAgent.cpp:98: for (WebLocalFrame* frame = m_webLocalFrameImpl; frame; frame = frame->traverseNextLocal(false)) { On 2016/07/05 15:30:35, dgozman wrote: > We have InspectedFrames class, which allows to iterate over frames being > inspected by this agent. Let's pass it in constructor and use here. I tried that but the problem is we need to get hold of the WebViewScheduler which is exposed on WebView but not FrameView (the InspectedFrames iterates over LocalFrame*).
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
https://codereview.chromium.org/2109843003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/InspectorEmulationAgent.cpp (right): https://codereview.chromium.org/2109843003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/web/InspectorEmulationAgent.cpp:98: for (WebLocalFrame* frame = m_webLocalFrameImpl; frame; frame = frame->traverseNextLocal(false)) { On 2016/07/05 16:51:23, alex clarke wrote: > On 2016/07/05 15:30:35, dgozman wrote: > > We have InspectedFrames class, which allows to iterate over frames being > > inspected by this agent. Let's pass it in constructor and use here. > > I tried that but the problem is we need to get hold of the WebViewScheduler > which is exposed on WebView but not FrameView (the InspectedFrames iterates over > LocalFrame*). I don't think we should be introducing new usages of traverseNextLocal. It was a bit of a hack that we added to make find-in-page not crash as much when it didn't fully support OOPI. I don't fully understand the requirements here, but I'm happy to schedule a short meeting so we can figure out the right path forward.
https://codereview.chromium.org/2109843003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/InspectorEmulationAgent.cpp (right): https://codereview.chromium.org/2109843003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/web/InspectorEmulationAgent.cpp:98: for (WebLocalFrame* frame = m_webLocalFrameImpl; frame; frame = frame->traverseNextLocal(false)) { On 2016/07/06 01:09:48, dcheng wrote: > On 2016/07/05 16:51:23, alex clarke wrote: > > On 2016/07/05 15:30:35, dgozman wrote: > > > We have InspectedFrames class, which allows to iterate over frames being > > > inspected by this agent. Let's pass it in constructor and use here. > > > > I tried that but the problem is we need to get hold of the WebViewScheduler > > which is exposed on WebView but not FrameView (the InspectedFrames iterates > over > > LocalFrame*). > > I don't think we should be introducing new usages of traverseNextLocal. It was a > bit of a hack that we added to make find-in-page not crash as much when it > didn't fully support OOPI. I don't fully understand the requirements here, but > I'm happy to schedule a short meeting so we can figure out the right path > forward. I didn't realize it was depreciated, good to know. Thinking about it now, I don't know why I thought it was necessary to iterate over all frames. They all have the same WebViewScheduler... We can safely remove that :)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
Thanks for the review :)
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/2109843003/#ps240001 (title: "Remove frame iteration")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
alexclarke@chromium.org changed reviewers: + jochen@chromium.org
+jochen Can I please get an OWNERS review for platform/ ?
platform lgtm
Thanks!
The CQ bit was checked by alexclarke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Adds enableVirtualTime and setVirtualTimePolicy To Emulation domain. This allows virtual time to be controlled via DevTools protocol. For more details about virtual time see here: https://docs.google.com/document/d/1y9KDT_ZEzT7pBeY6uzVt1dgKlwc1OB_vY4NZO1zBQ... BUG=546953 ========== to ========== Adds enableVirtualTime and setVirtualTimePolicy To Emulation domain. This allows virtual time to be controlled via DevTools protocol. For more details about virtual time see here: https://docs.google.com/document/d/1y9KDT_ZEzT7pBeY6uzVt1dgKlwc1OB_vY4NZO1zBQ... BUG=546953 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:240001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Adds enableVirtualTime and setVirtualTimePolicy To Emulation domain. This allows virtual time to be controlled via DevTools protocol. For more details about virtual time see here: https://docs.google.com/document/d/1y9KDT_ZEzT7pBeY6uzVt1dgKlwc1OB_vY4NZO1zBQ... BUG=546953 ========== to ========== Adds enableVirtualTime and setVirtualTimePolicy To Emulation domain. This allows virtual time to be controlled via DevTools protocol. For more details about virtual time see here: https://docs.google.com/document/d/1y9KDT_ZEzT7pBeY6uzVt1dgKlwc1OB_vY4NZO1zBQ... BUG=546953 Committed: https://crrev.com/9599120a7c4d3532a0aaeed18b81ed6e9892381c Cr-Commit-Position: refs/heads/master@{#403943} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/9599120a7c4d3532a0aaeed18b81ed6e9892381c Cr-Commit-Position: refs/heads/master@{#403943}
Message was sent while issue was closed.
On 2016/07/06 19:48:31, commit-bot: I haz the power wrote: > Patchset 11 (id:??) landed as > https://crrev.com/9599120a7c4d3532a0aaeed18b81ed6e9892381c > Cr-Commit-Position: refs/heads/master@{#403943} This causes a crash when I run layout tests locally. Happens for my colleague too. Not sure why it doesn't happen on the bots? $ third_party/WebKit/Tools/Scripts/run-webkit-tests --target=Default --no-retry-failures http/tests/serviceworker/fetch-csp.html [1/1] http/tests/serviceworker/fetch-csp.html failed unexpectedly (renderer crashed) crash log for renderer (pid <unknown>): STDOUT: CONSOLE ERROR: Refused to load the image 'http://localhost:8000/serviceworker/resources/fetch-access-control.php?PNGIMAGE' because it violates the following Content Security Policy directive: "img-src http://127.0.0.1:8000". STDOUT: STDOUT: CONSOLE ERROR: Refused to load the image 'http://localhost:8000/serviceworker/resources/fetch-access-control.php?PNGIMAGE' because it violates the following Content Security Policy directive: "img-src http://127.0.0.1:8000". STDOUT: STDOUT: #CRASHED - renderer STDERR: [1:1:0707/142610:1282082602883:FATAL:web_view_scheduler_impl.cc(135)] Check failed: pending_resource_load_count_ >= 0 (-1 vs. 0) STDERR: #0 0x7f2f9a1cb53e base::debug::StackTrace::StackTrace() STDERR: #1 0x7f2f9a1ec69b logging::LogMessage::~LogMessage() STDERR: #2 0x7f2f92a0a0eb scheduler::WebViewSchedulerImpl::decrementPendingResourceLoadCount() STDERR: #3 0x7f2f944e92b0 blink::FrameFetchContext::dispatchDidFail() STDERR: #4 0x7f2f943ab1e5 blink::ResourceFetcher::didReceiveResponse() STDERR: #5 0x7f2f943b3a7c blink::ResourceLoader::didReceiveResponse() STDERR: #6 0x7f2f98c84e25 content::WebURLLoaderImpl::Context::OnReceivedResponse() STDERR: #7 0x7f2f98c63158 content::ResourceDispatcher::OnReceivedResponse() STDERR: #8 0x7f2f98c6594d
Message was sent while issue was closed.
Filed https://bugs.chromium.org/p/chromium/issues/detail?id=626218
Message was sent while issue was closed.
https://youtu.be/s2mEASqcipA?list=UUWQUZfAdokV83KKjLl-sDeQ On Thu, Jul 7, 2016 at 11:11 AM, <falken@chromium.org> wrote: > Filed https://bugs.chromium.org/p/chromium/issues/detail?id=626218 > > https://codereview.chromium.org/2109843003/ > > -- > You received this message because you are subscribed to the Google Groups > "DevTools Reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to devtools-reviews+unsubscribe@chromium.org. > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
https://youtu.be/s2mEASqcipA?list=UUWQUZfAdokV83KKjLl-sDeQ On Thu, Jul 7, 2016 at 11:11 AM, <falken@chromium.org> wrote: > Filed https://bugs.chromium.org/p/chromium/issues/detail?id=626218 > > https://codereview.chromium.org/2109843003/ > > -- > You received this message because you are subscribed to the Google Groups > "DevTools Reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to devtools-reviews+unsubscribe@chromium.org. > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
