Basic android app implementation

Basic android app implementation.

BUG=417542, 439346
Committed: https://crrev.com/e7e5b08f01df0781b1022976a0d3ed56cae9cfd8
Cr-Commit-Position: refs/heads/master@{#307583}

[slamm, 2014-12-05 01:31:33]

Fix a couple errors

[nednguyen, 2014-12-05 02:24:05]

nednguyen@google.com changed reviewers:
+ nednguyen@google.com

[nednguyen, 2014-12-05 02:24:06]

You should talk to Zhen, he is working on refactoring the device, platform part
also.

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/android_app_backend.py (right):

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/android_app_backend.py:33: import
logging; logging.warn('Sleeping after start.')
Let's just wait for pid to come up in this one. Custom shared_user_story_state
can do further waiting that's specific to app.

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/user_story/android/shared_app_state.py (right):

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/android/shared_app_state.py:84: # CHEAT
HAXX0R!!
Please don't check this comment in ;)

[chrishenry, 2014-12-05 06:11:57]

On 2014/12/05 02:24:06, nednguyen wrote:
> You should talk to Zhen, he is working on refactoring the device, platform
part
> also.

Yep, he's aware. I mentioned this on another thread.

> 
>
https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
> File tools/telemetry/telemetry/core/backends/android_app_backend.py (right):
> 
>
https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/core/backends/android_app_backend.py:33: import
> logging; logging.warn('Sleeping after start.')
> Let's just wait for pid to come up in this one. Custom shared_user_story_state
> can do further waiting that's specific to app.

Let's push this to a follow up CL. This skeleton is very important as it enables
parallelization of follow up effort (adding TBM and fixing my haxx comment
below, adding ready state predicate to user_story, adding WPR support, etc.).
There is still work left to be done for this patch (unit tests mainly), so let's
focus on that first.

> 
>
https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
> File tools/telemetry/telemetry/user_story/android/shared_app_state.py (right):
> 
>
https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/user_story/android/shared_app_state.py:84: # CHEAT
> HAXX0R!!
> Please don't check this comment in ;)

Oopsie... (: Yes, please change this to actual TODO.

[chrishenry, 2014-12-05 06:21:00]

chrishenry@google.com changed reviewers:
+ chrishenry@google.com

[chrishenry, 2014-12-05 06:21:02]

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/benchmark.py (right):

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/benchmark.py:189: #  raise TypeError('"%s" is not a
PageTest.' % self.test.__name__)
Can you revert changes to this file? I'll work on a patch to properly fix this
tomorrow morning.

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/android_app_backend.py (right):

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/android_app_backend.py:32: # the right
thing to do here.
Update the HACK comment to say something about checking ps and adding
ready_state_predicate to UserStory.

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/android_app_backend.py:34:
time.sleep(10)
Let's keep this here, but update the HACK remark to say that we should use ps
hack for a default mechanism + an option in AppStory to customize the
wait-for-ready-state.

And kill the import logging stuff above.

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/app_backend.py (right):

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/app_backend.py:27: raise
NotImplementedError
(If you want to submit this separately as a separate bug fix patch, I'll stamp
it tomorrow morning.)

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/user_story/android/__init__.py (right):

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/android/__init__.py:16: return True
TODO here or remove ready_state_predicate in this patch. Let's follow up in
subsequent patch.

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/android/__init__.py:31: when the
application is ready to run the user story.
(We worked on this pydoc already, so I kept the text in an email--sent to
you--so that we can reuse in subsequent patch.)

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/android/__init__.py:35: def Run(self):
Can you add a TODO, to indicate that this would not work yet? Or better yet, add

def RunPageInteractions(self):
  self.Run()

instead of in perf internal. and add a TODO note to fix TBM here.

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/user_story/android/shared_app_state.py (right):

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/android/shared_app_state.py:27: class
SharedAppState(shared_user_story_state.SharedUserStoryState):
Can you add a WARNING statement here that this class is not ready for public
consumption (and email telemetry@chromium.org if you ever feel the urge to use
it).

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/android/shared_app_state.py:27: class
SharedAppState(shared_user_story_state.SharedUserStoryState):
pydoc pls.

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/android/shared_app_state.py:46: return
self._platform._platform_backend
This need pylint marker.

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/android/shared_app_state.py:78:
platform_backend = self.platform._platform_backend  # pylint: disable=W0212
Remove this and replace with self._platform_backend where it is being used.

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/android/shared_app_state.py:100: def
GetPlatform(finder_options):
Can you move this to android_device like in Zhen's patch that Ari sent to you?
And ping him on chat and the code review thread that you need a similar change
there (point him to this patch too).

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/android/shared_app_state.py:148: def
CanFindDevices():
CanDiscoverDevices (similar to DiscoverClasses) or CanUseDevices?

[slamm, 2014-12-05 18:12:19]

Address comments for modified files.

[slamm, 2014-12-05 18:19:23]

I am heading off to an interview in a few minutes. I did not make it to the
comments for the new files.

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/android_app_backend.py (right):

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/android_app_backend.py:32: # the right
thing to do here.
On 2014/12/05 06:21:01, chrishenry wrote:
> Update the HACK comment to say something about checking ps and adding
> ready_state_predicate to UserStory.

Instead of searching for the app in 'ps', I changed a StartActivity param:

   StartActivity(..., blocking=True)

If that does not actually do what we want, we can go back to the ps command
idea.

I added a doc string with the customization comment.

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/android_app_backend.py:33: import
logging; logging.warn('Sleeping after start.')
On 2014/12/05 02:24:06, nednguyen wrote:
> Let's just wait for pid to come up in this one. Custom shared_user_story_state
> can do further waiting that's specific to app.

Acknowledged.

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/android_app_backend.py:34:
time.sleep(10)
On 2014/12/05 06:21:01, chrishenry wrote:
> Let's keep this here, but update the HACK remark to say that we should use ps
> hack for a default mechanism + an option in AppStory to customize the
> wait-for-ready-state.
> 
> And kill the import logging stuff above.

Done.

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/app_backend.py (right):

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/app_backend.py:27: raise
NotImplementedError
On 2014/12/05 06:21:01, chrishenry wrote:
> (If you want to submit this separately as a separate bug fix patch, I'll stamp
> it tomorrow morning.)

Done: https://codereview.chromium.org/761863007

[chrishenry, 2014-12-05 18:22:36]

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/android_app_backend.py (right):

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/android_app_backend.py:32: # the right
thing to do here.
On 2014/12/05 18:19:23, slamm wrote:
> On 2014/12/05 06:21:01, chrishenry wrote:
> > Update the HACK comment to say something about checking ps and adding
> > ready_state_predicate to UserStory.
> 
> Instead of searching for the app in 'ps', I changed a StartActivity param:
> 
>    StartActivity(..., blocking=True)
> 
> If that does not actually do what we want, we can go back to the ps command
> idea.
> 
> I added a doc string with the customization comment.

There was a comment from wuhu in the original hackathon patch: "# TODO(wuhu):
GOOGLE_SEARCH intent does not signal completion." Can we make sure we test this
and confirm that it still works?

[chrishenry, 2014-12-05 20:07:21]

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/benchmark.py (right):

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/benchmark.py:189: #  raise TypeError('"%s" is not a
PageTest.' % self.test.__name__)
On 2014/12/05 06:21:01, chrishenry wrote:
> Can you revert changes to this file? I'll work on a patch to properly fix this
> tomorrow morning.

I just checked and there is no change necessary here. You need to override
CreateUserStorySet in your benchmark instead of using page_set = ... and it
should work fine.

CreatePageTest should also work since TBM is a PageTest.

[nednguyen, 2014-12-05 20:15:22]

https://codereview.chromium.org/776883004/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/user_story/android/shared_app_state.py (right):

https://codereview.chromium.org/776883004/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/android/shared_app_state.py:45: def
_platform_backend(self):
This is a hack. If there is no plan to fix this in this patch, please add
comment to explain that this pattern shouldn't be adopted.

[slamm, 2014-12-05 21:20:24]

Rebase.

[slamm, 2014-12-06 00:25:25]

Review comments.

[slamm, 2014-12-06 00:26:55]

Review comments.

[slamm, 2014-12-06 00:27:26]

Patchset #5 (id:80001) has been deleted

[slamm, 2014-12-06 00:40:00]

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/benchmark.py (right):

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/benchmark.py:189: #  raise TypeError('"%s" is not a
PageTest.' % self.test.__name__)
On 2014/12/05 20:07:21, chrishenry wrote:
> On 2014/12/05 06:21:01, chrishenry wrote:
> > Can you revert changes to this file? I'll work on a patch to properly fix
this
> > tomorrow morning.
> 
> I just checked and there is no change necessary here. You need to override
> CreateUserStorySet in your benchmark instead of using page_set = ... and it
> should work fine.
> 
> CreatePageTest should also work since TBM is a PageTest.

Done.

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/user_story/android/__init__.py (right):

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/android/__init__.py:16: return True
On 2014/12/05 06:21:01, chrishenry wrote:
> TODO here or remove ready_state_predicate in this patch. Let's follow up in
> subsequent patch.

Done.

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/android/__init__.py:31: when the
application is ready to run the user story.
On 2014/12/05 06:21:01, chrishenry wrote:
> (We worked on this pydoc already, so I kept the text in an email--sent to
> you--so that we can reuse in subsequent patch.)

Done.

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/android/__init__.py:35: def Run(self):
On 2014/12/05 06:21:01, chrishenry wrote:
> Can you add a TODO, to indicate that this would not work yet? Or better yet,
add
> 
> def RunPageInteractions(self):
>   self.Run()
> 
> instead of in perf internal. and add a TODO note to fix TBM here.

Done.

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/user_story/android/shared_app_state.py (right):

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/android/shared_app_state.py:27: class
SharedAppState(shared_user_story_state.SharedUserStoryState):
On 2014/12/05 06:21:01, chrishenry wrote:
> Can you add a WARNING statement here that this class is not ready for public
> consumption (and email mailto:telemetry@chromium.org if you ever feel the urge
to use
> it).

Done.

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/android/shared_app_state.py:27: class
SharedAppState(shared_user_story_state.SharedUserStoryState):
On 2014/12/05 06:21:01, chrishenry wrote:
> Can you add a WARNING statement here that this class is not ready for public
> consumption (and email mailto:telemetry@chromium.org if you ever feel the urge
to use
> it).

Done.

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/android/shared_app_state.py:46: return
self._platform._platform_backend
On 2014/12/05 06:21:01, chrishenry wrote:
> This need pylint marker.

Done.

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/android/shared_app_state.py:78:
platform_backend = self.platform._platform_backend  # pylint: disable=W0212
On 2014/12/05 06:21:02, chrishenry wrote:
> Remove this and replace with self._platform_backend where it is being used.

Done.

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/android/shared_app_state.py:84: # CHEAT
HAXX0R!!
On 2014/12/05 02:24:06, nednguyen wrote:
> Please don't check this comment in ;)

Done.

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/android/shared_app_state.py:100: def
GetPlatform(finder_options):
On 2014/12/05 06:21:01, chrishenry wrote:
> Can you move this to android_device like in Zhen's patch that Ari sent to you?
> And ping him on chat and the code review thread that you need a similar change
> there (point him to this patch too).

Email sent.
Move done. (unittests are next.)

https://codereview.chromium.org/776883004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/android/shared_app_state.py:148: def
CanFindDevices():
On 2014/12/05 06:21:02, chrishenry wrote:
> CanDiscoverDevices (similar to DiscoverClasses) or CanUseDevices?

Done.

https://codereview.chromium.org/776883004/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/user_story/android/shared_app_state.py (right):

https://codereview.chromium.org/776883004/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/android/shared_app_state.py:45: def
_platform_backend(self):
On 2014/12/05 20:15:22, nednguyen wrote:
> This is a hack. If there is no plan to fix this in this patch, please add
> comment to explain that this pattern shouldn't be adopted.

Done.

https://codereview.chromium.org/776883004/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/android/shared_app_state.py:45: def
_platform_backend(self):
On 2014/12/05 20:15:22, nednguyen wrote:
> This is a hack. If there is no plan to fix this in this patch, please add
> comment to explain that this pattern shouldn't be adopted.

Done.

[chrishenry, 2014-12-06 00:52:48]

Please add some unit test coverage too.

https://codereview.chromium.org/776883004/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/backends/android_app_backend.py (right):

https://codereview.chromium.org/776883004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/android_app_backend.py:38: raise
NotImplementedError
Should this return something reasonable (like an is_running boolean that gets
set in Start and reset in Close)?

https://codereview.chromium.org/776883004/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/user_story/android/__init__.py (right):

https://codereview.chromium.org/776883004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/user_story/android/__init__.py:16: return True
Delete this, this is unused.

https://codereview.chromium.org/776883004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/user_story/android/__init__.py:22: def Run(self):
Add a pydoc that this is the method that AppStory subclassers should implement
to do ...

https://codereview.chromium.org/776883004/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/user_story/android/shared_app_state.py (right):

https://codereview.chromium.org/776883004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/user_story/android/shared_app_state.py:73:
#self._PrepareWpr(self.platform.network_controller, user_story.archive_path)
Let's delete this along with def _PrepareWpr above. We'll add it back in a
separate patch.

https://codereview.chromium.org/776883004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/user_story/android/shared_app_state.py:76: #
SharedAndroidAppState.DidRunUserStory?
Kill the TODO, this is already in the right place.

[slamm, 2014-12-08 19:38:36]

Avoid duplicate code. Move unittests.

[slamm, 2014-12-09 00:57:12]

more tests

[nednguyen, 2014-12-09 03:01:47]

nednguyen@google.com changed reviewers:
+ zhenw@chromium.org

[nednguyen, 2014-12-09 03:01:49]

https://codereview.chromium.org/776883004/diff/140001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/android_device.py (right):

https://codereview.chromium.org/776883004/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/android_device.py:78: def
GetPlatform(finder_options):
This should belong to platform instead. We have
platform.GetPlatformForDevice(..), you can define another factory method like 
platform.GetPlatformFromFinderOptions(...) which maybe cover both android &
cros.

[chrishenry, 2014-12-09 18:24:45]

On 2014/12/09 03:01:49, nednguyen wrote:
>
https://codereview.chromium.org/776883004/diff/140001/tools/telemetry/telemet...
> File tools/telemetry/telemetry/core/platform/android_device.py (right):
> 
>
https://codereview.chromium.org/776883004/diff/140001/tools/telemetry/telemet...
> tools/telemetry/telemetry/core/platform/android_device.py:78: def
> GetPlatform(finder_options):
> This should belong to platform instead. We have
> platform.GetPlatformForDevice(..), you can define another factory method like 
> platform.GetPlatformFromFinderOptions(...) which maybe cover both android &
> cros.

We discussed this briefly. I don't think platform is the right home either. Here
is what we ended up doing:
1. android_device.GetDevice(platform_finder) returns a device
2. AndroidPlatformBackend performs adb stabilization during __init__
3. Users call
platform.GetPlatformForDevice(android_device.GetDevice(platform_finder)) -- a
bit longer but obvious what is happening.

Wdyt?

[nednguyen, 2014-12-09 18:33:39]

On 2014/12/09 18:24:45, chrishenry wrote:
> On 2014/12/09 03:01:49, nednguyen wrote:
> >
>
https://codereview.chromium.org/776883004/diff/140001/tools/telemetry/telemet...
> > File tools/telemetry/telemetry/core/platform/android_device.py (right):
> > 
> >
>
https://codereview.chromium.org/776883004/diff/140001/tools/telemetry/telemet...
> > tools/telemetry/telemetry/core/platform/android_device.py:78: def
> > GetPlatform(finder_options):
> > This should belong to platform instead. We have
> > platform.GetPlatformForDevice(..), you can define another factory method
like 
> > platform.GetPlatformFromFinderOptions(...) which maybe cover both android &
> > cros.
> 
> We discussed this briefly. I don't think platform is the right home either.
Here
> is what we ended up doing:
> 1. android_device.GetDevice(platform_finder) returns a device
What is platform_finder? Why not just finder_options?

> 2. AndroidPlatformBackend performs adb stabilization during __init__
> 3. Users call
> platform.GetPlatformForDevice(android_device.GetDevice(platform_finder)) -- a
> bit longer but obvious what is happening.
> 
> Wdyt?

[slamm, 2014-12-09 18:47:33]

Have android_device only find the device (not the platform too)

[chromium-reviews, 2014-12-09 18:55:55]

On Tue, Dec 9, 2014 at 10:33 AM, nednguyen via telemetry <
telemetry@chromium.org> wrote:

> On 2014/12/09 18:24:45, chrishenry wrote:
>
>> On 2014/12/09 03:01:49, nednguyen wrote:
>> >
>>
>
> https://codereview.chromium.org/776883004/diff/140001/
> tools/telemetry/telemetry/core/platform/android_device.py
>
>> > File tools/telemetry/telemetry/core/platform/android_device.py (right):
>> >
>> >
>>
>
> https://codereview.chromium.org/776883004/diff/140001/
> tools/telemetry/telemetry/core/platform/android_device.py#newcode78
>
>> > tools/telemetry/telemetry/core/platform/android_device.py:78: def
>> > GetPlatform(finder_options):
>> > This should belong to platform instead. We have
>> > platform.GetPlatformForDevice(..), you can define another factory
>> method
>>
> like
>
>> > platform.GetPlatformFromFinderOptions(...) which maybe cover both
>> android &
>> > cros.
>>
>
>  We discussed this briefly. I don't think platform is the right home
>> either.
>>
> Here
>
>> is what we ended up doing:
>> 1. android_device.GetDevice(platform_finder) returns a device
>>
> What is platform_finder? Why not just finder_options?


Typo: it is finder_options.


>
>
>  2. AndroidPlatformBackend performs adb stabilization during __init__
>> 3. Users call
>> platform.GetPlatformForDevice(android_device.GetDevice(platform_finder))
>> -- a
>> bit longer but obvious what is happening.
>>
>
>  Wdyt?
>>
>
>
>
> https://codereview.chromium.org/776883004/
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to telemetry+unsubscribe@chromium.org.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[slamm, 2014-12-09 19:15:20]

Update unit tests for previous change.

[slamm, 2014-12-09 19:34:39]

Finish review comments

[slamm, 2014-12-09 19:59:31]

PTAL

https://codereview.chromium.org/776883004/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/backends/android_app_backend.py (right):

https://codereview.chromium.org/776883004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/android_app_backend.py:38: raise
NotImplementedError
On 2014/12/06 00:52:48, chrishenry wrote:
> Should this return something reasonable (like an is_running boolean that gets
> set in Start and reset in Close)?

Done.

https://codereview.chromium.org/776883004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/android_app_backend.py:38: raise
NotImplementedError
On 2014/12/06 00:52:48, chrishenry wrote:
> Should this return something reasonable (like an is_running boolean that gets
> set in Start and reset in Close)?

Done.

https://codereview.chromium.org/776883004/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/user_story/android/__init__.py (right):

https://codereview.chromium.org/776883004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/user_story/android/__init__.py:16: return True
On 2014/12/06 00:52:48, chrishenry wrote:
> Delete this, this is unused.

Done.

https://codereview.chromium.org/776883004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/user_story/android/__init__.py:22: def Run(self):
On 2014/12/06 00:52:48, chrishenry wrote:
> Add a pydoc that this is the method that AppStory subclassers should implement
> to do ...

Done.

https://codereview.chromium.org/776883004/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/user_story/android/shared_app_state.py (right):

https://codereview.chromium.org/776883004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/user_story/android/shared_app_state.py:73:
#self._PrepareWpr(self.platform.network_controller, user_story.archive_path)
On 2014/12/06 00:52:48, chrishenry wrote:
> Let's delete this along with def _PrepareWpr above. We'll add it back in a
> separate patch.

Done.

https://codereview.chromium.org/776883004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/user_story/android/shared_app_state.py:76: #
SharedAndroidAppState.DidRunUserStory?
On 2014/12/06 00:52:48, chrishenry wrote:
> Kill the TODO, this is already in the right place.

Done.

[chrishenry, 2014-12-09 20:05:48]

lgtm, but please address the following comments as well.

https://codereview.chromium.org/776883004/diff/200001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/app.py (right):

https://codereview.chromium.org/776883004/diff/200001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/app.py:22: return self._app_backend
Actually App should not expose AppBackend, since that kinda defeat the purpose
of having the App/AppBackend separation.

https://codereview.chromium.org/776883004/diff/200001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/backends/chrome/android_browser_finder.py
(right):

https://codereview.chromium.org/776883004/diff/200001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/android_browser_finder.py:164:
"""Finds all the desktop browsers available on this machine."""
Not this machine, but the device.

https://codereview.chromium.org/776883004/diff/200001/tools/telemetry/telemet...
File tools/telemetry/telemetry/user_story/android/shared_app_state.py (right):

https://codereview.chromium.org/776883004/diff/200001/tools/telemetry/telemet...
tools/telemetry/telemetry/user_story/android/shared_app_state.py:20: Only
supports TimelineBasedMeasurement tests.
Kill this line. The class doesn't support TBM yet.

https://codereview.chromium.org/776883004/diff/200001/tools/telemetry/telemet...
tools/telemetry/telemetry/user_story/android/shared_app_state.py:35: assert
isinstance(test, timeline_based_measurement.TimelineBasedMeasurement)
Can we add a message for this assertion.

[nednguyen, 2014-12-09 20:17:45]

On 2014/12/09 20:05:48, chrishenry wrote:
> lgtm, but please address the following comments as well.
> 
>
https://codereview.chromium.org/776883004/diff/200001/tools/telemetry/telemet...
> File tools/telemetry/telemetry/core/app.py (right):
> 
>
https://codereview.chromium.org/776883004/diff/200001/tools/telemetry/telemet...
> tools/telemetry/telemetry/core/app.py:22: return self._app_backend
> Actually App should not expose AppBackend, since that kinda defeat the purpose
> of having the App/AppBackend separation.
> 
>
https://codereview.chromium.org/776883004/diff/200001/tools/telemetry/telemet...
> File tools/telemetry/telemetry/core/backends/chrome/android_browser_finder.py
> (right):
> 
>
https://codereview.chromium.org/776883004/diff/200001/tools/telemetry/telemet...
> tools/telemetry/telemetry/core/backends/chrome/android_browser_finder.py:164:
> """Finds all the desktop browsers available on this machine."""
> Not this machine, but the device.
> 
>
https://codereview.chromium.org/776883004/diff/200001/tools/telemetry/telemet...
> File tools/telemetry/telemetry/user_story/android/shared_app_state.py (right):
> 
>
https://codereview.chromium.org/776883004/diff/200001/tools/telemetry/telemet...
> tools/telemetry/telemetry/user_story/android/shared_app_state.py:20: Only
> supports TimelineBasedMeasurement tests.
> Kill this line. The class doesn't support TBM yet.
> 
>
https://codereview.chromium.org/776883004/diff/200001/tools/telemetry/telemet...
> tools/telemetry/telemetry/user_story/android/shared_app_state.py:35: assert
> isinstance(test, timeline_based_measurement.TimelineBasedMeasurement)
> Can we add a message for this assertion.

The new approach SGTM. Device should only do device logic.

[slamm, 2014-12-09 20:28:43]

Rebase

[slamm, 2014-12-09 20:31:22]

More review comments.

[slamm, 2014-12-09 20:31:44]

https://codereview.chromium.org/776883004/diff/200001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/app.py (right):

https://codereview.chromium.org/776883004/diff/200001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/app.py:22: return self._app_backend
On 2014/12/09 20:05:48, chrishenry wrote:
> Actually App should not expose AppBackend, since that kinda defeat the purpose
> of having the App/AppBackend separation.

Done.

https://codereview.chromium.org/776883004/diff/200001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/backends/chrome/android_browser_finder.py
(right):

https://codereview.chromium.org/776883004/diff/200001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/android_browser_finder.py:164:
"""Finds all the desktop browsers available on this machine."""
On 2014/12/09 20:05:48, chrishenry wrote:
> Not this machine, but the device.

Updated.

https://codereview.chromium.org/776883004/diff/200001/tools/telemetry/telemet...
File tools/telemetry/telemetry/user_story/android/shared_app_state.py (right):

https://codereview.chromium.org/776883004/diff/200001/tools/telemetry/telemet...
tools/telemetry/telemetry/user_story/android/shared_app_state.py:20: Only
supports TimelineBasedMeasurement tests.
On 2014/12/09 20:05:48, chrishenry wrote:
> Kill this line. The class doesn't support TBM yet.

Done.

https://codereview.chromium.org/776883004/diff/200001/tools/telemetry/telemet...
tools/telemetry/telemetry/user_story/android/shared_app_state.py:20: Only
supports TimelineBasedMeasurement tests.
On 2014/12/09 20:05:48, chrishenry wrote:
> Kill this line. The class doesn't support TBM yet.

Done.

https://codereview.chromium.org/776883004/diff/200001/tools/telemetry/telemet...
tools/telemetry/telemetry/user_story/android/shared_app_state.py:35: assert
isinstance(test, timeline_based_measurement.TimelineBasedMeasurement)
On 2014/12/09 20:05:48, chrishenry wrote:
> Can we add a message for this assertion.

Changed it to raise a ValueError with a message.

[slamm, 2014-12-09 22:08:56]

The CQ bit was checked by slamm@chromium.org

[commit-bot: I haz the power, 2014-12-09 22:09:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/776883004/240001

[commit-bot: I haz the power, 2014-12-09 23:28:19]

Committed patchset #12 (id:240001)

[commit-bot: I haz the power, 2014-12-09 23:30:05]

Patchset 12 (id:??) landed as
https://crrev.com/e7e5b08f01df0781b1022976a0d3ed56cae9cfd8
Cr-Commit-Position: refs/heads/master@{#307583}