Adding App, AndroidApp, AppBackend, AndroidAppBackend, PossibleApp.

Adding App, AndroidApp, AppBackend, AndroidAppBackend, PossibleApp.
Renaming variable name (browser_backend, browser) and function names
(under platform) where applicable.

BUG=417542
Committed: https://crrev.com/675a377e1e67de41b27d60ce4477d05efcb13675
Cr-Commit-Position: refs/heads/master@{#299357}

[wuhu, 2014-10-10 21:24:57]

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

[wuhu, 2014-10-10 21:24:58]

First pass at refactoring.  App option & finder etc will come in a another CL.

[nednguyen, 2014-10-10 21:46:37]

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

[nednguyen, 2014-10-10 21:46:38]

https://codereview.chromium.org/647193002/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/app.py (right):

https://codereview.chromium.org/647193002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/app.py:20: return self._app_backend
No, please don't expose the backend. The reason we have X & X_backend is that
X_backend contains the internal methods that we don't want to expose to users.

https://codereview.chromium.org/647193002/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/backends/android_app_backend.py (right):

https://codereview.chromium.org/647193002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/backends/android_app_backend.py:8: def
__init__(self, **more):
Please avoid this '**more' pattern.

https://codereview.chromium.org/647193002/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/backends/app_backend.py (right):

https://codereview.chromium.org/647193002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/backends/app_backend.py:6: def __init__(self,
**more):
Ditto.

https://codereview.chromium.org/647193002/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/backends/browser_backend.py (right):

https://codereview.chromium.org/647193002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/backends/browser_backend.py:22: **more):
ditto

https://codereview.chromium.org/647193002/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/browser.py (right):

https://codereview.chromium.org/647193002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/browser.py:36: 
assert isinstance(app_backend, browser_backend.BrowserBackend) ?

[nednguyen, 2014-10-10 21:57:48]

https://codereview.chromium.org/647193002/diff/1/tools/telemetry/telemetry/co...
File
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py
(right):

https://codereview.chromium.org/647193002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:40:
package = self._app._app_backend.package
I don't see @property  def package in defined in the abstract app_backend. So I
don't think this renaming is valid.

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

https://codereview.chromium.org/647193002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/profiler/__init__.py:5: import
collections
Everything under profiler/ still only apply for browser, I think.

https://codereview.chromium.org/647193002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/profiler/__init__.py:56: process_name =
self._app_backend.GetProcessName(cmd_line)
Example: I don't see GetProcessName() defined in _app_backend

[wuhu, 2014-10-10 22:37:56]

Thanks Ned, please see some comments below.

https://codereview.chromium.org/647193002/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/backends/android_app_backend.py (right):

https://codereview.chromium.org/647193002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/backends/android_app_backend.py:8: def
__init__(self, **more):
On 2014/10/10 21:46:38, nednguyen wrote:
> Please avoid this '**more' pattern.

Are there other ways to handle multiple inheritance where argument lists differ
in the parent classes?

https://codereview.chromium.org/647193002/diff/1/tools/telemetry/telemetry/co...
File
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py
(right):

https://codereview.chromium.org/647193002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:40:
package = self._app._app_backend.package
On 2014/10/10 21:57:48, nednguyen wrote:
> I don't see @property  def package in defined in the abstract app_backend. So
I
> don't think this renaming is valid.

Should we rename App._app_backend simply to App_backend?  This will avoid
mislead user to believe _app_backend is an instance of AppBackend (the root
class).  And we can add "assert isinstance(..)" to make it clear that we are
expecting a particular subclass (AndroidBrowserBackend in this case).

Or we can add @property browser_backend() in Browser class.

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

https://codereview.chromium.org/647193002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/profiler/__init__.py:5: import
collections
On 2014/10/10 21:57:48, nednguyen wrote:
> Everything under profiler/ still only apply for browser, I think. 

Some of them are generic to android apps (eg. TCPDumpProfiler, JavaHeapProfiler
etc.)

[nednguyen, 2014-10-10 23:03:15]

This change is too big to review. Can you do these refactoring first:
1) Speculate which logics would be shared between android_browser_backend and
android_app_backend, move those logic to android_platform
2) Figure out the places where browser and browser_backend are not really needed
(I suspect that there are many). Remove browser/browser_backend and replace with
s.t that you believe can apply for a general app (may be platform, or maybe
app.name or app.pid)

