Switch the offset conversion routines from...

net/base/net_util_unittest.cc

Index: net/base/net_util_unittest.cc
===================================================================
--- net/base/net_util_unittest.cc	(revision 221015)
+++ net/base/net_util_unittest.cc	(working copy)
@@ -460,18 +460,20 @@
     size_t offset = cases[i].input_offset;
     base::string16 formatted_url = FormatUrl(url, languages, format_types,
                                        unescape_rules, NULL, NULL, &offset);
-    VerboseExpect(cases[i].output_offset, offset, url_string, i, formatted_url);
+    VerboseExpect(cases[i].output_offset, offset, url_string,
+                  cases[i].input_offset, formatted_url);
   }
 
-  size_t url_size = url_string.length();
+  size_t url_length = url_string.length();
   std::vector<size_t> offsets;
-  for (size_t i = 0; i < url_size + 1; ++i)
+  for (size_t i = 0; i <= url_length; ++i)
     offsets.push_back(i);
   base::string16 formatted_url = FormatUrlWithOffsets(url, languages,
       format_types, unescape_rules, NULL, NULL, &offsets);
-  for (size_t i = 0; i < url_size; ++i)
+  for (size_t i = 0; i < url_length; ++i)
     VerboseExpect(all_offsets[i], offsets[i], url_string, i, formatted_url);
-  VerboseExpect(kNpos, offsets[url_size], url_string, url_size, formatted_url);
+  VerboseExpect(formatted_url.length(), offsets[url_length], url_string,
+                url_length, formatted_url);
 }
 
 // Helper to strignize an IP number (used to define expectations).
@@ -2887,10 +2889,12 @@
 
 TEST(NetUtilTest, FormatUrlWithOffsets) {
   const AdjustOffsetCase null_cases[] = {
-    {0, base::string16::npos},
+    {0, 0},
+    {1, base::string16::npos},
   };
   CheckAdjustedOffsets(std::string(), "en", kFormatUrlOmitNothing,
-      UnescapeRule::NORMAL, null_cases, arraysize(null_cases), NULL);
+                       UnescapeRule::NORMAL, null_cases, arraysize(null_cases),
+                       NULL);
 
   const AdjustOffsetCase basic_cases[] = {
     {0, 0},

[msw, 2013/09/06 00:31:08]

Aren't all these cases except 27, 500000, and base::string16::npos covered by
the basic_offsets verification of all offsets? That seems to be a general trend
in all of these tests. Can you remove the AdjustOffsetCase arrays, and simply
supply the |all_offsets| array to CheckAdjustedOffsets. It would be easy to
append a few extra test cases within CheckAdjustedOffsets that should yield npos
for all callers, like (|url_length| + 1), (|url_length| + 2), (|url_length| +
50), and npos.

[Peter Kasting, 2013/09/06 19:04:36]

Yes.
I could, but then every case would have to declare two long "every possible
offset" arrays instead of one.  Both routes seem roughly as lame as each other. 
I had an "eww, gross" reaction when I first read this code, but there doesn't
seem to be an answer that (to me, at least) makes me say "ahh, that's so much
less redundant and crappy".  YMMV.

[msw, 2013/09/09 19:15:52]

As I suggested, you can just remove all AdjustOffsetCase arrays. Just send
CheckAdjustedOffsets the full list of expected offsets from an automatically
generated input array { 0, 1, 2 ... |url_string.length()| }, like it does now.
CheckAdjustedOffsets can also generate the input and output offsets for cases >
url_string.length() and npos. The result would be the same coverage with ~160
fewer lines; which seems decidedly better.

[Peter Kasting, 2013/09/09 23:43:02]

Done.

@@ -2902,19 +2906,22 @@
     {22, 22},
     {23, 23},
     {25, 25},
-    {26, base::string16::npos},
+    {26, 26},
+    {27, base::string16::npos},
     {500000, base::string16::npos},
     {base::string16::npos, base::string16::npos},
   };
