Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1299643004:
Make synthesizeScrollGesture repetable and emit interaction markers (Closed)
|
Created:
5 years, 4 months ago by alex clarke (OOO till 29th) Modified:
5 years, 4 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, yurys, devtools-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake synthesizeScrollGesture repetable and emit interaction markers
Adds new optional parameters to Input.synthesizeScrollGesture to make it
possible to generate repeating scrolls from the browser process. This
functionality will be used by smoothness.scrolling_tough_ad_cases.
Patch 1 of 4 to fix the overly long delay between
each scroll in smoothness.scrolling_tough_ad_cases caused by the render
thread being unresponsive.
Patch 2: https://codereview.chromium.org/1296993002/
Patch 3: https://codereview.chromium.org/1291513004/
Patch 4: https://codereview.chromium.org/1285133008/
BUG=510398
Committed: https://crrev.com/20301011d994428bf5850842ae9c18bde9d24674
Cr-Commit-Position: refs/heads/master@{#343878}
Patch Set 1 #Patch Set 2 : Oh it has to be a 3-sided patch #
Total comments: 2
Patch Set 3 : Whoops send SendSynthesizeScrollGestureResponse when done #
Total comments: 4
Patch Set 4 : Added a TODO #
Messages
Total messages: 29 (9 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/1299643004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1299643004/1
alexclarke@chromium.org changed reviewers: + skyostil@chromium.org
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_...)
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/1299643004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1299643004/20001
skyostil@chromium.org changed reviewers: + pfeldman@chromium.org
Looks good. +pfeldman@ to review for real. https://codereview.chromium.org/1299643004/diff/20001/content/browser/devtool... File content/browser/devtools/protocol/input_handler.cc (right): https://codereview.chromium.org/1299643004/diff/20001/content/browser/devtool... content/browser/devtools/protocol/input_handler.cc:453: SendSynthesizePinchGestureResponse(command_id, result); Should this be a scroll gesture response instead?
https://codereview.chromium.org/1299643004/diff/20001/content/browser/devtool... File content/browser/devtools/protocol/input_handler.cc (right): https://codereview.chromium.org/1299643004/diff/20001/content/browser/devtool... content/browser/devtools/protocol/input_handler.cc:453: SendSynthesizePinchGestureResponse(command_id, result); On 2015/08/17 16:35:48, Sami wrote: > Should this be a scroll gesture response instead? Whoops. Yes :)
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/1299643004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1299643004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, but can we follow up on the blink.console please? https://codereview.chromium.org/1299643004/diff/40001/content/browser/devtool... File content/browser/devtools/protocol/input_handler.cc (right): https://codereview.chromium.org/1299643004/diff/40001/content/browser/devtool... content/browser/devtools/protocol/input_handler.cc:420: TRACE_EVENT_COPY_ASYNC_BEGIN0("blink.console", This is weird, does it really rely on those? Can we remove the dependency?
https://codereview.chromium.org/1299643004/diff/40001/content/browser/devtool... File content/browser/devtools/protocol/input_handler.cc (right): https://codereview.chromium.org/1299643004/diff/40001/content/browser/devtool... content/browser/devtools/protocol/input_handler.cc:420: TRACE_EVENT_COPY_ASYNC_BEGIN0("blink.console", On 2015/08/17 17:45:52, pfeldman wrote: > This is weird, does it really rely on those? Can we remove the dependency? Unfortunately yes. Telemetry looks for events between interaction markers and that's not an easy thing to change. We could potentially move there these are emitted (e.g. perhaps to the synthetic gesture code), although we'd need some way of telling that code the |interaction_marker_name|. I've added a TODO.
The CQ bit was checked by alexclarke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pfeldman@chromium.org Link to the patchset: https://codereview.chromium.org/1299643004/#ps60001 (title: "Added a TODO")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1299643004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1299643004/60001
On 2015/08/18 09:39:08, alexclarke1 wrote: > https://codereview.chromium.org/1299643004/diff/40001/content/browser/devtool... > File content/browser/devtools/protocol/input_handler.cc (right): > > https://codereview.chromium.org/1299643004/diff/40001/content/browser/devtool... > content/browser/devtools/protocol/input_handler.cc:420: > TRACE_EVENT_COPY_ASYNC_BEGIN0("blink.console", > On 2015/08/17 17:45:52, pfeldman wrote: > > This is weird, does it really rely on those? Can we remove the dependency? > > Unfortunately yes. Telemetry looks for events between interaction markers and > that's not an easy thing to change. We could potentially move there these are > emitted (e.g. perhaps to the synthetic gesture code), although we'd need some > way of telling that code the |interaction_marker_name|. > > I've added a TODO. s/potentially move there these/potentially move where these
https://codereview.chromium.org/1299643004/diff/40001/content/browser/devtool... File content/browser/devtools/protocol/input_handler.cc (right): https://codereview.chromium.org/1299643004/diff/40001/content/browser/devtool... content/browser/devtools/protocol/input_handler.cc:420: TRACE_EVENT_COPY_ASYNC_BEGIN0("blink.console", On 2015/08/18 09:39:07, alexclarke1 wrote: > On 2015/08/17 17:45:52, pfeldman wrote: > > This is weird, does it really rely on those? Can we remove the dependency? > > Unfortunately yes. Telemetry looks for events between interaction markers and > that's not an easy thing to change. We could potentially move there these are > emitted (e.g. perhaps to the synthetic gesture code), although we'd need some > way of telling that code the |interaction_marker_name|. > > I've added a TODO. Right, telemetry needs these markers to find where in the timeline the interesting interactions happened. Previously these were emitted by the JS in the renderer, but since we're doing scrolling in the browser now we also need to emit the records there to ensure that they fully enclose the individual scroll events and rendered frames. Like Alex also said, the other way I could imagine this being done would be for the synthetic gesture controller to emit these events. It'd be a little inconvenient for some of telemetry's actions which want to perform multiple gestures inside a single interaction record. Having the trace event here might be the best we can do for now.
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/20301011d994428bf5850842ae9c18bde9d24674 Cr-Commit-Position: refs/heads/master@{#343878}
Message was sent while issue was closed.
https://codereview.chromium.org/1299643004/diff/40001/content/browser/devtool... File content/browser/devtools/protocol/input_handler.cc (right): https://codereview.chromium.org/1299643004/diff/40001/content/browser/devtool... content/browser/devtools/protocol/input_handler.cc:420: TRACE_EVENT_COPY_ASYNC_BEGIN0("blink.console", Could we at least use a different category for this? This is clearly not blink.console.
Message was sent while issue was closed.
It doesn't work if its not blink.console although presumably something could change on the telemetry side. On 18 Aug 2015 18:58, <pfeldman@chromium.org> wrote: > > > https://codereview.chromium.org/1299643004/diff/40001/content/browser/devtool... > File content/browser/devtools/protocol/input_handler.cc (right): > > > https://codereview.chromium.org/1299643004/diff/40001/content/browser/devtool... > content/browser/devtools/protocol/input_handler.cc:420: > TRACE_EVENT_COPY_ASYNC_BEGIN0("blink.console", > Could we at least use a different category for this? This is clearly not > blink.console. > > https://codereview.chromium.org/1299643004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2015/08/18 18:01:42, chromium-reviews wrote: > It doesn't work if its not blink.console although presumably something > could change on the telemetry side. We should change it since blink.console is for console.log.
Message was sent while issue was closed.
On 2015/08/18 18:02:53, pfeldman wrote: > On 2015/08/18 18:01:42, chromium-reviews wrote: > > It doesn't work if its not blink.console although presumably something > > could change on the telemetry side. > > We should change it since blink.console is for console.log. Agreed. I think the metric should work if we use the "benchmark" category here. If not, we can fix the metric to work with what we choose here. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