https://codereview.chromium.org/647193002/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/backends/android_app_backend.py (right):

https://codereview.chromium.org/647193002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/backends/android_app_backend.py:8: def
__init__(self, **more):
On 2014/10/10 22:37:55, wuhu wrote:
> On 2014/10/10 21:46:38, nednguyen wrote:
> > Please avoid this '**more' pattern.
> 
> Are there other ways to handle multiple inheritance where argument lists
differ
> in the parent classes?
Python will only call one 1 parent super class's __init__.

Now I think about it, we may not need multiple inheritance. Shared logic between
android_browser_backend & android_app_backend should be moved to
android_platform.

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

https://codereview.chromium.org/647193002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/profiler/__init__.py:5: import
collections
On 2014/10/10 22:37:56, wuhu wrote:
> On 2014/10/10 21:57:48, nednguyen wrote:
> > Everything under profiler/ still only apply for browser, I think. 
> 
> Some of them are generic to android apps (eg. TCPDumpProfiler,
JavaHeapProfiler
> etc.)

If that's the case, you will need method like:
Profiler.SupportAppBackend(app_backend) so you can check whether some profiler
supports an app_backend instance. I still believe that most of these profiler
stuffs are more about controlling platform rather than app, so there should be
way to remove app_backend from Profiler.__init__.

[nednguyen, 2014-10-10 23:14:43]

On 2014/10/10 23:03:15, nednguyen wrote:
> This change is too big to review. Can you do these refactoring first:
> 1) Speculate which logics would be shared between android_browser_backend and
> android_app_backend, move those logic to android_platform
> 2) Figure out the places where browser and browser_backend are not really
needed
> (I suspect that there are many). Remove browser/browser_backend and replace
with
> s.t that you believe can apply for a general app (may be platform, or maybe
> app.name or app.pid)

Sorry, my bad. You don't have to do 1) & 2) before introducing app, app_backend,
... APIs. Another approach could be just introduce these APIs first, then figure
all the place that can take the generic app API instead of browser & replace
them gradually. For places that can't take generic app API, we can either make
that a runtime error or fallback safely with a warning message.

Do you want to undo most of your renaming changes and keep App, AndroidApp,
AppBackend, AndroidAppBackend, PossibleApp in this patch ?

> 
>
https://codereview.chromium.org/647193002/diff/1/tools/telemetry/telemetry/co...
> File tools/telemetry/telemetry/core/backends/android_app_backend.py (right):
> 
>
https://codereview.chromium.org/647193002/diff/1/tools/telemetry/telemetry/co...
> tools/telemetry/telemetry/core/backends/android_app_backend.py:8: def
> __init__(self, **more):
> On 2014/10/10 22:37:55, wuhu wrote:
> > On 2014/10/10 21:46:38, nednguyen wrote:
> > > Please avoid this '**more' pattern.
> > 
> > Are there other ways to handle multiple inheritance where argument lists
> differ
> > in the parent classes?
> Python will only call one 1 parent super class's __init__.
> 
> Now I think about it, we may not need multiple inheritance. Shared logic
between
> android_browser_backend & android_app_backend should be moved to
> android_platform.
> 
>
https://codereview.chromium.org/647193002/diff/1/tools/telemetry/telemetry/co...
> File tools/telemetry/telemetry/core/platform/profiler/__init__.py (right):
> 
>
https://codereview.chromium.org/647193002/diff/1/tools/telemetry/telemetry/co...
> tools/telemetry/telemetry/core/platform/profiler/__init__.py:5: import
> collections
> On 2014/10/10 22:37:56, wuhu wrote:
> > On 2014/10/10 21:57:48, nednguyen wrote:
> > > Everything under profiler/ still only apply for browser, I think. 
> > 
> > Some of them are generic to android apps (eg. TCPDumpProfiler,
> JavaHeapProfiler
> > etc.)
> 
> If that's the case, you will need method like:
> Profiler.SupportAppBackend(app_backend) so you can check whether some profiler
> supports an app_backend instance. I still believe that most of these profiler
> stuffs are more about controlling platform rather than app, so there should be
> way to remove app_backend from Profiler.__init__.

[chromium-reviews, 2014-10-10 23:17:17]

sounds good, will do.