-  const size_t basic_offsets[] = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13,
-     14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25};
+  const size_t basic_offsets[] = {

[msw, 2013/09/06 00:31:08]

Optionally, you could change the size_t arrays of expected offsets to int arrays
of expected offset deltas (if you think it'd be easier to read and worthwhile).
In this case they'd all be 0, but a case that does remove components, like
omit_auth_offsets_1, could be:
const size_t omit_auth_offsets_1[] = { 0, 0, 0, 0, 0, 0, 0, 0, kNpos, kNpos,
kNpos, kNpos, kNpos, kNpos, kNpos, -7, -7, -7, -7, -7, -7, -7, -7, -7, -7, -7,
-7, -7, -7, -7 };

[Peter Kasting, 2013/09/06 19:04:36]

Ultimately I find both somewhat confusing.

+    0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20,
+    21, 22, 23, 24, 25

[msw, 2013/09/06 00:31:08]

Shouldn't 26 now also be part of this array for the all-offsets check? Ditto
below for all the size_t arrays terminal values at the url length.

[Peter Kasting, 2013/09/06 19:04:36]

CheckAdjustedOffsets() checks this value automatically, so while we could write
the code such that we explicitly declared that at all the callsites instead, it
wouldn't gain us anything.

+  };
   CheckAdjustedOffsets("http://www.google.com/foo/", "en",
                        kFormatUrlOmitNothing, UnescapeRule::NORMAL, basic_cases,
                        arraysize(basic_cases), basic_offsets);
 
   const AdjustOffsetCase omit_auth_cases_1[] = {
     {6, 6},

[msw, 2013/09/06 00:31:08]

Again, all of these cases are redundant with the omit_auth_offsets_1
verification of all offsets. Hopefully you can simplify these tests a little;
I've removed other pending comments to the same effect after expanding on these
first two.

-    {7, base::string16::npos},
+    {7, 7},
     {8, base::string16::npos},
     {10, base::string16::npos},
     {12, base::string16::npos},
@@ -2922,40 +2929,48 @@
     {15, 7},
     {25, 17},
   };
-  const size_t omit_auth_offsets_1[] = {0, 1, 2, 3, 4, 5, 6, kNpos, kNpos,
-      kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, 7, 8, 9, 10, 11, 12, 13, 14, 15,
-      16, 17, 18, 19, 20, 21};
+  const size_t omit_auth_offsets_1[] = {
+    0, 1, 2, 3, 4, 5, 6, 7, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, 7,
+    8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21
+  };
   CheckAdjustedOffsets("http://foo:bar@www.google.com/", "en",
-      kFormatUrlOmitUsernamePassword, UnescapeRule::NORMAL, omit_auth_cases_1,
-      arraysize(omit_auth_cases_1), omit_auth_offsets_1);
+                       kFormatUrlOmitUsernamePassword, UnescapeRule::NORMAL,
+                       omit_auth_cases_1, arraysize(omit_auth_cases_1),
+                       omit_auth_offsets_1);
 
   const AdjustOffsetCase omit_auth_cases_2[] = {
     {9, base::string16::npos},
     {11, 7},
   };
-  const size_t omit_auth_offsets_2[] = {0, 1, 2, 3, 4, 5, 6, kNpos, kNpos,
-      kNpos, kNpos, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21};
+  const size_t omit_auth_offsets_2[] = {
+    0, 1, 2, 3, 4, 5, 6, 7, kNpos, kNpos, kNpos, 7, 8, 9, 10, 11, 12, 13, 14,
+    15, 16, 17, 18, 19, 20, 21
+  };
   CheckAdjustedOffsets("http://foo@www.google.com/", "en",
-      kFormatUrlOmitUsernamePassword, UnescapeRule::NORMAL, omit_auth_cases_2,
-      arraysize(omit_auth_cases_2), omit_auth_offsets_2);
+                       kFormatUrlOmitUsernamePassword, UnescapeRule::NORMAL,
+                       omit_auth_cases_2, arraysize(omit_auth_cases_2),
+                       omit_auth_offsets_2);
 
   // "http://foo\x30B0:\x30B0bar@www.google.com"

[msw, 2013/09/06 00:31:08]

nit: can you expand on this comment, I guess this is the expected formatted URL,
but saying so explicitly (perhaps moving this to be right before the input URL)
would clarify its meaning. Ditto for the other similar comments below.

[Peter Kasting, 2013/09/06 19:04:36]

OK, I expanded these slightly and moved them.

   const AdjustOffsetCase dont_omit_auth_cases[] = {
     {0, 0},
-    /*{3, base::string16::npos},
-    {7, 0},
-    {11, 4},
-    {12, base::string16::npos},
-    {20, 5},
-    {24, 9},*/
+    {11, base::string16::npos},
+    {19, 11},
+    {20, 12},
+    {21, base::string16::npos},
+    {29, 13},
+    {33, 17},
   };
