[chromedriver] Reuse forwarded adb ports.

[chromedriver] Reuse forwarded adb ports.

adb forward --remove is not available in older adb binaries;
to avoid leaks, maintain a pool of previously forwarded ports.

BUG=None

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

[craigdh, 2014-01-08 20:56:33]

https://codereview.chromium.org/127143003/diff/1/chrome/test/chromedriver/chr...
File chrome/test/chromedriver/chrome/adb_impl.cc (left):

https://codereview.chromium.org/127143003/diff/1/chrome/test/chromedriver/chr...
chrome/test/chromedriver/chrome/adb_impl.cc:110: if (response == "OKAY")
I don't think we should remove these checks. Let's remove useful user
information for hypothetical issues that may not happen.

https://codereview.chromium.org/127143003/diff/1/chrome/test/chromedriver/chr...
File chrome/test/chromedriver/chrome_launcher.cc (right):

https://codereview.chromium.org/127143003/diff/1/chrome/test/chromedriver/chr...
chrome/test/chromedriver/chrome_launcher.cc:415: port_status =
port_manager_android->ReservePort(&port, &port_reservation);
then just call the port_manager->ReservePortFromPool here

https://codereview.chromium.org/127143003/diff/1/chrome/test/chromedriver/chr...
File chrome/test/chromedriver/chrome_launcher.h (right):

https://codereview.chromium.org/127143003/diff/1/chrome/test/chromedriver/chr...
chrome/test/chromedriver/chrome_launcher.h:38: PortManager*
port_manager_android,
Instead of two PortManager objects, I think it would be cleaner to just have the
PortManager expose functions for both pool-managed and unmanaged ports.

https://codereview.chromium.org/127143003/diff/1/chrome/test/chromedriver/net...
File chrome/test/chromedriver/net/port_server.cc (right):

https://codereview.chromium.org/127143003/diff/1/chrome/test/chromedriver/net...
chrome/test/chromedriver/net/port_server.cc:180:
base::Bind(&PortManager::ReleasePort, base::Unretained(this),
then just bind the correct version of ReleasePort here

[frankf, 2014-01-08 21:57:24]

ptal

https://codereview.chromium.org/127143003/diff/1/chrome/test/chromedriver/chr...
File chrome/test/chromedriver/chrome/adb_impl.cc (left):

https://codereview.chromium.org/127143003/diff/1/chrome/test/chromedriver/chr...
chrome/test/chromedriver/chrome/adb_impl.cc:110: if (response == "OKAY")
On 2014/01/08 20:56:33, craigdh wrote:
> I don't think we should remove these checks. Let's remove useful user
> information for hypothetical issues that may not happen.

In general, parsing the output for success is very fragile. If any of these
steps fail, you'll gonna get an exception at some point, and the users can look
at the server log. I'm gonna hold of on this for now though since it's
overloading this CL.

https://codereview.chromium.org/127143003/diff/1/chrome/test/chromedriver/chr...
File chrome/test/chromedriver/chrome_launcher.cc (right):

https://codereview.chromium.org/127143003/diff/1/chrome/test/chromedriver/chr...
chrome/test/chromedriver/chrome_launcher.cc:415: port_status =
port_manager_android->ReservePort(&port, &port_reservation);
On 2014/01/08 20:56:33, craigdh wrote:
> then just call the port_manager->ReservePortFromPool here

Done.

https://codereview.chromium.org/127143003/diff/1/chrome/test/chromedriver/net...
File chrome/test/chromedriver/net/port_server.cc (right):

https://codereview.chromium.org/127143003/diff/1/chrome/test/chromedriver/net...
chrome/test/chromedriver/net/port_server.cc:180:
base::Bind(&PortManager::ReleasePort, base::Unretained(this),
On 2014/01/08 20:56:33, craigdh wrote:
> then just bind the correct version of ReleasePort here

Done.

[craigdh, 2014-01-09 01:45:05]

lgtm w/ nits

https://codereview.chromium.org/127143003/diff/80001/chrome/test/chromedriver...
File chrome/test/chromedriver/net/port_server.cc (right):

https://codereview.chromium.org/127143003/diff/80001/chrome/test/chromedriver...
chrome/test/chromedriver/net/port_server.cc:178: port_to_use),
nit: shouldn't this line be tabbed two more spaces?

https://codereview.chromium.org/127143003/diff/80001/chrome/test/chromedriver...
chrome/test/chromedriver/net/port_server.cc:200: port_to_use),
ditto

https://codereview.chromium.org/127143003/diff/80001/chrome/test/chromedriver...
File chrome/test/chromedriver/net/port_server_unittest.cc (right):

https://codereview.chromium.org/127143003/diff/80001/chrome/test/chromedriver...
chrome/test/chromedriver/net/port_server_unittest.cc:207: {
google style guide has { on the same line as the for

[frankf, 2014-01-09 01:55:18]

https://codereview.chromium.org/127143003/diff/80001/chrome/test/chromedriver...
File chrome/test/chromedriver/net/port_server.cc (right):

https://codereview.chromium.org/127143003/diff/80001/chrome/test/chromedriver...
chrome/test/chromedriver/net/port_server.cc:178: port_to_use),
On 2014/01/09 01:45:06, craigdh wrote:
> nit: shouldn't this line be tabbed two more spaces?

Done.

https://codereview.chromium.org/127143003/diff/80001/chrome/test/chromedriver...
chrome/test/chromedriver/net/port_server.cc:200: port_to_use),
On 2014/01/09 01:45:06, craigdh wrote:
> ditto

Done.

https://codereview.chromium.org/127143003/diff/80001/chrome/test/chromedriver...
File chrome/test/chromedriver/net/port_server_unittest.cc (right):

https://codereview.chromium.org/127143003/diff/80001/chrome/test/chromedriver...
chrome/test/chromedriver/net/port_server_unittest.cc:207: {
On 2014/01/09 01:45:06, craigdh wrote:
> google style guide has { on the same line as the for

Done.

[commit-bot: I haz the power, 2014-01-09 01:57:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/frankf@chromium.org/127143003/130001

[commit-bot: I haz the power, 2014-01-09 10:26:16]

Change committed as 243830

[chrisgao (Use stgao instead), 2014-01-14 02:11:46]

lgtm with nits.

Sorry for the late review.

https://codereview.chromium.org/127143003/diff/130001/chrome/test/chromedrive...
File chrome/test/chromedriver/net/port_server.cc (right):

https://codereview.chromium.org/127143003/diff/130001/chrome/test/chromedrive...
chrome/test/chromedriver/net/port_server.cc:154: if (taken_.count(try_port))
Maybe it is good to add a comment in port_server.h saying |lock_| should be held
before calling PortManager::FindAvailablePort?

https://codereview.chromium.org/127143003/diff/130001/chrome/test/chromedrive...
chrome/test/chromedriver/net/port_server.cc:155: continue;
In a corner case, chromedriver is used to test on both desktop version and
android version.
For the desktop version, if |try_port| is in |unused_forwarded_port_|, we could
assume it is used by adb and skip it too?