Add three of the six extensions to PAC that Internet Explorer supports. ...

Add three of the six extensions to PAC that Internet Explorer supports. 

The following descriptions were taken from <http://blogs.msdn.com/wndp/articles/IPV6_PAC_Extensions_v0_9.aspx>; 

---------------------------- 
* myIpAddressEx(): 
Returns a semi-colon delimited string containing all IP addresses for localhost (IPv6 and/or IPv4), or an empty string if unable to resolve localhost to an IP address. 

* dnsResolveEx(host): 
Returns semi-colon delimited string containing IPv6 and IPv4 addresses or an empty string if host is not resolvable. 

* isResolvableEx(): 
Returns TRUE if the host is resolvable to a IPv4 or IPv6 address, FALSE otherwise. 
---------------------------- 

These differ from the vanilla PAC functions in the following ways: 

* myIpAddressEx() returns all the addrsses for localhost (including IPv6 ones), whereas myIpAddress() only returns the first IPv4 one. 
* On failure, myIpAddress() returns "127.0.0.1" whereas on failure myIpAddressEx() returns empty string. 
* dnsResolveEx() returns a list of addresses (including IPV6 ones), whereas dnsResolve() only returns the first IPv4 address. 
* On failure, dnsResolve() returns |null|, whereas on failure dnsResolveEx() returns empty string. 

BUG=25407
TEST=ProxyResolverV8Test.DNSResolutionFailure, ProxyResolverJSBindingsTest.RestrictAddressFamily, ProxyResolverJSBindingsTest.ExFunctionsReturnList

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

[eroman, 2009-10-23 01:15:03]

http://codereview.chromium.org/333006/diff/1/2
File net/proxy/proxy_resolver_js_bindings.cc (left):

http://codereview.chromium.org/333006/diff/1/2#oldcode107
Line 107: // TODO(eroman): Should this return our IP address, or fail, or
I removed this TODO since it no longer applies.

See: http://codereview.chromium.org/164546 for the details.

[wtc, 2009-10-23 21:38:48]

LGTM.

http://codereview.chromium.org/333006/diff/7025/4011
File net/base/load_log_event_type_list.h (right):

http://codereview.chromium.org/333006/diff/7025/4011#newcode70
Line 70: // Measures the time taken to execute the "myIpAddress(Ex)" javascript
binding.
Typo: myIpAddress(Ex) => myIpAddressEx()

http://codereview.chromium.org/333006/diff/7025/4004
File net/proxy/proxy_resolver_js_bindings.cc (right):

http://codereview.chromium.org/333006/diff/7025/4004#newcode13
Line 13: #include "net/proxy/proxy_resolver_js_bindings.h"
This header needs to be included first according to our
style guide.

