Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2510443002:
[System Health] Change scrolling behaviour for news sites. (Closed)
|
Created:
4 years, 1 month ago by rnephew (Reviews Here) Modified:
4 years, 1 month ago CC:
chromium-reviews, telemetry-reviews_chromium.org, perezju Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[System Health] Change scrolling behaviour for news sites.
They currently would wait 3 seconds, then scroll then go back to the main site.
There was an issue where after scrolling, the scroll bar would constantly rewrite.
This moves half the wait period to after the scrolling, so that it has time to
sit idle after scrolling
*** TO PERF SHERRIFS ***
The change in default behaviour for the news test cases may cause regressions.
These are likely not real regressions, and we just need to establish a new baseline.
BUG=665220
Committed: https://crrev.com/11a42166b7ca5f2ac4ff371110d41054a2d1dc3e
Cr-Commit-Position: refs/heads/master@{#432288}
Patch Set 1 #
Total comments: 2
Messages
Total messages: 18 (8 generated)
rnephew@chromium.org changed reviewers: + charliea@chromium.org, erikchen@chromium.org, nednguyen@google.com
https://codereview.chromium.org/2510443002/diff/1/tools/perf/page_sets/system... File tools/perf/page_sets/system_health/browsing_stories.py (right): https://codereview.chromium.org/2510443002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/browsing_stories.py:79: action_runner.Wait(self.ITEM_READ_TIME_IN_SECONDS/2) We could also have it wait the entire time before and after, but that will increase the run time of the SH tests; but assuming my reason why we didn't catch it is correct it would increase the likelihood of us catching it..
On 2016/11/15 22:07:07, rnephew (Reviews Here) wrote: lgtm Clever fix, I like this! Can you add a note that this may cause some regression in the description?
lgtm
Description was changed from ========== [System Health] Change scrolling behaviour for news sites. They currently would wait 3 seconds, then scroll then go back to the main site. There was an issue where after scrolling, the scroll bar would constantly rewrite. This moves half the wait period to after the scrolling, so that it has time to sit idle after scrolling BUG=665220 ========== to ========== [System Health] Change scrolling behaviour for news sites. They currently would wait 3 seconds, then scroll then go back to the main site. There was an issue where after scrolling, the scroll bar would constantly rewrite. This moves half the wait period to after the scrolling, so that it has time to sit idle after scrolling BUG=665220 ==========
Description was changed from ========== [System Health] Change scrolling behaviour for news sites. They currently would wait 3 seconds, then scroll then go back to the main site. There was an issue where after scrolling, the scroll bar would constantly rewrite. This moves half the wait period to after the scrolling, so that it has time to sit idle after scrolling BUG=665220 ========== to ========== [System Health] Change scrolling behaviour for news sites. They currently would wait 3 seconds, then scroll then go back to the main site. There was an issue where after scrolling, the scroll bar would constantly rewrite. This moves half the wait period to after the scrolling, so that it has time to sit idle after scrolling The change in default behaviour for the news test cases may cause regressions. These are likely not real regressions, and we just need to establish a new baseline. BUG=665220 ==========
On 2016/11/15 22:09:32, nednguyen wrote: > On 2016/11/15 22:07:07, rnephew (Reviews Here) wrote: > > lgtm > > Clever fix, I like this! Can you add a note that this may cause some regression > in the description? Edited the description to say that it is likely to cause regressions, but they can likely be ignored since its changed test behavior we need to rebaseline against.
The CQ bit was checked by rnephew@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [System Health] Change scrolling behaviour for news sites. They currently would wait 3 seconds, then scroll then go back to the main site. There was an issue where after scrolling, the scroll bar would constantly rewrite. This moves half the wait period to after the scrolling, so that it has time to sit idle after scrolling The change in default behaviour for the news test cases may cause regressions. These are likely not real regressions, and we just need to establish a new baseline. BUG=665220 ========== to ========== [System Health] Change scrolling behaviour for news sites. They currently would wait 3 seconds, then scroll then go back to the main site. There was an issue where after scrolling, the scroll bar would constantly rewrite. This moves half the wait period to after the scrolling, so that it has time to sit idle after scrolling *** TO PERF SHERRIFS *** The change in default behaviour for the news test cases may cause regressions. These are likely not real regressions, and we just need to establish a new baseline. BUG=665220 ==========
Message was sent while issue was closed.
Description was changed from ========== [System Health] Change scrolling behaviour for news sites. They currently would wait 3 seconds, then scroll then go back to the main site. There was an issue where after scrolling, the scroll bar would constantly rewrite. This moves half the wait period to after the scrolling, so that it has time to sit idle after scrolling *** TO PERF SHERRIFS *** The change in default behaviour for the news test cases may cause regressions. These are likely not real regressions, and we just need to establish a new baseline. BUG=665220 ========== to ========== [System Health] Change scrolling behaviour for news sites. They currently would wait 3 seconds, then scroll then go back to the main site. There was an issue where after scrolling, the scroll bar would constantly rewrite. This moves half the wait period to after the scrolling, so that it has time to sit idle after scrolling *** TO PERF SHERRIFS *** The change in default behaviour for the news test cases may cause regressions. These are likely not real regressions, and we just need to establish a new baseline. BUG=665220 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== [System Health] Change scrolling behaviour for news sites. They currently would wait 3 seconds, then scroll then go back to the main site. There was an issue where after scrolling, the scroll bar would constantly rewrite. This moves half the wait period to after the scrolling, so that it has time to sit idle after scrolling *** TO PERF SHERRIFS *** The change in default behaviour for the news test cases may cause regressions. These are likely not real regressions, and we just need to establish a new baseline. BUG=665220 ========== to ========== [System Health] Change scrolling behaviour for news sites. They currently would wait 3 seconds, then scroll then go back to the main site. There was an issue where after scrolling, the scroll bar would constantly rewrite. This moves half the wait period to after the scrolling, so that it has time to sit idle after scrolling *** TO PERF SHERRIFS *** The change in default behaviour for the news test cases may cause regressions. These are likely not real regressions, and we just need to establish a new baseline. BUG=665220 Committed: https://crrev.com/11a42166b7ca5f2ac4ff371110d41054a2d1dc3e Cr-Commit-Position: refs/heads/master@{#432288} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/11a42166b7ca5f2ac4ff371110d41054a2d1dc3e Cr-Commit-Position: refs/heads/master@{#432288}
Message was sent while issue was closed.
perezju@chromium.org changed reviewers: + perezju@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2510443002/diff/1/tools/perf/page_sets/system... File tools/perf/page_sets/system_health/browsing_stories.py (right): https://codereview.chromium.org/2510443002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/browsing_stories.py:79: action_runner.Wait(self.ITEM_READ_TIME_IN_SECONDS/2) On 2016/11/15 22:08:43, rnephew (Reviews Here) wrote: > We could also have it wait the entire time before and after, but that will > increase the run time of the SH tests; but assuming my reason why we didn't > catch it is correct it would increase the likelihood of us catching it.. Note: This is doing integer division. So before it was waiting 3 seconds in total, it now waits 2 * (3 / 2) = 2 * 1 = 2 seconds! I think what you want is: action_runner.Wait(self.ITEM_READ_TIME_IN_SECONDS / 2.0)
Message was sent while issue was closed.
On 2016/11/16 09:58:05, perezju wrote: > https://codereview.chromium.org/2510443002/diff/1/tools/perf/page_sets/system... > File tools/perf/page_sets/system_health/browsing_stories.py (right): > > https://codereview.chromium.org/2510443002/diff/1/tools/perf/page_sets/system... > tools/perf/page_sets/system_health/browsing_stories.py:79: > action_runner.Wait(self.ITEM_READ_TIME_IN_SECONDS/2) > On 2016/11/15 22:08:43, rnephew (Reviews Here) wrote: > > We could also have it wait the entire time before and after, but that will > > increase the run time of the SH tests; but assuming my reason why we didn't > > catch it is correct it would increase the likelihood of us catching it.. > > Note: This is doing integer division. So before it was waiting 3 seconds in > total, it now waits 2 * (3 / 2) = 2 * 1 = 2 seconds! > > I think what you want is: > > action_runner.Wait(self.ITEM_READ_TIME_IN_SECONDS / 2.0) Good catch! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