On Fri, Oct 10, 2014 at 4:14 PM, <nednguyen@google.com> wrote:

> On 2014/10/10 23:03:15, nednguyen wrote:
>
>> This change is too big to review. Can you do these refactoring first:
>> 1) Speculate which logics would be shared between android_browser_backend
>> and
>> android_app_backend, move those logic to android_platform
>> 2) Figure out the places where browser and browser_backend are not really
>>
> needed
>
>> (I suspect that there are many). Remove browser/browser_backend and
>> replace
>>
> with
>
>> s.t that you believe can apply for a general app (may be platform, or
>> maybe
>> app.name or app.pid)
>>
>
> Sorry, my bad. You don't have to do 1) & 2) before introducing app,
> app_backend,
> ... APIs. Another approach could be just introduce these APIs first, then
> figure
> all the place that can take the generic app API instead of browser &
> replace
> them gradually. For places that can't take generic app API, we can either
> make
> that a runtime error or fallback safely with a warning message.
>
> Do you want to undo most of your renaming changes and keep App, AndroidApp,
> AppBackend, AndroidAppBackend, PossibleApp in this patch ?
>
>
>
>
> https://codereview.chromium.org/647193002/diff/1/tools/
> telemetry/telemetry/core/backends/android_app_backend.py
>
>> File tools/telemetry/telemetry/core/backends/android_app_backend.py
>> (right):
>>
>
>
> https://codereview.chromium.org/647193002/diff/1/tools/
> telemetry/telemetry/core/backends/android_app_backend.py#newcode8
>
>> tools/telemetry/telemetry/core/backends/android_app_backend.py:8: def
>> __init__(self, **more):
>> On 2014/10/10 22:37:55, wuhu wrote:
>> > On 2014/10/10 21:46:38, nednguyen wrote:
>> > > Please avoid this '**more' pattern.
>> >
>> > Are there other ways to handle multiple inheritance where argument lists
>> differ
>> > in the parent classes?
>> Python will only call one 1 parent super class's __init__.
>>
>
>  Now I think about it, we may not need multiple inheritance. Shared logic
>>
> between
>
>> android_browser_backend & android_app_backend should be moved to
>> android_platform.
>>
>
>
> https://codereview.chromium.org/647193002/diff/1/tools/
> telemetry/telemetry/core/platform/profiler/__init__.py
>
>> File tools/telemetry/telemetry/core/platform/profiler/__init__.py
>> (right):
>>
>
>
> https://codereview.chromium.org/647193002/diff/1/tools/
> telemetry/telemetry/core/platform/profiler/__init__.py#newcode5
>
>> tools/telemetry/telemetry/core/platform/profiler/__init__.py:5: import
>> collections
>> On 2014/10/10 22:37:56, wuhu wrote:
>> > On 2014/10/10 21:57:48, nednguyen wrote:
>> > > Everything under profiler/ still only apply for browser, I think.
>> >
>> > Some of them are generic to android apps (eg. TCPDumpProfiler,
>> JavaHeapProfiler
>> > etc.)
>>
>
>  If that's the case, you will need method like:
>> Profiler.SupportAppBackend(app_backend) so you can check whether some
>> profiler
>> supports an app_backend instance. I still believe that most of these
>> profiler
>> stuffs are more about controlling platform rather than app, so there
>> should be
>> way to remove app_backend from Profiler.__init__.
>>
>
> https://codereview.chromium.org/647193002/
>

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

[wuhu, 2014-10-10 23:47:18]

Keeping Browser._browser_backend, BrowserBackend.browser for now to avoid large
scale renaming.

[nednguyen, 2014-10-13 16:06:20]

Thanks, this is a lot easier to manage.

https://codereview.chromium.org/647193002/diff/970001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/android_app.py (right):

https://codereview.chromium.org/647193002/diff/970001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/android_app.py:8: """ An Android app that can be
controlled.
This doc also needs improvement.

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

https://codereview.chromium.org/647193002/diff/970001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/app.py:6: """ An application that can be
controlled.
"A running application instance that can be controlled in a limited way.

To create a application instance, use possible_app.Create()..."


Can you copy some of the doc in browser.py? There are three important points
that we should address:
1) This app class represents a running instance of an application. i.e: there is
no app.Start(). When this object is created, the app must be started already.
2) Once who wants to create this should use a corresponding possible_app with
possible_app.Create() method. 
3) To make sure that clean up happens automatically, one should use the pattern:
with possible_app.Create() as app: 
   ...do all the operations with the app...

