Support replacement of IP address resolutions via command line flag

Support replacement of IP address resolutions via command line flag
Provide a command line flag that can accept a list or range of resolution IP
address (such as those for a known CDN), and place an alternative resolution
at the start of the resolution list (driving towrads a test server).

r=wtc
BUG=342116
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252289

[jar (doing other things), 2014-02-11 19:32:41]

Wan-teh: PTAL

Eric: This is very similar (analogous) to your host-mapping code, and you may
want to comment as well.

[wtc, 2014-02-11 23:52:30]

Patch set 11 LGTM.

I hope we can find a better name than "IP resolver".

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
File net/base/ip_mapping_rules.cc (right):

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
net/base/ip_mapping_rules.cc:32: const IPAddressNumber& Address() const { return
address_; }

Nit: this getter method should be lowercase "address()".

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
net/base/ip_mapping_rules.cc:37: scoped_ptr<IPPattern> pattern_;  // The pattern
we can match.

You should be able to declare this member as
  IPPattern pattern_;

This eliminates one heap allocation. It will require you to provide a getter
method for the pattern_ member. I think you can just declare this class as a
struct and make all data members public.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
net/base/ip_mapping_rules.cc:38: IPAddressNumber address_;        // The
replacement address we provide.

Nit: I suggest renaming this member "replacement_address_".

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
net/base/ip_mapping_rules.cc:49: for (MapRule* next_rule = first_rule_;

Nit: I suggest "next_rule" be renamed "rule".

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
net/base/ip_mapping_rules.cc:55: }

Nit: omit curly braces.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
net/base/ip_mapping_rules.cc:69: if (trimmed == "")

Nit: test trimmed.empty().

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
net/base/ip_mapping_rules.cc:81: DVLOG(1) << "Rule missing directive: " <<
rule_string;

The error message should be "Unknown directive".

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
File net/base/ip_mapping_rules.h (right):

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
net/base/ip_mapping_rules.h:11: #include "net/base/address_list.h"

1. Nit: I think you can include "base/macros.h" instead of "base/basictypes.h".
I think the only thing you need from the header is DISALLOW_COPY_AND_ASSIGN.