http://codereview.chromium.org/333006/diff/7025/4004#newcode108
Line 108: virtual std::string MyIpAddress() {
Add a TODO that we should get our IP address without using
host name resolution.  On Linux and Mac, we can call the
getifaddrs function:
http://developer.apple.com/mac/library/documentation/Darwin/Reference/ManPage...

Windows also has functions to enumerate the network interfaces.

http://codereview.chromium.org/333006/diff/7025/4005
File net/proxy/proxy_resolver_js_bindings.h (right):

http://codereview.chromium.org/333006/diff/7025/4005#newcode29
Line 29: // This is an Internet Explorer extension to PAC for IPv6, see:
Let's call these "Microsoft extensions" because WinHTTP may
support them, too.

http://codereview.chromium.org/333006/diff/7025/4008
File net/proxy/proxy_resolver_js_bindings_unittest.cc (right):

http://codereview.chromium.org/333006/diff/7025/4008#newcode13
Line 13: #if defined(OS_WIN)
Our style guide says we're supposed to include C system
headers before our own headers.

http://codereview.chromium.org/333006/diff/7025/4008#newcode26
Line 26: //     127.0.0.1
Are you sure we should return 127.0.0.1?

http://codereview.chromium.org/333006/diff/7025/4008#newcode121
Line 121: // but the Internet Explorer extensions to PAC do not. We test this
Internet Explorer extensions => Microsoft extensions

http://codereview.chromium.org/333006/diff/7025/4008#newcode174
Line 174: // separated list of addresses (as opposed to the non-ex versions
which
Nit: non-ex => non-Ex

http://codereview.chromium.org/333006/diff/7025/4007
File net/proxy/proxy_resolver_script.h (right):

http://codereview.chromium.org/333006/diff/7025/4007#newcode271
Line 271: "function isResolvableEx(host) { return dnsResolveEx(host) != ''; }\n"
Should we compare with null instead of ''?  See line 96.

Do you want to add extra \n and space characters to the
string to match PROXY_RESOLVER_SCRIPT?

http://codereview.chromium.org/333006/diff/7025/4006
File net/proxy/proxy_resolver_v8.cc (right):

http://codereview.chromium.org/333006/diff/7025/4006#newcode131
Line 131: // Internet Explorer's PAC extensions (incomplete):
Internet Explorer's => Microsoft's

http://codereview.chromium.org/333006/diff/7025/4006#newcode136
Line 136: dns_resolve_ex_template);
Should this be aligned with the opening '(' on the previous
line?

http://codereview.chromium.org/333006/diff/7025/4006#newcode261
Line 261: if (result.empty())
The spec doesn't say what should be returned on failure.

The 127.0.0.1 return value and the null return value for
DnsResolveCallback are copied from Mozilla code, but the
Mozilla code was added in rev. 1.4 of that file in year
2000, which was long after the orignal Netscape Navigator's
time:
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/netwerk/base/src/nsProxyAut...

So we may not need to match Firefox's return values in these
two cases.

http://codereview.chromium.org/333006/diff/7025/4006#newcode309
Line 309: return result.empty() ? v8::Null() : StdStringToV8String(result);
What's the v8::Null() for?  Why don't we have v8::Null() in
DnsResolveExCallback?

http://codereview.chromium.org/333006/diff/7025/4006#newcode334
Line 334: // DnsResolveEx() returns empty string on failure.
Without the ? : expression like the one in
DnsResolveCallback, this comment doesn't make sense and
should be removed.

[eroman, 2009-10-23 23:55:20]

Thanks for the review!

http://codereview.chromium.org/333006/diff/7025/4011
File net/base/load_log_event_type_list.h (right):

http://codereview.chromium.org/333006/diff/7025/4011#newcode70
Line 70: // Measures the time taken to execute the "myIpAddress(Ex)" javascript
binding.
On 2009/10/23 21:38:48, wtc wrote:
> Typo: myIpAddress(Ex) => myIpAddressEx()

Done.

http://codereview.chromium.org/333006/diff/7025/4004
File net/proxy/proxy_resolver_js_bindings.cc (right):

http://codereview.chromium.org/333006/diff/7025/4004#newcode13
Line 13: #include "net/proxy/proxy_resolver_js_bindings.h"
On 2009/10/23 21:38:48, wtc wrote:
> This header needs to be included first according to our
> style guide.

Done.

As a sidenote, I am getting very bored with duplicating this IF/ELSE header
block all over the place to define struct addrinfo... How would you feel about
me creating a wrapper header, so we can just do:

      #include "net/base/addrinfo.h"

Where "net/base/addrinfo.h" is:

    #include "build/build_config.h"

    #if defined(OS_WIN)
    #include <ws2tcpip.h>
    #else
    #include <netdb.h>
     #endif