https://codereview.chromium.org/647193002/diff/970001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/app.py:19: def backend(self):
Please remove this. The reason we have app_backend in the first place is to hide
the implementation details of the backend from the users.

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

https://codereview.chromium.org/647193002/diff/970001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/android_app_backend.py:6: 
nits: 2 blank lines.

https://codereview.chromium.org/647193002/diff/970001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/android_app_backend.py:8: def
__init__(self, **more):
Please remove **more.

https://codereview.chromium.org/647193002/diff/970001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/backends/app_backend.py (right):

https://codereview.chromium.org/647193002/diff/970001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/app_backend.py:4: 
ditto

https://codereview.chromium.org/647193002/diff/970001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/app_backend.py:6: def __init__(self,
**more):
ditto

https://codereview.chromium.org/647193002/diff/970001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/app_backend.py:10: def SetApp(self,
app):
This is probably some architectural bug that app_backend (browser_backend) needs
to have handle to app (browser). But we can fix it in a different patch.

https://codereview.chromium.org/647193002/diff/970001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/possible_app.py (right):

https://codereview.chromium.org/647193002/diff/970001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/possible_app.py:7: """An app that can be
controlled.
A factory class that can be used to create a running instance of app.

https://codereview.chromium.org/647193002/diff/970001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/possible_app.py:23: def browser_type(self):
browser_type?

[wuhu, 2014-10-13 17:43:16]

https://codereview.chromium.org/647193002/diff/970001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/possible_app.py (right):

https://codereview.chromium.org/647193002/diff/970001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/possible_app.py:23: def browser_type(self):
On 2014/10/13 16:06:20, nednguyen wrote:
> browser_type?

I was going to do the renaming in another patch.  I've added an adapter method
for now in possible_browser

[nednguyen, 2014-10-13 17:55:10]

LGTM

https://codereview.chromium.org/647193002/diff/1120001/tools/telemetry/teleme...
File tools/telemetry/telemetry/core/android_app.py (right):

https://codereview.chromium.org/647193002/diff/1120001/tools/telemetry/teleme...
tools/telemetry/telemetry/core/android_app.py:6: 
2 blank lines.

https://codereview.chromium.org/647193002/diff/1120001/tools/telemetry/teleme...
tools/telemetry/telemetry/core/android_app.py:10: To create an android app
instance, use android_app_finder.FindAndroidApp.
maybe remove this. I am not sure whether we will have the android_app_finder yet
:-)

https://codereview.chromium.org/647193002/diff/1120001/tools/telemetry/teleme...
tools/telemetry/telemetry/core/android_app.py:14: possible_android_app =
FindAndroidApp(options)
Remove this line.

https://codereview.chromium.org/647193002/diff/1120001/tools/telemetry/teleme...
File tools/telemetry/telemetry/core/app.py (right):

https://codereview.chromium.org/647193002/diff/1120001/tools/telemetry/teleme...
tools/telemetry/telemetry/core/app.py:9: of possible_app.
Remve this also.

https://codereview.chromium.org/647193002/diff/1120001/tools/telemetry/teleme...
tools/telemetry/telemetry/core/app.py:13: possible_app = FindApp(options)
Remove this also.

[wuhu, 2014-10-13 19:15:06]

The CQ bit was checked by wuhu@google.com

[commit-bot: I haz the power, 2014-10-13 19:17:23]

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

[commit-bot: I haz the power, 2014-10-13 20:37:07]

Committed patchset #5 (id:1190001)

[commit-bot: I haz the power, 2014-10-13 20:37:44]

Patchset 5 (id:??) landed as
https://crrev.com/675a377e1e67de41b27d60ce4477d05efcb13675
Cr-Commit-Position: refs/heads/master@{#299357}

[nednguyen, 2014-10-13 20:43:09]

On 2014/10/13 20:37:44, I haz the power (commit-bot) wrote:
> Patchset 5 (id:??) landed as
> https://crrev.com/675a377e1e67de41b27d60ce4477d05efcb13675
> Cr-Commit-Position: refs/heads/master@{#299357}

Eww, we forgot to specify the bug number of this patch with #417542. :-(