-  const size_t dont_omit_auth_offsets[] = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10,
-      kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, 11, 12, kNpos,
-      kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, 13, 14, 15, 16, 17, 18,
-      19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31};
+  const size_t dont_omit_auth_offsets[] = {
+    0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos,
+    kNpos, kNpos, 11, 12, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos,
+    kNpos, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29,
+    30, 31
+  };
   CheckAdjustedOffsets("http://foo%E3%82%B0:%E3%82%B0bar@www.google.com/", "en",
-      kFormatUrlOmitNothing, UnescapeRule::NORMAL, dont_omit_auth_cases,
-      arraysize(dont_omit_auth_cases), dont_omit_auth_offsets);
+                       kFormatUrlOmitNothing, UnescapeRule::NORMAL,
+                       dont_omit_auth_cases, arraysize(dont_omit_auth_cases),
+                       dont_omit_auth_offsets);
 
   const AdjustOffsetCase view_source_cases[] = {
     {0, 0},
@@ -2964,18 +2979,20 @@
     {12, 12},
     {13, 13},
     {18, 18},
-    {19, base::string16::npos},
+    {19, 19},
     {20, base::string16::npos},
     {23, 19},
     {26, 22},
     {base::string16::npos, base::string16::npos},
   };
-  const size_t view_source_offsets[] = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11,
-      12, 13, 14, 15, 16, 17, 18, kNpos, kNpos, kNpos, kNpos, 19, 20, 21, 22,
-      23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33};
+  const size_t view_source_offsets[] = {
+    0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, kNpos,
+    kNpos, kNpos, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33
+  };
   CheckAdjustedOffsets("view-source:http://foo@www.google.com/", "en",
-      kFormatUrlOmitUsernamePassword, UnescapeRule::NORMAL, view_source_cases,
-      arraysize(view_source_cases), view_source_offsets);
+                       kFormatUrlOmitUsernamePassword, UnescapeRule::NORMAL,
+                       view_source_cases, arraysize(view_source_cases),
+                       view_source_offsets);
 
   // "http://\x671d\x65e5\x3042\x3055\x3072.jp/foo/"
   const AdjustOffsetCase idn_hostname_cases_1[] = {
@@ -2985,12 +3002,15 @@
     {25, 12},
     {30, 17},
   };
-  const size_t idn_hostname_offsets_1[] = {0, 1, 2, 3, 4, 5, 6, 7, kNpos, kNpos,
-      kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos,
-      kNpos, kNpos, kNpos, kNpos, kNpos, 12, 13, 14, 15, 16, 17, 18, 19};
+  const size_t idn_hostname_offsets_1[] = {
+    0, 1, 2, 3, 4, 5, 6, 7, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos,
+    kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, 12,
+    13, 14, 15, 16, 17, 18, 19
+  };
   CheckAdjustedOffsets("http://xn--l8jvb1ey91xtjb.jp/foo/", "ja",
-      kFormatUrlOmitNothing, UnescapeRule::NORMAL, idn_hostname_cases_1,
-      arraysize(idn_hostname_cases_1), idn_hostname_offsets_1);
+                       kFormatUrlOmitNothing, UnescapeRule::NORMAL,
+                       idn_hostname_cases_1, arraysize(idn_hostname_cases_1),
+                       idn_hostname_offsets_1);
 
   // "http://test.\x89c6\x9891.\x5317\x4eac\x5927\x5b78.test/"
   const AdjustOffsetCase idn_hostname_cases_2[] = {
@@ -3008,13 +3028,15 @@
     {42, 20},
     {45, 23},
     {46, 24},
-    {47, base::string16::npos},
+    {47, 25},
     {base::string16::npos, base::string16::npos},
   };
-  const size_t idn_hostname_offsets_2[] = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11,
-      12, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos,
-      kNpos, 14, 15, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos,
-      kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, 19, 20, 21, 22, 23, 24};
+  const size_t idn_hostname_offsets_2[] = {
+    0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, kNpos, kNpos, kNpos, kNpos, kNpos,
+    kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, 14, 15, kNpos, kNpos, kNpos,
+    kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos,
+    kNpos, 19, 20, 21, 22, 23, 24
+  };
   CheckAdjustedOffsets("http://test.xn--cy2a840a.xn--1lq90ic7f1rc.test/",
                        "zh-CN", kFormatUrlOmitNothing, UnescapeRule::NORMAL,
                        idn_hostname_cases_2, arraysize(idn_hostname_cases_2),
@@ -3031,14 +3053,15 @@
     {59, 33},
     {60, base::string16::npos},
     {67, base::string16::npos},
-    {68, base::string16::npos},
+    {68, 34},
   };