http://codereview.chromium.org/333006/diff/7025/4004#newcode108
Line 108: virtual std::string MyIpAddress() {
On 2009/10/23 21:38:48, wtc wrote:
> Add a TODO that we should get our IP address without using
> host name resolution.  On Linux and Mac, we can call the
> getifaddrs function:
>
http://developer.apple.com/mac/library/documentation/Darwin/Reference/ManPage...
> 
> Windows also has functions to enumerate the network interfaces.

I don't think that is worth it.

The other browser's implementations do the same thing as us here, so it isn't
worth returning something different. If script authors want more control they
can always use myIpAddressEx().

http://codereview.chromium.org/333006/diff/7025/4005
File net/proxy/proxy_resolver_js_bindings.h (right):

http://codereview.chromium.org/333006/diff/7025/4005#newcode29
Line 29: // This is an Internet Explorer extension to PAC for IPv6, see:
On 2009/10/23 21:38:48, wtc wrote:
> Let's call these "Microsoft extensions" because WinHTTP may
> support them, too.

Done.

http://codereview.chromium.org/333006/diff/7025/4008
File net/proxy/proxy_resolver_js_bindings_unittest.cc (right):

http://codereview.chromium.org/333006/diff/7025/4008#newcode13
Line 13: #if defined(OS_WIN)
On 2009/10/23 21:38:48, wtc wrote:
> Our style guide says we're supposed to include C system
> headers before our own headers.

Done.

http://codereview.chromium.org/333006/diff/7025/4008#newcode26
Line 26: //     127.0.0.1
On 2009/10/23 21:38:48, wtc wrote:
> Are you sure we should return 127.0.0.1?

127.0.0.1 is just a random value I added to the test, it doesn't have special
meaning.

I have changed this to a different numeric value, so it doesn't stand out as
special.

http://codereview.chromium.org/333006/diff/7025/4008#newcode121
Line 121: // but the Internet Explorer extensions to PAC do not. We test this
On 2009/10/23 21:38:48, wtc wrote:
> Internet Explorer extensions => Microsoft extensions

Done.

http://codereview.chromium.org/333006/diff/7025/4008#newcode174
Line 174: // separated list of addresses (as opposed to the non-ex versions
which
On 2009/10/23 21:38:48, wtc wrote:
> Nit: non-ex => non-Ex

Done.

http://codereview.chromium.org/333006/diff/7025/4007
File net/proxy/proxy_resolver_script.h (right):

http://codereview.chromium.org/333006/diff/7025/4007#newcode271
Line 271: "function isResolvableEx(host) { return dnsResolveEx(host) != ''; }\n"
On 2009/10/23 21:38:48, wtc wrote:
> Should we compare with null instead of ''?  See line 96.

Can't compare with null since unlike the implementation of dnsResolve(),
dnsResolveEx() returns empty string on failure.

> 
> Do you want to add extra \n and space characters to the
> string to match PROXY_RESOLVER_SCRIPT?

Doesn't it already end in newline?

http://codereview.chromium.org/333006/diff/7025/4006
File net/proxy/proxy_resolver_v8.cc (right):

http://codereview.chromium.org/333006/diff/7025/4006#newcode131
Line 131: // Internet Explorer's PAC extensions (incomplete):
On 2009/10/23 21:38:48, wtc wrote:
> Internet Explorer's => Microsoft's

Done.

http://codereview.chromium.org/333006/diff/7025/4006#newcode136
Line 136: dns_resolve_ex_template);
On 2009/10/23 21:38:48, wtc wrote:
> Should this be aligned with the opening '(' on the previous
> line?

Done.

http://codereview.chromium.org/333006/diff/7025/4006#newcode261
Line 261: if (result.empty())
On 2009/10/23 21:38:48, wtc wrote:
> The spec doesn't say what should be returned on failure.
> 
> The 127.0.0.1 return value and the null return value for
> DnsResolveCallback are copied from Mozilla code, but the
> Mozilla code was added in rev. 1.4 of that file in year
> 2000, which was long after the orignal Netscape Navigator's
> time:
>
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/netwerk/base/src/nsProxyAut...
> 
> So we may not need to match Firefox's return values in these
> two cases.

Right, unfortunately the spec doesn't define the return values on failure, and
different browsers do different things.

Since we inherit the PAC utility library from Mozilla, we should match their
behavior (for example functions like isResolvable() expect a return of null).

