Move remote platform creation logic from cros_browser_finders to platform.

* Move remote platform creation logic from cros_browser_finders to platform.
* Move CrOSInterface from backends into platform (CrOSPlatformBackend)
* Introduce Device/CrOSDevice.
* Replace IsRunningOnCrosDevice with GetHostPlatform().GetOSName()

BUG=413637
Committed: https://crrev.com/350d10fb359c461f2dce9abbc151b94a01094a41
Cr-Commit-Position: refs/heads/master@{#294585}

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

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

[tonyg, 2014-09-04 16:59:33]

https://codereview.chromium.org/541693004/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/platform/__init__.py (right):

https://codereview.chromium.org/541693004/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/__init__.py:17: def
IsRunningOnCrosDevice():
It doesn't seem like the word Cros belongs in this file. We've already got the
concept of host and target platforms. Maybe we should generalize this to a
method that indicates whether the host and target are the same or distinct. Then
the cros-specific implementation of that would go in the cros_platform_backend.
Android would always return True and desktop would always return False.

[nednguyen, 2014-09-04 17:13:12]

https://codereview.chromium.org/541693004/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/platform/__init__.py (right):

https://codereview.chromium.org/541693004/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/__init__.py:17: def
IsRunningOnCrosDevice():
On 2014/09/04 16:59:32, tonyg wrote:
> It doesn't seem like the word Cros belongs in this file. We've already got the
> concept of host and target platforms. Maybe we should generalize this to a
> method that indicates whether the host and target are the same or distinct.
Then
> the cros-specific implementation of that would go in the
cros_platform_backend.
> Android would always return True and desktop would always return False.

This method can be hidden just to be used only by _InitHostPlatformIfNeeded
function below or can be refactored to "IsRunningOnHostPlatform(platform_name)"
that takes 'linux', 'darwin',... as platform_name.

Removing the check "If platform is X" would require the call sites to always
construct the host platform even if they are only interested in target platform.
I worry that constructing the host platform may not be side effect free.

[nednguyen, 2014-09-05 17:52:58]

On 2014/09/04 17:13:12, nednguyen wrote:
>
https://codereview.chromium.org/541693004/diff/1/tools/telemetry/telemetry/co...
> File tools/telemetry/telemetry/core/platform/__init__.py (right):
> 
>
https://codereview.chromium.org/541693004/diff/1/tools/telemetry/telemetry/co...
> tools/telemetry/telemetry/core/platform/__init__.py:17: def
> IsRunningOnCrosDevice():
> On 2014/09/04 16:59:32, tonyg wrote:
> > It doesn't seem like the word Cros belongs in this file. We've already got
the
> > concept of host and target platforms. Maybe we should generalize this to a
> > method that indicates whether the host and target are the same or distinct.
> Then
> > the cros-specific implementation of that would go in the
> cros_platform_backend.
> > Android would always return True and desktop would always return False.
> 
> This method can be hidden just to be used only by _InitHostPlatformIfNeeded
> function below or can be refactored to
"IsRunningOnHostPlatform(platform_name)"
> that takes 'linux', 'darwin',... as platform_name.
> 
> Removing the check "If platform is X" would require the call sites to always
> construct the host platform even if they are only interested in target
platform.
> I worry that constructing the host platform may not be side effect free.

Turn out we don't have the use cases where we need to check whether host and
target are the same or distinct. 
I also replace the check c ros_interface.IsRunningOnCrosDevice() with
platform.GetHostPlatform().GetOSName() == 'chromeos'. 

This may cause some regression if creating a platform is expensive or have side
effect, but it shouldn't.

[nednguyen, 2014-09-08 21:17:19]

Patchset #3 (id:40001) has been deleted

[nednguyen, 2014-09-09 20:08:21]

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

[nednguyen, 2014-09-09 20:08:21]

PTAL again, since this is blocking my "telemetry as android app benchmarking"
effort.

[nednguyen, 2014-09-09 21:34:11]

Patchset #4 (id:80001) has been deleted

[nednguyen, 2014-09-09 21:57:15]

Patchset #4 (id:100001) has been deleted

[nednguyen, 2014-09-09 22:33:02]

Patchset #4 (id:120001) has been deleted

[achuithb, 2014-09-09 23:20:17]

achuith@chromium.org changed reviewers:
+ achuith@chromium.org

[achuithb, 2014-09-09 23:20:18]

I'll do a full pass tomorrow EMEA time. Sorry for the delay.

https://codereview.chromium.org/541693004/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/chrome/cros_browser_finder.py
(right):

https://codereview.chromium.org/541693004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/cros_browser_finder.py:124: #
pylint: disable=W0212
Let's just delete lines 124-127. This check is unnecessary

[nednguyen, 2014-09-10 22:47:10]

On 2014/09/09 23:20:18, achuithb wrote:
> I'll do a full pass tomorrow EMEA time. Sorry for the delay.
> 
>
https://codereview.chromium.org/541693004/diff/20001/tools/telemetry/telemetr...
> File tools/telemetry/telemetry/core/backends/chrome/cros_browser_finder.py
> (right):
> 
>
https://codereview.chromium.org/541693004/diff/20001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/core/backends/chrome/cros_browser_finder.py:124: #
> pylint: disable=W0212
> Let's just delete lines 124-127. This check is unnecessary
Done.

[tonyg, 2014-09-11 00:05:58]

lgtm

https://codereview.chromium.org/541693004/diff/160001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/profiler/perf_profiler.py (right):

https://codereview.chromium.org/541693004/diff/160001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/profiler/perf_profiler.py:202: if
platform.GetHostPlatform().GetOSName() == 'android':
android?

[nednguyen, 2014-09-11 03:16:07]

https://codereview.chromium.org/541693004/diff/160001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/profiler/perf_profiler.py (right):

https://codereview.chromium.org/541693004/diff/160001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/profiler/perf_profiler.py:202: if
platform.GetHostPlatform().GetOSName() == 'android':
On 2014/09/11 00:05:58, tonyg wrote:
> android?

Good catch! Done.

[achuithb, 2014-09-11 09:43:58]

Thanks, this looks good.

Let me know if you're still unable to run this patch on the cbuildbots - I'll
run it on your behalf after you've made the changes below.

Do you have a cros device for testing? It would be nice if you could run the
BrowserTests locally as a sanity check.

https://codereview.chromium.org/541693004/diff/160001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/profiler/perf_profiler.py (right):

https://codereview.chromium.org/541693004/diff/160001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/profiler/perf_profiler.py:202: if
platform.GetHostPlatform().GetOSName() == 'android':
On 2014/09/11 03:16:07, nednguyen wrote:
> On 2014/09/11 00:05:58, tonyg wrote:
> > android?
> 
> Good catch! Done.

I think you changed the wrong one - this one is chromeos.

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

https://codereview.chromium.org/541693004/diff/180001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/cros_browser_finder.py:59: if
platform_module.GetHostPlatform().GetOSName() == 'chromeos':
Maybe for this file we could have a private method like _IsRunningOnCrOS() since
this line appears many times and is a bit verbose

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

https://codereview.chromium.org/541693004/diff/180001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/__init__.py:46: elif
_IsRunningOnCrosDevice():
This should be the first if condition, otherwise we always create a
LinuxPlatformBackend. sys.platform.startswith('linux') is true on cros.

https://codereview.chromium.org/541693004/diff/180001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/__init__.py:68: cri =
cros_interface.CrOSInterface(
does it make sense to move this logic into CrOSDevice, like 
CrOSDevice.CreateCrOSInterface() (which also does the TryLogin(). ssh_identity
and perhaps host_name can remain private.

https://codereview.chromium.org/541693004/diff/180001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/profiler/perf_profiler.py (right):

https://codereview.chromium.org/541693004/diff/180001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/profiler/perf_profiler.py:77:
self._is_android = platform_backend.GetOSName() == 'chromeos'
android here

[achuithb, 2014-09-11 10:09:46]

Sorry about the late review!

Is there no bug that covers this? It's handy to have a bug to track reverts,
etc.

Also, if you could flesh out the description a bit more:
* Move CrOSInterface from backends into platform (CrOSPlatformBackend)
* Introduce Device/CrOSDevice.
* Replace IsRunningOnCrosDevice with GetHostPlatform().GetOSName()

[nednguyen, 2014-09-11 18:32:24]

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

https://codereview.chromium.org/541693004/diff/180001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/cros_browser_finder.py:59: if
platform_module.GetHostPlatform().GetOSName() == 'chromeos':
On 2014/09/11 09:43:58, achuithb wrote:
> Maybe for this file we could have a private method like _IsRunningOnCrOS()
since
> this line appears many times and is a bit verbose

Done.

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

https://codereview.chromium.org/541693004/diff/180001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/__init__.py:46: elif
_IsRunningOnCrosDevice():
On 2014/09/11 09:43:58, achuithb wrote:
> This should be the first if condition, otherwise we always create a
> LinuxPlatformBackend. sys.platform.startswith('linux') is true on cros.

Done.

https://codereview.chromium.org/541693004/diff/180001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/__init__.py:68: cri =
cros_interface.CrOSInterface(
On 2014/09/11 09:43:58, achuithb wrote:
> does it make sense to move this logic into CrOSDevice, like 
> CrOSDevice.CreateCrOSInterface() (which also does the TryLogin(). ssh_identity
> and perhaps host_name can remain private.

CrOsDevice is the public interface that we will give to users. So I try to hide
all of the low level logic from users and keep its API simple. This is more like
a language limitation. If this is Java, we would use s.t like package private
access.

https://codereview.chromium.org/541693004/diff/180001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/profiler/perf_profiler.py (right):

https://codereview.chromium.org/541693004/diff/180001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/profiler/perf_profiler.py:77:
self._is_android = platform_backend.GetOSName() == 'chromeos'
On 2014/09/11 09:43:58, achuithb wrote:
> android here

Done.

[nednguyen, 2014-09-12 04:13:52]

On 2014/09/11 18:32:24, nednguyen wrote:
>
https://codereview.chromium.org/541693004/diff/180001/tools/telemetry/telemet...
> File tools/telemetry/telemetry/core/backends/chrome/cros_browser_finder.py
> (right):
> 
>
https://codereview.chromium.org/541693004/diff/180001/tools/telemetry/telemet...
> tools/telemetry/telemetry/core/backends/chrome/cros_browser_finder.py:59: if
> platform_module.GetHostPlatform().GetOSName() == 'chromeos':
> On 2014/09/11 09:43:58, achuithb wrote:
> > Maybe for this file we could have a private method like _IsRunningOnCrOS()
> since
> > this line appears many times and is a bit verbose
> 
> Done.
> 
>
https://codereview.chromium.org/541693004/diff/180001/tools/telemetry/telemet...
> File tools/telemetry/telemetry/core/platform/__init__.py (right):
> 
>
https://codereview.chromium.org/541693004/diff/180001/tools/telemetry/telemet...
> tools/telemetry/telemetry/core/platform/__init__.py:46: elif
> _IsRunningOnCrosDevice():
> On 2014/09/11 09:43:58, achuithb wrote:
> > This should be the first if condition, otherwise we always create a
> > LinuxPlatformBackend. sys.platform.startswith('linux') is true on cros.
> 
> Done.
> 
>
https://codereview.chromium.org/541693004/diff/180001/tools/telemetry/telemet...
> tools/telemetry/telemetry/core/platform/__init__.py:68: cri =
> cros_interface.CrOSInterface(
> On 2014/09/11 09:43:58, achuithb wrote:
> > does it make sense to move this logic into CrOSDevice, like 
> > CrOSDevice.CreateCrOSInterface() (which also does the TryLogin().
ssh_identity
> > and perhaps host_name can remain private.
> 
> CrOsDevice is the public interface that we will give to users. So I try to
hide
> all of the low level logic from users and keep its API simple. This is more
like
> a language limitation. If this is Java, we would use s.t like package private
> access.
> 
>
https://codereview.chromium.org/541693004/diff/180001/tools/telemetry/telemet...
> File tools/telemetry/telemetry/core/platform/profiler/perf_profiler.py
(right):
> 
>
https://codereview.chromium.org/541693004/diff/180001/tools/telemetry/telemet...
> tools/telemetry/telemetry/core/platform/profiler/perf_profiler.py:77:
> self._is_android = platform_backend.GetOSName() == 'chromeos'
> On 2014/09/11 09:43:58, achuithb wrote:
> > android here
> 
> Done.

Hi Achuith, can you help me run this test on cros bot? I will add a bug to track
this platform cleaning up effort.

[achuithb, 2014-09-12 11:03:13]

Ned, I downloaded your patch and ran the cros unit tests remotely, and also one
of the telemetry based autotests (which runs telemetry on the dut). Both passed,
so I think this patch is safe to land.

[achuithb, 2014-09-12 11:05:59]

Please add the bug id (file a bug if necessary) to cover this feature, and
update the BUG= field.

LGTM

[nednguyen, 2014-09-12 13:15:59]

The CQ bit was checked by nednguyen@google.com

[commit-bot: I haz the power, 2014-09-12 13:16:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/541693004/200001

[commit-bot: I haz the power, 2014-09-12 14:21:30]

Committed patchset #7 (id:200001) as 2bd9ef212c6820009fb0dc85ce136be033f61c40

[commit-bot: I haz the power, 2014-09-12 14:25:39]

Patchset 7 (id:??) landed as
https://crrev.com/350d10fb359c461f2dce9abbc151b94a01094a41
Cr-Commit-Position: refs/heads/master@{#294585}