2. Nit: You should be able to use a forward declaration of class AddressList
instead of including "net/base/address_list.h".

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
net/base/ip_mapping_rules.h:16: class NET_EXPORT_PRIVATE IPMappingRules {

Nit: it would be nice to have a short comment describing what this class is.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
net/base/ip_mapping_rules.h:26: // Rules are evaluated against an address list
until one is found that

Nit: "one" is ambiguous because it could mean a rule or an address. I suggest
saying "a rule".

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
net/base/ip_mapping_rules.h:27: // applies, causing a modification of the
address resolution list.

Nit: remove "resolution".

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
net/base/ip_mapping_rules.h:31: //   RULE : CHANGE_DIRECTIVE | EMPTY

Nit: remove the space before ":".

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
net/base/ip_mapping_rules.h:32: //   CHANGE_DIRECTIVE: "PREFACE" SPACE
IP_PATTERN SPACE IP_LITERAL

You should document the PREFACE directive somewhere.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
net/base/ip_mapping_rules.h:41: //   IP_V6_LITERAL:  HEX_COMPONENT  |
IP_V6_LITERAL ":" HEX_COMPONENT

This may not be a complete specification of IPv6 literals. For eample, "::" and
"::1" are both IPv6 literals.

Update: I see that you require an IP_V6_LITERAL to have 8 components. In that
case, I think this IP_V6_LITERAL is still not correct because it allows a single
HEX_COMPONENT.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
net/base/ip_mapping_rules.h:42: //   HEX_COMPONENT: hexidecimal values with no
more than 4 hex digits

Typo: hexidecimal => hexadecimal

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
net/base/ip_mapping_rules.h:55: //   OCTET_ITEM: OCTET | OCTET "-" OCTET

I think OCTET_ITEM should be renamed OCTET_RANGE to be consistent with
HEX_RANGE.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
net/base/ip_mapping_rules.h:58: //  components for IPv4, and 8 for IPv6).

Nit: these two lines probably should have only one space in the front.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
net/base/ip_mapping_rules.h:70: class MapRule;

Nit: our Style Guide seems to recommend placing the forward declarations of
classes at the beginning of the "private" section.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
File net/base/ip_mapping_rules_unittest.cc (right):

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
net/base/ip_mapping_rules_unittest.cc:111: MatchingInsertionHelper(matchable_ip,
 matching_ip_pattern, new_preface_ip,

Nit: two spaces before the comma. You can find all occurrences by searching for
"  ,".

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
net/base/ip_mapping_rules_unittest.cc:172: // IPv6 component.  If the code
confused  them, then this range would not

Nit: search for "  them". There are two spaces.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
File net/base/ip_pattern.cc (right):

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.cc:20: ComponentPattern* Reverse(ComponentPattern*
previous);

At least Reverse should be documented. (The other methods are somewhat
self-explanatory.)

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.cc:31: DISALLOW_COPY_AND_ASSIGN(ComponentPattern);

Nit: you should add more blank lines. For example, a blank line before the
beginning of "private:", and a blank line to separate the typedef from data
members.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.cc:34: 

Nit: delete one blank line.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.cc:50: }

Nit: we usually omit curly braces for one-line if statement bodies, except for
QUIC code.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.cc:56: ComponentPattern* previous) {

Nit: the function parameter should be indented by four.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.cc:77: if (ip_mask_.empty()) {

It seems that ip_mask_ should not be empty. ip_mask_ should always be of size 4
or 8.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.cc:85: ComponentPattern* next_pattern =
first_component_pattern_;

Nit: name this variable "pattern".

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.cc:139: if ((*component_it)[component_it->size() - 1] !=
']') {

This can be component_it->back().

Similarly, on line 129, *component_it)[0] can be component_it->front(), but I
think back() is more useful.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.cc:148: //  We'll need a pattern to match this bracketed
component.

Nit: two spaces before "We'll".

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.cc:154: if (!ParseComponentPattern(component_it->substr(

If you change ParseComponentPattern to take a base::StringPiece, then you don't
need to modify the std::string.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.cc:164: // is needed to process a match, so we need to
reverse it.

If you maintain a last_component_pattern pointer as you add component patterns
to the list, then you don't need to reverse the list.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.cc:196: pattern->AppendRange(min, max);

Is it OK to append some ranges, and then fail and return false?

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.cc:204: : base::HexStringToUInt(input, output);

Nit: we usually put an operator (such as ":") at the end of the previous line. I
am not sure if this is recommended by the Style Guide though.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.cc:206: DVLOG(1) << "IPv4 component greater than 255";

For IPv6, you should also verify that *output <= 0xFFFF.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.cc:211: }

I think we should first check !ok, then check the is_ipv4_ && *output > 255u
case. If base::StringToUint returns false, we should not assume we can use the
output.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
File net/base/ip_pattern.h (right):

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.h:13: #include "net/base/address_list.h"

Include "net/base/net_util.h" instead. You just need the definition of
IPAddressNumber.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.h:26: bool Match(const IPAddressNumber& address) const;

Nit: document these two methods.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.h:38: bool ValueTextToInt(const base::StringPiece& input,
uint32* output) const;

All uses of uint32 in this class probably should be changed to uint16. Four
hexadecimal digits represent two bytes, or uint16. It is possible that using
uint32 is more convenient though.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.h:41: // The |ip_mask_| indicates,for each component, if
this pattern requires an

Nit: add a space before "for".

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.h:59: DISALLOW_COPY_AND_ASSIGN(IPPattern);

Nit: I suggest adding a blank line to separate these members.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.h:62: 

Nit: delete one blank line.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/dns/mapped_i...
File net/dns/mapped_ip_resolver.cc (right):

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/dns/mapped_i...
net/dns/mapped_ip_resolver.cc:23: const CompletionCallback& original_callback,

Why did you rename this parameter "original_callback"? I think it should be
named "callback".

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/dns/mapped_i...
File net/dns/mapped_ip_resolver.h (right):

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/dns/mapped_i...
net/dns/mapped_ip_resolver.h:19: // resolution response by inserting or
replacing IP addresses before returning

You only implemented the "PREFACE" directive, right? So you can't really replace
IP addresses. Replacing means you also delete the replaced address.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/dns/mapped_i...
net/dns/mapped_ip_resolver.h:21: class NET_EXPORT MappedIPResolver : public
HostResolver {

Nit: "IPResolver" is a poor name because there is no such thing as IP
resolution. That is, an IP address is the result of host name resolution.

Maybe this should be named "IPMappingHostResolver".

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/dns/mapped_i...
net/dns/mapped_ip_resolver.h:23: // Creates a MappedHostResolver that forwards
all of its requests through

MappedHostResolver => MappedIPResolver

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/dns/mapped_i...
net/dns/mapped_ip_resolver.h:35: // Takes a comma separated list of rules, and
assigns them to this resolver.

Typo: comma => semicolon

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/dns/mapped_i...
net/dns/mapped_ip_resolver.h:58: void ApplyRules(const CompletionCallback&
original_cb,

Nit: it may be nice to be verbose and name this parameter "original_callback".

[wtc, 2014-02-12 00:11:15]

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
File net/base/ip_mapping_rules.h (right):

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
net/base/ip_mapping_rules.h:47: //   HEX_PATTERN:  HEX_COMPONENT | "["
HEX_GROUP_PATTERN "]" | "[*]"

Please fix the grammar here and on line 53. '*' should not be inside [].

[Ryan Hamilton, 2014-02-12 00:44:02]

https://codereview.chromium.org/156963003/diff/950001/net/base/ip_mapping_rul...
File net/base/ip_mapping_rules.h (right):

https://codereview.chromium.org/156963003/diff/950001/net/base/ip_mapping_rul...
net/base/ip_mapping_rules.h:73: MapRule* first_rule_;
nit: instead of implementing a linked-list manually, I recommend using an
std::list (or std::vector) of MapRule*s.  You'll need to add
STLDeleteElements(&rules_) to your destructor, but since you already had to add
(and implement) DeleteRules, it will actually end up less code.

https://codereview.chromium.org/156963003/diff/950001/net/base/ip_pattern.h
File net/base/ip_pattern.h (right):

https://codereview.chromium.org/156963003/diff/950001/net/base/ip_pattern.h#n...
net/base/ip_pattern.h:58: ComponentPattern* first_component_pattern_;
Ditto here.  Instead of implementing the list traversal and element deletion
manually in the destructor, using a list<ComponentPattern> would let you simply
say:

STLDeleteElements(&component_patterns_) in the destructor.

I think this would improve readability.

[eroman, 2014-02-12 21:38:20]

I have some concerns with this:

 * Multiple rules can be given, however only the first matching IP will cause an
insertion - If this is to be a generic mechanism rules should stack

 * Only insertion is allowed an not replacement

 * Custom pattern format for representing IP addresses (limited understanding of
IPv6 literals - no support for contractions, or embedded IPv4 literals)

 * Implementing custom linked list. At worst use base/containers/linked_list.h.
However a std::vector<> would be most appropriate (both performance and
readability). I suspect the reason you wanted a list is because you made the
rules non-copiable because of the scoped_ptr<>.

Can you give some concrete examples of all the IP patterns you intend to match?
This seems like a lot of parsing/custom code to accomplish something that
perhaps could be satisfied by a simpler format.

Have you considered using an input of an IP bit mask? For instance for
CIDR-style ranges we support inputs like "a:b:c/46" or "255.2550.0.0/16", which
is fine for prefix matches.

For a more generalized match you for instance consider a match if (input &
pattern) == pattern. Or provide a second number which is the value to compare
against after making.

[jar (doing other things), 2014-02-15 21:13:03]

On 2014/02/12 21:38:20, eroman wrote:
> I have some concerns with this:
> 
>  * Multiple rules can be given, however only the first matching IP will cause
an
> insertion - If this is to be a generic mechanism rules should stack

Interesting suggestion.  I couldn't see a use case that would use/require
stacking, and could see that the planned(?) use case would suffer from
potentially a lot more checking if all rules needed to be tested, ever after one
had fired. In fact, I was even tempted to have a rule only test the first
address in a list.  After going back-n-forth on a lot of this, I decided that we
could use a different directive to stack, and have some directives that would
effectively terminate further IP rule matching consideration. This seemed most
appropriate to the planned(?) use case... so I called it a day, would advocate
for extending the plans when we have a clearer use case.

> 
>  * Only insertion is allowed an not replacement

I documented this now. At first I started to implement a wide variety of things
(exclusions, replacements, etc.), and then started to realize this was probably
going to be useless code, until I had a user. As a result, I backed down to just
supporting what I knew was needed... but I tried to leave the door open by
providing an explicit "directive" (re: "PREFACE") which could in the future be
used to specify REPLACE or POSTFIX, or etc. etc.  ...but only when we had a good
reason or use case.

> 
>  * Custom pattern format for representing IP addresses (limited understanding
of
> IPv6 literals - no support for contractions, or embedded IPv4 literals)

I wrote the syntax for the rules to allow for the "::" contractions, but again,
decided such support was going to have minimal utility for the resulting
complexity... and decided to punt on it.

> 
>  * Implementing custom linked list. At worst use
base/containers/linked_list.h.
> However a std::vector<> would be most appropriate (both performance and
> readability). I suspect the reason you wanted a list is because you made the
> rules non-copiable because of the scoped_ptr<>.

All true... I've now switched to vector of pointers in the most recent patch
set, for all cases of lists (and the classes do indeed get to
DISALLOW_COPY_AND_ASSIGN).

> 
> Can you give some concrete examples of all the IP patterns you intend to
match?
> This seems like a lot of parsing/custom code to accomplish something that
> perhaps could be satisfied by a simpler format.

The most common case would be matching a discrete set of IP addresses associated
with single data center, such as:

1,2,3.[23-42,49]
and providing a prefix to a specific test server, such as:
1.2.3.174

The hassle is that the ranges in the match do not have to be consecutive (though
they commonly have some consecutive runs), and worst of all, they don't have to
align with any nice binary patterns (so I can't just do a mask check).  In the
above example, it may be critical to *not* match:

1.2.3.43

Most resolutions are expected to "fail to match" because of some prefix
difference, where only an exact match is specified. I *expect* this to result in
a pretty fast "pattern didn't match" test, for random IPs on the internet.

I wanted the ranges to go in an arbitrary position, because it seemed likely
that for an IPv6 address the "highly variable" component would not be the last
component (as it would be commonly(?) in IPv4).  

Hopefully that justifies the patter matching choices.  I guess it could have
been simpler... but not by much IMO.  

Did you have some other thoughts in mind?


> Have you considered using an input of an IP bit mask? For instance for
> CIDR-style ranges we support inputs like "a:b:c/46" or "255.2550.0.0/16",
which
> is fine for prefix matches.

I thought of that at first, but the two things that pushed me away were:
a) IPAddress in an AddressList in a resolution comes out as a vector of ints. 
If I was given the address as something more like an int32 for IPv4, or a pair
of int64 for IPv6, then a bit mask would start looking very attractive.  Given
that I *expected* that many random addresses would fail in a match against a
first integral component, it didn't seem worth converting the address into a
larger int.

b) Even if I did get a bit mask... I'd still have a hard time handling a few
isolated values in a component, or a range of values for a component.

> For a more generalized match you for instance consider a match if (input &
> pattern) == pattern. Or provide a second number which is the value to compare
> against after making.

Yes... that was my initial thought... until I realized data (resolutions) didn't
arrive as an int... and my pattern checking didn't align well with bit patterns
(for some data center examples I looked at).

[jar (doing other things), 2014-02-15 21:14:56]

Wan Teh: Great and detailed review!!!! Thanks! PTAL

Ryan: switched to using std::vector: PTAL

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
File net/base/ip_mapping_rules.cc (right):

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
net/base/ip_mapping_rules.cc:32: const IPAddressNumber& Address() const { return
address_; }
On 2014/02/11 23:52:30, wtc wrote:
> 
> Nit: this getter method should be lowercase "address()".

Done.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
net/base/ip_mapping_rules.cc:37: scoped_ptr<IPPattern> pattern_;  // The pattern
we can match.
On 2014/02/11 23:52:30, wtc wrote:
> 
> You should be able to declare this member as
>   IPPattern pattern_;
> 
> This eliminates one heap allocation. It will require you to provide a getter
> method for the pattern_ member. I think you can just declare this class as a
> struct and make all data members public.

Done.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
net/base/ip_mapping_rules.cc:38: IPAddressNumber address_;        // The
replacement address we provide.
On 2014/02/11 23:52:30, wtc wrote:
> 
> Nit: I suggest renaming this member "replacement_address_".

Done.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
net/base/ip_mapping_rules.cc:49: for (MapRule* next_rule = first_rule_;
On 2014/02/11 23:52:30, wtc wrote:
> 
> Nit: I suggest "next_rule" be renamed "rule".

Done (plus new container refector)

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
net/base/ip_mapping_rules.cc:55: }
On 2014/02/11 23:52:30, wtc wrote:
> 
> Nit: omit curly braces.

I thought net/* style was to *always* use curlies.  I'm assuming this is an
exception....  I like it better.. and will do so consistently.

Done.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
net/base/ip_mapping_rules.cc:69: if (trimmed == "")
On 2014/02/11 23:52:30, wtc wrote:
> 
> Nit: test trimmed.empty().

Done.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
net/base/ip_mapping_rules.cc:81: DVLOG(1) << "Rule missing directive: " <<
rule_string;
On 2014/02/11 23:52:30, wtc wrote:
> 
> The error message should be "Unknown directive".

Done.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
File net/base/ip_mapping_rules.h (right):

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
net/base/ip_mapping_rules.h:11: #include "net/base/address_list.h"
On 2014/02/11 23:52:30, wtc wrote:
> 
> 1. Nit: I think you can include "base/macros.h" instead of
"base/basictypes.h".
> I think the only thing you need from the header is DISALLOW_COPY_AND_ASSIGN.
> 
> 2. Nit: You should be able to use a forward declaration of class AddressList
> instead of including "net/base/address_list.h".

Done.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
net/base/ip_mapping_rules.h:16: class NET_EXPORT_PRIVATE IPMappingRules {
On 2014/02/11 23:52:30, wtc wrote:
> 
> Nit: it would be nice to have a short comment describing what this class is.

Done.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
net/base/ip_mapping_rules.h:26: // Rules are evaluated against an address list
until one is found that
On 2014/02/11 23:52:30, wtc wrote:
> 
> Nit: "one" is ambiguous because it could mean a rule or an address. I suggest
> saying "a rule".

Done.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
net/base/ip_mapping_rules.h:27: // applies, causing a modification of the
address resolution list.
On 2014/02/11 23:52:30, wtc wrote:
> 
> Nit: remove "resolution".

Done.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
net/base/ip_mapping_rules.h:31: //   RULE : CHANGE_DIRECTIVE | EMPTY
On 2014/02/11 23:52:30, wtc wrote:
> 
> Nit: remove the space before ":".

Done.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
net/base/ip_mapping_rules.h:32: //   CHANGE_DIRECTIVE: "PREFACE" SPACE
IP_PATTERN SPACE IP_LITERAL
On 2014/02/11 23:52:30, wtc wrote:
> 
> You should document the PREFACE directive somewhere.

Done.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
net/base/ip_mapping_rules.h:41: //   IP_V6_LITERAL:  HEX_COMPONENT  |
IP_V6_LITERAL ":" HEX_COMPONENT
On 2014/02/11 23:52:30, wtc wrote:
> 
> This may not be a complete specification of IPv6 literals. For eample, "::"
and
> "::1" are both IPv6 literals.
> 
> Update: I see that you require an IP_V6_LITERAL to have 8 components. In that
> case, I think this IP_V6_LITERAL is still not correct because it allows a
single
> HEX_COMPONENT.

I made this explicit in a new comment below.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
net/base/ip_mapping_rules.h:42: //   HEX_COMPONENT: hexidecimal values with no
more than 4 hex digits
On 2014/02/11 23:52:30, wtc wrote:
> 
> Typo: hexidecimal => hexadecimal

Done.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
net/base/ip_mapping_rules.h:47: //   HEX_PATTERN:  HEX_COMPONENT | "["
HEX_GROUP_PATTERN "]" | "[*]"
On 2014/02/12 00:11:15, wtc wrote:
> 
> Please fix the grammar here and on line 53. '*' should not be inside [].

Done.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
net/base/ip_mapping_rules.h:55: //   OCTET_ITEM: OCTET | OCTET "-" OCTET
On 2014/02/11 23:52:30, wtc wrote:
> 
> I think OCTET_ITEM should be renamed OCTET_RANGE to be consistent with
> HEX_RANGE.

Done.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
net/base/ip_mapping_rules.h:58: //  components for IPv4, and 8 for IPv6).
On 2014/02/11 23:52:30, wtc wrote:
> 
> Nit: these two lines probably should have only one space in the front.

Done.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
net/base/ip_mapping_rules.h:70: class MapRule;
On 2014/02/11 23:52:30, wtc wrote:
> 
> Nit: our Style Guide seems to recommend placing the forward declarations of
> classes at the beginning of the "private" section.

Done.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
net/base/ip_mapping_rules.h:73: MapRule* first_rule_;
On 2014/02/12 00:44:03, Ryan Hamilton wrote:
> nit: instead of implementing a linked-list manually, I recommend using an
> std::list (or std::vector) of MapRule*s.  You'll need to add
> STLDeleteElements(&rules_) to your destructor, but since you already had to
add
> (and implement) DeleteRules, it will actually end up less code.

Done.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
File net/base/ip_mapping_rules_unittest.cc (right):

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
net/base/ip_mapping_rules_unittest.cc:111: MatchingInsertionHelper(matchable_ip,
 matching_ip_pattern, new_preface_ip,
On 2014/02/11 23:52:30, wtc wrote:
> 
> Nit: two spaces before the comma. You can find all occurrences by searching
for
> "  ,".

Done.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_mapp...
net/base/ip_mapping_rules_unittest.cc:172: // IPv6 component.  If the code
confused  them, then this range would not
On 2014/02/11 23:52:30, wtc wrote:
> 
> Nit: search for "  them". There are two spaces.

Done.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
File net/base/ip_pattern.cc (right):

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.cc:20: ComponentPattern* Reverse(ComponentPattern*
previous);
On 2014/02/11 23:52:30, wtc wrote:
> 
> At least Reverse should be documented. (The other methods are somewhat
> self-explanatory.)

Done.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.cc:31: DISALLOW_COPY_AND_ASSIGN(ComponentPattern);
On 2014/02/11 23:52:30, wtc wrote:
> 
> Nit: you should add more blank lines. For example, a blank line before the
> beginning of "private:", and a blank line to separate the typedef from data
> members.

Done.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.cc:34: 
On 2014/02/11 23:52:30, wtc wrote:
> 
> Nit: delete one blank line.

Done.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.cc:50: }
On 2014/02/11 23:52:30, wtc wrote:
> 
> Nit: we usually omit curly braces for one-line if statement bodies, except for
> QUIC code.

Done.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.cc:56: ComponentPattern* previous) {
On 2014/02/11 23:52:30, wtc wrote:
> 
> Nit: the function parameter should be indented by four.

Done.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.cc:77: if (ip_mask_.empty()) {
On 2014/02/11 23:52:30, wtc wrote:
> 
> It seems that ip_mask_ should not be empty. ip_mask_ should always be of size
4
> or 8.

Rather than DCHECKing, it seemed nice to insist that until a pattern is loaded
(parsed into place), the instance "does not match" anything.

If you'd really rather, I could change this, and change the associated test
case.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.cc:85: ComponentPattern* next_pattern =
first_component_pattern_;
On 2014/02/11 23:52:30, wtc wrote:
> 
> Nit: name this variable "pattern".

Done.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.cc:139: if ((*component_it)[component_it->size() - 1] !=
']') {
On 2014/02/11 23:52:30, wtc wrote:
> 
> This can be component_it->back().
> 
> Similarly, on line 129, *component_it)[0] can be component_it->front(), but I
> think back() is more useful.

Much nicer! thanks.

Done.

...but then... the try bot for patch set 12 blew up with:
../../net/base/ip_pattern.cc: In member function 'bool
net::IPPattern::ParsePattern(const string&)':
../../net/base/ip_pattern.cc:108:30: error: 'struct std::basic_string<char>' has
no member named 'front'
../../net/base/ip_pattern.cc:117:23: error: 'struct std::basic_string<char>' has
no member named 'back'

and I had to go back to the original form.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.cc:148: //  We'll need a pattern to match this bracketed
component.
On 2014/02/11 23:52:30, wtc wrote:
> 
> Nit: two spaces before "We'll".

Done.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.cc:154: if (!ParseComponentPattern(component_it->substr(
On 2014/02/11 23:52:30, wtc wrote:
> 
> If you change ParseComponentPattern to take a base::StringPiece, then you
don't
> need to modify the std::string.

Done.... but we end up converting it std::string in order to do a SplitString in
the parse method.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.cc:164: // is needed to process a match, so we need to
reverse it.
On 2014/02/11 23:52:30, wtc wrote:
> 
> If you maintain a last_component_pattern pointer as you add component patterns
> to the list, then you don't need to reverse the list.

Transitioned to std::vector<pointers>

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.cc:196: pattern->AppendRange(min, max);
On 2014/02/11 23:52:30, wtc wrote:
> 
> Is it OK to append some ranges, and then fail and return false?

The caller discards the partially constructed pattern, but the pattern (in the
incomplete form) is fully functional.  Clearing it out would leave a similarly
odd instance, which would also "function" to some extent (never matching
anything).  The current returned state seems reasonable.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.cc:204: : base::HexStringToUInt(input, output);
On 2014/02/11 23:52:30, wtc wrote:
> 
> Nit: we usually put an operator (such as ":") at the end of the previous line.
I
> am not sure if this is recommended by the Style Guide though.

Done.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.cc:206: DVLOG(1) << "IPv4 component greater than 255";
On 2014/02/11 23:52:30, wtc wrote:
> 
> For IPv6, you should also verify that *output <= 0xFFFF.

Done.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.cc:211: }
On 2014/02/11 23:52:30, wtc wrote:
> 
> I think we should first check !ok, then check the is_ipv4_ && *output > 255u
> case. If base::StringToUint returns false, we should not assume we can use the
> output.

Good catch!

Done.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
File net/base/ip_pattern.h (right):

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.h:13: #include "net/base/address_list.h"
On 2014/02/11 23:52:30, wtc wrote:
> 
> Include "net/base/net_util.h" instead. You just need the definition of
> IPAddressNumber.

Done.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.h:26: bool Match(const IPAddressNumber& address) const;
On 2014/02/11 23:52:30, wtc wrote:
> 
> Nit: document these two methods.

Done.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.h:38: bool ValueTextToInt(const base::StringPiece& input,
uint32* output) const;
On 2014/02/11 23:52:30, wtc wrote:
> 
> All uses of uint32 in this class probably should be changed to uint16. Four
> hexadecimal digits represent two bytes, or uint16. It is possible that using
> uint32 is more convenient though.

It was a mistake... but uint32 is more convenient for both conversion and bounds
checking.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.h:41: // The |ip_mask_| indicates,for each component, if
this pattern requires an
On 2014/02/11 23:52:30, wtc wrote:
> 
> Nit: add a space before "for".

Done.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.h:58: ComponentPattern* first_component_pattern_;
On 2014/02/12 00:44:03, Ryan Hamilton wrote:
> Ditto here.  Instead of implementing the list traversal and element deletion
> manually in the destructor, using a list<ComponentPattern> would let you
simply
> say:
> 
> STLDeleteElements(&component_patterns_) in the destructor.
> 
> I think this would improve readability.

Done.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.h:59: DISALLOW_COPY_AND_ASSIGN(IPPattern);
On 2014/02/11 23:52:30, wtc wrote:
> 
> Nit: I suggest adding a blank line to separate these members.

Done.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.h:62: 
On 2014/02/11 23:52:30, wtc wrote:
> 
> Nit: delete one blank line.

Done.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/dns/mapped_i...
File net/dns/mapped_ip_resolver.cc (right):

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/dns/mapped_i...
net/dns/mapped_ip_resolver.cc:23: const CompletionCallback& original_callback,
On 2014/02/11 23:52:30, wtc wrote:
> 
> Why did you rename this parameter "original_callback"? I think it should be
> named "callback".

Done.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/dns/mapped_i...
File net/dns/mapped_ip_resolver.h (right):

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/dns/mapped_i...
net/dns/mapped_ip_resolver.h:19: // resolution response by inserting or
replacing IP addresses before returning
On 2014/02/11 23:52:30, wtc wrote:
> 
> You only implemented the "PREFACE" directive, right? So you can't really
replace
> IP addresses. Replacing means you also delete the replaced address.

I added clarifying comments.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/dns/mapped_i...
net/dns/mapped_ip_resolver.h:21: class NET_EXPORT MappedIPResolver : public
HostResolver {
As per offline discussion.... I parsed the names (that lead to this name)
differently.

I parsed MappingHostResolver
as a "Resolver" that performed "Mapping of Hosts"

Hence, I called tihs a MappingIPResolver
as a "Resolver" that performs "Mapping of IP addresses"

Your parsing seems to be that MappingHostResolver is
a "HostResolver" that performs some "Mapping"

To be honest, both alternatives for the original name are a bit clunky, and make
it hard to see a clear pattern.

I think your suggestion might be better... but it doesn't match well with the
existing class, which perhaps should then be:
HostMappingHostResolver
or
NameMappingHostResolver
so we could be:
IPMappingHostResolver

I could probably change the existing class name to move in sync if that is what
you'd like.

Comments?


On 2014/02/11 23:52:30, wtc wrote:
> 
> Nit: "IPResolver" is a poor name because there is no such thing as IP
> resolution. That is, an IP address is the result of host name resolution.
> 
> Maybe this should be named "IPMappingHostResolver".

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/dns/mapped_i...
net/dns/mapped_ip_resolver.h:23: // Creates a MappedHostResolver that forwards
all of its requests through
On 2014/02/11 23:52:30, wtc wrote:
> 
> MappedHostResolver => MappedIPResolver

Done.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/dns/mapped_i...
net/dns/mapped_ip_resolver.h:35: // Takes a comma separated list of rules, and
assigns them to this resolver.
On 2014/02/11 23:52:30, wtc wrote:
> 
> Typo: comma => semicolon

Done.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/dns/mapped_i...
net/dns/mapped_ip_resolver.h:58: void ApplyRules(const CompletionCallback&
original_cb,
On 2014/02/11 23:52:30, wtc wrote:
> 
> Nit: it may be nice to be verbose and name this parameter "original_callback".

Done.

[jar (doing other things), 2014-02-15 21:14:59]

Wan Teh: Great and detailed review!!!! Thanks! PTAL

Ryan: switched to using std::vector: PTAL

[Ryan Hamilton, 2014-02-19 00:11:48]

I'm happy to let eroman run with the review of this CL, so I just have a few
cosmetic comments.  Thanks for switching to std::vector.

https://codereview.chromium.org/156963003/diff/1270001/net/base/ip_mapping_ru...
File net/base/ip_mapping_rules.cc (right):

https://codereview.chromium.org/156963003/diff/1270001/net/base/ip_mapping_ru...
net/base/ip_mapping_rules.cc:46: ::STLDeleteElements(&rules_);
nit: do you need :: here?

https://codereview.chromium.org/156963003/diff/1270001/net/base/ip_mapping_ru...
File net/base/ip_mapping_rules.h (right):

https://codereview.chromium.org/156963003/diff/1270001/net/base/ip_mapping_ru...
net/base/ip_mapping_rules.h:79: class MapRule;  // Forward declaration.
nit: Since the list is called RuleList and there are no other rule types I'd
probably just named this "Rule" (since this is an inner class, there's no naming
confusion)

https://codereview.chromium.org/156963003/diff/1270001/net/base/ip_mapping_ru...
net/base/ip_mapping_rules.h:82: // Singly linked list of rules.  We own (and
destroy) all rules by walking
nit: I think this comment is perhaps a bit confusing. "We own .. all rules by
walking the list"   Perhaps just comment on ownership?

https://codereview.chromium.org/156963003/diff/1270001/net/base/ip_mapping_ru...
net/base/ip_mapping_rules.h:82: // Singly linked list of rules.  We own (and
destroy) all rules by walking
nit: this is no longer a singly linked list, right (it's now a vector)

https://codereview.chromium.org/156963003/diff/1270001/net/base/ip_pattern.h
File net/base/ip_pattern.h (right):

https://codereview.chromium.org/156963003/diff/1270001/net/base/ip_pattern.h#...
net/base/ip_pattern.h:36: typedef std::vector<std::string> Strings;
nit: is this used?

[wtc, 2014-02-19 00:24:11]

Patch set 13 LGTM.

I only reviewed the diffs between patch sets 11 and 13.

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
File net/base/ip_pattern.cc (right):

https://chromiumcodereview.appspot.com/156963003/diff/950001/net/base/ip_patt...
net/base/ip_pattern.cc:139: if ((*component_it)[component_it->size() - 1] !=
']') {

On 2014/02/15 21:14:56, jar wrote:
[...]
> and I had to go back to the original form.

Sorry. front() and back() are only available in C++11.

https://chromiumcodereview.appspot.com/156963003/diff/1270001/net/base/ip_map...
File net/base/ip_mapping_rules.cc (right):

https://chromiumcodereview.appspot.com/156963003/diff/1270001/net/base/ip_map...
net/base/ip_mapping_rules.cc:46: ::STLDeleteElements(&rules_);

Nit: we don't usually use :: in this case.

Also on lines 109 and 115.

https://chromiumcodereview.appspot.com/156963003/diff/1270001/net/base/ip_map...
net/base/ip_mapping_rules.cc:83: DVLOG(1) << "Uknown directive: " <<
rule_string;

Typo: Uknown => Unknown

https://chromiumcodereview.appspot.com/156963003/diff/1270001/net/base/ip_map...
File net/base/ip_mapping_rules.h (right):

https://chromiumcodereview.appspot.com/156963003/diff/1270001/net/base/ip_map...
net/base/ip_mapping_rules.h:16: class AddressList;  // Forward declaration.

Nit: the "Forward declaration" comment is not necessary. This is well understood
in our source code.

Also remove the comment on line 79.

https://chromiumcodereview.appspot.com/156963003/diff/1270001/net/base/ip_map...
net/base/ip_mapping_rules.h:82: // Singly linked list of rules.  We own (and
destroy) all rules by walking

Remove "Singly linked". This is no longer true when you use std:;vector.

I think "by walking the list" can also be removed.

https://chromiumcodereview.appspot.com/156963003/diff/1270001/net/base/ip_pat...
File net/base/ip_pattern.cc (right):

https://chromiumcodereview.appspot.com/156963003/diff/1270001/net/base/ip_pat...
net/base/ip_pattern.cc:56: ::STLDeleteElements(&component_patterns_);

Nit: omit :: in this case.

https://chromiumcodereview.appspot.com/156963003/diff/1270001/net/base/ip_pat...
net/base/ip_pattern.cc:172: DVLOG(1) << "Could not convert value to number: " <<
input;

This is missing a "return false" statement.

[jar (doing other things), 2014-02-19 02:47:03]

jam: I need a (mostly rubber stamp?) owner review for chrome_switches.* and
io_thread.cc

The switches are placed almost adjacent to the similar switches that are
processed in the same section of io_thread.cc.

https://codereview.chromium.org/156963003/diff/1270001/net/base/ip_mapping_ru...
File net/base/ip_mapping_rules.cc (right):

https://codereview.chromium.org/156963003/diff/1270001/net/base/ip_mapping_ru...
net/base/ip_mapping_rules.cc:46: ::STLDeleteElements(&rules_);
On 2014/02/19 00:24:11, wtc wrote:
> 
> Nit: we don't usually use :: in this case.
> 
> Also on lines 109 and 115.

MSVC *was* complaining (when I didn't qualify the name).... but now I'm able to
get rid of the colons... so...

DONE

https://codereview.chromium.org/156963003/diff/1270001/net/base/ip_mapping_ru...
net/base/ip_mapping_rules.cc:83: DVLOG(1) << "Uknown directive: " <<
rule_string;
On 2014/02/19 00:24:11, wtc wrote:
> 
> Typo: Uknown => Unknown

Done.

https://codereview.chromium.org/156963003/diff/1270001/net/base/ip_mapping_ru...
File net/base/ip_mapping_rules.h (right):

https://codereview.chromium.org/156963003/diff/1270001/net/base/ip_mapping_ru...
net/base/ip_mapping_rules.h:16: class AddressList;  // Forward declaration.
On 2014/02/19 00:24:11, wtc wrote:
> 
> Nit: the "Forward declaration" comment is not necessary. This is well
understood
> in our source code.
> 
> Also remove the comment on line 79.

Done.

https://codereview.chromium.org/156963003/diff/1270001/net/base/ip_mapping_ru...
net/base/ip_mapping_rules.h:79: class MapRule;  // Forward declaration.
On 2014/02/19 00:11:48, Ryan Hamilton wrote:
> nit: Since the list is called RuleList and there are no other rule types I'd
> probably just named this "Rule" (since this is an inner class, there's no
naming
> confusion)

Done.

https://codereview.chromium.org/156963003/diff/1270001/net/base/ip_mapping_ru...
net/base/ip_mapping_rules.h:82: // Singly linked list of rules.  We own (and
destroy) all rules by walking
On 2014/02/19 00:24:11, wtc wrote:
> 
> Remove "Singly linked". This is no longer true when you use std:;vector.
> 
> I think "by walking the list" can also be removed.

Done.

https://codereview.chromium.org/156963003/diff/1270001/net/base/ip_pattern.cc
File net/base/ip_pattern.cc (right):

https://codereview.chromium.org/156963003/diff/1270001/net/base/ip_pattern.cc...
net/base/ip_pattern.cc:56: ::STLDeleteElements(&component_patterns_);
On 2014/02/19 00:24:11, wtc wrote:
> 
> Nit: omit :: in this case.

Done.

https://codereview.chromium.org/156963003/diff/1270001/net/base/ip_pattern.cc...
net/base/ip_pattern.cc:172: DVLOG(1) << "Could not convert value to number: " <<
input;
On 2014/02/19 00:24:11, wtc wrote:
> 
> This is missing a "return false" statement.

Done.

https://codereview.chromium.org/156963003/diff/1270001/net/base/ip_pattern.h
File net/base/ip_pattern.h (right):

https://codereview.chromium.org/156963003/diff/1270001/net/base/ip_pattern.h#...
net/base/ip_pattern.h:36: typedef std::vector<std::string> Strings;
On 2014/02/19 00:11:48, Ryan Hamilton wrote:
> nit: is this used?

Nope.  I switched to StringPiece.

Deleted include.

[wtc, 2014-02-19 17:59:30]

Patch set 14 LGTM.

[jar (doing other things), 2014-02-19 20:49:32]

darin: Can you give a rubber stamp on chrome_switches.* and io_thread? jam@ is
OOO.

[sky, 2014-02-19 23:50:49]

LGTM for io_thread. I'm not an owner of content/public.

[Charlie Reis, 2014-02-19 23:55:31]

content/ LGTM

[jar (doing other things), 2014-02-20 02:23:46]

The CQ bit was checked by jar@chromium.org

[commit-bot: I haz the power, 2014-02-20 02:31:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jar@chromium.org/156963003/1510001

[commit-bot: I haz the power, 2014-02-20 05:20:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jar@chromium.org/156963003/1510001

[commit-bot: I haz the power, 2014-02-20 09:20:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jar@chromium.org/156963003/1510001

[commit-bot: I haz the power, 2014-02-20 12:36:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jar@chromium.org/156963003/1510001

[commit-bot: I haz the power, 2014-02-20 17:25:55]

Change committed as 252289