autofill: switch autofill_regexes to RE2 library

components/autofill/core/common/autofill_regexes.cc

Index: components/autofill/core/common/autofill_regexes.cc
diff --git a/components/autofill/core/common/autofill_regexes.cc b/components/autofill/core/common/autofill_regexes.cc
index cdea63cb5c83c1ab3bca8008a6fa2fe0c44b7a73..f5e502ec61f5410be1eaa38717986799f04c4b0e 100644
--- a/components/autofill/core/common/autofill_regexes.cc
+++ b/components/autofill/core/common/autofill_regexes.cc
@@ -9,7 +9,8 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/singleton.h"
 #include "base/strings/string16.h"
-#include "third_party/icu/source/i18n/unicode/regex.h"
+#include "base/strings/utf_string_conversions.h"
+#include "third_party/re2/re2/re2.h"
 
 namespace {
 
@@ -19,7 +20,7 @@ class AutofillRegexes {
   static AutofillRegexes* GetInstance();
 
   // Returns the compiled regex matcher corresponding to |pattern|.
-  icu::RegexMatcher* GetMatcher(const base::string16& pattern);
+  re2::RE2* GetMatcher(const base::string16& pattern);
 
  private:
   AutofillRegexes();
@@ -27,8 +28,7 @@ class AutofillRegexes {
   friend struct base::DefaultSingletonTraits<AutofillRegexes>;
 
   // Maps patterns to their corresponding regex matchers.
-  base::ScopedPtrHashMap<base::string16, scoped_ptr<icu::RegexMatcher>>
-      matchers_;
+  base::ScopedPtrHashMap<base::string16, scoped_ptr<re2::RE2>> matchers_;
 
   DISALLOW_COPY_AND_ASSIGN(AutofillRegexes);
 };
@@ -44,16 +44,10 @@ AutofillRegexes::AutofillRegexes() {
 AutofillRegexes::~AutofillRegexes() {
 }
 
-icu::RegexMatcher* AutofillRegexes::GetMatcher(const base::string16& pattern) {
+re2::RE2* AutofillRegexes::GetMatcher(const base::string16& pattern) {

[vabr (Chromium), 2015/11/18 09:26:36]

Tracing this to the callsites, it appears that we start with 8-bit strings,
convert them to 16-bit, and then here back to 8-bit. Please keep them 8-bit
throughout.

[tfarina, 2015/11/18 18:03:10]

I don't think that was a good suggestion. That requires many API changes and
required me to add a bunch of UTF16ToASCII to many places. I think this was
beyond the scope of this change. But what options did I have? I did anyway!

[Ilya Sherman, 2015/11/19 02:08:59]

I think you might have misunderstood Václav's suggestion.  The |pattern| starts
out as a UTF8 string in the codebase, whereas the |input| (below) typically
starts out as a UTF16 string.  So, it's appropriate to change the type for the
|pattern|, but not for the |input|.

[vabr (Chromium), 2015/11/19 08:09:04]

@tfarina -- sorry that I was unclear; I really meant just |pattern|, as Ilya
already explained.

Please revert the changes for |input| below, which should stay 16-bit, just the
patterns should be 8-bit.

Also, for the future, if you disagree with a suggestion, it is certainly OK to
push back and explain why you push back. That will spare you coding back and
forth.

[tfarina, 2015/11/20 19:03:37]

Ilya, can I do this API change in a follow up? I don't think it is appropriate
to do within this patch. I would like to keep it focused on re2 switch.

[Ilya Sherman, 2015/11/20 22:27:59]

I'd prefer that you make the change as part of this CL; but if you choose not
to, please at least add TODOs to come back to it, in the spots that I've pointed
out.

   auto it = matchers_.find(pattern);
   if (it == matchers_.end()) {
-    const icu::UnicodeString icu_pattern(pattern.data(), pattern.length());
-
-    UErrorCode status = U_ZERO_ERROR;
-    scoped_ptr<icu::RegexMatcher> matcher(
-        new icu::RegexMatcher(icu_pattern, UREGEX_CASE_INSENSITIVE, status));
-    DCHECK(U_SUCCESS(status));
-
+    scoped_ptr<re2::RE2> matcher(new re2::RE2(base::UTF16ToUTF8(pattern)));
     auto result = matchers_.add(pattern, matcher.Pass());
     DCHECK(result.second);
     it = result.first;
@@ -67,15 +61,8 @@ namespace autofill {
 
 bool MatchesPattern(const base::string16& input,
                     const base::string16& pattern) {
-  icu::RegexMatcher* matcher =
-      AutofillRegexes::GetInstance()->GetMatcher(pattern);
-  icu::UnicodeString icu_input(input.data(), input.length());
-  matcher->reset(icu_input);
-
-  UErrorCode status = U_ZERO_ERROR;
-  UBool match = matcher->find(0, status);
-  DCHECK(U_SUCCESS(status));
-  return match == TRUE;
+  re2::RE2* matcher = AutofillRegexes::GetInstance()->GetMatcher(pattern);
+  return matcher->ok();

[Ilya Sherman, 2015/11/18 06:48:50]

Hmm, it looks like you aren't using the |input| at all.  Do the tests pass with
your changes?  If they do, seems like we need to add test coverage for this
code...

[vabr (Chromium), 2015/11/18 09:26:36]

+1, ok() only checks if the matcher was created without errors (so that should
appear between lines 50 and 51 instead.
Here you probably want to call RE2::FullMatch(input, *matcher).

[tfarina, 2015/11/18 18:03:10]

Done.

 }
 
 }  // namespace autofill