-  const size_t unescape_offsets[] = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12,
-      13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, kNpos, kNpos, 26, 27,
-      28, 29, 30, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, 31,
-      kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, 32, kNpos, kNpos,
-      kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, 33, kNpos, kNpos, kNpos, kNpos,
-      kNpos, kNpos, kNpos, kNpos};
+  const size_t unescape_offsets[] = {
+    0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20,
+    21, 22, 23, 24, 25, kNpos, kNpos, 26, 27, 28, 29, 30, kNpos, kNpos, kNpos,
+    kNpos, kNpos, kNpos, kNpos, kNpos, 31, kNpos, kNpos, kNpos, kNpos, kNpos,
+    kNpos, kNpos, kNpos, 32, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos,
+    kNpos, 33, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos
+  };
   CheckAdjustedOffsets(
       "http://www.google.com/foo%20bar/%E3%82%B0%E3%83%BC%E3%82%B0%E3%83%AB",
       "en", kFormatUrlOmitNothing, UnescapeRule::SPACES, unescape_cases,
@@ -3052,48 +3075,55 @@
     {34, 32},
     {35, base::string16::npos},
     {37, 33},
-    {38, base::string16::npos},
+    {38, 34},
   };
-  const size_t ref_offsets[] = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13,
-      14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31,
-      kNpos, kNpos, 32, kNpos, kNpos, 33};
+  const size_t ref_offsets[] = {
+    0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20,
+    21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, kNpos, kNpos, 32, kNpos, kNpos,
+    33
+  };
   CheckAdjustedOffsets(
       "http://www.google.com/foo.html#\xE3\x82\xB0\xE3\x82\xB0z", "en",
       kFormatUrlOmitNothing, UnescapeRule::NORMAL, ref_cases,
       arraysize(ref_cases), ref_offsets);
 
   const AdjustOffsetCase omit_http_cases[] = {
-    {0, base::string16::npos},
+    {0, 0},
     {3, base::string16::npos},
     {7, 0},
     {8, 1},
   };
-  const size_t omit_http_offsets[] = {kNpos, kNpos, kNpos, kNpos, kNpos, kNpos,
-      kNpos, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14};
-  CheckAdjustedOffsets("http://www.google.com/", "en",
-      kFormatUrlOmitHTTP, UnescapeRule::NORMAL, omit_http_cases,
-      arraysize(omit_http_cases), omit_http_offsets);
+  const size_t omit_http_offsets[] = {
+    0, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9,
+    10, 11, 12, 13, 14
+  };
+  CheckAdjustedOffsets("http://www.google.com/", "en", kFormatUrlOmitHTTP,
+                       UnescapeRule::NORMAL, omit_http_cases,
+                       arraysize(omit_http_cases), omit_http_offsets);
 
   const AdjustOffsetCase omit_http_start_with_ftp_cases[] = {
     {0, 0},
     {3, 3},
     {8, 8},
   };
-  const size_t omit_http_start_with_ftp_offsets[] = {0, 1, 2, 3, 4, 5, 6, 7, 8,
-      9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21};
+  const size_t omit_http_start_with_ftp_offsets[] = {
+    0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21
+  };
   CheckAdjustedOffsets("http://ftp.google.com/", "en", kFormatUrlOmitHTTP,
                        UnescapeRule::NORMAL, omit_http_start_with_ftp_cases,
                        arraysize(omit_http_start_with_ftp_cases),
                        omit_http_start_with_ftp_offsets);
 
   const AdjustOffsetCase omit_all_cases[] = {
+    {0, 0},
+    {3, base::string16::npos},
     {12, 0},
     {13, 1},
-    {0, base::string16::npos},
-    {3, base::string16::npos},
   };
-  const size_t omit_all_offsets[] = {kNpos, kNpos, kNpos, kNpos, kNpos, kNpos,
-      kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, 0, 1, 2, 3, 4, 5, 6, kNpos};
+  const size_t omit_all_offsets[] = {
+    0, kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, 0, kNpos, kNpos, kNpos, kNpos,
+    0, 1, 2, 3, 4, 5, 6, 7
+  };
   CheckAdjustedOffsets("http://user@foo.com/", "en", kFormatUrlOmitAll,
                        UnescapeRule::NORMAL, omit_all_cases,
                        arraysize(omit_all_cases), omit_all_offsets);