Either way you slice it, there is going to be inconsistency between the Ex and
non-Ex versions.

(Whereas in Firefox dnsResolve() returns |null| on failure, in In IE it returns
|false|. And the Ex version returns |""|)

Note that I have added a compatibility chart to "dns_fail.js" to show what we do
with respect other browsers.

http://codereview.chromium.org/333006/diff/7025/4006#newcode309
Line 309: return result.empty() ? v8::Null() : StdStringToV8String(result);
On 2009/10/23 21:38:48, wtc wrote:
> What's the v8::Null() for?  Why don't we have v8::Null() in
> DnsResolveExCallback?

See comment above -- in the Ex flavored function I match Internet Explorer,
which uses empty string as their failure code.

In the non-Ex version, no implementation returns empty string (IE returns false
and Firefox returns |null|).

http://codereview.chromium.org/333006/diff/7025/4006#newcode334
Line 334: // DnsResolveEx() returns empty string on failure.
On 2009/10/23 21:38:48, wtc wrote:
> Without the ? : expression like the one in
> DnsResolveCallback, this comment doesn't make sense and
> should be removed.

Done.

[wtc, 2009-10-24 00:35:17]

LGTM.

http://codereview.chromium.org/333006/diff/7025/4004
File net/proxy/proxy_resolver_js_bindings.cc (right):

http://codereview.chromium.org/333006/diff/7025/4004#newcode13
Line 13: #include "net/proxy/proxy_resolver_js_bindings.h"
On 2009/10/23 23:55:20, eroman wrote:
> How would you feel about
> me creating a wrapper header, so we can just do:
> 
>       #include "net/base/addrinfo.h"

This is a good idea.

