Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2262343002:
Fix page interaction with CnnArticlePage (Closed)
|
Created:
4 years, 4 months ago by Navid Zolghadr Modified:
4 years, 3 months ago CC:
chromium-reviews, telemetry-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix page interaction with CnnArticlePage
BUG=633489
Committed: https://crrev.com/5f32fe5b412c4726ba10d053e6958a9186375e4a
Cr-Commit-Position: refs/heads/master@{#414185}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add comment for why this is needed #Messages
Total messages: 26 (12 generated)
The CQ bit was checked by nzolghadr@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...
nzolghadr@chromium.org changed reviewers: + tdresser@chromium.org
LGTM, thanks! (Assuming you've tested this locally, and it works consistently)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
On 2016/08/22 21:08:34, tdresser wrote: > LGTM, thanks! > > (Assuming you've tested this locally, and it works consistently) I don't quite understand what the consistency means in this context. When I run locally I can see the numbers instead of missing values for some of the rows. But do you expect the numbers to be the same in multiple runs?
On 2016/08/23 13:49:37, Navid Zolghadr wrote: > On 2016/08/22 21:08:34, tdresser wrote: > > LGTM, thanks! > > > > (Assuming you've tested this locally, and it works consistently) > > I don't quite understand what the consistency means in this context. When I run > locally I can see the numbers instead of missing values for some of the rows. > But do you expect the numbers to be the same in multiple runs? Sorry, by consistency here I just mean that when run repeatedly, we're never seeing null values. When I was testing this, even without the fix I was occasionally seeing passing runs.
On 2016/08/23 13:50:53, tdresser wrote: > On 2016/08/23 13:49:37, Navid Zolghadr wrote: > > On 2016/08/22 21:08:34, tdresser wrote: > > > LGTM, thanks! > > > > > > (Assuming you've tested this locally, and it works consistently) > > > > I don't quite understand what the consistency means in this context. When I > run > > locally I can see the numbers instead of missing values for some of the rows. > > But do you expect the numbers to be the same in multiple runs? > > Sorry, by consistency here I just mean that when run repeatedly, we're never > seeing null values. > > When I was testing this, even without the fix I was occasionally seeing passing > runs. As far as I ran the test with a for loop on my machine they are all having numbers ranging from 130 to 200. So I guess we can assume it is reliable enough to always generate the numbers after the fix.
nzolghadr@chromium.org changed reviewers: + dtu@chromium.org
dtu@chromium.org: Please review changes in tools/perf/
Great, thanks for checking. Still LGTM.
sullivan@chromium.org changed reviewers: + sullivan@chromium.org
Snagging owners review from dtu, lgtm with a nit. https://codereview.chromium.org/2262343002/diff/1/tools/perf/page_sets/key_mo... File tools/perf/page_sets/key_mobile_sites_smooth.py (right): https://codereview.chromium.org/2262343002/diff/1/tools/perf/page_sets/key_mo... tools/perf/page_sets/key_mobile_sites_smooth.py:136: top_start_ratio=0.01) Reading the bug, we need to do this because the default scroll action is trying to scroll an input element? Would be awesome to add some comments explaining why we special-case this page in case anyone needs to understand it in the future.
https://codereview.chromium.org/2262343002/diff/1/tools/perf/page_sets/key_mo... File tools/perf/page_sets/key_mobile_sites_smooth.py (right): https://codereview.chromium.org/2262343002/diff/1/tools/perf/page_sets/key_mo... tools/perf/page_sets/key_mobile_sites_smooth.py:136: top_start_ratio=0.01) On 2016/08/24 14:40:03, sullivan wrote: > Reading the bug, we need to do this because the default scroll action is trying > to scroll an input element? Would be awesome to add some comments explaining why > we special-case this page in case anyone needs to understand it in the future. Done.
The CQ bit was checked by nzolghadr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sullivan@chromium.org, tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/2262343002/#ps20001 (title: "Add comment for why this is needed")
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
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by nzolghadr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Fix page interaction with CnnArticlePage BUG=633489 ========== to ========== Fix page interaction with CnnArticlePage BUG=633489 Committed: https://crrev.com/5f32fe5b412c4726ba10d053e6958a9186375e4a Cr-Commit-Position: refs/heads/master@{#414185} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/5f32fe5b412c4726ba10d053e6958a9186375e4a Cr-Commit-Position: refs/heads/master@{#414185} |
