Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
24194002:
[android] Try killing adb server if no devices found
|
Created:
7 years, 3 months ago by wang16 Modified:
7 years, 2 months ago CC:
chromium-reviews, craigdh+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org, klundberg+watch_chromium.org, frankf+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Description[android] Try killing adb server if no devices found
It randomly happens that android devices are connected
but "adb devices" prints empty list.
Restarting adb server can fix it.
BUG=https://code.google.com/p/chromium/issues/detail?id=293149
Patch Set 1 #
Total comments: 1
Patch Set 2 : Set waiting interval to 10,20,40,80,160 #Messages
Total messages: 10 (0 generated)
The patch doesn't seem to apply, can you check it's on top of trunk? You should also add some reviewers to the CL, like Armand Navabi and Peter Beverloo.
On 2013/09/17 07:43:03, Raphael Kubo da Costa (rakuco) wrote: > The patch doesn't seem to apply, can you check it's on top of trunk? > > You should also add some reviewers to the CL, like Armand Navabi and Peter > Beverloo. It's on top of svn revision 223559, i am using git. It's the head of master at the moment.
For some reason, clicking "View" in the side-by-side diffs column shows an error.
lgtm with one comment to consider. https://codereview.chromium.org/24194002/diff/1/build/android/adb_install_apk.py File build/android/adb_install_apk.py (right): https://codereview.chromium.org/24194002/diff/1/build/android/adb_install_apk... build/android/adb_install_apk.py:71: Waiting 15, 30, 60, 120, 240 seconds seems high. I think we could get away with 5, 10, 20, 40, 80, but I don't have hard evidence, just what I recall from my experience. Maybe 10, 20, 40, 80, 160? If you think it's better to be safe, I'm fine with how it is. Just a thought.
On 2013/09/17 17:27:41, navabi wrote: > lgtm with one comment to consider. > > https://codereview.chromium.org/24194002/diff/1/build/android/adb_install_apk.py > File build/android/adb_install_apk.py (right): > > https://codereview.chromium.org/24194002/diff/1/build/android/adb_install_apk... > build/android/adb_install_apk.py:71: > Waiting 15, 30, 60, 120, 240 seconds seems high. I think we could get away with > 5, 10, 20, 40, 80, but I don't have hard evidence, just what I recall from my > experience. Maybe 10, 20, 40, 80, 160? > If you think it's better to be safe, I'm fine with how it is. Just a thought. Thanks for your comments. I agree the previous waiting is a little high, go with 10 based interval.
On 2013/09/18 00:57:55, shiliu.wang wrote: > On 2013/09/17 17:27:41, navabi wrote: > > lgtm with one comment to consider. > > > > > https://codereview.chromium.org/24194002/diff/1/build/android/adb_install_apk.py > > File build/android/adb_install_apk.py (right): > > > > > https://codereview.chromium.org/24194002/diff/1/build/android/adb_install_apk... > > build/android/adb_install_apk.py:71: > > Waiting 15, 30, 60, 120, 240 seconds seems high. I think we could get away > with > > 5, 10, 20, 40, 80, but I don't have hard evidence, just what I recall from my > > experience. Maybe 10, 20, 40, 80, 160? > > If you think it's better to be safe, I'm fine with how it is. Just a thought. > > Thanks for your comments. I agree the previous waiting is a little high, go with > 10 based interval. cool. still lgtm
On 2013/09/18 01:16:18, navabi wrote: > On 2013/09/18 00:57:55, shiliu.wang wrote: > > On 2013/09/17 17:27:41, navabi wrote: > > > lgtm with one comment to consider. > > > > > > > > > https://codereview.chromium.org/24194002/diff/1/build/android/adb_install_apk.py > > > File build/android/adb_install_apk.py (right): > > > > > > > > > https://codereview.chromium.org/24194002/diff/1/build/android/adb_install_apk... > > > build/android/adb_install_apk.py:71: > > > Waiting 15, 30, 60, 120, 240 seconds seems high. I think we could get away > > with > > > 5, 10, 20, 40, 80, but I don't have hard evidence, just what I recall from > my > > > experience. Maybe 10, 20, 40, 80, 160? > > > If you think it's better to be safe, I'm fine with how it is. Just a > thought. > > > > Thanks for your comments. I agree the previous waiting is a little high, go > with > > 10 based interval. > > cool. still lgtm (Just an FYI, this won't help if you're calling AndroidCommands.ManagedInstall() directly, which is the case for intalling test apks)
On 2013/09/18 01:26:06, frankf wrote: > On 2013/09/18 01:16:18, navabi wrote: > > On 2013/09/18 00:57:55, shiliu.wang wrote: > > > On 2013/09/17 17:27:41, navabi wrote: > > > > lgtm with one comment to consider. > > > > > > > > > > > > > > https://codereview.chromium.org/24194002/diff/1/build/android/adb_install_apk.py > > > > File build/android/adb_install_apk.py (right): > > > > > > > > > > > > > > https://codereview.chromium.org/24194002/diff/1/build/android/adb_install_apk... > > > > build/android/adb_install_apk.py:71: > > > > Waiting 15, 30, 60, 120, 240 seconds seems high. I think we could get away > > > with > > > > 5, 10, 20, 40, 80, but I don't have hard evidence, just what I recall from > > my > > > > experience. Maybe 10, 20, 40, 80, 160? > > > > If you think it's better to be safe, I'm fine with how it is. Just a > > thought. > > > > > > Thanks for your comments. I agree the previous waiting is a little high, go > > with > > > 10 based interval. > > > > cool. still lgtm > > (Just an FYI, this won't help if you're calling AndroidCommands.ManagedInstall() > directly, which is the case for intalling test apks) Are you suggesting to put the change inside android_commands.GetAttachedDevices()?
On 2013/09/18 01:56:20, shiliu.wang wrote: > On 2013/09/18 01:26:06, frankf wrote: > > On 2013/09/18 01:16:18, navabi wrote: > > > On 2013/09/18 00:57:55, shiliu.wang wrote: > > > > On 2013/09/17 17:27:41, navabi wrote: > > > > > lgtm with one comment to consider. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/24194002/diff/1/build/android/adb_install_apk.py > > > > > File build/android/adb_install_apk.py (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/24194002/diff/1/build/android/adb_install_apk... > > > > > build/android/adb_install_apk.py:71: > > > > > Waiting 15, 30, 60, 120, 240 seconds seems high. I think we could get > away > > > > with > > > > > 5, 10, 20, 40, 80, but I don't have hard evidence, just what I recall > from > > > my > > > > > experience. Maybe 10, 20, 40, 80, 160? > > > > > If you think it's better to be safe, I'm fine with how it is. Just a > > > thought. > > > > > > > > Thanks for your comments. I agree the previous waiting is a little high, > go > > > with > > > > 10 based interval. > > > > > > cool. still lgtm > > > > (Just an FYI, this won't help if you're calling > AndroidCommands.ManagedInstall() > > directly, which is the case for intalling test apks) > > Are you suggesting to put the change inside > android_commands.GetAttachedDevices()? I need more detail on what exactly the issue is. Perhaps, it makes more sense to discuss this first with navabi on that bug. It's strange that adb-server fails to show any device as online.
On 2013/09/18 02:17:48, frankf wrote: > I need more detail on what exactly the issue is. Perhaps, it makes more sense to > discuss this first with navabi on that bug. > It's strange that adb-server fails to show any device as online. Ping. Shiliu, can you answer frank or get this moving somehow? |
