Move remote platforms creation logic from android_browser_finder to platform

Move remote platforms creation logic from android_browser_finder to platform

Previously, creation of platforms is done in browser_finder
classes. This patch aims to detach the coupling between
creating browsers from platform by moving such methods to  platform.
Also modify android_platform_backend so it owns the adb instance.

SHERRIFS: reverting this patch will require reverting https://codereview.chromium.org/541693004/ first.

BUG=413637
Committed: https://crrev.com/5289ba2d6a2c1eb3f429673ac0088f658d8aee5a
Cr-Commit-Position: refs/heads/master@{#296310}

[nednguyen, 2014-08-29 03:23:15]

nednguyen@google.com changed reviewers:
+ nduca@chromium.org, tonyg@chromium.org

[nednguyen, 2014-08-29 03:30:50]

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

[nednguyen, 2014-09-02 16:21:16]

On 2014/09/02 15:32:37, nednguyen wrote:

Discussed with Nat offline, I will make some further changes to this patch to
include cros devices.

[nduca, 2014-09-02 16:33:31]

per chat, how about

abstract class Device, Android and Cros being two subclasses. staticmethod
Device.FindAllConnected(), which calls android_device.py and cros_device.py
methods.

platform/__init__.py already has GetHostPlatform, so then just
GetPlatformForDevice(Device). Should only ever be 1 platform per device, so we
can enforce it.

This then makes it possible to create a platform without a finder, I think,
which is a good first step to then launching an app

[nednguyen, 2014-09-03 18:10:14]

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

[nednguyen, 2014-09-03 18:10:14]

+achuith for changes to cros

[nednguyen, 2014-09-03 18:46:53]

Patchset #4 (id:80001) has been deleted

[nednguyen, 2014-09-03 18:57:35]

Patchset #4 (id:100001) has been deleted

[nednguyen, 2014-09-03 23:01:21]

Patchset #4 (id:120001) has been deleted

[nednguyen, 2014-09-04 00:39:28]

On 2014/09/03 18:10:14, nednguyen wrote:
> +achuith for changes to cros

PTAL

[achuithb, 2014-09-04 14:08:51]

On 2014/09/04 00:39:28, nednguyen wrote:
> On 2014/09/03 18:10:14, nednguyen wrote:
> > +achuith for changes to cros
> 
> PTAL

please try this on cros trybots:
cbuildbot --remote -G 522553002 x86-generic-tot-chrome-pfq-informational
amd64-generic-tot-chrome-pfq-informational daisy-tot-chrome-pfq-informational

[achuithb, 2014-09-04 14:17:06]

Overall this looks ok.

Is there a way to split this CL up? It would be nice if you could pull the cros
changes out into a separate CL.

[nednguyen, 2014-09-04 15:56:44]

On 2014/09/04 14:17:06, achuithb wrote:
> Overall this looks ok.
> 
> Is there a way to split this CL up? It would be nice if you could pull the
cros
> changes out into a separate CL.

Done. I split cros related changes to https://codereview.chromium.org/541693004/

[nednguyen, 2014-09-16 05:03:47]

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

[nednguyen, 2014-09-16 05:03:48]

PTAL again

[chrishenry, 2014-09-22 16:16:41]

https://codereview.chromium.org/522553002/diff/180001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/backends/chrome/android_browser_finder.py
(left):

https://codereview.chromium.org/522553002/diff/180001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/android_browser_finder.py:277:
return []
Where is this logic to return [] now?

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

https://codereview.chromium.org/522553002/diff/180001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/android_browser_finder.py:196:
devices = android_device.AndroidDevice.GetAllConnectedDevices()
Shouldn't this also be moved out from browser_finder?

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

https://codereview.chromium.org/522553002/diff/180001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/android_device.py:1: # Copyright 2014
The Chromium Authors. All rights reserved.
Is this branched from anywhere? force copy detection if it is (to maintain
history).

https://codereview.chromium.org/522553002/diff/180001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/android_device.py:14: Attributes:
Nit: 1 blank line before Attributes:

https://codereview.chromium.org/522553002/diff/180001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/android_device.py:18:
no_performance_mode: when this is set to True, android platform will be set
Eh.. The naming is a bit weird, why is no_performance_mode = True means the
platform will be set to high perf mode?

I also dislike naming param with negative, can you just say
enable_performance_mode?

https://codereview.chromium.org/522553002/diff/180001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/android_device.py:20: logging: where to
store the log results.
What is this for? Unit test? Probably just mock logging directly there instead
of complicating the API (android_device.logging = mock)?

https://codereview.chromium.org/522553002/diff/180001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/android_device.py:35: m =
monsoon.Monsoon(wait=False)
This is a bit weird, maybe comment a little as to what this code block is doing?
What does monsoon got to do with getting attached device? Why is it only enabled
if we can not get attached device the first time around?

https://codereview.chromium.org/522553002/diff/180001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/android_device.py:45: 2. The Device is
connected to the Monsoon's main and USB channels.
nit: s/Device/device/

https://codereview.chromium.org/522553002/diff/180001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/android_platform_backend.py
(right):

https://codereview.chromium.org/522553002/diff/180001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:44:
self._adb.EnableAdbRoot()
What's the reason why we do this here instead of FromDevice? If you don't need
to do any related teardown logic (disabling adb root), it's probably better to
put this in FromDevice so that it's more easily unittest-able.

https://codereview.chromium.org/522553002/diff/180001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:73:
android_device.logging.error(
Why must this be using android_device.logging? What's the reasoning?

https://codereview.chromium.org/522553002/diff/180001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/platform_backend.py (right):

https://codereview.chromium.org/522553002/diff/180001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/platform_backend.py:55: @classmethod
doc? what does this method do?

[nednguyen, 2014-09-23 16:43:13]

https://codereview.chromium.org/522553002/diff/180001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/backends/chrome/android_browser_finder.py
(left):

https://codereview.chromium.org/522553002/diff/180001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/android_browser_finder.py:277:
return []
On 2014/09/22 16:16:41, chrishenry wrote:
> Where is this logic to return [] now?

This step now in android_platform_backend.FromDevice method. It will raise a
plaformError if prebuilt android tools could not be used. See: the try catch
block around creating android_platform.

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

https://codereview.chromium.org/522553002/diff/180001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/android_browser_finder.py:196:
devices = android_device.AndroidDevice.GetAllConnectedDevices()
On 2014/09/22 16:16:41, chrishenry wrote:
> Shouldn't this also be moved out from browser_finder?

Move to page_runner or where? I think putting this logic here is fine.

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

https://codereview.chromium.org/522553002/diff/180001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/android_device.py:1: # Copyright 2014
The Chromium Authors. All rights reserved.
On 2014/09/22 16:16:41, chrishenry wrote:
> Is this branched from anywhere? force copy detection if it is (to maintain
> history).

Thanks. This is useful to know about.

https://codereview.chromium.org/522553002/diff/180001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/android_device.py:14: Attributes:
On 2014/09/22 16:16:41, chrishenry wrote:
> Nit: 1 blank line before Attributes:

Done.

https://codereview.chromium.org/522553002/diff/180001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/android_device.py:18:
no_performance_mode: when this is set to True, android platform will be set
On 2014/09/22 16:16:41, chrishenry wrote:
> Eh.. The naming is a bit weird, why is no_performance_mode = True means the
> platform will be set to high perf mode?
> 
> I also dislike naming param with negative, can you just say
> enable_performance_mode?

Done.

https://codereview.chromium.org/522553002/diff/180001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/android_device.py:20: logging: where to
store the log results.
On 2014/09/22 16:16:41, chrishenry wrote:
> What is this for? Unit test? Probably just mock logging directly there instead
> of complicating the API (android_device.logging = mock)?

Done.

https://codereview.chromium.org/522553002/diff/180001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/android_device.py:35: m =
monsoon.Monsoon(wait=False)
On 2014/09/22 16:16:41, chrishenry wrote:
> This is a bit weird, maybe comment a little as to what this code block is
doing?
> What does monsoon got to do with getting attached device? Why is it only
enabled
> if we can not get attached device the first time around?

Added comment explain what is going on.

https://codereview.chromium.org/522553002/diff/180001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/android_device.py:45: 2. The Device is
connected to the Monsoon's main and USB channels.
On 2014/09/22 16:16:41, chrishenry wrote:
> nit: s/Device/device/

Done.

https://codereview.chromium.org/522553002/diff/180001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/android_platform_backend.py
(right):

https://codereview.chromium.org/522553002/diff/180001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:44:
self._adb.EnableAdbRoot()
On 2014/09/22 16:16:41, chrishenry wrote:
> What's the reason why we do this here instead of FromDevice? If you don't need
> to do any related teardown logic (disabling adb root), it's probably better to
> put this in FromDevice so that it's more easily unittest-able.

Hmh, backend is the abstraction around the way you control platform. So I
believe the logic of trying to root the device gain super user control of the
android device fits into this.

https://codereview.chromium.org/522553002/diff/180001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:73:
android_device.logging.error(
On 2014/09/22 16:16:41, chrishenry wrote:
> Why must this be using android_device.logging? What's the reasoning?

This is to prevent logging from spamming unittest. I change to use stub for this
purpose.

https://codereview.chromium.org/522553002/diff/180001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/platform_backend.py (right):

https://codereview.chromium.org/522553002/diff/180001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/platform_backend.py:55: @classmethod
On 2014/09/22 16:16:41, chrishenry wrote:
> doc? what does this method do?

Done.

[nednguyen, 2014-09-23 16:44:41]

Patchset #8 (id:220001) has been deleted

[chrishenry, 2014-09-23 17:46:32]

lgtm

https://codereview.chromium.org/522553002/diff/180001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/android_platform_backend.py
(right):

https://codereview.chromium.org/522553002/diff/180001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:44:
self._adb.EnableAdbRoot()
On 2014/09/23 16:43:13, nednguyen wrote:
> On 2014/09/22 16:16:41, chrishenry wrote:
> > What's the reason why we do this here instead of FromDevice? If you don't
need
> > to do any related teardown logic (disabling adb root), it's probably better
to
> > put this in FromDevice so that it's more easily unittest-able.
> 
> Hmh, backend is the abstraction around the way you control platform. So I
> believe the logic of trying to root the device gain super user control of the
> android device fits into this.

Well, FromDevice is part of backend too. I don't care that much, but thought
that this will make it harder to unittest since every unittest will need to
stub/mock this.

[nduca, 2014-09-23 18:50:10]

lgtm but please make the change noted below. i suggested this before, there's a
clean way to do it and a bad way. Right now, @FromDevice looks like you give it
a device. But if I just told you "implement FromDevice" with no context, would
it be obvious to you that you might be given a device that wasn't your platform?

By factoring it to supports and a standard constructor, its very clear what the
expectations on each function are.

https://codereview.chromium.org/522553002/diff/200001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/platform_backend.py (right):

https://codereview.chromium.org/522553002/diff/200001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/platform_backend.py:57: """ Returns an
instance of PlatformBackend from this device if possible.
I really think this should be a constructor and a @classmethod SupportsDevice().

[nduca, 2014-09-23 18:50:54]

(eg its ambiguous whether FromDevice should raise if its given an unsupported
thing, but you define it as returning none. Thats not obvious from the
signature.)

[nednguyen, 2014-09-23 21:30:36]

The CQ bit was checked by nednguyen@google.com

[nednguyen, 2014-09-23 21:30:40]

https://codereview.chromium.org/522553002/diff/200001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/platform_backend.py (right):

https://codereview.chromium.org/522553002/diff/200001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/platform_backend.py:57: """ Returns an
instance of PlatformBackend from this device if possible.
On 2014/09/23 18:50:10, nduca wrote:
> I really think this should be a constructor and a @classmethod
SupportsDevice().

Done.

[commit-bot: I haz the power, 2014-09-23 21:31:48]

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

[nednguyen, 2014-09-23 22:25:19]

The CQ bit was checked by nednguyen@google.com

[commit-bot: I haz the power, 2014-09-23 22:25:24]

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

[commit-bot: I haz the power, 2014-09-24 00:11:17]

Committed patchset #9 (id:260001) as 701560076257bafbb68ce2b147f44a8968fc8685

[commit-bot: I haz the power, 2014-09-24 00:12:00]

Patchset 9 (id:??) landed as
https://crrev.com/5289ba2d6a2c1eb3f429673ac0088f658d8aee5a
Cr-Commit-Position: refs/heads/master@{#296310}

[Lei Zhang, 2014-09-24 09:11:05]

http://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/15903
<-- bot turned red.

[achuithb, 2014-09-24 10:59:17]

On 2014/09/24 09:11:05, Lei Zhang wrote:
>
http://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/15903
> <-- bot turned red.

cros bot likewise:
http://chromegw.corp.google.com/i/chromiumos.chromium/builders/X86%20%28chrom...

[achuithb, 2014-09-24 11:00:38]

On 2014/09/24 10:59:17, achuithb wrote:
> On 2014/09/24 09:11:05, Lei Zhang wrote:
> >
>
http://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/15903
> > <-- bot turned red.
> 
> cros bot likewise:
>
http://chromegw.corp.google.com/i/chromiumos.chromium/builders/X86%20%28chrom...

I think you can remove the sherriff note from the CL description - the other
patch appears to be stable.

[achuithb, 2014-09-24 11:05:41]

https://codereview.chromium.org/522553002/diff/260001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/__init__.py (right):

https://codereview.chromium.org/522553002/diff/260001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/__init__.py:38: backend =
cros_platform_backend.CrosPlatformBackend()
This takes a CrOSDevice.

[achuithb, 2014-09-24 11:07:08]

Filed http://crbug.com/417224

[nednguyen, 2014-10-08 18:05:15]

A revert of this CL (patchset #9 id:260001) has been created in
https://codereview.chromium.org/640813002/ by nednguyen@google.com.

The reason for reverting is: Speculative revert due to regression.

BUG=419901.

[chromium-reviews, 2014-10-09 00:49:12]

As we discussed earlier, probably better not to land the revert (especially
given how many patches have gone in since then).


Chris~

On Wed, Oct 8, 2014 at 11:05 AM, <nednguyen@google.com> wrote:

> A revert of this CL (patchset #9 id:260001) has been created in
> https://codereview.chromium.org/640813002/ by nednguyen@google.com.
>
> The reason for reverting is: Speculative revert due to regression.
>
> BUG=419901.
>
> https://codereview.chromium.org/522553002/
>

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

[chromium-reviews, 2014-10-09 01:43:32]

Even better, the bug reporter thinks that the cause of 15% regression is
likely not due to this patch but because the devices in the lab have
changed: https://code.google.com/p/chromium/issues/detail?id=419901

On Wed, Oct 8, 2014 at 5:48 PM, Chris Henry <chrishenry@google.com> wrote:

> As we discussed earlier, probably better not to land the revert
> (especially given how many patches have gone in since then).
>
>
> Chris~
>
> On Wed, Oct 8, 2014 at 11:05 AM, <nednguyen@google.com> wrote:
>
>> A revert of this CL (patchset #9 id:260001) has been created in
>> https://codereview.chromium.org/640813002/ by nednguyen@google.com.
>>
>> The reason for reverting is: Speculative revert due to regression.
>>
>> BUG=419901.
>>
>> https://codereview.chromium.org/522553002/
>>
>
>

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