Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1013753014:
Re-introduce ScrollY trace event. (Closed)
DescriptionRe-introduce ScrollY trace event.
The refactor in https://codereview.chromium.org/23983047 removed a
trace event that monitored y scroll offset. This CL restores that event.
BUG=468304
Committed: https://crrev.com/56f35ff950a6c04776212d9934c57c23f189cb73
Cr-Commit-Position: refs/heads/master@{#322266}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Revise trace category and comment. #
Total comments: 3
Patch Set 3 : Limit trace to InnerViewportScrollLayer. #
Total comments: 2
Patch Set 4 : Remove comment. #Messages
Total messages: 16 (3 generated)
wjmaclean@chromium.org changed reviewers: + enne@chromium.org
enne@ - I believe this restores the desired trace event, can you please take a look?
https://codereview.chromium.org/1013753014/diff/1/cc/layers/layer_impl.cc File cc/layers/layer_impl.cc (right): https://codereview.chromium.org/1013753014/diff/1/cc/layers/layer_impl.cc#new... cc/layers/layer_impl.cc:1332: // Get the current_offset_.y() value for a sanity-check on scrolling This comment seems unnecessary. https://codereview.chromium.org/1013753014/diff/1/cc/layers/layer_impl.cc#new... cc/layers/layer_impl.cc:1336: TRACE_COUNTER_ID1("gpu", "scroll_offset_y", this->id(), This should be a cc trace, I think? Should we only do this on the main scrolling layer, or does every scrolling layer get a trace?
wjmaclean@chromium.org changed reviewers: + paulirish@chromium.org
https://codereview.chromium.org/1013753014/diff/1/cc/layers/layer_impl.cc File cc/layers/layer_impl.cc (right): https://codereview.chromium.org/1013753014/diff/1/cc/layers/layer_impl.cc#new... cc/layers/layer_impl.cc:1332: // Get the current_offset_.y() value for a sanity-check on scrolling On 2015/03/24 17:53:21, enne wrote: > This comment seems unnecessary. Don't we want to indicate why we're interested in recording the scroll offsets? I've edited the comment to make it briefer and indicate current_offset isn't a member variable, but it can go away entirely if you like. https://codereview.chromium.org/1013753014/diff/1/cc/layers/layer_impl.cc#new... cc/layers/layer_impl.cc:1336: TRACE_COUNTER_ID1("gpu", "scroll_offset_y", this->id(), On 2015/03/24 17:53:21, enne wrote: > This should be a cc trace, I think? Done. > Should we only do this on the main scrolling layer, or does every scrolling > layer get a trace? The previous implementation seems to have reported for every scrolling layer (presumably this is why the id() was previously included). +paulirish@ Paul, did you just want this trace for the main scrolling layer? (If so, does that mean both inner/outer viewports for pinch-zoom?)
https://codereview.chromium.org/1013753014/diff/20001/cc/layers/layer_impl.cc File cc/layers/layer_impl.cc (right): https://codereview.chromium.org/1013753014/diff/20001/cc/layers/layer_impl.cc... cc/layers/layer_impl.cc:1333: // BasicMouseWheelSmoothScrollGesture has proper scroll curves. What is BasicMouseWheelSmoothScrollGesture? I can't find any reference to that in Chromium.
https://codereview.chromium.org/1013753014/diff/20001/cc/layers/layer_impl.cc File cc/layers/layer_impl.cc (right): https://codereview.chromium.org/1013753014/diff/20001/cc/layers/layer_impl.cc... cc/layers/layer_impl.cc:1333: // BasicMouseWheelSmoothScrollGesture has proper scroll curves. What is BasicMouseWheelSmoothScrollGesture? I can't find any reference to that in Chromium.
> Paul, did you just want this trace for the main scrolling layer? (If so, does that mean both inner/outer viewports for pinch-zoom?) The pinch-zoom use-case isn't too important to me for this scroll reporting, so only reporting for the main scrolling layer sgtm.
https://codereview.chromium.org/1013753014/diff/20001/cc/layers/layer_impl.cc File cc/layers/layer_impl.cc (right): https://codereview.chromium.org/1013753014/diff/20001/cc/layers/layer_impl.cc... cc/layers/layer_impl.cc:1333: // BasicMouseWheelSmoothScrollGesture has proper scroll curves. On 2015/03/25 17:21:33, enne wrote: > What is BasicMouseWheelSmoothScrollGesture? I can't find any reference to that > in Chromium. Revised the comment to reflect the rationale given in the associated bug, namely the desire to evaluate scroll latency via tracing. https://codereview.chromium.org/1013753014/diff/20001/cc/layers/layer_impl.cc... cc/layers/layer_impl.cc:1334: if (y_offset_did_change && layer_tree_impl()->IsActiveTree()) Will limit this to the InnerViewportScrollLayer() as per Paul's suggestion.
https://codereview.chromium.org/1013753014/diff/40001/cc/layers/layer_impl.cc File cc/layers/layer_impl.cc (right): https://codereview.chromium.org/1013753014/diff/40001/cc/layers/layer_impl.cc... cc/layers/layer_impl.cc:1332: // Record the current_offset.y() value so we can evaluate scroll latency At this point, I think this comment doesn't add anything. It's a trace event for scroll offset, and so saying it's about evaluating scrolling in traces is redundant.
https://codereview.chromium.org/1013753014/diff/40001/cc/layers/layer_impl.cc File cc/layers/layer_impl.cc (right): https://codereview.chromium.org/1013753014/diff/40001/cc/layers/layer_impl.cc... cc/layers/layer_impl.cc:1332: // Record the current_offset.y() value so we can evaluate scroll latency On 2015/03/25 20:52:04, enne wrote: > At this point, I think this comment doesn't add anything. It's a trace event > for scroll offset, and so saying it's about evaluating scrolling in traces is > redundant. Removed. I thought the notion of measuring latency might be worth documenting, but I guess that's implicit in that all trace events have time associated with them.
The CQ bit was checked by enne@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1013753014/60001
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/56f35ff950a6c04776212d9934c57c23f189cb73 Cr-Commit-Position: refs/heads/master@{#322266} |
