Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2185033002:
Emulation.setVirtualTimePolicy to have an optional virtual time budge (Closed)
|
Created:
4 years, 4 months ago by alex clarke (OOO till 29th) Modified:
4 years, 4 months ago CC:
chromium-reviews, caseq+blink_chromium.org, jam, lushnikov+blink_chromium.org, blink-reviews-api_chromium.org, pfeldman+blink_chromium.org, dglazkov+blink, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, darin-cc_chromium.org, sergeyv+blink_chromium.org, kinuko+watch, pfeldman, kozyatinskiy+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEmulation.setVirtualTimePolicy to have an optional virtual time budge
If virtualTimeBudgetSeconds is set then after the specified virtual
seconds have elapsed, virtual time pauses and an
Emulation.virtualTimeBudgetExpired event is sent.
BUG=546953
Committed: https://crrev.com/bf703da2bab3ddb36ce1b342b1a844e6dc048387
Cr-Commit-Position: refs/heads/master@{#409218}
Patch Set 1 #Patch Set 2 : Simplify patch #
Total comments: 8
Patch Set 3 : . #
Total comments: 4
Patch Set 4 : Rename parameter #
Messages
Total messages: 26 (13 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/v2/patch-status/codereview.chromium.or...
alexclarke@chromium.org changed reviewers: + dgozman@chromium.org, pfeldman@chromium.org, skyostil@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I wonder if we should instead have "virtual time limit" which both triggers a notification and also pauses everything at the same time? Otherwise you might end up in a different state depending on how quickly you can react to the time notification. WDYT?
What do we expect the client to do in the notification? Could you please provide some examples of intended usage?
Description was changed from ========== New Devtools event to notify when X virtual seconds have elapsed Adds Emulation.notifyAfterVirtualTimePeriod which sends a virtualTimeNotification after the specified number of virtual seconds since start have elapsed. BUG=546953 ========== to ========== Emulation.setVirtualTimePolicy to have an optional virtual time budge If virtualTimeBudgetSeconds is set then after the specified virtual seconds have elapsed, virtual time pauses and an Emulation.virtualTimeBudgetExpired event is sent. BUG=546953 ==========
This is a simplification of the patch. Does it look better?
This approach looks good. https://codereview.chromium.org/2185033002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/InspectorEmulationAgent.cpp (right): https://codereview.chromium.org/2185033002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/InspectorEmulationAgent.cpp:110: m_virtualTimeEnabled = true; Should we instead make enableVirtualTime() idempotent and remove this flag? https://codereview.chromium.org/2185033002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/InspectorEmulationAgent.cpp:113: if (in_virtualTimeBudgetSeconds.isJust()) { What happens if client calls setVirtualTimePolicy twice, with different budgets or policies? Let's maybe cancel previous timer? https://codereview.chromium.org/2185033002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/InspectorEmulationAgent.cpp:115: long long delayMillis = static_cast<long long>(in_virtualTimeBudgetSeconds.fromJust() * 1000.0); Let's make it milliseconds in protocol. https://codereview.chromium.org/2185033002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/InspectorEmulationAgent.cpp:116: taskRunner->postDelayedTask(BLINK_FROM_HERE, WTF::bind(&InspectorEmulationAgent::virtualTimeBudgetExpired, wrapPersistent(this)), delayMillis); Can we use weak pointer here? If client detaches before this fires, we will have a bunch of null pointer dereferences.
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
All done. https://codereview.chromium.org/2185033002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/InspectorEmulationAgent.cpp (right): https://codereview.chromium.org/2185033002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/InspectorEmulationAgent.cpp:110: m_virtualTimeEnabled = true; On 2016/08/01 16:03:31, dgozman wrote: > Should we instead make enableVirtualTime() idempotent and remove this flag? Actually it is idempotent already. We can remove the flag :) https://codereview.chromium.org/2185033002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/InspectorEmulationAgent.cpp:113: if (in_virtualTimeBudgetSeconds.isJust()) { On 2016/08/01 16:03:31, dgozman wrote: > What happens if client calls setVirtualTimePolicy twice, with different budgets > or policies? Let's maybe cancel previous timer? Canceling the previous timer seems like the best approach. https://codereview.chromium.org/2185033002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/InspectorEmulationAgent.cpp:115: long long delayMillis = static_cast<long long>(in_virtualTimeBudgetSeconds.fromJust() * 1000.0); On 2016/08/01 16:03:31, dgozman wrote: > Let's make it milliseconds in protocol. Done. https://codereview.chromium.org/2185033002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/InspectorEmulationAgent.cpp:116: taskRunner->postDelayedTask(BLINK_FROM_HERE, WTF::bind(&InspectorEmulationAgent::virtualTimeBudgetExpired, wrapPersistent(this)), delayMillis); On 2016/08/01 16:03:31, dgozman wrote: > Can we use weak pointer here? If client detaches before this fires, we will have > a bunch of null pointer dereferences. Good point. Using CancellableTaskFactory here does that (it's based on WeakPtr).
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.
https://codereview.chromium.org/2185033002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2185033002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:818: { "name": "virtualTimeBudgetMs", "type": "integer", "optional": true, "description": "If set, after virtualTimeBudgetMs has elapsed, virtual time pauses and a virtualTimeBudgetExpired event is sent." } Let's rename this to |budget|. https://codereview.chromium.org/2185033002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/InspectorEmulationAgent.cpp (right): https://codereview.chromium.org/2185033002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/InspectorEmulationAgent.cpp:120: frontend()->virtualTimeBudgetExpired(); Please also update devtools frontend code (add empty function for this event). Unfortunately, presubmit check does not trigger when we don't touch frontend, but we are working on it. Meanwhile, running third_party/WebKit/Source/devtools/scripts/compile_frontend.py will show all the errors.
PTAL https://codereview.chromium.org/2185033002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2185033002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:818: { "name": "virtualTimeBudgetMs", "type": "integer", "optional": true, "description": "If set, after virtualTimeBudgetMs has elapsed, virtual time pauses and a virtualTimeBudgetExpired event is sent." } On 2016/08/01 19:53:13, dgozman wrote: > Let's rename this to |budget|. Done. https://codereview.chromium.org/2185033002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/InspectorEmulationAgent.cpp (right): https://codereview.chromium.org/2185033002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/InspectorEmulationAgent.cpp:120: frontend()->virtualTimeBudgetExpired(); On 2016/08/01 19:53:13, dgozman wrote: > Please also update devtools frontend code (add empty function for this event). I'm not clear where such changes would be needed? I can't find anything on the lines of: EmulationDispatcher.prototype = ... > Unfortunately, presubmit check does not trigger when we don't touch frontend, > but we are working on it. > > Meanwhile, running > third_party/WebKit/Source/devtools/scripts/compile_frontend.py will show all the > errors. That doesn't show any any errors when I run it.
> https://codereview.chromium.org/2185033002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/web/InspectorEmulationAgent.cpp (right): > > https://codereview.chromium.org/2185033002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/web/InspectorEmulationAgent.cpp:120: > frontend()->virtualTimeBudgetExpired(); > On 2016/08/01 19:53:13, dgozman wrote: > > Please also update devtools frontend code (add empty function for this event). > > I'm not clear where such changes would be needed? I can't find anything on the > lines of: EmulationDispatcher.prototype = ... > > > Unfortunately, presubmit check does not trigger when we don't touch frontend, > > but we are working on it. > > > > Meanwhile, running > > third_party/WebKit/Source/devtools/scripts/compile_frontend.py will show all > the > > errors. > > That doesn't show any any errors when I run it. Sorry, my bad. We actually don't have an EmulationDispatcher because we never had any events there. Let's land as is. 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 ========== Emulation.setVirtualTimePolicy to have an optional virtual time budge If virtualTimeBudgetSeconds is set then after the specified virtual seconds have elapsed, virtual time pauses and an Emulation.virtualTimeBudgetExpired event is sent. BUG=546953 ========== to ========== Emulation.setVirtualTimePolicy to have an optional virtual time budge If virtualTimeBudgetSeconds is set then after the specified virtual seconds have elapsed, virtual time pauses and an Emulation.virtualTimeBudgetExpired event is sent. BUG=546953 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Emulation.setVirtualTimePolicy to have an optional virtual time budge If virtualTimeBudgetSeconds is set then after the specified virtual seconds have elapsed, virtual time pauses and an Emulation.virtualTimeBudgetExpired event is sent. BUG=546953 ========== to ========== Emulation.setVirtualTimePolicy to have an optional virtual time budge If virtualTimeBudgetSeconds is set then after the specified virtual seconds have elapsed, virtual time pauses and an Emulation.virtualTimeBudgetExpired event is sent. BUG=546953 Committed: https://crrev.com/bf703da2bab3ddb36ce1b342b1a844e6dc048387 Cr-Commit-Position: refs/heads/master@{#409218} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/bf703da2bab3ddb36ce1b342b1a844e6dc048387 Cr-Commit-Position: refs/heads/master@{#409218} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
