DevTools: Extract ADB specific requests from DevToolsAndroidBridge into a separate class.

DevTools: Extract ADB specific requests from DevToolsAndroidBridge into a separate class.

BUG=None

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

[Vladislav Kaznacheev, 2014-05-08 15:21:55]

PTAL

[Vladislav Kaznacheev, 2014-05-12 09:26:53]

On 2014/05/08 15:21:55, Vladislav Kaznacheev wrote:
> PTAL

PTAL indeed.

[pfeldman, 2014-05-13 12:44:57]

You should split the change in behavior and the code extraction first.

https://codereview.chromium.org/274573002/diff/40001/chrome/browser/devtools/...
File chrome/browser/devtools/device/adb/adb_device_provider.cc (right):

https://codereview.chromium.org/274573002/diff/40001/chrome/browser/devtools/...
chrome/browser/devtools/device/adb/adb_device_provider.cc:39: new
AdbDeviceInfoQuery(
Seems like you could inline this.

https://codereview.chromium.org/274573002/diff/40001/chrome/browser/devtools/...
File chrome/browser/devtools/device/android_device_manager.h (right):

https://codereview.chromium.org/274573002/diff/40001/chrome/browser/devtools/...
chrome/browser/devtools/device/android_device_manager.h:36: std::string socket;
You call it socket_name below.

https://codereview.chromium.org/274573002/diff/40001/chrome/browser/devtools/...
File chrome/browser/devtools/device/devtools_android_bridge.cc (right):

https://codereview.chromium.org/274573002/diff/40001/chrome/browser/devtools/...
chrome/browser/devtools/device/devtools_android_bridge.cc:208:
AdbDeviceInfoQuery::FindDisplayNameByPackage(package);
This is confusing. Why do you need to convert package to display name here?

https://codereview.chromium.org/274573002/diff/40001/chrome/browser/devtools/...
File chrome/browser/devtools/device/devtools_android_bridge.h (right):

https://codereview.chromium.org/274573002/diff/40001/chrome/browser/devtools/...
chrome/browser/devtools/device/devtools_android_bridge.h:205: const std::string
serial_;
We don't use const consistently in this code, why?

[Vladislav Kaznacheev, 2014-05-15 13:56:34]

PTAL

https://chromiumcodereview.appspot.com/274573002/diff/40001/chrome/browser/de...
File chrome/browser/devtools/device/adb/adb_device_provider.cc (right):

https://chromiumcodereview.appspot.com/274573002/diff/40001/chrome/browser/de...
chrome/browser/devtools/device/adb/adb_device_provider.cc:39: new
AdbDeviceInfoQuery(
Sorry, inline what?

On 2014/05/13 12:44:58, pfeldman wrote:
> Seems like you could inline this.

https://chromiumcodereview.appspot.com/274573002/diff/40001/chrome/browser/de...
File chrome/browser/devtools/device/android_device_manager.h (right):

https://chromiumcodereview.appspot.com/274573002/diff/40001/chrome/browser/de...
chrome/browser/devtools/device/android_device_manager.h:36: std::string socket;
On 2014/05/13 12:44:58, pfeldman wrote:
> You call it socket_name below.

Done.

https://chromiumcodereview.appspot.com/274573002/diff/40001/chrome/browser/de...
File chrome/browser/devtools/device/devtools_android_bridge.cc (right):

https://chromiumcodereview.appspot.com/274573002/diff/40001/chrome/browser/de...
chrome/browser/devtools/device/devtools_android_bridge.cc:208:
AdbDeviceInfoQuery::FindDisplayNameByPackage(package);
Newer versions of Chrome on Android report their package name in /json/version
response. If we use this we can remove 'pm list packages' logic completely.

On 2014/05/13 12:44:58, pfeldman wrote:
> This is confusing. Why do you need to convert package to display name here?

https://chromiumcodereview.appspot.com/274573002/diff/40001/chrome/browser/de...
File chrome/browser/devtools/device/devtools_android_bridge.h (right):

https://chromiumcodereview.appspot.com/274573002/diff/40001/chrome/browser/de...
chrome/browser/devtools/device/devtools_android_bridge.h:205: const std::string
serial_;
Removed.

On 2014/05/13 12:44:58, pfeldman wrote:
> We don't use const consistently in this code, why?

[pfeldman, 2014-05-16 14:00:55]

lgtm

https://codereview.chromium.org/274573002/diff/60001/chrome/browser/devtools/...
File chrome/browser/devtools/device/adb/adb_device_info_query.h (right):

https://codereview.chromium.org/274573002/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/device/adb/adb_device_info_query.h:11: class
AdbDeviceInfoQuery : public base::RefCounted<AdbDeviceInfoQuery>,
You don't need it to be ref-counted.

[Vladislav Kaznacheev, 2014-05-16 15:53:48]

https://codereview.chromium.org/274573002/diff/60001/chrome/browser/devtools/...
File chrome/browser/devtools/device/adb/adb_device_info_query.h (right):

https://codereview.chromium.org/274573002/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/device/adb/adb_device_info_query.h:11: class
AdbDeviceInfoQuery : public base::RefCounted<AdbDeviceInfoQuery>,
On 2014/05/16 14:00:56, pfeldman wrote:
> You don't need it to be ref-counted.

Done.

[Vladislav Kaznacheev, 2014-05-16 15:53:52]

The CQ bit was checked by kaznacheev@chromium.org

[commit-bot: I haz the power, 2014-05-16 15:54:09]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kaznacheev@chromium.org/274573002/70001

[commit-bot: I haz the power, 2014-05-16 18:57:48]

Change committed as 271071