Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
964903002:
Moving yahoo_sport into pathological pageset and adding sync scroll. (Closed)
DescriptionMoving yahoo_sport into pathological pageset and adding sync scroll.
BUG=447508
Patch Set 1 #Patch Set 2 : WPR file updated #
Total comments: 1
Messages
Total messages: 25 (2 generated)
picksi@google.com changed reviewers: + jdduke@chromium.org, miletus@chromium.org, picksi@google.com, skyostil@chromium.org
I've moved sports.yahoo across to pathological_mobile_sites. Jared, I'm not 100% clear on what you were after with regard to sync scroll, so this patch adds another benchmark SmoothnessSyncScrollPathologicalMobileSites which is sync-scroll'd as well as the already existing SmoothnessPathologicalMobileSites (i.e. it does the same page set sync'd and unsync'd). Is this what you were after? NB I need to record a new WPR file before this gets committed, which will be the next patch if this looks OK to everyone.
Looks good to me. Note that we currently don't have any alerting enabled for pathological sites so there's a risk we'll miss regressions in them. If there are metrics with high enough signal to noise ratio in that page set, we could add alerting just for those.
On 2015/02/27 13:55:26, picksi wrote: > I've moved sports.yahoo across to pathological_mobile_sites. > > Jared, I'm not 100% clear on what you were after with regard to sync scroll, so > this patch adds another benchmark SmoothnessSyncScrollPathologicalMobileSites > which is sync-scroll'd as well as the already existing > SmoothnessPathologicalMobileSites (i.e. it does the same page set sync'd and > unsync'd). Is this what you were after? > Yup, this looks good. Yufeng, you OK with this change?
On 2015/02/27 13:55:26, picksi wrote: > I've moved sports.yahoo across to pathological_mobile_sites. > > Jared, I'm not 100% clear on what you were after with regard to sync scroll, so > this patch adds another benchmark SmoothnessSyncScrollPathologicalMobileSites > which is sync-scroll'd as well as the already existing > SmoothnessPathologicalMobileSites (i.e. it does the same page set sync'd and > unsync'd). Is this what you were after? > > NB I need to record a new WPR file before this gets committed, which will be the > next patch if this looks OK to everyone. I don't know exactly how WPR file works, but will the existing WPR file just work? e.g, can you make yahoo sports a different type of PathologicalMobileSitesPage but with the archive_data_file pointing to 'data/key_mobile_sites.json', will it work ?
Re-recording the sports.yahoo page was uncooperative. The current site throws up an interstitial asking me to download the app (even if I have the app on the device). Sami has suggested I change the benchmark to click on the 'X' to move through to the actual site, which I'm happy to do... However, cross-wiring the json to re-use the existing WPR data for sports.yahoo works fine. The try-bot log is here: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus10_... I am concerned about cross-wiring these page-sets - i.e. If key_mobile_sites_000.wpr gets re-recorded (now without sports.yahoo) it will (presumably) cause this page-set to fail. Can anyone guide me on the fragility of this solution? Are WPRs ever deleted, or are new ones added (I've seen 001,002 being created)? If key_mobile_sites_000.wpr will always exist in its current form then this patch is a valid solution. Any thoughts?
https://codereview.chromium.org/964903002/diff/20001/tools/perf/page_sets/dat... File tools/perf/page_sets/data/pathological_mobile_sites.json (right): https://codereview.chromium.org/964903002/diff/20001/tools/perf/page_sets/dat... tools/perf/page_sets/data/pathological_mobile_sites.json:12: "key_mobile_sites_000.wpr": [ I think this may be a little too fragile and/or weird. The next person who tries to update the recording for key_mobile_sites might break this page set instead. If the data in key_mobile_sites_000.wpr is good, how about checking in a copy of it under the name pathological_mobile_sites_001.wpr?
I have copied key_mobile_sites_000.wpr into pathological_mobile_sites_001.wpr (out of interest) and it works OK, but in a related issue (https://codereview.chromium.org/976083003/) nednguyen@ has asked that we don't copy WPR files around (as they are very large). There is also the issue of Yufengs changes and my changes potentially overwriting each other as we are abusing the wpr recording system.
On 2015/03/05 10:56:49, picksi wrote: > I have copied key_mobile_sites_000.wpr into pathological_mobile_sites_001.wpr > (out of interest) and it works OK, but in a related issue > (https://codereview.chromium.org/976083003/) nednguyen@ has asked that we don't > copy WPR files around (as they are very large). There is also the issue of > Yufengs changes and my changes potentially overwriting each other as we are > abusing the wpr recording system. Fair enough, let's try to work around the interstitial then.
On 2015/03/05 12:10:07, Sami wrote: > On 2015/03/05 10:56:49, picksi wrote: > > I have copied key_mobile_sites_000.wpr into pathological_mobile_sites_001.wpr > > (out of interest) and it works OK, but in a related issue > > (https://codereview.chromium.org/976083003/) nednguyen@ has asked that we > don't > > copy WPR files around (as they are very large). There is also the issue of > > Yufengs changes and my changes potentially overwriting each other as we are > > abusing the wpr recording system. > > Fair enough, let's try to work around the interstitial then. any update on this ?
I have given up on Yahoo sports as it relies on a cookie to access the main site and cookies are suppressed during WPR recording and playback (the same issue is also blocking Forbes.com from being recorded); I've created a bug https://code.google.com/p/chromium/issues/detail?id=464712 for this. In another CL (https://codereview.chromium.org/993623002) I've added The Guardian page. I have the sync-scroll benchmark locally, and need to find half an hour to get this commit'd and landed. I'm a bit snowed with the perf cycle at the moment!
On 2015/03/16 17:55:00, picksi wrote: > I have given up on Yahoo sports as it relies on a cookie to access the main site > and cookies are suppressed during WPR recording and playback (the same issue is > also blocking http://Forbes.com from being recorded); I've created a bug > https://code.google.com/p/chromium/issues/detail?id=464712 for this. > > In another CL (https://codereview.chromium.org/993623002) I've added The > Guardian page. > > I have the sync-scroll benchmark locally, and need to find half an hour to get > this commit'd and landed. I'm a bit snowed with the perf cycle at the moment! Then lets just reuse the existing WPR record, e.g. copying the key_mobile_sites_xxx.wpr into a pathological_mobile_sites_xxx.wpr, what do you guys think ? I can take this on if you don't have cycles on this.
I did exactly this already, but got some push back; see comment #8 above (ironically the WPR is already uploaded to cloud storage from my R&D into this). My main concern is what would happen if the page was re-recorded. Being pragmatic though, having a decent-page recording that works for a while and then gets replaced with an pointless interstitial page (when it gets re-recorded) is still on-balance a positive thing. If no one else on this chain objects I'd be happy to see this happen. If miletus@ you are able to take over this work it will certainly happen sooner than it sitting in my to-do list!
miletus@google.com changed reviewers: + nednguyen@google.com
On 2015/03/19 10:23:20, picksi wrote: > I did exactly this already, but got some push back; see comment #8 above > (ironically the WPR is already uploaded to cloud storage from my R&D into this). > My main concern is what would happen if the page was re-recorded. Being > pragmatic though, having a decent-page recording that works for a while and then > gets replaced with an pointless interstitial page (when it gets re-recorded) is > still on-balance a positive thing. > > If no one else on this chain objects I'd be happy to see this happen. If > miletus@ you are able to take over this work it will certainly happen sooner > than it sitting in my to-do list! Ned, I know you are against copying WPR file around. But in this case that we don't have a working WPR recording tool that can workaround the interstitial page, I would vote for taking the copying approach so that we can move forward. WDYT ?
On 2015/03/19 19:08:35, miletus1 wrote: > On 2015/03/19 10:23:20, picksi wrote: > > I did exactly this already, but got some push back; see comment #8 above > > (ironically the WPR is already uploaded to cloud storage from my R&D into > this). > > My main concern is what would happen if the page was re-recorded. Being > > pragmatic though, having a decent-page recording that works for a while and > then > > gets replaced with an pointless interstitial page (when it gets re-recorded) > is > > still on-balance a positive thing. > > > > If no one else on this chain objects I'd be happy to see this happen. If > > miletus@ you are able to take over this work it will certainly happen sooner > > than it sitting in my to-do list! > > Ned, I know you are against copying WPR file around. But in this case that we > don't have a working WPR recording tool that can workaround the interstitial > page, I would vote for taking the copying approach so that we can move forward. > WDYT ? How hard is it to fix the interstitial problem? Can we just add some action_runner commands to skip them? In the long term, we still need to update these pages, so I don't think we can punt on the interstitial forever.
On 2015/03/19 19:44:47, nednguyen wrote: > On 2015/03/19 19:08:35, miletus1 wrote: > > On 2015/03/19 10:23:20, picksi wrote: > > > I did exactly this already, but got some push back; see comment #8 above > > > (ironically the WPR is already uploaded to cloud storage from my R&D into > > this). > > > My main concern is what would happen if the page was re-recorded. Being > > > pragmatic though, having a decent-page recording that works for a while and > > then > > > gets replaced with an pointless interstitial page (when it gets re-recorded) > > is > > > still on-balance a positive thing. > > > > > > If no one else on this chain objects I'd be happy to see this happen. If > > > miletus@ you are able to take over this work it will certainly happen sooner > > > than it sitting in my to-do list! > > > > Ned, I know you are against copying WPR file around. But in this case that we > > don't have a working WPR recording tool that can workaround the interstitial > > page, I would vote for taking the copying approach so that we can move > forward. > > WDYT ? > > How hard is it to fix the interstitial problem? Can we just add some > action_runner commands to skip them? > > In the long term, we still need to update these pages, so I don't think we can > punt on the interstitial forever. We tried to work around the interstitial and failed. It's basically down to the page checking for specific cookies that indicate the interstitial has been shown. Suggestions welcome.
On 2015/03/19 19:44:47, nednguyen wrote: > On 2015/03/19 19:08:35, miletus1 wrote: > > On 2015/03/19 10:23:20, picksi wrote: > > > I did exactly this already, but got some push back; see comment #8 above > > > (ironically the WPR is already uploaded to cloud storage from my R&D into > > this). > > > My main concern is what would happen if the page was re-recorded. Being > > > pragmatic though, having a decent-page recording that works for a while and > > then > > > gets replaced with an pointless interstitial page (when it gets re-recorded) > > is > > > still on-balance a positive thing. > > > > > > If no one else on this chain objects I'd be happy to see this happen. If > > > miletus@ you are able to take over this work it will certainly happen sooner > > > than it sitting in my to-do list! > > > > Ned, I know you are against copying WPR file around. But in this case that we > > don't have a working WPR recording tool that can workaround the interstitial > > page, I would vote for taking the copying approach so that we can move > forward. > > WDYT ? > > How hard is it to fix the interstitial problem? Can we just add some > action_runner commands to skip them? > > In the long term, we still need to update these pages, so I don't think we can > punt on the interstitial forever. Simon filed a bug on the possible solution of allowing cookie to be turned on during recording: https://code.google.com/p/chromium/issues/detail?id=464712 I am now familiar with that part of the telemetry system so can't scope the difficulty. Do you think anyone from the telemetry team can work on it?
On 2015/03/19 19:50:27, miletus1 wrote: > On 2015/03/19 19:44:47, nednguyen wrote: > > On 2015/03/19 19:08:35, miletus1 wrote: > > > On 2015/03/19 10:23:20, picksi wrote: > > > > I did exactly this already, but got some push back; see comment #8 above > > > > (ironically the WPR is already uploaded to cloud storage from my R&D into > > > this). > > > > My main concern is what would happen if the page was re-recorded. Being > > > > pragmatic though, having a decent-page recording that works for a while > and > > > then > > > > gets replaced with an pointless interstitial page (when it gets > re-recorded) > > > is > > > > still on-balance a positive thing. > > > > > > > > If no one else on this chain objects I'd be happy to see this happen. If > > > > miletus@ you are able to take over this work it will certainly happen > sooner > > > > than it sitting in my to-do list! > > > > > > Ned, I know you are against copying WPR file around. But in this case that > we > > > don't have a working WPR recording tool that can workaround the interstitial > > > page, I would vote for taking the copying approach so that we can move > > forward. > > > WDYT ? > > > > How hard is it to fix the interstitial problem? Can we just add some > > action_runner commands to skip them? > > > > In the long term, we still need to update these pages, so I don't think we can > > punt on the interstitial forever. > > Simon filed a bug on the possible solution of allowing cookie to be turned on > during recording: > https://code.google.com/p/chromium/issues/detail?id=464712 > > I am now familiar with that part of the telemetry system so can't scope the > difficulty. > Do you think anyone from the telemetry team can work on it? We are lacking of expertise in WebPageReplay at the moment. I am trying to learn more of it from slamm@. You can go ahead with this patch by doing the WPR recording copy for now.
On 2015/03/19 20:01:03, nednguyen wrote: > On 2015/03/19 19:50:27, miletus1 wrote: > > On 2015/03/19 19:44:47, nednguyen wrote: > > > On 2015/03/19 19:08:35, miletus1 wrote: > > > > On 2015/03/19 10:23:20, picksi wrote: > > > > > I did exactly this already, but got some push back; see comment #8 above > > > > > (ironically the WPR is already uploaded to cloud storage from my R&D > into > > > > this). > > > > > My main concern is what would happen if the page was re-recorded. Being > > > > > pragmatic though, having a decent-page recording that works for a while > > and > > > > then > > > > > gets replaced with an pointless interstitial page (when it gets > > re-recorded) > > > > is > > > > > still on-balance a positive thing. > > > > > > > > > > If no one else on this chain objects I'd be happy to see this happen. If > > > > > miletus@ you are able to take over this work it will certainly happen > > sooner > > > > > than it sitting in my to-do list! > > > > > > > > Ned, I know you are against copying WPR file around. But in this case that > > we > > > > don't have a working WPR recording tool that can workaround the > interstitial > > > > page, I would vote for taking the copying approach so that we can move > > > forward. > > > > WDYT ? > > > > > > How hard is it to fix the interstitial problem? Can we just add some > > > action_runner commands to skip them? > > > > > > In the long term, we still need to update these pages, so I don't think we > can > > > punt on the interstitial forever. > > > > Simon filed a bug on the possible solution of allowing cookie to be turned on > > during recording: > > https://code.google.com/p/chromium/issues/detail?id=464712 > > > > I am now familiar with that part of the telemetry system so can't scope the > > difficulty. > > Do you think anyone from the telemetry team can work on it? > > We are lacking of expertise in WebPageReplay at the moment. I am trying to learn > more of it from slamm@. You can go ahead with this patch by doing the WPR > recording copy for now. Can you run the trybot on android for this patch?
On 2015/03/19 20:01:03, nednguyen wrote: > On 2015/03/19 19:50:27, miletus1 wrote: > > On 2015/03/19 19:44:47, nednguyen wrote: > > > On 2015/03/19 19:08:35, miletus1 wrote: > > > > On 2015/03/19 10:23:20, picksi wrote: > > > > > I did exactly this already, but got some push back; see comment #8 above > > > > > (ironically the WPR is already uploaded to cloud storage from my R&D > into > > > > this). > > > > > My main concern is what would happen if the page was re-recorded. Being > > > > > pragmatic though, having a decent-page recording that works for a while > > and > > > > then > > > > > gets replaced with an pointless interstitial page (when it gets > > re-recorded) > > > > is > > > > > still on-balance a positive thing. > > > > > > > > > > If no one else on this chain objects I'd be happy to see this happen. If > > > > > miletus@ you are able to take over this work it will certainly happen > > sooner > > > > > than it sitting in my to-do list! > > > > > > > > Ned, I know you are against copying WPR file around. But in this case that > > we > > > > don't have a working WPR recording tool that can workaround the > interstitial > > > > page, I would vote for taking the copying approach so that we can move > > > forward. > > > > WDYT ? > > > > > > How hard is it to fix the interstitial problem? Can we just add some > > > action_runner commands to skip them? > > > > > > In the long term, we still need to update these pages, so I don't think we > can > > > punt on the interstitial forever. > > > > Simon filed a bug on the possible solution of allowing cookie to be turned on > > during recording: > > https://code.google.com/p/chromium/issues/detail?id=464712 > > > > I am now familiar with that part of the telemetry system so can't scope the > > difficulty. > > Do you think anyone from the telemetry team can work on it? > > We are lacking of expertise in WebPageReplay at the moment. I am trying to learn > more of it from slamm@. You can go ahead with this patch by doing the WPR > recording copy for now. Cool, I continue Simon's work at https://codereview.chromium.org/999173006/
On 2015/03/19 21:06:58, nednguyen wrote: > On 2015/03/19 20:01:03, nednguyen wrote: > > On 2015/03/19 19:50:27, miletus1 wrote: > > > On 2015/03/19 19:44:47, nednguyen wrote: > > > > On 2015/03/19 19:08:35, miletus1 wrote: > > > > > On 2015/03/19 10:23:20, picksi wrote: > > > > > > I did exactly this already, but got some push back; see comment #8 > above > > > > > > (ironically the WPR is already uploaded to cloud storage from my R&D > > into > > > > > this). > > > > > > My main concern is what would happen if the page was re-recorded. > Being > > > > > > pragmatic though, having a decent-page recording that works for a > while > > > and > > > > > then > > > > > > gets replaced with an pointless interstitial page (when it gets > > > re-recorded) > > > > > is > > > > > > still on-balance a positive thing. > > > > > > > > > > > > If no one else on this chain objects I'd be happy to see this happen. > If > > > > > > miletus@ you are able to take over this work it will certainly happen > > > sooner > > > > > > than it sitting in my to-do list! > > > > > > > > > > Ned, I know you are against copying WPR file around. But in this case > that > > > we > > > > > don't have a working WPR recording tool that can workaround the > > interstitial > > > > > page, I would vote for taking the copying approach so that we can move > > > > forward. > > > > > WDYT ? > > > > > > > > How hard is it to fix the interstitial problem? Can we just add some > > > > action_runner commands to skip them? > > > > > > > > In the long term, we still need to update these pages, so I don't think we > > can > > > > punt on the interstitial forever. > > > > > > Simon filed a bug on the possible solution of allowing cookie to be turned > on > > > during recording: > > > https://code.google.com/p/chromium/issues/detail?id=464712 > > > > > > I am now familiar with that part of the telemetry system so can't scope the > > > difficulty. > > > Do you think anyone from the telemetry team can work on it? > > > > We are lacking of expertise in WebPageReplay at the moment. I am trying to > learn > > more of it from slamm@. You can go ahead with this patch by doing the WPR > > recording copy for now. > > Can you run the trybot on android for this patch? Yep, I started android trybot at https://codereview.chromium.org/999173006/
On 2015/03/19 21:14:07, Yufeng Shen wrote: > On 2015/03/19 21:06:58, nednguyen wrote: > > On 2015/03/19 20:01:03, nednguyen wrote: > > > On 2015/03/19 19:50:27, miletus1 wrote: > > > > On 2015/03/19 19:44:47, nednguyen wrote: > > > > > On 2015/03/19 19:08:35, miletus1 wrote: > > > > > > On 2015/03/19 10:23:20, picksi wrote: > > > > > > > I did exactly this already, but got some push back; see comment #8 > > above > > > > > > > (ironically the WPR is already uploaded to cloud storage from my R&D > > > into > > > > > > this). > > > > > > > My main concern is what would happen if the page was re-recorded. > > Being > > > > > > > pragmatic though, having a decent-page recording that works for a > > while > > > > and > > > > > > then > > > > > > > gets replaced with an pointless interstitial page (when it gets > > > > re-recorded) > > > > > > is > > > > > > > still on-balance a positive thing. > > > > > > > > > > > > > > If no one else on this chain objects I'd be happy to see this > happen. > > If > > > > > > > miletus@ you are able to take over this work it will certainly > happen > > > > sooner > > > > > > > than it sitting in my to-do list! > > > > > > > > > > > > Ned, I know you are against copying WPR file around. But in this case > > that > > > > we > > > > > > don't have a working WPR recording tool that can workaround the > > > interstitial > > > > > > page, I would vote for taking the copying approach so that we can move > > > > > forward. > > > > > > WDYT ? > > > > > > > > > > How hard is it to fix the interstitial problem? Can we just add some > > > > > action_runner commands to skip them? > > > > > > > > > > In the long term, we still need to update these pages, so I don't think > we > > > can > > > > > punt on the interstitial forever. > > > > > > > > Simon filed a bug on the possible solution of allowing cookie to be turned > > on > > > > during recording: > > > > https://code.google.com/p/chromium/issues/detail?id=464712 > > > > > > > > I am now familiar with that part of the telemetry system so can't scope > the > > > > difficulty. > > > > Do you think anyone from the telemetry team can work on it? > > > > > > We are lacking of expertise in WebPageReplay at the moment. I am trying to > > learn > > > more of it from slamm@. You can go ahead with this patch by doing the WPR > > > recording copy for now. > > > > Can you run the trybot on android for this patch? > > Yep, I started android trybot at https://codereview.chromium.org/999173006/ Please ping me again when that run is green.
Do we still need this?
No... well remembered! This has been covered by https://codereview.chromium.org/999173006/. I'll close it. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
