Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2386083003:
Add background stories to MobileSystemHealth (Closed)
DescriptionAdd background stories to MobileSystemHealth
This adds some background stories to the system health
story set. These stories put the browser into the background
before measuring memory use. At the moment these stories are
only enabled for mobile since backgrounding only works on
mobile.
BUG=chromium:652205
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq
Committed: https://crrev.com/81b7c28dc5a75396861fca85ace8c1420224af6b
Cr-Commit-Position: refs/heads/master@{#427320}
Patch Set 1 #
Total comments: 3
Patch Set 2 : attempt 2 #Patch Set 3 : use Background/Foreground methods #
Total comments: 8
Patch Set 4 : address comments #
Total comments: 4
Patch Set 5 : add other stories #Patch Set 6 : remove wait #
Total comments: 6
Patch Set 7 : update urls #Patch Set 8 : add gmail story #
Total comments: 1
Patch Set 9 : no mixin #Patch Set 10 : fixup stories #
Total comments: 7
Patch Set 11 : fix a typo & click on nytime video #
Total comments: 1
Patch Set 12 : update to use new scroll #Patch Set 13 : record pages #
Total comments: 2
Patch Set 14 : use ScrollPageToElement #
Total comments: 9
Patch Set 15 : rerecord gmail #Patch Set 16 : tweak stories some more #Messages
Total messages: 49 (10 generated)
Description was changed from ========== Add a background case of MobileMemorySystemHealth This adds a version of the system_health.memory_mobile benchmark named system_health.memory_mobile_background that measures memory usage after loading some pages and then pushing the browser to the background. BUG=chromium:652205 ========== to ========== Add a background case of MobileMemorySystemHealth This adds a version of the system_health.memory_mobile benchmark named system_health.memory_mobile_background that measures memory usage after loading some pages and then pushing the browser to the background. BUG=chromium:652205 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq ==========
Description was changed from ========== Add a background case of MobileMemorySystemHealth This adds a version of the system_health.memory_mobile benchmark named system_health.memory_mobile_background that measures memory usage after loading some pages and then pushing the browser to the background. BUG=chromium:652205 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq ========== to ========== Add a background case of MobileMemorySystemHealth This adds a version of the system_health.memory_mobile benchmark named system_health.memory_mobile_background that measures memory usage after loading some pages and then pushing the browser to the background. BUG=chromium:652205 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq ==========
hjd@chromium.org changed reviewers: + perezju@chromium.org
First pass of the background system health memory benchmarks :) Maybe we could chat in person about it when you have sec? Thanks! https://codereview.chromium.org/2386083003/diff/1/tools/perf/page_sets/system... File tools/perf/page_sets/system_health/system_health_story.py (right): https://codereview.chromium.org/2386083003/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/system_health_story.py:124: app_has_webviews=False) The block above seems kind of ugly, it seems like background/foreground should be pushed into platform so we just always get the right impl for the OS we're on?
Description was changed from ========== Add a background case of MobileMemorySystemHealth This adds a version of the system_health.memory_mobile benchmark named system_health.memory_mobile_background that measures memory usage after loading some pages and then pushing the browser to the background. BUG=chromium:652205 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq ========== to ========== Add a background story to MobileSystemHealth This adds a basic background story to the system health story set which loads google.com then puts the browser into the background before measuring memory. At the moment this story is only enabled for mobile. BUG=chromium:652205 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq ==========
hjd@chromium.org changed reviewers: + nednguyen@google.com, primiano@chromium.org
https://codereview.chromium.org/2386083003/diff/40001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/background_stories.py (right): https://codereview.chromium.org/2386083003/diff/40001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/background_stories.py:18: action_runner.tab.browser.Background() Should we wait few seconds after backgrounding to measure memory? https://codereview.chromium.org/2386083003/diff/40001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/background_stories.py:20: action_runner.tab.browser.Foreground() What happens if we don't foreground here? Would the next telemetry test run fails? https://codereview.chromium.org/2386083003/diff/40001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/background_stories.py:25: URL = 'https://www.google.co.uk/' Given that we only have around 5 single page stories for background, I am not sure whether we should pick google.co.uk
https://codereview.chromium.org/2386083003/diff/1/tools/perf/benchmarks/syste... File tools/perf/benchmarks/system_health.py (right): https://codereview.chromium.org/2386083003/diff/1/tools/perf/benchmarks/syste... tools/perf/benchmarks/system_health.py:121: return 'system_health.memory_%s' % cls.PLATFORM Instead of adding a new benchmark, I think we should add a new story group (or groups), e.g. like the current browse:news or load:media, named maybe something like background:news, background:media, etc. https://codereview.chromium.org/2386083003/diff/40001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/background_stories.py (right): https://codereview.chromium.org/2386083003/diff/40001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/background_stories.py:18: action_runner.tab.browser.Background() On 2016/10/05 13:10:49, nednguyen wrote: > Should we wait few seconds after backgrounding to measure memory? +1 https://codereview.chromium.org/2386083003/diff/40001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/background_stories.py:20: action_runner.tab.browser.Foreground() On 2016/10/05 13:10:49, nednguyen wrote: > What happens if we don't foreground here? Would the next telemetry test run > fails? I agree, we probably don't need to "Foreground" again; because the browser will be closed/restarted before going to the next page. https://codereview.chromium.org/2386083003/diff/40001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/background_stories.py:25: URL = 'https://www.google.co.uk/' On 2016/10/05 13:10:50, nednguyen wrote: > Given that we only have around 5 single page stories for background, I am not > sure whether we should pick google.co.uk I'm coming up with a list of stories to pick. Will share it in a few more minutes.
https://codereview.chromium.org/2386083003/diff/1/tools/perf/benchmarks/syste... File tools/perf/benchmarks/system_health.py (right): https://codereview.chromium.org/2386083003/diff/1/tools/perf/benchmarks/syste... tools/perf/benchmarks/system_health.py:121: return 'system_health.memory_%s' % cls.PLATFORM On 2016/10/05 13:14:54, perezju wrote: > Instead of adding a new benchmark, I think we should add a new story group (or > groups), e.g. like the current browse:news or load:media, named maybe something > like background:news, background:media, etc. Ignore that comment. Noise from a previous draft I never actually sent.
thanks :) https://codereview.chromium.org/2386083003/diff/40001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/background_stories.py (right): https://codereview.chromium.org/2386083003/diff/40001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/background_stories.py:18: action_runner.tab.browser.Background() On 2016/10/05 13:14:54, perezju wrote: > On 2016/10/05 13:10:49, nednguyen wrote: > > Should we wait few seconds after backgrounding to measure memory? > > +1 Sounds good; 2 seconds? https://codereview.chromium.org/2386083003/diff/40001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/background_stories.py:20: action_runner.tab.browser.Foreground() On 2016/10/05 13:14:54, perezju wrote: > On 2016/10/05 13:10:49, nednguyen wrote: > > What happens if we don't foreground here? Would the next telemetry test run > > fails? > > I agree, we probably don't need to "Foreground" again; because the browser will > be closed/restarted before going to the next page. Yep the restart brings up the browser again :) Shall I go ahead and remove this?
https://codereview.chromium.org/2386083003/diff/60001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/background_stories.py (right): https://codereview.chromium.org/2386083003/diff/60001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/background_stories.py:23: action_runner.Wait(PRE_MEASUREMENT_DELAY_SECONDS) Actually, no, sorry about that. The _Measure method will eventually call MeasureMemory in deterministic_mode, and that will already include a wait. https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... https://codereview.chromium.org/2386083003/diff/60001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/background_stories.py:29: URL = 'https://www.google.co.uk/' My proposal would be to include: - background:social:facebook - background:search:google - background:news:nytimes - background:tools:gmail - background:media:imgur Details and rationale: https://docs.google.com/a/google.com/document/d/1fQXX2W1qNdRSRWmVuhiFrDcyqxHV...
On 2016/10/05 13:55:38, perezju wrote: > https://codereview.chromium.org/2386083003/diff/60001/tools/perf/page_sets/sy... > File tools/perf/page_sets/system_health/background_stories.py (right): > > https://codereview.chromium.org/2386083003/diff/60001/tools/perf/page_sets/sy... > tools/perf/page_sets/system_health/background_stories.py:23: > action_runner.Wait(PRE_MEASUREMENT_DELAY_SECONDS) > Actually, no, sorry about that. The _Measure method will eventually call > MeasureMemory in deterministic_mode, and that will already include a wait. > > https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... > > https://codereview.chromium.org/2386083003/diff/60001/tools/perf/page_sets/sy... > tools/perf/page_sets/system_health/background_stories.py:29: URL = > 'https://www.google.co.uk/' > My proposal would be to include: > > - background:social:facebook > - background:search:google > - background:news:nytimes > - background:tools:gmail > - background:media:imgur > > Details and rationale: > https://docs.google.com/a/google.com/document/d/1fQXX2W1qNdRSRWmVuhiFrDcyqxHV... Can we pick a subset of pages with audio, video, gif.... basically things that keep running in background?
On 2016/10/05 14:07:28, nednguyen wrote: > On 2016/10/05 13:55:38, perezju wrote: > > > https://codereview.chromium.org/2386083003/diff/60001/tools/perf/page_sets/sy... > > File tools/perf/page_sets/system_health/background_stories.py (right): > > > > > https://codereview.chromium.org/2386083003/diff/60001/tools/perf/page_sets/sy... > > tools/perf/page_sets/system_health/background_stories.py:23: > > action_runner.Wait(PRE_MEASUREMENT_DELAY_SECONDS) > > Actually, no, sorry about that. The _Measure method will eventually call > > MeasureMemory in deterministic_mode, and that will already include a wait. > > > > > https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... > > > > > https://codereview.chromium.org/2386083003/diff/60001/tools/perf/page_sets/sy... > > tools/perf/page_sets/system_health/background_stories.py:29: URL = > > 'https://www.google.co.uk/' > > My proposal would be to include: > > > > - background:social:facebook > > - background:search:google > > - background:news:nytimes > > - background:tools:gmail > > - background:media:imgur > > > > Details and rationale: > > > https://docs.google.com/a/google.com/document/d/1fQXX2W1qNdRSRWmVuhiFrDcyqxHV... > > > Can we pick a subset of pages with audio, video, gif.... basically things that > keep running in background? At the moment, on mobile, I think we don't have any stories that actually play audio/video. I agree we should have those, but for now maybe this is a reasonable v0 selection?
On 2016/10/05 14:15:20, perezju wrote: > On 2016/10/05 14:07:28, nednguyen wrote: > > On 2016/10/05 13:55:38, perezju wrote: > > > > > > https://codereview.chromium.org/2386083003/diff/60001/tools/perf/page_sets/sy... > > > File tools/perf/page_sets/system_health/background_stories.py (right): > > > > > > > > > https://codereview.chromium.org/2386083003/diff/60001/tools/perf/page_sets/sy... > > > tools/perf/page_sets/system_health/background_stories.py:23: > > > action_runner.Wait(PRE_MEASUREMENT_DELAY_SECONDS) > > > Actually, no, sorry about that. The _Measure method will eventually call > > > MeasureMemory in deterministic_mode, and that will already include a wait. > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... > > > > > > > > > https://codereview.chromium.org/2386083003/diff/60001/tools/perf/page_sets/sy... > > > tools/perf/page_sets/system_health/background_stories.py:29: URL = > > > 'https://www.google.co.uk/' > > > My proposal would be to include: > > > > > > - background:social:facebook > > > - background:search:google > > > - background:news:nytimes > > > - background:tools:gmail > > > - background:media:imgur > > > > > > Details and rationale: > > > > > > https://docs.google.com/a/google.com/document/d/1fQXX2W1qNdRSRWmVuhiFrDcyqxHV... > > > > > > Can we pick a subset of pages with audio, video, gif.... basically things that > > keep running in background? > > At the moment, on mobile, I think we don't have any stories that actually play > audio/video. I agree we should have those, but for now maybe this is a > reasonable v0 selection? The thing is our total budget is fixed for a while. So in the future, adding those stories mean we must removing some existing ones. I am fine if you're ok with this strategy. On another note, we still maybe able to pick some leaf pages of those domains that have a bunch of activities going on
On 2016/10/05 14:19:54, nednguyen wrote: > On 2016/10/05 14:15:20, perezju wrote: > > On 2016/10/05 14:07:28, nednguyen wrote: > > > On 2016/10/05 13:55:38, perezju wrote: > > > > > > > > > > https://codereview.chromium.org/2386083003/diff/60001/tools/perf/page_sets/sy... > > > > File tools/perf/page_sets/system_health/background_stories.py (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2386083003/diff/60001/tools/perf/page_sets/sy... > > > > tools/perf/page_sets/system_health/background_stories.py:23: > > > > action_runner.Wait(PRE_MEASUREMENT_DELAY_SECONDS) > > > > Actually, no, sorry about that. The _Measure method will eventually call > > > > MeasureMemory in deterministic_mode, and that will already include a wait. > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... > > > > > > > > > > > > > > https://codereview.chromium.org/2386083003/diff/60001/tools/perf/page_sets/sy... > > > > tools/perf/page_sets/system_health/background_stories.py:29: URL = > > > > 'https://www.google.co.uk/' > > > > My proposal would be to include: > > > > > > > > - background:social:facebook > > > > - background:search:google > > > > - background:news:nytimes > > > > - background:tools:gmail > > > > - background:media:imgur > > > > > > > > Details and rationale: > > > > > > > > > > https://docs.google.com/a/google.com/document/d/1fQXX2W1qNdRSRWmVuhiFrDcyqxHV... > > > > > > > > > Can we pick a subset of pages with audio, video, gif.... basically things > that > > > keep running in background? > > > > At the moment, on mobile, I think we don't have any stories that actually play > > audio/video. I agree we should have those, but for now maybe this is a > > reasonable v0 selection? > > The thing is our total budget is fixed for a while. So in the future, adding > those stories mean we must removing some existing ones. I am fine if you're ok > with this strategy. That sgtm.
https://codereview.chromium.org/2386083003/diff/60001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/background_stories.py (right): https://codereview.chromium.org/2386083003/diff/60001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/background_stories.py:23: action_runner.Wait(PRE_MEASUREMENT_DELAY_SECONDS) On 2016/10/05 13:55:38, perezju wrote: > Actually, no, sorry about that. The _Measure method will eventually call > MeasureMemory in deterministic_mode, and that will already include a wait. > > https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... Done. https://codereview.chromium.org/2386083003/diff/60001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/background_stories.py:29: URL = 'https://www.google.co.uk/' On 2016/10/05 13:55:38, perezju wrote: > My proposal would be to include: > > - background:social:facebook > - background:search:google > - background:news:nytimes > - background:tools:gmail > - background:media:imgur > > Details and rationale: > https://docs.google.com/a/google.com/document/d/1fQXX2W1qNdRSRWmVuhiFrDcyqxHV... Done, except gmail which is a little tricky.
https://codereview.chromium.org/2386083003/diff/100001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/background_stories.py (right): https://codereview.chromium.org/2386083003/diff/100001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/background_stories.py:24: URL = 'https://www.google.co.uk/' Can you change this to https://www.google.co.uk/#q=tom+cruise+movies? Bonus point if you add code to trigger the immersive movie browsing experience https://codereview.chromium.org/2386083003/diff/100001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/background_stories.py:36: URL = 'http://mobile.nytimes.com' Can you use http://www.nytimes.com/2016/10/04/us/politics/vice-presidential-debate.html?_r=0 ? https://codereview.chromium.org/2386083003/diff/100001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/background_stories.py:42: URL = 'http://imgur.com/gallery/5UlBN' How about using http://imgur.com/gallery/hUita ?
Description was changed from ========== Add a background story to MobileSystemHealth This adds a basic background story to the system health story set which loads google.com then puts the browser into the background before measuring memory. At the moment this story is only enabled for mobile. BUG=chromium:652205 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq ========== to ========== Add background stories to MobileSystemHealth This adds some background stories to the system health story set. These stories put the browser into the background before measuring memory use. At the moment these stories are only enabled for mobile since backgrounding only works on mobile. BUG=chromium:652205 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq ==========
ptal, I changed the BackgroundStory to be a mixin so we could take advantage of the gmail specific logic which I didn't want to replicate. Thanks! https://codereview.chromium.org/2386083003/diff/100001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/background_stories.py (right): https://codereview.chromium.org/2386083003/diff/100001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/background_stories.py:24: URL = 'https://www.google.co.uk/' On 2016/10/05 15:15:22, nednguyen wrote: > Can you change this to https://www.google.co.uk/#q=tom+cruise+movies? > > Bonus point if you add code to trigger the immersive movie browsing experience Done :) https://codereview.chromium.org/2386083003/diff/100001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/background_stories.py:36: URL = 'http://mobile.nytimes.com' On 2016/10/05 15:15:23, nednguyen wrote: > Can you use > http://www.nytimes.com/2016/10/04/us/politics/vice-presidential-debate.html?_r=0 > ? Done. https://codereview.chromium.org/2386083003/diff/100001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/background_stories.py:42: URL = 'http://imgur.com/gallery/5UlBN' On 2016/10/05 15:15:22, nednguyen wrote: > How about using http://imgur.com/gallery/hUita ? Done.
https://codereview.chromium.org/2386083003/diff/140001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/background_stories.py (right): https://codereview.chromium.org/2386083003/diff/140001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/background_stories.py:53: class BackgroundGmailMobileStory(_LoadGmailBaseStory, _BackgroundStoryMixin): I'm not super excited about the multiple inheritance, it tends to get kind of confusing quickly. Can we keep all other stories as in your patch set #7; and then for the gmail case subclass the public LoadGmailMobileStory and override methods as needed? (Even if it leads to a bit of code duplication, it would be easier to understand).
On 2016/10/06 12:41:45, perezju wrote: > https://codereview.chromium.org/2386083003/diff/140001/tools/perf/page_sets/s... > File tools/perf/page_sets/system_health/background_stories.py (right): > > https://codereview.chromium.org/2386083003/diff/140001/tools/perf/page_sets/s... > tools/perf/page_sets/system_health/background_stories.py:53: class > BackgroundGmailMobileStory(_LoadGmailBaseStory, _BackgroundStoryMixin): > I'm not super excited about the multiple inheritance, it tends to get kind of > confusing quickly. > > Can we keep all other stories as in your patch set #7; and then for the gmail > case subclass the public LoadGmailMobileStory and override methods as needed? > (Even if it leads to a bit of code duplication, it would be easier to > understand). done :)
I think this is looking good. If Ned is happy, maybe upload recordings now? https://codereview.chromium.org/2386083003/diff/180001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/background_stories.py (right): https://codereview.chromium.org/2386083003/diff/180001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/background_stories.py:15: nit: remove that blank space https://codereview.chromium.org/2386083003/diff/180001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/loading_stories.py (right): https://codereview.chromium.org/2386083003/diff/180001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/loading_stories.py:294: action_runner.tab.WaitForDocumentReadyStateToBeComplete() why do we need this now?
I think this is looking good. If Ned is happy, maybe upload recordings now?
Yeah, we can work on the recording now. If we can enable the video playing in nytimes one, it would be great. https://codereview.chromium.org/2386083003/diff/180001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/background_stories.py (right): https://codereview.chromium.org/2386083003/diff/180001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/background_stories.py:45: URL = 'http://www.nytimes.com/2016/10/04/us/politics/vice-presidential-debate.html?_r=0' Turns out this page has few videos in it, could you add logic to expand the article, scroll down, click on playing any video, then background the browser?
https://codereview.chromium.org/2386083003/diff/180001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/background_stories.py (right): https://codereview.chromium.org/2386083003/diff/180001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/background_stories.py:15: On 2016/10/06 13:32:42, perezju wrote: > nit: remove that blank space Done. https://codereview.chromium.org/2386083003/diff/180001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/background_stories.py:45: URL = 'http://www.nytimes.com/2016/10/04/us/politics/vice-presidential-debate.html?_r=0' On 2016/10/06 13:39:02, nednguyen wrote: > Turns out this page has few videos in it, could you add logic to expand the > article, scroll down, click on playing any video, then background the browser? Okay done, seems to kinda work although the video doesn't get past the buffering stage on my N5. https://codereview.chromium.org/2386083003/diff/180001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/loading_stories.py (right): https://codereview.chromium.org/2386083003/diff/180001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/loading_stories.py:294: action_runner.tab.WaitForDocumentReadyStateToBeComplete() On 2016/10/06 13:32:42, perezju wrote: > why do we need this now? Otherwise we never end up seeing the Navigate below. I'm not entirely sure why, it feels like we try to navigate before the page is done loading or something and then the navigate gets lost. I think it is caused by something on the page changing (the original Gmail story no longer works with --use-live-sites).
https://codereview.chromium.org/2386083003/diff/180001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/loading_stories.py (right): https://codereview.chromium.org/2386083003/diff/180001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/loading_stories.py:294: action_runner.tab.WaitForDocumentReadyStateToBeComplete() On 2016/10/06 15:27:35, hjd wrote: > On 2016/10/06 13:32:42, perezju wrote: > > why do we need this now? > > Otherwise we never end up seeing the Navigate below. > I'm not entirely sure why, it feels like we try to navigate before the page is > done loading or something and then the navigate gets lost. > I think it is caused by something on the page changing (the original Gmail story > no longer works with --use-live-sites). Ok, sounds good. Maybe just double-check after recording that both the original and the new story work.
https://codereview.chromium.org/2386083003/diff/200001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/background_stories.py (right): https://codereview.chromium.org/2386083003/diff/200001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/background_stories.py:56: 'document.querySelector(".nytd-player-poster").scrollIntoView();') Instead of doing this, can you use action_runner.Scroll? Also you may want to use new API randy is about to add: https://bugs.chromium.org/p/chromium/issues/detail?id=651909
On 2016/10/08 14:30:13, nednguyen wrote: > https://codereview.chromium.org/2386083003/diff/200001/tools/perf/page_sets/s... > File tools/perf/page_sets/system_health/background_stories.py (right): > > https://codereview.chromium.org/2386083003/diff/200001/tools/perf/page_sets/s... > tools/perf/page_sets/system_health/background_stories.py:56: > 'document.querySelector(".nytd-player-poster").scrollIntoView();') > Instead of doing this, can you use action_runner.Scroll? > > Also you may want to use new API randy is about to add: > https://bugs.chromium.org/p/chromium/issues/detail?id=651909 I used action_runner.Scroll for now, when that API lands I can update it :)
Okay, I recorded and upload the stories with Python 2.7.6 due to bug (https://github.com/catapult-project/catapult/issues/2920). PTAL, thanks! :)
On 2016/10/14 10:30:53, hjd wrote: > Okay, I recorded and upload the stories with Python 2.7.6 > due to bug (https://github.com/catapult-project/catapult/issues/2920). PTAL, > thanks! :) When trying to test this I get: NotFoundError: CommandException: No URLs matched: gs://chrome-partner-telemetry/913416b460b819ad2152a941ef99e183279487c9 Did you also upload the recordings?
Yep but I put them in gs://chromium-telemetry I guess it looks in gs://chrome-partner-telemetry? I can upload them there. On Fri, Oct 14, 2016 at 2:08 PM <perezju@chromium.org> wrote: > On 2016/10/14 10:30:53, hjd wrote: > > Okay, I recorded and upload the stories with Python 2.7.6 > > due to bug (https://github.com/catapult-project/catapult/issues/2920). > PTAL, > > thanks! :) > > When trying to test this I get: > > NotFoundError: CommandException: No URLs matched: > gs://chrome-partner-telemetry/913416b460b819ad2152a941ef99e183279487c9 > > Did you also upload the recordings? > > https://codereview.chromium.org/2386083003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/10/14 13:54:58, chromium-reviews wrote: > Yep but I put them in gs://chromium-telemetry I guess it looks in > gs://chrome-partner-telemetry? > I can upload them there. > > On Fri, Oct 14, 2016 at 2:08 PM <mailto:perezju@chromium.org> wrote: > > > On 2016/10/14 10:30:53, hjd wrote: > > > Okay, I recorded and upload the stories with Python 2.7.6 > > > due to bug (https://github.com/catapult-project/catapult/issues/2920). > > PTAL, > > > thanks! :) > > > > When trying to test this I get: > > > > NotFoundError: CommandException: No URLs matched: > > gs://chrome-partner-telemetry/913416b460b819ad2152a941ef99e183279487c9 > > > > Did you also upload the recordings? > > > > https://codereview.chromium.org/2386083003/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Yes, they should go in chrome-partner-telemetry.
https://codereview.chromium.org/2386083003/diff/240001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/background_stories.py (right): https://codereview.chromium.org/2386083003/diff/240001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/background_stories.py:50: action_runner.ScrollElement(selector='.nytd-player-poster') Randy has finished his work. You can now use: ScrollPageToElement Note that ScrollPageToElement is different from ScrollElement. The first one is scrolling the page until element is in the viewport, whereas the 2nd one issues scroll inside an element.
Thanks :) https://codereview.chromium.org/2386083003/diff/240001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/background_stories.py (right): https://codereview.chromium.org/2386083003/diff/240001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/background_stories.py:50: action_runner.ScrollElement(selector='.nytd-player-poster') On 2016/10/14 22:55:58, nednguyen wrote: > Randy has finished his work. You can now use: ScrollPageToElement > > Note that ScrollPageToElement is different from ScrollElement. The first one is > scrolling the page until element is in the viewport, whereas the 2nd one issues > scroll inside an element. Done.
lgtm, although gsutil is not working for me at the moment, and I can't test the patch :(
sorry, I am setting up my test phone to run your stories.. Will give extra comments if there are soon. https://codereview.chromium.org/2386083003/diff/260001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/loading_stories.py (left): https://codereview.chromium.org/2386083003/diff/260001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/loading_stories.py:310: def _DidLoadDocument(self, action_runner): What is this change about?
When I test this locally with my test phone's Wifi set to off, the pages fail to
load ("You are offline" pages are shown). :-/
https://codereview.chromium.org/2386083003/diff/260001/tools/perf/page_sets/s...
File tools/perf/page_sets/system_health/background_stories.py (right):
https://codereview.chromium.org/2386083003/diff/260001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/background_stories.py:32:
'document.querySelector("g-fab").click()')
ditto
https://codereview.chromium.org/2386083003/diff/260001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/background_stories.py:51:
'document.querySelector("#additional-content button").click()')
ditto
https://codereview.chromium.org/2386083003/diff/260001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/background_stories.py:57:
'document.querySelector(".nytd-player-poster").click();')
nits: use action_runner.TapElement(
'document.querySelector(".nytd-player-poster"') instead.
Always use action_runner... method if possible since they simulate user gesture.
https://codereview.chromium.org/2386083003/diff/260001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/background_stories.py:58:
After clicking playing the video, can you add a wait for like 5s here?
Sorry, please ignore my comments about not able to load the page when turning off wifi, turn out my catapult folder in chromium/src/third_party was in the "NoTsproxy" branch :P
ptal, I think everything should work now! I had to do a bit more tweaking to the gmail story. Also at one point I had to use .click() still because I couldn't get TapElement to work. https://codereview.chromium.org/2386083003/diff/260001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/background_stories.py (right): https://codereview.chromium.org/2386083003/diff/260001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/background_stories.py:32: 'document.querySelector("g-fab").click()') On 2016/10/18 15:27:29, nednguyen wrote: > ditto Done. https://codereview.chromium.org/2386083003/diff/260001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/background_stories.py:51: 'document.querySelector("#additional-content button").click()') On 2016/10/18 15:27:29, nednguyen wrote: > ditto Done. https://codereview.chromium.org/2386083003/diff/260001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/background_stories.py:57: 'document.querySelector(".nytd-player-poster").click();') On 2016/10/18 15:27:29, nednguyen wrote: > nits: use action_runner.TapElement( > 'document.querySelector(".nytd-player-poster"') instead. > > Always use action_runner... method if possible since they simulate user gesture. Done. https://codereview.chromium.org/2386083003/diff/260001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/loading_stories.py (left): https://codereview.chromium.org/2386083003/diff/260001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/loading_stories.py:310: def _DidLoadDocument(self, action_runner): On 2016/10/18 14:37:26, nednguyen wrote: > What is this change about? The "Get Inbox by Gmail" interstitial no longer appears* so this code breaks BackgroundGmailMobileStory which inherits from this class. It also breaks the LoadGmailMobileStory story if you use --use-live-sites so I was planning to re-record that story. I meant to leave in the '# Wait until the UI loads.' code, so I have readded that in the next CL. *At least when I try running the story.
On 2016/10/21 16:17:22, hjd wrote: > ptal, I think everything should work now! > I had to do a bit more tweaking to the gmail story. > Also at one point I had to use .click() still because I couldn't get TapElement > to work. > > https://codereview.chromium.org/2386083003/diff/260001/tools/perf/page_sets/s... > File tools/perf/page_sets/system_health/background_stories.py (right): > > https://codereview.chromium.org/2386083003/diff/260001/tools/perf/page_sets/s... > tools/perf/page_sets/system_health/background_stories.py:32: > 'document.querySelector("g-fab").click()') > On 2016/10/18 15:27:29, nednguyen wrote: > > ditto > > Done. > > https://codereview.chromium.org/2386083003/diff/260001/tools/perf/page_sets/s... > tools/perf/page_sets/system_health/background_stories.py:51: > 'document.querySelector("#additional-content button").click()') > On 2016/10/18 15:27:29, nednguyen wrote: > > ditto > > Done. > > https://codereview.chromium.org/2386083003/diff/260001/tools/perf/page_sets/s... > tools/perf/page_sets/system_health/background_stories.py:57: > 'document.querySelector(".nytd-player-poster").click();') > On 2016/10/18 15:27:29, nednguyen wrote: > > nits: use action_runner.TapElement( > > 'document.querySelector(".nytd-player-poster"') instead. > > > > Always use action_runner... method if possible since they simulate user > gesture. > > Done. > > https://codereview.chromium.org/2386083003/diff/260001/tools/perf/page_sets/s... > File tools/perf/page_sets/system_health/loading_stories.py (left): > > https://codereview.chromium.org/2386083003/diff/260001/tools/perf/page_sets/s... > tools/perf/page_sets/system_health/loading_stories.py:310: def > _DidLoadDocument(self, action_runner): > On 2016/10/18 14:37:26, nednguyen wrote: > > What is this change about? > > The "Get Inbox by Gmail" interstitial no longer appears* so this code breaks > BackgroundGmailMobileStory which inherits from this class. > > It also breaks the LoadGmailMobileStory story if you use --use-live-sites so I > was planning to re-record that story. > > I meant to leave in the '# Wait until the UI loads.' code, so I have readded > that in the next CL. > > *At least when I try running the story. For some reason I've got "The mail.google.com page isn't working. mail.google.com redirected you too many times." when trying to run the story. Although this could be related to crbug.com/657433 ?
On 2016/10/25 09:31:28, perezju wrote: > On 2016/10/21 16:17:22, hjd wrote: > > ptal, I think everything should work now! > > I had to do a bit more tweaking to the gmail story. > > Also at one point I had to use .click() still because I couldn't get > TapElement > > to work. > > > > > https://codereview.chromium.org/2386083003/diff/260001/tools/perf/page_sets/s... > > File tools/perf/page_sets/system_health/background_stories.py (right): > > > > > https://codereview.chromium.org/2386083003/diff/260001/tools/perf/page_sets/s... > > tools/perf/page_sets/system_health/background_stories.py:32: > > 'document.querySelector("g-fab").click()') > > On 2016/10/18 15:27:29, nednguyen wrote: > > > ditto > > > > Done. > > > > > https://codereview.chromium.org/2386083003/diff/260001/tools/perf/page_sets/s... > > tools/perf/page_sets/system_health/background_stories.py:51: > > 'document.querySelector("#additional-content button").click()') > > On 2016/10/18 15:27:29, nednguyen wrote: > > > ditto > > > > Done. > > > > > https://codereview.chromium.org/2386083003/diff/260001/tools/perf/page_sets/s... > > tools/perf/page_sets/system_health/background_stories.py:57: > > 'document.querySelector(".nytd-player-poster").click();') > > On 2016/10/18 15:27:29, nednguyen wrote: > > > nits: use action_runner.TapElement( > > > 'document.querySelector(".nytd-player-poster"') instead. > > > > > > Always use action_runner... method if possible since they simulate user > > gesture. > > > > Done. > > > > > https://codereview.chromium.org/2386083003/diff/260001/tools/perf/page_sets/s... > > File tools/perf/page_sets/system_health/loading_stories.py (left): > > > > > https://codereview.chromium.org/2386083003/diff/260001/tools/perf/page_sets/s... > > tools/perf/page_sets/system_health/loading_stories.py:310: def > > _DidLoadDocument(self, action_runner): > > On 2016/10/18 14:37:26, nednguyen wrote: > > > What is this change about? > > > > The "Get Inbox by Gmail" interstitial no longer appears* so this code breaks > > BackgroundGmailMobileStory which inherits from this class. > > > > It also breaks the LoadGmailMobileStory story if you use --use-live-sites so I > > was planning to re-record that story. > > > > I meant to leave in the '# Wait until the UI loads.' code, so I have readded > > that in the next CL. > > > > *At least when I try running the story. > > For some reason I've got "The http://mail.google.com page isn't working. > http://mail.google.com redirected you too many times." when trying to run the story. > Although this could be related to crbug.com/657433 ? Ran it again and this time it worked. Looks like the story is flaky :( Ned, should we try to land this and investigate the gmail flakiness separately? Other stories do work fine without issues.
On 2016/10/25 09:33:24, perezju wrote: > On 2016/10/25 09:31:28, perezju wrote: > > On 2016/10/21 16:17:22, hjd wrote: > > > ptal, I think everything should work now! > > > I had to do a bit more tweaking to the gmail story. > > > Also at one point I had to use .click() still because I couldn't get > > TapElement > > > to work. > > > > > > > > > https://codereview.chromium.org/2386083003/diff/260001/tools/perf/page_sets/s... > > > File tools/perf/page_sets/system_health/background_stories.py (right): > > > > > > > > > https://codereview.chromium.org/2386083003/diff/260001/tools/perf/page_sets/s... > > > tools/perf/page_sets/system_health/background_stories.py:32: > > > 'document.querySelector("g-fab").click()') > > > On 2016/10/18 15:27:29, nednguyen wrote: > > > > ditto > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/2386083003/diff/260001/tools/perf/page_sets/s... > > > tools/perf/page_sets/system_health/background_stories.py:51: > > > 'document.querySelector("#additional-content button").click()') > > > On 2016/10/18 15:27:29, nednguyen wrote: > > > > ditto > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/2386083003/diff/260001/tools/perf/page_sets/s... > > > tools/perf/page_sets/system_health/background_stories.py:57: > > > 'document.querySelector(".nytd-player-poster").click();') > > > On 2016/10/18 15:27:29, nednguyen wrote: > > > > nits: use action_runner.TapElement( > > > > 'document.querySelector(".nytd-player-poster"') instead. > > > > > > > > Always use action_runner... method if possible since they simulate user > > > gesture. > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/2386083003/diff/260001/tools/perf/page_sets/s... > > > File tools/perf/page_sets/system_health/loading_stories.py (left): > > > > > > > > > https://codereview.chromium.org/2386083003/diff/260001/tools/perf/page_sets/s... > > > tools/perf/page_sets/system_health/loading_stories.py:310: def > > > _DidLoadDocument(self, action_runner): > > > On 2016/10/18 14:37:26, nednguyen wrote: > > > > What is this change about? > > > > > > The "Get Inbox by Gmail" interstitial no longer appears* so this code breaks > > > BackgroundGmailMobileStory which inherits from this class. > > > > > > It also breaks the LoadGmailMobileStory story if you use --use-live-sites so > I > > > was planning to re-record that story. > > > > > > I meant to leave in the '# Wait until the UI loads.' code, so I have readded > > > that in the next CL. > > > > > > *At least when I try running the story. > > > > For some reason I've got "The http://mail.google.com page isn't working. > > http://mail.google.com redirected you too many times." when trying to run the > story. > > Although this could be related to crbug.com/657433 ? > > Ran it again and this time it worked. Looks like the story is flaky :( > > Ned, should we try to land this and investigate the gmail flakiness separately? > > Other stories do work fine without issues. yes, we should land & investigate separately lgtm
The CQ bit was checked by hjd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from perezju@chromium.org Link to the patchset: https://codereview.chromium.org/2386083003/#ps300001 (title: "tweak stories some more")
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.
Description was changed from ========== Add background stories to MobileSystemHealth This adds some background stories to the system health story set. These stories put the browser into the background before measuring memory use. At the moment these stories are only enabled for mobile since backgrounding only works on mobile. BUG=chromium:652205 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq ========== to ========== Add background stories to MobileSystemHealth This adds some background stories to the system health story set. These stories put the browser into the background before measuring memory use. At the moment these stories are only enabled for mobile since backgrounding only works on mobile. BUG=chromium:652205 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== Add background stories to MobileSystemHealth This adds some background stories to the system health story set. These stories put the browser into the background before measuring memory use. At the moment these stories are only enabled for mobile since backgrounding only works on mobile. BUG=chromium:652205 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq ========== to ========== Add background stories to MobileSystemHealth This adds some background stories to the system health story set. These stories put the browser into the background before measuring memory use. At the moment these stories are only enabled for mobile since backgrounding only works on mobile. BUG=chromium:652205 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq Committed: https://crrev.com/81b7c28dc5a75396861fca85ace8c1420224af6b Cr-Commit-Position: refs/heads/master@{#427320} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/81b7c28dc5a75396861fca85ace8c1420224af6b Cr-Commit-Position: refs/heads/master@{#427320} |
