adb_profile_chrome: Refactor into multiple modules and add tests

adb_profile_chrome: Refactor into multiple modules and add tests

The adb_profile_chrome tool has grown quite a bit since its inception,
so now seems to be a good time to split it into smaller modules and add
some rudimentary tests.

BUG=375754
TEST=build/android/chrome_profiler/run_tests
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272946

[Dominik Grewe, 2014-05-22 18:07:10]

Would it be better to split the controllers into separate files? If we ever
wanted to add more, that would keep it cleaner.
And would it make sense to have a common superclass for the controllers?

https://codereview.chromium.org/290013006/diff/1/build/android/chrome_profile...
File build/android/chrome_profiler/controllers_unittest.py (right):

https://codereview.chromium.org/290013006/diff/1/build/android/chrome_profile...
build/android/chrome_profiler/controllers_unittest.py:22:
self.assertEquals(len(devices), 1)
Is this always true on the bots?

https://codereview.chromium.org/290013006/diff/1/build/android/chrome_profile...
build/android/chrome_profiler/controllers_unittest.py:33: if _BROWSER ==
'stable':
Currently this will always be false, won't it? Are you planning to change this
later?

https://codereview.chromium.org/290013006/diff/1/build/android/chrome_profile...
File build/android/chrome_profiler/main.py (right):

https://codereview.chromium.org/290013006/diff/1/build/android/chrome_profile...
build/android/chrome_profiler/main.py:115: options, _args = parser.parse_args()
How about moving the setup of options in its own method? The main method is
quite long.