http://codereview.chromium.org/333006/diff/7025/4004#newcode108
Line 108: virtual std::string MyIpAddress() {
On 2009/10/23 23:55:20, eroman wrote:
>
> The other browser's implementations do the same thing as us here, so it isn't
> worth returning something different. If script authors want more control they
> can always use myIpAddressEx().

The TODO comment I suggested applies to myIpAddressEx()
as well.  getifaddrs() is the right way to get the IP
addresses of the local host, but few people know about
this function, which is why it's common to see code that
uses a host name resolution to get the local IP addresses.

If you think "TODO" implies that we're planning to make
this change, you can make it a normal comment.  I just want
to document the correct way to get the local IP addresses,
so that people know we made an informed decision to take a
shortcut here.

http://codereview.chromium.org/333006/diff/7025/4007
File net/proxy/proxy_resolver_script.h (right):

http://codereview.chromium.org/333006/diff/7025/4007#newcode266
Line 266: "}\n" \
Nit: delete this \

http://codereview.chromium.org/333006/diff/7025/4007#newcode271
Line 271: "function isResolvableEx(host) { return dnsResolveEx(host) != ''; }\n"
On 2009/10/23 23:55:20, eroman wrote:
>
> Doesn't it already end in newline?

What I meant is this:

#define PROXY_RESOLVER_SCRIPT_EX \
  "function isResolvableEx(host) {\n" \
  "    return dnsResolveEx(host) != '';\n" \
  "}\n"

http://codereview.chromium.org/333006/diff/4014/7034
File net/data/proxy_resolver_v8_unittest/dns_fail.js (right):

http://codereview.chromium.org/333006/diff/4014/7034#newcode4
Line 4: // Note that there are compatibility difference in what browsers will
return
Nit: difference => differences

This useful info shouldn't be hidden in a unit test file.

What does ??? mean?

[eroman, 2009-10-24 01:03:00]

http://codereview.chromium.org/333006/diff/7025/4004
File net/proxy/proxy_resolver_js_bindings.cc (right):

http://codereview.chromium.org/333006/diff/7025/4004#newcode108
Line 108: virtual std::string MyIpAddress() {
On 2009/10/24 00:35:17, wtc wrote:
> On 2009/10/23 23:55:20, eroman wrote:
> >
> > The other browser's implementations do the same thing as us here, so it
isn't
> > worth returning something different. If script authors want more control
they
> > can always use myIpAddressEx().
> 
> The TODO comment I suggested applies to myIpAddressEx()
> as well.  getifaddrs() is the right way to get the IP
> addresses of the local host, but few people know about
> this function, which is why it's common to see code that
> uses a host name resolution to get the local IP addresses.
> 
> If you think "TODO" implies that we're planning to make
> this change, you can make it a normal comment.  I just want
> to document the correct way to get the local IP addresses,
> so that people know we made an informed decision to take a
> shortcut here.

Done.

I was just reluctant to put more TODO(eroman): into the code :)

http://codereview.chromium.org/333006/diff/7025/4007
File net/proxy/proxy_resolver_script.h (right):

http://codereview.chromium.org/333006/diff/7025/4007#newcode266
Line 266: "}\n" \
On 2009/10/24 00:35:17, wtc wrote:
> Nit: delete this \

Done.

http://codereview.chromium.org/333006/diff/7025/4007#newcode271
Line 271: "function isResolvableEx(host) { return dnsResolveEx(host) != ''; }\n"
On 2009/10/24 00:35:17, wtc wrote:
> On 2009/10/23 23:55:20, eroman wrote:
> >
> > Doesn't it already end in newline?
> 
> What I meant is this:
> 
> #define PROXY_RESOLVER_SCRIPT_EX \
>   "function isResolvableEx(host) {\n" \
>   "    return dnsResolveEx(host) != '';\n" \
>   "}\n"

Ah good idea. Done.

http://codereview.chromium.org/333006/diff/4014/7034
File net/data/proxy_resolver_v8_unittest/dns_fail.js (right):

http://codereview.chromium.org/333006/diff/4014/7034#newcode4
Line 4: // Note that there are compatibility difference in what browsers will
return
On 2009/10/24 00:35:18, wtc wrote:
> Nit: difference => differences
> 
> This useful info shouldn't be hidden in a unit test file.

Indeed. 

I have added the following fairly length section explaining how the javascript
environment works, and how it differs between browsers:

http://codereview.chromium.org/333006/diff2/10008:2018/3009

Please let me know if that is helpful.

> 
> What does ??? mean?

??? means I didn't get around to testing it yet.
Added a TODO and explained that fact.

[wtc, 2009-10-24 04:40:59]

LGTM!

http://codereview.chromium.org/333006/diff/2018/3007
File net/proxy/proxy_resolver_js_bindings.cc (right):

http://codereview.chromium.org/333006/diff/2018/3007#newcode123
Line 123: // Disable IPv6 results. We do this because Internet Explorer does it
--
I studied the original specification of the PAC script helper functions today.
Although not explicitly stated, it's clear that those functions should only
return IPv4 addresses.  The evidence is the mention of "dotted decimal"
notation and "0" and "255" as masks for addresses.

So we now have a stronger reason to get only IPv4 results than "because
Internet Explorer does it".

http://codereview.chromium.org/333006/diff/2018/3010
File net/proxy/proxy_resolver_script.h (right):

http://codereview.chromium.org/333006/diff/2018/3010#newcode272
Line 272: "    var ip = dnsResolveEx(host);\n" \
Nit: since dnsResolveEx returns all the addresses of the local host,
perhaps this variable should be named addrs or ipList.

http://codereview.chromium.org/333006/diff/2018/3009
File net/proxy/proxy_resolver_v8.cc (right):

http://codereview.chromium.org/333006/diff/2018/3009#newcode17
Line 17: // Notes on the javascript environment:
Very nice!  Thanks.

http://codereview.chromium.org/333006/diff/2018/3009#newcode55
Line 55: // dnsResolve()     | IPv4/IPv6   |  IPv4             |  IPv4
It's possible that Firefox's dnsResolve returns IPv6 results unintentionally.
We should file a bug report.