Put screenshot.py back to work

Put screenshot.py back to work

build/android/screenshot.py does not work anymore if you have only one
device attached and do not specify device id.

Committed: https://crrev.com/8f82087e90a9f7ef41250a82032bd367b3cc01cb
Cr-Commit-Position: refs/heads/master@{#308645}

[Ian Wen, 2014-12-16 02:09:43]

ianwen@chromium.org changed reviewers:
+ jbudorick@chromium.org

[jbudorick, 2014-12-16 02:18:52]

https://codereview.chromium.org/804963003/diff/1/build/android/screenshot.py
File build/android/screenshot.py (right):

https://codereview.chromium.org/804963003/diff/1/build/android/screenshot.py#...
build/android/screenshot.py:75: elif not options.device and
len(android_commands.GetAttachedDevices()) == 1:
Couple of problems in here:
 - We're no longer using android_commands in new code. adb_wrapper.GetDevices()
from b/a/pylib/device/adb_wrapper.py is what you're looking for.
 - No need to call that 3 times -- just call it once and store the result.

[jbudorick, 2014-12-16 02:20:34]

Also wanted to ask -- in what context are you using this that you noticed this
bug? Locally?

[Ian Wen, 2014-12-16 02:24:53]

On 2014/12/16 02:20:34, jbudorick wrote:
> Also wanted to ask -- in what context are you using this that you noticed this
> bug? Locally?

Sorry for not filing a bug before giving you this CL. Now screenshot.py does not
work anymore. When you attach an android device to gubuntu and do
src/build/android/screenshot.py DESTINATION, it will generate this error:
Traceback (most recent call last):
  File "/usr/local/google/code/clankium/src/build/android/screenshot.py", line
89, in <module>
    sys.exit(main())
  File "/usr/local/google/code/clankium/src/build/android/screenshot.py", line
79, in main
    device = device_utils.DeviceUtils(options.device)
  File
"/usr/local/google/code/clankium/src/build/android/pylib/device/device_utils.py",
line 105, in __init__
    raise ValueError('Unsupported device value: %r' % device)
ValueError: Unsupported device value: None

[jbudorick, 2014-12-16 02:35:41]

On 2014/12/16 02:24:53, Ian Wen wrote:
> On 2014/12/16 02:20:34, jbudorick wrote:
> > Also wanted to ask -- in what context are you using this that you noticed
this
> > bug? Locally?
> 
> Sorry for not filing a bug before giving you this CL. Now screenshot.py does
not
> work anymore. When you attach an android device to gubuntu and do
> src/build/android/screenshot.py DESTINATION, it will generate this error:
> Traceback (most recent call last):
>   File "/usr/local/google/code/clankium/src/build/android/screenshot.py", line
> 89, in <module>
>     sys.exit(main())
>   File "/usr/local/google/code/clankium/src/build/android/screenshot.py", line
> 79, in main
>     device = device_utils.DeviceUtils(options.device)
>   File
>
"/usr/local/google/code/clankium/src/build/android/pylib/device/device_utils.py",
> line 105, in __init__
>     raise ValueError('Unsupported device value: %r' % device)
> ValueError: Unsupported device value: None

Yeah, perezju@ and I removed support for passing None to that constructor. We
must have missed this script, as I don't think it's called by any of the bots.

[Ian Wen, 2014-12-16 05:16:29]

No it's not called by any bots. It's called by developers. I use this
script to take screenshots. :)

On Mon, Dec 15, 2014 at 6:35 PM, <jbudorick@chromium.org> wrote:
>
> On 2014/12/16 02:24:53, Ian Wen wrote:
>
>> On 2014/12/16 02:20:34, jbudorick wrote:
>> > Also wanted to ask -- in what context are you using this that you
>> noticed
>>
> this
>
>> > bug? Locally?
>>
>
>  Sorry for not filing a bug before giving you this CL. Now screenshot.py
>> does
>>
> not
>
>> work anymore. When you attach an android device to gubuntu and do
>> src/build/android/screenshot.py DESTINATION, it will generate this error:
>> Traceback (most recent call last):
>>    File "/usr/local/google/code/clankium/src/build/android/screenshot.py",
>> line
>> 89, in <module>
>>      sys.exit(main())
>>    File "/usr/local/google/code/clankium/src/build/android/screenshot.py",
>> line
>> 79, in main
>>      device = device_utils.DeviceUtils(options.device)
>>    File
>>
>
> "/usr/local/google/code/clankium/src/build/android/
> pylib/device/device_utils.py",
>
>> line 105, in __init__
>>      raise ValueError('Unsupported device value: %r' % device)
>> ValueError: Unsupported device value: None
>>
>
> Yeah, perezju@ and I removed support for passing None to that
> constructor. We
> must have missed this script, as I don't think it's called by any of the
> bots.
>
> https://codereview.chromium.org/804963003/
>

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

[jbudorick, 2014-12-16 05:48:12]

On 2014/12/16 05:16:29, Ian Wen wrote:
> No it's not called by any bots. It's called by developers. I use this
> script to take screenshots. :)