https://codereview.chromium.org/290013006/diff/1/build/android/chrome_profile...
build/android/chrome_profiler/main.py:130: parser.error('Exactly 1 device much
be attached.')
much -> must

https://codereview.chromium.org/290013006/diff/1/build/android/chrome_profile...
File build/android/chrome_profiler/profiler.py (right):

https://codereview.chromium.org/290013006/diff/1/build/android/chrome_profile...
build/android/chrome_profiler/profiler.py:104: interval: Time interval to
capture in seconds.
Add that passing 0 (or None) means it will run until stopped by the user.

[Sami, 2014-05-27 11:03:54]

On 2014/05/22 18:07:10, Dominik Grewe wrote:
> Would it be better to split the controllers into separate files? If we ever
> wanted to add more, that would keep it cleaner.
> And would it make sense to have a common superclass for the controllers?

Yeah, that single controller file is already looking pretty unwieldy with the
addition of perf. I've now split the controllers off into their own files and
added a common base class. PTAL.

https://codereview.chromium.org/290013006/diff/1/build/android/chrome_profile...
File build/android/chrome_profiler/controllers_unittest.py (right):

https://codereview.chromium.org/290013006/diff/1/build/android/chrome_profile...
build/android/chrome_profiler/controllers_unittest.py:22:
self.assertEquals(len(devices), 1)
On 2014/05/22 18:07:10, Dominik Grewe wrote:
> Is this always true on the bots?

You're right, it isn't. I think I'll just pick the first device arbitrarily for
the tests.

https://codereview.chromium.org/290013006/diff/1/build/android/chrome_profile...
build/android/chrome_profiler/controllers_unittest.py:33: if _BROWSER ==
'stable':
On 2014/05/22 18:07:10, Dominik Grewe wrote:
> Currently this will always be false, won't it? Are you planning to change this
> later?

That's right, this is really here only for manual testing for now. I'll add a
TODO for turning this test on once category queries hit stable.

https://codereview.chromium.org/290013006/diff/1/build/android/chrome_profile...
File build/android/chrome_profiler/main.py (right):

https://codereview.chromium.org/290013006/diff/1/build/android/chrome_profile...
build/android/chrome_profiler/main.py:115: options, _args = parser.parse_args()
On 2014/05/22 18:07:10, Dominik Grewe wrote:
> How about moving the setup of options in its own method? The main method is
> quite long.

Good idea, done. Eventually we might move the options to the individual
controllers but for now I think we should keep things simple.

https://codereview.chromium.org/290013006/diff/1/build/android/chrome_profile...
build/android/chrome_profiler/main.py:130: parser.error('Exactly 1 device much
be attached.')
On 2014/05/22 18:07:10, Dominik Grewe wrote:
> much -> must

Done.

https://codereview.chromium.org/290013006/diff/1/build/android/chrome_profile...
File build/android/chrome_profiler/profiler.py (right):

https://codereview.chromium.org/290013006/diff/1/build/android/chrome_profile...
build/android/chrome_profiler/profiler.py:104: interval: Time interval to
capture in seconds.
On 2014/05/22 18:07:10, Dominik Grewe wrote:
> Add that passing 0 (or None) means it will run until stopped by the user.

Done.

[Dominik Grewe, 2014-05-27 11:11:16]

On 2014/05/27 11:03:54, Sami wrote:
> On 2014/05/22 18:07:10, Dominik Grewe wrote:
> > Would it be better to split the controllers into separate files? If we ever
> > wanted to add more, that would keep it cleaner.
> > And would it make sense to have a common superclass for the controllers?
> 
> Yeah, that single controller file is already looking pretty unwieldy with the
> addition of perf. I've now split the controllers off into their own files and
> added a common base class. PTAL.
> 
>
https://codereview.chromium.org/290013006/diff/1/build/android/chrome_profile...
> File build/android/chrome_profiler/controllers_unittest.py (right):
> 
>
https://codereview.chromium.org/290013006/diff/1/build/android/chrome_profile...
> build/android/chrome_profiler/controllers_unittest.py:22:
> self.assertEquals(len(devices), 1)
> On 2014/05/22 18:07:10, Dominik Grewe wrote:
> > Is this always true on the bots?
> 
> You're right, it isn't. I think I'll just pick the first device arbitrarily
for
> the tests.
> 
>
https://codereview.chromium.org/290013006/diff/1/build/android/chrome_profile...
> build/android/chrome_profiler/controllers_unittest.py:33: if _BROWSER ==
> 'stable':
> On 2014/05/22 18:07:10, Dominik Grewe wrote:
> > Currently this will always be false, won't it? Are you planning to change
this
> > later?
> 
> That's right, this is really here only for manual testing for now. I'll add a
> TODO for turning this test on once category queries hit stable.
> 
>
https://codereview.chromium.org/290013006/diff/1/build/android/chrome_profile...
> File build/android/chrome_profiler/main.py (right):
> 
>
https://codereview.chromium.org/290013006/diff/1/build/android/chrome_profile...
> build/android/chrome_profiler/main.py:115: options, _args =
parser.parse_args()
> On 2014/05/22 18:07:10, Dominik Grewe wrote:
> > How about moving the setup of options in its own method? The main method is
> > quite long.
> 
> Good idea, done. Eventually we might move the options to the individual
> controllers but for now I think we should keep things simple.
> 
>
https://codereview.chromium.org/290013006/diff/1/build/android/chrome_profile...
> build/android/chrome_profiler/main.py:130: parser.error('Exactly 1 device much
> be attached.')
> On 2014/05/22 18:07:10, Dominik Grewe wrote:
> > much -> must
> 
> Done.
> 
>
https://codereview.chromium.org/290013006/diff/1/build/android/chrome_profile...
> File build/android/chrome_profiler/profiler.py (right):
> 
>
https://codereview.chromium.org/290013006/diff/1/build/android/chrome_profile...
> build/android/chrome_profiler/profiler.py:104: interval: Time interval to
> capture in seconds.
> On 2014/05/22 18:07:10, Dominik Grewe wrote:
> > Add that passing 0 (or None) means it will run until stopped by the user.
> 
> Done.

lgtm

[Sami, 2014-05-27 11:24:46]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-05-27 11:25:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/skyostil@chromium.org/290013006/20001

[commit-bot: I haz the power, 2014-05-27 11:26:29]

Change committed as 272946