Allow hostnames in socket.connect().

Do hostname resolution. Interesting questions to ponder:

  - Do we still need the DNS API? Originally this was broken out separately to make it match the low-level network APIs. But (a) it felt like needless busywork for the developer who just wanted to connect to a host, and (b) it's necessary to pass a hostname to connect() to provide a match with host permissions in the manifest.

  - We do NOT do the same work for socket.bind(). It doesn't quite make sense to bind to a hostname (unless the hostname is "localhost"). Using a hostname is certainly possible, but requiring an IP address for now is probably reasonable.

BUG=110520
TEST=They still work when enhanced to use "localhost" appropriately.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=141101

[Mihai Parparita -not on Chrome, 2012-06-07 00:29:44]

http://codereview.chromium.org/10537017/diff/1015/chrome/browser/extensions/a...
File chrome/browser/extensions/api/socket/socket_api.cc (right):

http://codereview.chromium.org/10537017/diff/1015/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/socket_api.cc:147: Release();  // Added in
StartDnsLookup().
Do you need to the AddRef/Release in this method? As long as you're guaranteed
that you always start in StartDnsLookup and end up in OnConnect (even for
errors), then you can just have an AddRef in the former and a Release in the
latter, with no ref count changes in between.

http://codereview.chromium.org/10537017/diff/1015/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/socket_api.cc:154: OnConnect(-1);
If you do keep the current ref counting scheme, this also needs an AddRef call
to balance the Release in OnConnect.

http://codereview.chromium.org/10537017/diff/1015/chrome/common/extensions/ap...
File chrome/common/extensions/api/experimental_socket.idl (right):

http://codereview.chromium.org/10537017/diff/1015/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental_socket.idl:92: // |address| : The
address of the remote machine.
I think part of what confused me about bind() is that its docs talk about the
remote machine/port. Presumably it should say local interface address and port?

[Mihai Parparita -not on Chrome, 2012-06-07 00:32:47]

On Wed, Jun 6, 2012 at 2:10 PM, <miket@chromium.org> wrote:

>  - Do we still need the DNS API? Originally this was broken out separately
> to
> make it match the low-level network APIs. But (a) it felt like needless
> busywork
> for the developer who just wanted to connect to a host, and (b) it's
> necessary
> to pass a hostname to connect() to provide a match with host permissions
> in the
> manifest.
>

We should remove the DNS API for now.

 - We do NOT do the same work for socket.bind(). It doesn't quite make
> sense to
> bind to a hostname (unless the hostname is "localhost"). Using a hostname
> is
> certainly possible, but requiring an IP address for now is probably
> reasonable.
>

Beyond the scope of this CL, but as discussed in person, bind() could
special-case the "any interface's IP address" mode, so that the average
developer doesn't have to worry about what to pass in. This could be as
simple as providing a INADDR_ANY-like constant.

Mihai

[miket_OOO, 2012-06-07 15:48:11]

http://codereview.chromium.org/10537017/diff/1015/chrome/common/extensions/ap...
> chrome/common/extensions/api/experimental_socket.idl:92: // |address| : The
> address of the remote machine.
> I think part of what confused me about bind() is that its docs talk about the
> remote machine/port. Presumably it should say local interface address and
port?

Great minds get confused alike. See
https://chromiumcodereview.appspot.com/10541031/

[miket_OOO, 2012-06-07 16:02:29]

> We should remove the DNS API for now.

OK (see announcement I just sent out). Do you mind if I do that in a follow-on
CL that's considered part of this crbug? I think it's a sensible division, and
I'd like to do some refactoring once /dns is gone.

[Mihai Parparita -not on Chrome, 2012-06-07 17:30:17]

On Thu, Jun 7, 2012 at 9:02 AM, <miket@chromium.org> wrote:

> OK (see announcement I just sent out). Do you mind if I do that in a
> follow-on
> CL that's considered part of this crbug? I think it's a sensible division,
> and
> I'd like to do some refactoring once /dns is gone.
>

SGTM

Mihai

[miket_OOO, 2012-06-07 17:36:12]

Changes uploaded. Peng's right about sendTo(), so I'll have to add support for
that before removing DNS. So I'd like to do two follow-on CLs.

http://codereview.chromium.org/10537017/diff/1015/chrome/browser/extensions/a...
File chrome/browser/extensions/api/socket/socket_api.cc (right):

http://codereview.chromium.org/10537017/diff/1015/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/socket_api.cc:147: Release();  // Added in
StartDnsLookup().
On 2012/06/07 00:29:44, Mihai Parparita wrote:
> Do you need to the AddRef/Release in this method?

You're right. I simplified the code path in an earlier revision, but didn't
simplify the refcounting.

http://codereview.chromium.org/10537017/diff/1015/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/socket_api.cc:154: OnConnect(-1);
On 2012/06/07 00:29:44, Mihai Parparita wrote:
> If you do keep the current ref counting scheme, this also needs an AddRef call
> to balance the Release in OnConnect.

Doh. (Moot because I followed your earlier suggestion, but yes, thanks for
catching that.)

[Mihai Parparita -not on Chrome, 2012-06-07 17:38:58]

LGTM

[commit-bot: I haz the power, 2012-06-07 21:24:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miket@chromium.org/10537017/9018

[commit-bot: I haz the power, 2012-06-07 22:38:32]

Change committed as 141101