Move DNS resolution from websocket-to-TCP proxy to ExtensionFunction

Move DNS resolution from websocket-to-TCP proxy to ExtensionFunction

DNS resolution in proxy is libevent-based, not regular HostResolver.
Now we use HostResolver.

(Original CL was written by dilmah@ )

BUG=chromium-os:23329
TEST=manual


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

[altimofeev, 2011-11-18 15:24:23]

PTAL

[Denis Lagno, 2011-11-18 23:05:25]

http://codereview.chromium.org/8586027/diff/8001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_web_socket_proxy_private_api.cc
(right):

http://codereview.chromium.org/8586027/diff/8001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_web_socket_proxy_private_api.cc:207:
passport += map_["addr"];
nit: this will fail if hostname resolved into IPv6 address
(because they contain colons).

[altimofeev, 2011-11-21 17:29:42]

Fixed comment and added unittests. PTAL.

http://codereview.chromium.org/8586027/diff/8001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_web_socket_proxy_private_api.cc
(right):

http://codereview.chromium.org/8586027/diff/8001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_web_socket_proxy_private_api.cc:207:
passport += map_["addr"];
On 2011/11/18 23:05:25, Denis Lagno wrote:
> nit: this will fail if hostname resolved into IPv6 address
> (because they contain colons).

Good point, thanks! Fixed parsing on the WebSocketProxy side.

[Denis Lagno, 2011-11-21 22:38:24]

http://codereview.chromium.org/8586027/diff/15001/chrome/browser/chromeos/web...
File chrome/browser/chromeos/web_socket_proxy_helper.h (right):

http://codereview.chromium.org/8586027/diff/15001/chrome/browser/chromeos/web...
chrome/browser/chromeos/web_socket_proxy_helper.h:15: class WebSocketProxyHelper
{
I do not like creating separate classes/abstractions for such non-generic tasks.
 Such code better be placed into anonymous namespace in .cc file.

Or even better eliminate it at all.  Maybe just escape address (for example
base64-encoded address will not contain any colons).

[altimofeev, 2011-11-22 07:42:44]

Thank you for the comment. Please find my answer inline.

http://codereview.chromium.org/8586027/diff/15001/chrome/browser/chromeos/web...
File chrome/browser/chromeos/web_socket_proxy_helper.h (right):

http://codereview.chromium.org/8586027/diff/15001/chrome/browser/chromeos/web...
chrome/browser/chromeos/web_socket_proxy_helper.h:15: class WebSocketProxyHelper
{
On 2011/11/21 22:38:24, Denis Lagno wrote:
> I do not like creating separate classes/abstractions for such non-generic
tasks.
>  Such code better be placed into anonymous namespace in .cc file.
> 
> Or even better eliminate it at all.  Maybe just escape address (for example
> base64-encoded address will not contain any colons).

1. The reason I have placed it separately is an ability to unittest it.
2. From my point of view, escaping the address will not simplify the parsing
code a lot.

[Denis Lagno, 2011-11-22 08:17:48]

On 2011/11/22 07:42:44, altimofeev wrote:
> 2. From my point of view, escaping the address will not simplify the parsing
> code a lot.

escaping address is matter of 2-4 lines of code added: Base64Encode when putting
it into result and Base64Decode when fetching it (parsing code remains the
same).

Your change is much bigger (that is bad for merging) and it depends on
particular representation of IPv6 addresses.

[altimofeev, 2011-11-22 09:04:34]

On 2011/11/22 08:17:48, Denis Lagno wrote:
> escaping address is matter of 2-4 lines of code added: Base64Encode when
putting
> it into result and Base64Decode when fetching it (parsing code remains the
> same).

Actually, parsing should also be changed since a new part was added.

> Your change is much bigger (that is bad for merging) and it depends on
> particular representation of IPv6 addresses.

I don't think merging will be a problem. Furthermore having an unittest is a
plus. The fact parsing relies on is that IPv6 address couldn't contain ']:' as a
substring. At the same time it avoids Base64 encoding/decoding.

[Dmitry Polukhin, 2011-11-22 09:04:58]

LGTM

[commit-bot: I haz the power, 2011-11-22 12:33:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/altimofeev@chromium.org/8586027/15001

[commit-bot: I haz the power, 2011-11-22 12:33:57]

Can't process patch for file
chrome/browser/chromeos/web_socket_proxy_helper_unittest.cc.
File's status is None, patchset upload is incomplete.

[commit-bot: I haz the power, 2011-11-22 12:52:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/altimofeev@chromium.org/8586027/22010

[commit-bot: I haz the power, 2011-11-22 13:54:25]

Change committed as 111148