I figured; if it had broken a bot, I'd have heard about it a while ago :)

> 
> On Mon, Dec 15, 2014 at 6:35 PM, <mailto:jbudorick@chromium.org> wrote:
> >
> > On 2014/12/16 02:24:53, Ian Wen wrote:
> >
> >> On 2014/12/16 02:20:34, jbudorick wrote:
> >> > Also wanted to ask -- in what context are you using this that you
> >> noticed
> >>
> > this
> >
> >> > bug? Locally?
> >>
> >
> >  Sorry for not filing a bug before giving you this CL. Now screenshot.py
> >> does
> >>
> > not
> >
> >> work anymore. When you attach an android device to gubuntu and do
> >> src/build/android/screenshot.py DESTINATION, it will generate this error:
> >> Traceback (most recent call last):
> >>    File "/usr/local/google/code/clankium/src/build/android/screenshot.py",
> >> line
> >> 89, in <module>
> >>      sys.exit(main())
> >>    File "/usr/local/google/code/clankium/src/build/android/screenshot.py",
> >> line
> >> 79, in main
> >>      device = device_utils.DeviceUtils(options.device)
> >>    File
> >>
> >
> > "/usr/local/google/code/clankium/src/build/android/
> > pylib/device/device_utils.py",
> >
> >> line 105, in __init__
> >>      raise ValueError('Unsupported device value: %r' % device)
> >> ValueError: Unsupported device value: None
> >>
> >
> > Yeah, perezju@ and I removed support for passing None to that
> > constructor. We
> > must have missed this script, as I don't think it's called by any of the
> > bots.
> >
> > https://codereview.chromium.org/804963003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Ian Wen, 2014-12-16 18:11:53]

https://codereview.chromium.org/804963003/diff/1/build/android/screenshot.py
File build/android/screenshot.py (right):

https://codereview.chromium.org/804963003/diff/1/build/android/screenshot.py#...
build/android/screenshot.py:75: elif not options.device and
len(android_commands.GetAttachedDevices()) == 1:
On 2014/12/16 02:18:51, jbudorick wrote:
> Couple of problems in here:
>  - We're no longer using android_commands in new code.
adb_wrapper.GetDevices()
> from b/a/pylib/device/adb_wrapper.py is what you're looking for.
>  - No need to call that 3 times -- just call it once and store the result.

Did a small improvement by storing it locally. However it seems
adb_wrapper.GetDevices() is not a static method; it needs to initialize
AdbWrapper instance, which takes device number as arguments.

[jbudorick, 2014-12-16 18:14:11]

On 2014/12/16 18:11:53, Ian Wen wrote:
> https://codereview.chromium.org/804963003/diff/1/build/android/screenshot.py
> File build/android/screenshot.py (right):
> 
>
https://codereview.chromium.org/804963003/diff/1/build/android/screenshot.py#...
> build/android/screenshot.py:75: elif not options.device and
> len(android_commands.GetAttachedDevices()) == 1:
> On 2014/12/16 02:18:51, jbudorick wrote:
> > Couple of problems in here:
> >  - We're no longer using android_commands in new code.
> adb_wrapper.GetDevices()
> > from b/a/pylib/device/adb_wrapper.py is what you're looking for.
> >  - No need to call that 3 times -- just call it once and store the result.
> 
> Did a small improvement by storing it locally. However it seems
> adb_wrapper.GetDevices() is not a static method; it needs to initialize
> AdbWrapper instance, which takes device number as arguments.

Ah, sorry, my mistake. It's a classmethod and should be callable with
adb_wrapper.AdbWrapper.GetDevices(), but we can get that later.

[jbudorick, 2014-12-16 18:14:20]

lgtm w/ nit

https://codereview.chromium.org/804963003/diff/20001/build/android/screenshot.py
File build/android/screenshot.py (right):

https://codereview.chromium.org/804963003/diff/20001/build/android/screenshot...
build/android/screenshot.py:17: from pylib.device import adb_wrapper
nit: either switch to adb_wrapper.AdbWrapper.GetDevices() or remove this.

[Ian Wen, 2014-12-16 18:48:37]

https://codereview.chromium.org/804963003/diff/20001/build/android/screenshot.py
File build/android/screenshot.py (right):

https://codereview.chromium.org/804963003/diff/20001/build/android/screenshot...
build/android/screenshot.py:17: from pylib.device import adb_wrapper
On 2014/12/16 18:14:20, jbudorick wrote:
> nit: either switch to adb_wrapper.AdbWrapper.GetDevices() or remove this.

Done.

[Ian Wen, 2014-12-16 18:48:42]

The CQ bit was checked by ianwen@chromium.org

[commit-bot: I haz the power, 2014-12-16 18:49:33]

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

[commit-bot: I haz the power, 2014-12-16 19:54:25]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-12-16 19:55:49]

Patchset 3 (id:??) landed as
https://crrev.com/8f82087e90a9f7ef41250a82032bd367b3cc01cb
Cr-Commit-Position: refs/heads/master@{#308645}