|
|
DescriptionChanges threshold frequency for investigation (based on video in the bug)
Looking some more at the video the frequency is not that high, so
expecting 100 repetitions in 10 seconds was too optimistic and the logic in the implementation was wrong in any case.
Expecting 20 invocations with at most 1 second in between subsequent events is more realistic while still very much improbable to do by hand, I think.
BUG=665093
TEST=NONE (but will visit crash stats to check on this)
Committed: https://crrev.com/3189d914bae27b9194aa9de802f2c55ae83632ea
Cr-Commit-Position: refs/heads/master@{#440017}
Patch Set 1 : Changes threshold frequency for investigation (based on video in the bug) #
Total comments: 2
Patch Set 2 : Changes threshold frequency for investigation (fxed a test and reworked math) #
Messages
Total messages: 32 (22 generated)
The CQ bit was checked by varkha@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...
Patchset #1 (id:1) has been deleted
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Description was changed from ========== Changes treshold frequency for investigation (based on video in the bug) Looking some more at the video the frequency is not that high so expecting 100 repetitions in 10 seconds was too optimistic. Expecting 30 invocations in 30 seconds is more realistic while still very much impossible to do by hand I think. BUG=665093 TEST=NONE (but will visit crash stats to check on this) ========== to ========== Changes treshold frequency for investigation (based on video in the bug) Looking some more at the video the frequency is not that high so expecting 100 repetitions in 10 seconds was too optimistic. Expecting 15 invocations in 15 seconds is more realistic while still very much impossible to do by hand I think. BUG=665093 TEST=NONE (but will visit crash stats to check on this) ==========
oshima@, can you please take a look? I thought the previous change would not actually trigger so posting this before merge.
varkha@chromium.org changed reviewers: + oshima@chromium.org
oshima@, can you please take a look? I thought the previous change would not actually trigger so posting this.
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_ozone_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 varkha@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_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Looks like test is failing because it does change the state often and quickly. Can you disable this in the test?
On 2016/12/13 16:47:42, oshima wrote: > Looks like test is failing because it does change the state often and quickly. > Can you disable this in the test? Yes, saw this and indeed will need to disable this in tests.
Need to rework this, I will ping again when it is ready for review. https://codereview.chromium.org/2562413003/diff/20001/ash/common/shelf/wm_she... File ash/common/shelf/wm_shelf.cc (right): https://codereview.chromium.org/2562413003/diff/20001/ash/common/shelf/wm_she... ash/common/shelf/wm_shelf.cc:174: kAutoHideRepeatInterval) { This actually feels wrong. My intention was to CHECK when number of events in interval is > threshold. Both current and suggested code is doing something very different so both are prone to being triggered manually. I will need to rework this and also make the test that repeatedly swipes up and down on the shelf take this into account.
The CQ bit was checked by varkha@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...
I think I've fixed the troubles. PTAL. https://codereview.chromium.org/2562413003/diff/20001/ash/common/shelf/wm_she... File ash/common/shelf/wm_shelf.cc (right): https://codereview.chromium.org/2562413003/diff/20001/ash/common/shelf/wm_she... ash/common/shelf/wm_shelf.cc:174: kAutoHideRepeatInterval) { On 2016/12/14 16:50:54, varkha wrote: > This actually feels wrong. My intention was to CHECK when number of events in > interval is > threshold. Both current and suggested code is doing something very > different so both are prone to being triggered manually. I will need to rework > this and also make the test that repeatedly swipes up and down on the shelf take > this into account. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Changes treshold frequency for investigation (based on video in the bug) Looking some more at the video the frequency is not that high so expecting 100 repetitions in 10 seconds was too optimistic. Expecting 15 invocations in 15 seconds is more realistic while still very much impossible to do by hand I think. BUG=665093 TEST=NONE (but will visit crash stats to check on this) ========== to ========== Changes threshold frequency for investigation (based on video in the bug) Looking some more at the video the frequency is not that high, so expecting 100 repetitions in 10 seconds was too optimistic and the logic in the implementation was wrong in any case. Expecting 20 invocations with at most 1 second in between subsequent events is more realistic while still very much improbable to do by hand, I think. BUG=665093 TEST=NONE (but will visit crash stats to check on this) ==========
lgtm
The CQ bit was checked by varkha@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": 40001, "attempt_start_ts": 1482293501308870, "parent_rev": "441a0ef3ebcd230a099ade2fb86bf75c9aa40deb", "commit_rev": "06cef99b321a0f0ee42af02499f3f9c5ad3ca217"}
Message was sent while issue was closed.
Description was changed from ========== Changes threshold frequency for investigation (based on video in the bug) Looking some more at the video the frequency is not that high, so expecting 100 repetitions in 10 seconds was too optimistic and the logic in the implementation was wrong in any case. Expecting 20 invocations with at most 1 second in between subsequent events is more realistic while still very much improbable to do by hand, I think. BUG=665093 TEST=NONE (but will visit crash stats to check on this) ========== to ========== Changes threshold frequency for investigation (based on video in the bug) Looking some more at the video the frequency is not that high, so expecting 100 repetitions in 10 seconds was too optimistic and the logic in the implementation was wrong in any case. Expecting 20 invocations with at most 1 second in between subsequent events is more realistic while still very much improbable to do by hand, I think. BUG=665093 TEST=NONE (but will visit crash stats to check on this) Review-Url: https://codereview.chromium.org/2562413003 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Changes threshold frequency for investigation (based on video in the bug) Looking some more at the video the frequency is not that high, so expecting 100 repetitions in 10 seconds was too optimistic and the logic in the implementation was wrong in any case. Expecting 20 invocations with at most 1 second in between subsequent events is more realistic while still very much improbable to do by hand, I think. BUG=665093 TEST=NONE (but will visit crash stats to check on this) Review-Url: https://codereview.chromium.org/2562413003 ========== to ========== Changes threshold frequency for investigation (based on video in the bug) Looking some more at the video the frequency is not that high, so expecting 100 repetitions in 10 seconds was too optimistic and the logic in the implementation was wrong in any case. Expecting 20 invocations with at most 1 second in between subsequent events is more realistic while still very much improbable to do by hand, I think. BUG=665093 TEST=NONE (but will visit crash stats to check on this) Committed: https://crrev.com/3189d914bae27b9194aa9de802f2c55ae83632ea Cr-Commit-Position: refs/heads/master@{#440017} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/3189d914bae27b9194aa9de802f2c55ae83632ea Cr-Commit-Position: refs/heads/master@{#440017} |