Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2532443002:
[telemetry] Bullet proof scroll.js (Closed)
Description[telemetry] Bullet proof scroll.js
Make scroll.js give a nice error message if the scroll point is
off screen or if the element has zero area. Previously we just
got a stack trace from chrome.
Also fix various lint errors since this is the first time this file
has been touched in a while.
BUG=chromium:664515
Review-Url: https://codereview.chromium.org/2532443002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/c3ec514d8726530643844182677304ce7f15a5c2
Patch Set 1 #Patch Set 2 : [telemetry] Bullet proof scroll.js #Patch Set 3 : fix typo #
Total comments: 7
Patch Set 4 : fix for comments #
Total comments: 1
Messages
Total messages: 17 (8 generated)
Description was changed from ========== [telemetry] Bullet proof scroll.js Make scroll.js give a nice error message if the scroll point is off screen. Previously we just got a stack trace from chrome. Also fix various lint errors since this is the first time this file has been touched in a while. BUG=chromium:664515 ========== to ========== [telemetry] Bullet proof scroll.js Make scroll.js give a nice error message if the scroll point is off screen or if the element has zero area. Previously we just got a stack trace from chrome. Also fix various lint errors since this is the first time this file has been touched in a while. BUG=chromium:664515 ==========
hjd@chromium.org changed reviewers: + perezju@chromium.org, skyostil@chromium.org
ptal, thanks! :)
https://codereview.chromium.org/2532443002/diff/40001/telemetry/telemetry/int... File telemetry/telemetry/internal/actions/scroll.js (right): https://codereview.chromium.org/2532443002/diff/40001/telemetry/telemetry/int... telemetry/telemetry/internal/actions/scroll.js:119: var rect = __GestureCommon_GetBoundingVisibleRect(this.options_.element_); I hope moving this query here from the rAF doesn't break stuff, but at this point I'm very paranoid about these things :) https://codereview.chromium.org/2532443002/diff/40001/telemetry/telemetry/int... telemetry/telemetry/internal/actions/scroll.js:120: this.start_left = nit: startLeft/startTop? https://codereview.chromium.org/2532443002/diff/40001/telemetry/telemetry/int... telemetry/telemetry/internal/actions/scroll.js:126: if (this.start_left < 0 || If GetBoundingVisibleRect() clips to the window then this should never happen, right?
Thanks! https://codereview.chromium.org/2532443002/diff/40001/telemetry/telemetry/int... File telemetry/telemetry/internal/actions/scroll.js (right): https://codereview.chromium.org/2532443002/diff/40001/telemetry/telemetry/int... telemetry/telemetry/internal/actions/scroll.js:119: var rect = __GestureCommon_GetBoundingVisibleRect(this.options_.element_); On 2016/11/24 12:39:59, Sami wrote: > I hope moving this query here from the rAF doesn't break stuff, but at this > point I'm very paranoid about these things :) Yeah, sadly if I leave it in the rAF we never see the exception because start has already returned :( I guess I could save the exception and then raise somehow from the wait for expression here: https://codesearch.chromium.org/chromium/src/third_party/catapult/telemetry/t... but that seems quite complicated. What do you think? I could try it this way first on the perf try bots and see if it causes problems. https://codereview.chromium.org/2532443002/diff/40001/telemetry/telemetry/int... telemetry/telemetry/internal/actions/scroll.js:120: this.start_left = On 2016/11/24 12:39:59, Sami wrote: > nit: startLeft/startTop? Done, thanks! https://codereview.chromium.org/2532443002/diff/40001/telemetry/telemetry/int... telemetry/telemetry/internal/actions/scroll.js:126: if (this.start_left < 0 || On 2016/11/24 12:39:59, Sami wrote: > If GetBoundingVisibleRect() clips to the window then this should never happen, > right? Yeah removed, I was being over defensive - just annoyed about having spent so long tracking this down.
lgtm! https://codereview.chromium.org/2532443002/diff/40001/telemetry/telemetry/int... File telemetry/telemetry/internal/actions/scroll.js (right): https://codereview.chromium.org/2532443002/diff/40001/telemetry/telemetry/int... telemetry/telemetry/internal/actions/scroll.js:119: var rect = __GestureCommon_GetBoundingVisibleRect(this.options_.element_); On 2016/11/24 14:39:03, hjd wrote: > On 2016/11/24 12:39:59, Sami wrote: > > I hope moving this query here from the rAF doesn't break stuff, but at this > > point I'm very paranoid about these things :) > > Yeah, sadly if I leave it in the rAF we never see the exception because start > has already returned :( > > I guess I could save the exception and then raise somehow from the > wait for expression here: > https://codesearch.chromium.org/chromium/src/third_party/catapult/telemetry/t... > but that seems quite complicated. What do you think? I could try it this way > first on the perf try bots and see if it causes problems. Oh I see. I think that's a good enough reason to try and move the setup code here. https://codereview.chromium.org/2532443002/diff/60001/telemetry/telemetry/int... File telemetry/telemetry/internal/actions/scroll.js (right): https://codereview.chromium.org/2532443002/diff/60001/telemetry/telemetry/int... telemetry/telemetry/internal/actions/scroll.js:124: throw new Error('Scroll element has zero height and/or width.'); "... or is outside the viewport"
lgtm, although only mildly familiar with the code in this regions
The CQ bit was checked by hjd@chromium.org
The CQ bit was unchecked by hjd@chromium.org
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 hjd@chromium.org
The CQ bit was checked by hjd@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484739669678000,
"parent_rev": "cfcae9b972f24e331a8ed0110b5afc96e75919a0", "commit_rev":
"c3ec514d8726530643844182677304ce7f15a5c2"}
Message was sent while issue was closed.
Description was changed from ========== [telemetry] Bullet proof scroll.js Make scroll.js give a nice error message if the scroll point is off screen or if the element has zero area. Previously we just got a stack trace from chrome. Also fix various lint errors since this is the first time this file has been touched in a while. BUG=chromium:664515 ========== to ========== [telemetry] Bullet proof scroll.js Make scroll.js give a nice error message if the scroll point is off screen or if the element has zero area. Previously we just got a stack trace from chrome. Also fix various lint errors since this is the first time this file has been touched in a while. BUG=chromium:664515 Review-Url: https://codereview.chromium.org/2532443002 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2644073003/ by hjd@chromium.org. The reason for reverting is: CL breaks catapult->chrome autoroller see: https://github.com/catapult-project/catapult/issues/3149. |
