Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1453193002:
autofill: switch autofill_regexes to RE2 library (Closed)
|
Created:
5 years, 1 month ago by tfarina Modified:
5 years ago CC:
chromium-reviews, droger+watchlist_chromium.org, rouslan+autofill_chromium.org, blundell+watchlist_chromium.org, sdefresne+watchlist_chromium.org, browser-components-watch_chromium.org, jdonnelly+autofillwatch_chromium.org, bondd+autofillwatch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionautofill: switch autofill_regexes to RE2 library
This moves it away from using regex library from icu to the in-house re2
library that is known to have a better performance and be much more
lightweight and much less verbose to write code.
BUG=470065
TEST=components_unittests --gtest_filter=*Autofill*
R=vabr@chromium.org,isherman@chromium.org
Committed: https://crrev.com/2df535152b8023dc3dcbc7dd8c7f1a1e3d959e13
Cr-Commit-Position: refs/heads/master@{#363835}
Patch Set 1 #Patch Set 2 : deps #
Total comments: 9
Patch Set 3 : std::string #Patch Set 4 : fix #
Total comments: 1
Patch Set 5 : macros.h #
Total comments: 5
Patch Set 6 : case_insensitive #Patch Set 7 : add TODO #Patch Set 8 : input is UTF8 #
Total comments: 4
Patch Set 9 : pattern - ASCII- std::string #
Total comments: 3
Patch Set 10 : 14 tests failing #
Total comments: 14
Patch Set 11 : *Autofill* - all tests passed #
Total comments: 3
Patch Set 12 : lets see if the tests passes #Patch Set 13 : REBASE - to see if the skia errors go way #Patch Set 14 : another try #
Total comments: 2
Patch Set 15 : WORDBREAK macro #
Total comments: 6
Patch Set 16 : address reviews #
Total comments: 2
Messages
Total messages: 56 (8 generated)
PTAL. Is it correct? Am I missing some check?
https://codereview.chromium.org/1453193002/diff/20001/components/autofill/cor... File components/autofill/core/common/autofill_regexes.cc (right): https://codereview.chromium.org/1453193002/diff/20001/components/autofill/cor... components/autofill/core/common/autofill_regexes.cc:65: return matcher->ok(); 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...
Thanks for switching this one! I left some comments. Also, adding the tests as Ilya suggested would be really great. Cheers, Vaclav https://codereview.chromium.org/1453193002/diff/20001/components/autofill/cor... File components/autofill/core/common/autofill_regexes.cc (right): https://codereview.chromium.org/1453193002/diff/20001/components/autofill/cor... components/autofill/core/common/autofill_regexes.cc:47: re2::RE2* AutofillRegexes::GetMatcher(const base::string16& pattern) { 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. https://codereview.chromium.org/1453193002/diff/20001/components/autofill/cor... components/autofill/core/common/autofill_regexes.cc:65: return matcher->ok(); On 2015/11/18 06:48:50, Ilya Sherman wrote: > 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... +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).
PTAL! https://codereview.chromium.org/1453193002/diff/20001/components/autofill/cor... File components/autofill/core/common/autofill_regexes.cc (right): https://codereview.chromium.org/1453193002/diff/20001/components/autofill/cor... components/autofill/core/common/autofill_regexes.cc:47: re2::RE2* AutofillRegexes::GetMatcher(const base::string16& pattern) { On 2015/11/18 09:26:36, vabr (Chromium) wrote: > 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. 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! https://codereview.chromium.org/1453193002/diff/20001/components/autofill/cor... components/autofill/core/common/autofill_regexes.cc:65: return matcher->ok(); On 2015/11/18 09:26:36, vabr (Chromium) wrote: > On 2015/11/18 06:48:50, Ilya Sherman wrote: > > 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... > > +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). Done. https://codereview.chromium.org/1453193002/diff/60001/components/autofill/cor... File components/autofill/core/common/autofill_regexes_unittest.cc (right): https://codereview.chromium.org/1453193002/diff/60001/components/autofill/cor... components/autofill/core/common/autofill_regexes_unittest.cc:12: TEST(AutofillRegexesTest, AutofillRegexes) { Are these test cases enough Ilya? I'm not sure about what to test here, please, advice!
Thanks, Thiago, for working on this! https://codereview.chromium.org/1453193002/diff/20001/components/autofill/cor... File components/autofill/core/common/autofill_regexes.cc (right): https://codereview.chromium.org/1453193002/diff/20001/components/autofill/cor... components/autofill/core/common/autofill_regexes.cc:47: re2::RE2* AutofillRegexes::GetMatcher(const base::string16& pattern) { On 2015/11/18 18:03:10, tfarina wrote: > On 2015/11/18 09:26:36, vabr (Chromium) wrote: > > 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. > 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! 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|. https://codereview.chromium.org/1453193002/diff/80001/components/autofill/cor... File components/autofill/core/browser/validation.cc (right): https://codereview.chromium.org/1453193002/diff/80001/components/autofill/cor... components/autofill/core/browser/validation.cc:132: return MatchesPattern(base::UTF16ToASCII(text), kEmailPattern); Please note: UTF16ToASCII is generally unsafe for user-provided data. Instead, you should use UTF16ToUTF8. That said, this shouldn't be relevant for this CL, as you should simply preserve the string16 type for the |input| parameter. https://codereview.chromium.org/1453193002/diff/80001/components/autofill/cor... File components/autofill/core/common/autofill_regexes.cc (right): https://codereview.chromium.org/1453193002/diff/80001/components/autofill/cor... components/autofill/core/common/autofill_regexes.cc:62: re2::RE2* matcher = AutofillRegexes::GetInstance()->GetMatcher(pattern); Please add a TODO to verify whether it's still worthwhile to cache the regex objects. For example, """ TODO(isherman): Run performance tests to determine whether caching regex matchers is still useful now that we've switched from ICU to RE2. http://crbug.com/XXXXXX" https://codereview.chromium.org/1453193002/diff/80001/components/autofill/cor... components/autofill/core/common/autofill_regexes.cc:63: return re2::RE2::FullMatch(input, *matcher); Do the tests pass with this change? I would have expected PartialMatch rather than FullMatch.
Thanks Thiago. Some clarification below. Cheers, Vaclav https://codereview.chromium.org/1453193002/diff/20001/components/autofill/cor... File components/autofill/core/common/autofill_regexes.cc (right): https://codereview.chromium.org/1453193002/diff/20001/components/autofill/cor... components/autofill/core/common/autofill_regexes.cc:47: re2::RE2* AutofillRegexes::GetMatcher(const base::string16& pattern) { On 2015/11/19 02:08:59, Ilya Sherman wrote: > On 2015/11/18 18:03:10, tfarina wrote: > > On 2015/11/18 09:26:36, vabr (Chromium) wrote: > > > 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. > > 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! > > 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|. @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.
PTAL guys! https://codereview.chromium.org/1453193002/diff/20001/components/autofill/cor... File components/autofill/core/common/autofill_regexes.cc (right): https://codereview.chromium.org/1453193002/diff/20001/components/autofill/cor... components/autofill/core/common/autofill_regexes.cc:47: re2::RE2* AutofillRegexes::GetMatcher(const base::string16& pattern) { On 2015/11/19 02:08:59, Ilya Sherman wrote: > On 2015/11/18 18:03:10, tfarina wrote: > > On 2015/11/18 09:26:36, vabr (Chromium) wrote: > > > 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. > > 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! > > 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|. 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. https://codereview.chromium.org/1453193002/diff/80001/components/autofill/cor... File components/autofill/core/common/autofill_regexes.cc (right): https://codereview.chromium.org/1453193002/diff/80001/components/autofill/cor... components/autofill/core/common/autofill_regexes.cc:62: re2::RE2* matcher = AutofillRegexes::GetInstance()->GetMatcher(pattern); On 2015/11/19 02:09:00, Ilya Sherman wrote: > Please add a TODO to verify whether it's still worthwhile to cache the regex > objects. For example, > > """ TODO(isherman): Run performance tests to determine whether caching regex > matchers is still useful now that we've switched from ICU to RE2. > http://crbug.com/XXXXXX%22 Done. https://codereview.chromium.org/1453193002/diff/80001/components/autofill/cor... components/autofill/core/common/autofill_regexes.cc:63: return re2::RE2::FullMatch(input, *matcher); On 2015/11/19 02:09:00, Ilya Sherman wrote: > Do the tests pass with this change? I would have expected PartialMatch rather > than FullMatch. Now they do. Sorry for not getting to test this locally before. Now I did run them properly and they all passed!
Thanks, Thiago. Please take a look at the test failures for your current patch. It looks like the regex syntax needs to be updated to match RE2 expectations. In particular, the negative lookbehind operator "(?<!" is not supported by RE2. https://codereview.chromium.org/1453193002/diff/20001/components/autofill/cor... File components/autofill/core/common/autofill_regexes.cc (right): https://codereview.chromium.org/1453193002/diff/20001/components/autofill/cor... components/autofill/core/common/autofill_regexes.cc:47: re2::RE2* AutofillRegexes::GetMatcher(const base::string16& pattern) { On 2015/11/20 19:03:37, tfarina wrote: > On 2015/11/19 02:08:59, Ilya Sherman wrote: > > On 2015/11/18 18:03:10, tfarina wrote: > > > On 2015/11/18 09:26:36, vabr (Chromium) wrote: > > > > 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. > > > 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! > > > > 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|. > 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. 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. https://codereview.chromium.org/1453193002/diff/140001/components/autofill/co... File components/autofill/core/common/autofill_regexes.cc (right): https://codereview.chromium.org/1453193002/diff/140001/components/autofill/co... components/autofill/core/common/autofill_regexes.cc:47: re2::RE2* AutofillRegexes::GetMatcher(const base::string16& pattern) { Please either change this API to use std::string, or add a TODO to make this change later. https://codereview.chromium.org/1453193002/diff/140001/components/autofill/co... components/autofill/core/common/autofill_regexes.cc:67: const base::string16& pattern) { Please either change this API to use std::string for the |pattern| (but not the |input|), or add a TODO to make this change later.
Hi Thiago! I'm a bit confused by the name of the patch set 8. It says "input is UTF8", but the input remains UTF16. Was there any trouble with keeping the changes you already had, and just dropping them for the |pattern|? Cheers, Vaclav
On 2015/11/23 08:48:25, vabr (Chromium) wrote: > Hi Thiago! > > I'm a bit confused by the name of the patch set 8. It says "input is UTF8", but > the input remains UTF16. Was there any trouble with keeping the changes you > already had, and just dropping them for the |pattern|? > Please, refer to line 72 of autofill_regexes.cc and Ilyas's comment about my unsafe UTF16ToASCII. Thanks,
On 2015/11/23 13:01:10, tfarina wrote: > On 2015/11/23 08:48:25, vabr (Chromium) wrote: > > Hi Thiago! > > > > I'm a bit confused by the name of the patch set 8. It says "input is UTF8", > but > > the input remains UTF16. Was there any trouble with keeping the changes you > > already had, and just dropping them for the |pattern|? > > > Please, refer to line 72 of autofill_regexes.cc and Ilyas's comment about my > unsafe UTF16ToASCII. > > Thanks, Sorry, Thiago, I meant keeping |input| 16-bit and making |pattern| 8-bit, not vice versa as I mistakenly indicated in my last e-mail. I see that Ilya already pointed that out as well in https://codereview.chromium.org/1453193002/diff/140001/components/autofill/co.... I support that comment, and would like to encourage making that change in this CL instead of postponing it. Thanks! Vaclav
On 2015/11/23 13:07:25, vabr (Chromium) wrote: > I see that Ilya already pointed that out as well in > https://codereview.chromium.org/1453193002/diff/140001/components/autofill/co.... > I support that comment, and would like to encourage making that change in this > CL instead of postponing it. > I think there are more important issues to solve, like the negative lookbehind operator not supported by re2, than doing this 8-bit/16-bit changes.
On 2015/11/23 13:10:14, tfarina wrote: > On 2015/11/23 13:07:25, vabr (Chromium) wrote: > > I see that Ilya already pointed that out as well in > > > https://codereview.chromium.org/1453193002/diff/140001/components/autofill/co.... > > I support that comment, and would like to encourage making that change in this > > CL instead of postponing it. > > > I think there are more important issues to solve, like the negative lookbehind > operator not supported by re2, than doing this 8-bit/16-bit changes. I'm not saying you should only do the bit-width change. But given you already had that done in one of the earlier patches, it is worth it, and it is totally on-topic for this CL.
estade@chromium.org changed reviewers: + estade@chromium.org
https://codereview.chromium.org/1453193002/diff/140001/components/autofill/co... File components/autofill/core/common/autofill_regexes.cc (right): https://codereview.chromium.org/1453193002/diff/140001/components/autofill/co... components/autofill/core/common/autofill_regexes.cc:23: re2::RE2* GetMatcher(const base::string16& pattern); can you make this return a const ref?
Ilya, I will try to put up your suggestion for the lookbehind later tonight. In the meanwhile, please take a look at my reply below about the ascii change requested. https://codereview.chromium.org/1453193002/diff/140001/components/autofill/co... File components/autofill/core/common/autofill_regexes.cc (right): https://codereview.chromium.org/1453193002/diff/140001/components/autofill/co... components/autofill/core/common/autofill_regexes.cc:67: const base::string16& pattern) { On 2015/11/20 22:27:59, Ilya Sherman wrote: > Please either change this API to use std::string for the |pattern| (but not the > |input|), or add a TODO to make this change later. Done. But if I'm not wrong from looking at the logs there are more tests failing because of this change. IsStringASCII check fails. So I'm still not sure if this is a good suggestion.
https://codereview.chromium.org/1453193002/diff/160001/components/autofill/co... File components/autofill/core/browser/form_field.cc (right): https://codereview.chromium.org/1453193002/diff/160001/components/autofill/co... components/autofill/core/browser/form_field.cc:183: MatchesPattern(field->label, base::UTF16ToASCII(pattern))) { Yes, Ilya's suggestion is good :) This code is not :( should be base::UTF16ToUTF8
https://codereview.chromium.org/1453193002/diff/160001/components/autofill/co... File components/autofill/core/browser/form_field.cc (right): https://codereview.chromium.org/1453193002/diff/160001/components/autofill/co... components/autofill/core/browser/form_field.cc:183: MatchesPattern(field->label, base::UTF16ToASCII(pattern))) { On 2015/11/25 19:33:01, Evan Stade wrote: > Yes, Ilya's suggestion is good :) This code is not :( > > should be base::UTF16ToUTF8 Actually, there should not be any conversion here at all. Instead, you should make the |pattern| above also be an std::string, since that's what the patterns start out as. That said, Evan's comment is also worth keeping in mind: UTF16 is typically not safe to convert to ASCII; instead, you should convert it to UTF8. Again, this sort of conversion shouldn't be needed at all for this CL; but it's a good thing to keep in mind for the future.
https://codereview.chromium.org/1453193002/diff/160001/components/autofill/co... File components/autofill/core/browser/form_field.cc (right): https://codereview.chromium.org/1453193002/diff/160001/components/autofill/co... components/autofill/core/browser/form_field.cc:183: MatchesPattern(field->label, base::UTF16ToASCII(pattern))) { On 2015/11/25 22:18:14, Ilya Sherman wrote: > On 2015/11/25 19:33:01, Evan Stade wrote: > > Yes, Ilya's suggestion is good :) This code is not :( > > > > should be base::UTF16ToUTF8 > > Actually, there should not be any conversion here at all. Instead, you should > make the |pattern| above also be an std::string, since that's what the patterns > start out as. > > That said, Evan's comment is also worth keeping in mind: UTF16 is typically not > safe to convert to ASCII; instead, you should convert it to UTF8. Again, this > sort of conversion shouldn't be needed at all for this CL; but it's a good thing > to keep in mind for the future. Done. Though the changes are scaling.
Thanks, Thiago! This is getting pretty close =) https://codereview.chromium.org/1453193002/diff/180001/components/autofill/co... File components/autofill/core/browser/address_field.cc (right): https://codereview.chromium.org/1453193002/diff/180001/components/autofill/co... components/autofill/core/browser/address_field.cc:20: using base::UTF8ToUTF16; Are these needed? https://codereview.chromium.org/1453193002/diff/180001/components/autofill/co... components/autofill/core/browser/address_field.cc:192: !ParseFieldSpecifics(scanner, kAddressLinesExtraRe, Please use kAddressLine2LabelRe here, as the code did before. https://codereview.chromium.org/1453193002/diff/180001/components/autofill/co... components/autofill/core/browser/address_field.cc:250: size_t saved_cursor = scanner->SaveCursor(); Please add a comment above this block of code like "Ignore spurious matches for 'United States'". https://codereview.chromium.org/1453193002/diff/180001/components/autofill/co... components/autofill/core/browser/address_field.cc:362: scanner->RewindTo(saved_cursor); This logic is not correct. I still think it should be basically the same as above, but returning RESULT_MATCH_NONE instead of false if the string "United States" is found. https://codereview.chromium.org/1453193002/diff/180001/components/autofill/co... File components/autofill/core/browser/credit_card_field.cc (right): https://codereview.chromium.org/1453193002/diff/180001/components/autofill/co... components/autofill/core/browser/credit_card_field.cc:14: #include "base/strings/utf_string_conversions.h" Is this include still needed? https://codereview.chromium.org/1453193002/diff/180001/components/autofill/co... components/autofill/core/browser/credit_card_field.cc:34: bool FindConsecutiveStrings(const std::vector<base::string16>& regex_needles, Please make this type also be std::string https://codereview.chromium.org/1453193002/diff/180001/components/autofill/co... File components/autofill/core/browser/form_field.cc (right): https://codereview.chromium.org/1453193002/diff/180001/components/autofill/co... components/autofill/core/browser/form_field.cc:15: #include "base/strings/utf_string_conversions.h" nit: Is this include still needed? Please check this for other files in this CL as well.
Description was changed from ========== autofill: switch autofill_regexes to RE2 library This moves it away from using regex library from icu to the in-house re2 library that is known to have a better performance and be much more lightweight and much less verbose to write code. BUG=470065 TEST=components_unittests --gtest_filter=AutofillRegexesTest.* R=vabr@chromium.org,isherman@chromium.org ========== to ========== autofill: switch autofill_regexes to RE2 library This moves it away from using regex library from icu to the in-house re2 library that is known to have a better performance and be much more lightweight and much less verbose to write code. BUG=470065 TEST=components_unittests --gtest_filter=*Autofill.* R=vabr@chromium.org,isherman@chromium.org ==========
Description was changed from ========== autofill: switch autofill_regexes to RE2 library This moves it away from using regex library from icu to the in-house re2 library that is known to have a better performance and be much more lightweight and much less verbose to write code. BUG=470065 TEST=components_unittests --gtest_filter=*Autofill.* R=vabr@chromium.org,isherman@chromium.org ========== to ========== autofill: switch autofill_regexes to RE2 library This moves it away from using regex library from icu to the in-house re2 library that is known to have a better performance and be much more lightweight and much less verbose to write code. BUG=470065 TEST=components_unittests --gtest_filter=*Autofill* R=vabr@chromium.org,isherman@chromium.org ==========
Thanks for the support Ilya! PTAL. https://codereview.chromium.org/1453193002/diff/180001/components/autofill/co... File components/autofill/core/browser/address_field.cc (right): https://codereview.chromium.org/1453193002/diff/180001/components/autofill/co... components/autofill/core/browser/address_field.cc:20: using base::UTF8ToUTF16; On 2015/11/26 02:25:09, Ilya Sherman wrote: > Are these needed? Done. https://codereview.chromium.org/1453193002/diff/180001/components/autofill/co... components/autofill/core/browser/address_field.cc:192: !ParseFieldSpecifics(scanner, kAddressLinesExtraRe, On 2015/11/26 02:25:09, Ilya Sherman wrote: > Please use kAddressLine2LabelRe here, as the code did before. ops, that is what happens when you try to make patches sleepy and at 01:30 AM :/ https://codereview.chromium.org/1453193002/diff/180001/components/autofill/co... components/autofill/core/browser/address_field.cc:250: size_t saved_cursor = scanner->SaveCursor(); On 2015/11/26 02:25:09, Ilya Sherman wrote: > Please add a comment above this block of code like "Ignore spurious matches for > 'United States'". Done. https://codereview.chromium.org/1453193002/diff/180001/components/autofill/co... components/autofill/core/browser/address_field.cc:362: scanner->RewindTo(saved_cursor); On 2015/11/26 02:25:09, Ilya Sherman wrote: > This logic is not correct. I still think it should be basically the same as > above, but returning RESULT_MATCH_NONE instead of false if the string "United > States" is found. Done. https://codereview.chromium.org/1453193002/diff/180001/components/autofill/co... File components/autofill/core/browser/credit_card_field.cc (right): https://codereview.chromium.org/1453193002/diff/180001/components/autofill/co... components/autofill/core/browser/credit_card_field.cc:14: #include "base/strings/utf_string_conversions.h" On 2015/11/26 02:25:09, Ilya Sherman wrote: > Is this include still needed? Done. https://codereview.chromium.org/1453193002/diff/180001/components/autofill/co... components/autofill/core/browser/credit_card_field.cc:34: bool FindConsecutiveStrings(const std::vector<base::string16>& regex_needles, On 2015/11/26 02:25:09, Ilya Sherman wrote: > Please make this type also be std::string Done. https://codereview.chromium.org/1453193002/diff/180001/components/autofill/co... File components/autofill/core/browser/form_field.cc (right): https://codereview.chromium.org/1453193002/diff/180001/components/autofill/co... components/autofill/core/browser/form_field.cc:15: #include "base/strings/utf_string_conversions.h" On 2015/11/26 02:25:09, Ilya Sherman wrote: > nit: Is this include still needed? Please check this for other files in this CL > as well. Done.
Thanks, Thiago, LGTM! You improved this code very much, and even cut 60+ lines out of it! :) Cheers, Vaclav https://codereview.chromium.org/1453193002/diff/200001/components/autofill/co... File components/autofill/core/browser/credit_card_field.cc (right): https://codereview.chromium.org/1453193002/diff/200001/components/autofill/co... components/autofill/core/browser/credit_card_field.cc:33: bool FindConsecutiveStrings(const std::vector<std::string>& regex_needles, Please #include <string> for std::string. https://codereview.chromium.org/1453193002/diff/200001/components/autofill/co... File components/autofill/core/browser/form_field.h (right): https://codereview.chromium.org/1453193002/diff/200001/components/autofill/co... components/autofill/core/browser/form_field.h:72: const std::string& pattern, Please #include <string> for std::string.
LGTM % a final nit. Thanks, Thiago! https://codereview.chromium.org/1453193002/diff/200001/components/autofill/co... File components/autofill/core/browser/address_field.cc (right): https://codereview.chromium.org/1453193002/diff/200001/components/autofill/co... components/autofill/core/browser/address_field.cc:352: size_t saved_cursor = scanner->SaveCursor(); Please add the "// Ignore spurious matches for "United States"." comment above this line as well.
Still have to fix the test failure of FormField::Match. components/autofill/core/browser/form_field_unittest.cc:93 Value of: FormField::Match(&field, "\\bcr\\b", FormField::MATCH_LABEL) Actual: true Expected: false
isherman@chromium.org changed reviewers: + mathp@chromium.org
On 2015/11/26 21:04:20, tfarina wrote: > Still have to fix the test failure of FormField::Match. > > components/autofill/core/browser/form_field_unittest.cc:93 > > Value of: FormField::Match(&field, "\\bcr\\b", FormField::MATCH_LABEL) > Actual: true > Expected: false Hmm, that's a problem. It looks like RE2 only supports ASCII word-boundaries, which doesn't work very well for regexes in other languages. I'm not sure how we'd fix this, actually -- it seems much harder to work around than the lack of negative lookbehind. Václav, Mathieu, Evan -- any ideas? In the meantime, not lgtm. We shouldn't land this CL if it'll regress our heuristics in non-English locales.
On 2015/11/26 21:32:33, Ilya Sherman wrote: > On 2015/11/26 21:04:20, tfarina wrote: > > Still have to fix the test failure of FormField::Match. > > > > components/autofill/core/browser/form_field_unittest.cc:93 > > > > Value of: FormField::Match(&field, "\\bcr\\b", FormField::MATCH_LABEL) > > Actual: true > > Expected: false > > Hmm, that's a problem. It looks like RE2 only supports ASCII word-boundaries, > which doesn't work very well for regexes in other languages. I'm not sure how > we'd fix this, actually -- it seems much harder to work around than the lack of > negative lookbehind. Václav, Mathieu, Evan -- any ideas? > > In the meantime, not lgtm. We shouldn't land this CL if it'll regress our > heuristics in non-English locales. I think we could change every \b to (\A|\z|[^\pL]) ?
On 2015/12/01 00:00:29, Evan Stade wrote: > On 2015/11/26 21:32:33, Ilya Sherman wrote: > > On 2015/11/26 21:04:20, tfarina wrote: > > > Still have to fix the test failure of FormField::Match. > > > > > > components/autofill/core/browser/form_field_unittest.cc:93 > > > > > > Value of: FormField::Match(&field, "\\bcr\\b", FormField::MATCH_LABEL) > > > Actual: true > > > Expected: false > > > > Hmm, that's a problem. It looks like RE2 only supports ASCII word-boundaries, > > which doesn't work very well for regexes in other languages. I'm not sure how > > we'd fix this, actually -- it seems much harder to work around than the lack > of > > negative lookbehind. Václav, Mathieu, Evan -- any ideas? > > > > In the meantime, not lgtm. We shouldn't land this CL if it'll regress our > > heuristics in non-English locales. > > I think we could change every \b to (\A|\z|[^\pL]) ? Yes, I think you're right. I think a slightly briefer way to write that is (\A|\z|\PL). That's a little bit icky to read, but I think it will give the correct behavior. I wonder whether there's a way to create some sort of alias so that we don't need to write that in each regex. Probably not. But probably if we just have a comment at the top of the file that explains what on Earth that bit of regex goop means, that'll be good enough -- the file isn't read all that often, I think.
On 2015/12/01 00:13:48, Ilya Sherman wrote: > On 2015/12/01 00:00:29, Evan Stade wrote: > > On 2015/11/26 21:32:33, Ilya Sherman wrote: > > > On 2015/11/26 21:04:20, tfarina wrote: > > > > Still have to fix the test failure of FormField::Match. > > > > > > > > components/autofill/core/browser/form_field_unittest.cc:93 > > > > > > > > Value of: FormField::Match(&field, "\\bcr\\b", FormField::MATCH_LABEL) > > > > Actual: true > > > > Expected: false > > > > > > Hmm, that's a problem. It looks like RE2 only supports ASCII > word-boundaries, > > > which doesn't work very well for regexes in other languages. I'm not sure > how > > > we'd fix this, actually -- it seems much harder to work around than the lack > > of > > > negative lookbehind. Václav, Mathieu, Evan -- any ideas? > > > > > > In the meantime, not lgtm. We shouldn't land this CL if it'll regress our > > > heuristics in non-English locales. > > > > I think we could change every \b to (\A|\z|[^\pL]) ? > > Yes, I think you're right. I think a slightly briefer way to write that is > (\A|\z|\PL). That's a little bit icky to read, but I think it will give the > correct behavior. > > I wonder whether there's a way to create some sort of alias so that we don't > need to write that in each regex. Probably not. But probably if we just have a > comment at the top of the file that explains what on Earth that bit of regex > goop means, that'll be good enough -- the file isn't read all that often, I > think. ¯\_(ツ)_/¯ we could do it at runtime. Performance wouldn't necessarily be impacted much if we only did it once and cached the result. We could use a c macro: #define WORDBREAK "(\A|\z|\PL)" [...] "|" WORDBREAK "cp" WORDBREAK // es [...]
On 2015/12/01 00:22:21, Evan Stade wrote: > On 2015/12/01 00:13:48, Ilya Sherman wrote: > > On 2015/12/01 00:00:29, Evan Stade wrote: > > > On 2015/11/26 21:32:33, Ilya Sherman wrote: > > > > On 2015/11/26 21:04:20, tfarina wrote: > > > > > Still have to fix the test failure of FormField::Match. > > > > > > > > > > components/autofill/core/browser/form_field_unittest.cc:93 > > > > > > > > > > Value of: FormField::Match(&field, "\\bcr\\b", FormField::MATCH_LABEL) > > > > > Actual: true > > > > > Expected: false > > > > > > > > Hmm, that's a problem. It looks like RE2 only supports ASCII > > word-boundaries, > > > > which doesn't work very well for regexes in other languages. I'm not sure > > how > > > > we'd fix this, actually -- it seems much harder to work around than the > lack > > > of > > > > negative lookbehind. Václav, Mathieu, Evan -- any ideas? > > > > > > > > In the meantime, not lgtm. We shouldn't land this CL if it'll regress our > > > > heuristics in non-English locales. > > > > > > I think we could change every \b to (\A|\z|[^\pL]) ? > > > > Yes, I think you're right. I think a slightly briefer way to write that is > > (\A|\z|\PL). That's a little bit icky to read, but I think it will give the > > correct behavior. > > > > I wonder whether there's a way to create some sort of alias so that we don't > > need to write that in each regex. Probably not. But probably if we just have > a > > comment at the top of the file that explains what on Earth that bit of regex > > goop means, that'll be good enough -- the file isn't read all that often, I > > think. > > ¯\_(ツ)_/¯ > > we could do it at runtime. Performance wouldn't necessarily be impacted much if > we only did it once and cached the result. > > We could use a c macro: > #define WORDBREAK "(\A|\z|\PL)" > > [...] > "|" WORDBREAK "cp" WORDBREAK // es > [...] (to be clear those are two separate ideas)
On 2015/12/01 00:23:01, Evan Stade wrote: > On 2015/12/01 00:22:21, Evan Stade wrote: > > On 2015/12/01 00:13:48, Ilya Sherman wrote: > > > On 2015/12/01 00:00:29, Evan Stade wrote: > > > > On 2015/11/26 21:32:33, Ilya Sherman wrote: > > > > > On 2015/11/26 21:04:20, tfarina wrote: > > > > > > Still have to fix the test failure of FormField::Match. > > > > > > > > > > > > components/autofill/core/browser/form_field_unittest.cc:93 > > > > > > > > > > > > Value of: FormField::Match(&field, "\\bcr\\b", FormField::MATCH_LABEL) > > > > > > Actual: true > > > > > > Expected: false > > > > > > > > > > Hmm, that's a problem. It looks like RE2 only supports ASCII > > > word-boundaries, > > > > > which doesn't work very well for regexes in other languages. I'm not > sure > > > how > > > > > we'd fix this, actually -- it seems much harder to work around than the > > lack > > > > of > > > > > negative lookbehind. Václav, Mathieu, Evan -- any ideas? > > > > > > > > > > In the meantime, not lgtm. We shouldn't land this CL if it'll regress > our > > > > > heuristics in non-English locales. > > > > > > > > I think we could change every \b to (\A|\z|[^\pL]) ? > > > > > > Yes, I think you're right. I think a slightly briefer way to write that is > > > (\A|\z|\PL). That's a little bit icky to read, but I think it will give the > > > correct behavior. > > > > > > I wonder whether there's a way to create some sort of alias so that we don't > > > need to write that in each regex. Probably not. But probably if we just > have > > a > > > comment at the top of the file that explains what on Earth that bit of regex > > > goop means, that'll be good enough -- the file isn't read all that often, I > > > think. > > > > ¯\_(ツ)_/¯ > > > > we could do it at runtime. Performance wouldn't necessarily be impacted much > if > > we only did it once and cached the result. > > > > We could use a c macro: > > #define WORDBREAK "(\A|\z|\PL)" > > > > [...] > > "|" WORDBREAK "cp" WORDBREAK // es > > [...] > > (to be clear those are two separate ideas) Yep, both good ideas -- thanks! I'm fine with either option, since we do cache the regexes. Thiago, if you're not sure how to implement these, feel free to just replace "\b" with "(\A|\z|\PL)" directly in the regexes, and one of us can clean up the syntax in a follow-up CL.
On 2015/12/01 00:31:25, Ilya Sherman wrote: > On 2015/12/01 00:23:01, Evan Stade wrote: > > On 2015/12/01 00:22:21, Evan Stade wrote: > > > On 2015/12/01 00:13:48, Ilya Sherman wrote: > > > > On 2015/12/01 00:00:29, Evan Stade wrote: > > > > > On 2015/11/26 21:32:33, Ilya Sherman wrote: > > > > > > On 2015/11/26 21:04:20, tfarina wrote: > > > > > > > Still have to fix the test failure of FormField::Match. > > > > > > > > > > > > > > components/autofill/core/browser/form_field_unittest.cc:93 > > > > > > > > > > > > > > Value of: FormField::Match(&field, "\\bcr\\b", > FormField::MATCH_LABEL) > > > > > > > Actual: true > > > > > > > Expected: false > > > > > > > > > > > > Hmm, that's a problem. It looks like RE2 only supports ASCII > > > > word-boundaries, > > > > > > which doesn't work very well for regexes in other languages. I'm not > > sure > > > > how > > > > > > we'd fix this, actually -- it seems much harder to work around than > the > > > lack > > > > > of > > > > > > negative lookbehind. Václav, Mathieu, Evan -- any ideas? > > > > > > > > > > > > In the meantime, not lgtm. We shouldn't land this CL if it'll regress > > our > > > > > > heuristics in non-English locales. > > > > > > > > > > I think we could change every \b to (\A|\z|[^\pL]) ? > > > > > > > > Yes, I think you're right. I think a slightly briefer way to write that > is > > > > (\A|\z|\PL). That's a little bit icky to read, but I think it will give > the > > > > correct behavior. > > > > > > > > I wonder whether there's a way to create some sort of alias so that we > don't > > > > need to write that in each regex. Probably not. But probably if we just > > have > > > a > > > > comment at the top of the file that explains what on Earth that bit of > regex > > > > goop means, that'll be good enough -- the file isn't read all that often, > I > > > > think. > > > > > > ¯\_(ツ)_/¯ > > > > > > we could do it at runtime. Performance wouldn't necessarily be impacted much > > if > > > we only did it once and cached the result. > > > > > > We could use a c macro: > > > #define WORDBREAK "(\A|\z|\PL)" > > > > > > [...] > > > "|" WORDBREAK "cp" WORDBREAK // es > > > [...] > > > > (to be clear those are two separate ideas) > > Yep, both good ideas -- thanks! I'm fine with either option, since we do cache > the regexes. Thiago, if you're not sure how to implement these, feel free to > just replace "\b" with "(\A|\z|\PL)" directly in the regexes, and one of us can > clean up the syntax in a follow-up CL. how do we make sure that people don't add \b in the future?
On 2015/12/01 00:34:05, Evan Stade wrote: > On 2015/12/01 00:31:25, Ilya Sherman wrote: > > On 2015/12/01 00:23:01, Evan Stade wrote: > > > On 2015/12/01 00:22:21, Evan Stade wrote: > > > > On 2015/12/01 00:13:48, Ilya Sherman wrote: > > > > > On 2015/12/01 00:00:29, Evan Stade wrote: > > > > > > On 2015/11/26 21:32:33, Ilya Sherman wrote: > > > > > > > On 2015/11/26 21:04:20, tfarina wrote: > > > > > > > > Still have to fix the test failure of FormField::Match. > > > > > > > > > > > > > > > > components/autofill/core/browser/form_field_unittest.cc:93 > > > > > > > > > > > > > > > > Value of: FormField::Match(&field, "\\bcr\\b", > > FormField::MATCH_LABEL) > > > > > > > > Actual: true > > > > > > > > Expected: false > > > > > > > > > > > > > > Hmm, that's a problem. It looks like RE2 only supports ASCII > > > > > word-boundaries, > > > > > > > which doesn't work very well for regexes in other languages. I'm > not > > > sure > > > > > how > > > > > > > we'd fix this, actually -- it seems much harder to work around than > > the > > > > lack > > > > > > of > > > > > > > negative lookbehind. Václav, Mathieu, Evan -- any ideas? > > > > > > > > > > > > > > In the meantime, not lgtm. We shouldn't land this CL if it'll > regress > > > our > > > > > > > heuristics in non-English locales. > > > > > > > > > > > > I think we could change every \b to (\A|\z|[^\pL]) ? > > > > > > > > > > Yes, I think you're right. I think a slightly briefer way to write that > > is > > > > > (\A|\z|\PL). That's a little bit icky to read, but I think it will give > > the > > > > > correct behavior. > > > > > > > > > > I wonder whether there's a way to create some sort of alias so that we > > don't > > > > > need to write that in each regex. Probably not. But probably if we > just > > > have > > > > a > > > > > comment at the top of the file that explains what on Earth that bit of > > regex > > > > > goop means, that'll be good enough -- the file isn't read all that > often, > > I > > > > > think. > > > > > > > > ¯\_(ツ)_/¯ > > > > > > > > we could do it at runtime. Performance wouldn't necessarily be impacted > much > > > if > > > > we only did it once and cached the result. > > > > > > > > We could use a c macro: > > > > #define WORDBREAK "(\A|\z|\PL)" > > > > > > > > [...] > > > > "|" WORDBREAK "cp" WORDBREAK // es > > > > [...] > > > > > > (to be clear those are two separate ideas) > > > > Yep, both good ideas -- thanks! I'm fine with either option, since we do > cache > > the regexes. Thiago, if you're not sure how to implement these, feel free to > > just replace "\b" with "(\A|\z|\PL)" directly in the regexes, and one of us > can > > clean up the syntax in a follow-up CL. > > how do we make sure that people don't add \b in the future? If we go with the runtime solution, then we can just replace "\b" with "(\A|\z|\PL)" at runtime. But in practice, I think most of these regexes are built up using copy/paste... so if we consistently use the WORDBREAK macro elsewhere in the file, it's relatively unlikely for a "\b" to sneak in.
Thank you Evan and Ilya for saving this patch! You guys are awesome and rock on regex! ;) I will try to make those changes later tonight.
Sorry for the delay Ilya! Looking at this now.
Ilya, unfortunatelly the regex support of RE2 is very limited! https://github.com/google/re2/wiki/Syntax \A not \a (NOT SUPPORTED) VIM ../../components/autofill/core/browser/form_field_unittest.cc:89:40: error: unknown escape sequence: '\A' [-Werror] Am I doing the escaping wrong? I will try with \\A to see. But given the above, I don't think it will help much.
Anyhow, I have green trybots with \\A. If it is OK I will look into the Evan's macro tomorrow.
On 2015/12/05 01:05:03, tfarina wrote: > Anyhow, I have green trybots with \\A. > > If it is OK I will look into the Evan's macro tomorrow. That's fine. Just to be clear, though, you should update autofill_regex_constants.cc as well as updating the regex used in the test code. Thanks =)
Ilya, I have updated autofill_regex_constants.cc. PTAL.
Thanks, Thiago. LGTM % the remaining nits: https://codereview.chromium.org/1453193002/diff/260001/components/autofill/co... File components/autofill/core/browser/form_field_unittest.cc (right): https://codereview.chromium.org/1453193002/diff/260001/components/autofill/co... components/autofill/core/browser/form_field_unittest.cc:89: EXPECT_TRUE(FormField::Match(&field, "(\\A|\\z|\\PL)word(\\A|\\z|\\PL)", nit: Please define a variable like const string kWordBoundary = "(\\A|\\z|\\PL)" and use that when writing the expectations, e.g. EXPECT_TRUE(FormField::Match(&field, kWordBoundary + "word" + kWordBoundary, FormField::MATCH_LABEL) https://codereview.chromium.org/1453193002/diff/280001/components/autofill/co... File components/autofill/core/browser/autofill_regex_constants.cc (right): https://codereview.chromium.org/1453193002/diff/280001/components/autofill/co... components/autofill/core/browser/autofill_regex_constants.cc:130: ".*card|(card|cc).?name|cc.?full.?name" nit: Please wrap this as "card.?(holder|owner)|name.*" WORDBREAK "on" WORDBREAK ".*card" "|(card|cc).?name|cc.?full.?name" https://codereview.chromium.org/1453193002/diff/280001/components/autofill/co... components/autofill/core/browser/autofill_regex_constants.cc:138: "|持卡人姓名"; // zh-TW nit: Please revert these alignment changes.
https://codereview.chromium.org/1453193002/diff/280001/components/autofill/co... File components/autofill/core/browser/autofill_regex_constants.cc (right): https://codereview.chromium.org/1453193002/diff/280001/components/autofill/co... components/autofill/core/browser/autofill_regex_constants.cc:11: #define WORDBREAK "(\\A|\\z|\\PL)" could you add a comment explaining this is supposed to be the same as \b and why we can't use \b. Also google style guide says to undef afterwards.
https://codereview.chromium.org/1453193002/diff/260001/components/autofill/co... File components/autofill/core/browser/form_field_unittest.cc (right): https://codereview.chromium.org/1453193002/diff/260001/components/autofill/co... components/autofill/core/browser/form_field_unittest.cc:89: EXPECT_TRUE(FormField::Match(&field, "(\\A|\\z|\\PL)word(\\A|\\z|\\PL)", On 2015/12/07 23:49:25, Ilya Sherman wrote: > nit: Please define a variable like > > const string kWordBoundary = "(\\A|\\z|\\PL)" > > and use that when writing the expectations, e.g. > > EXPECT_TRUE(FormField::Match(&field, kWordBoundary + "word" + kWordBoundary, > FormField::MATCH_LABEL) Done. https://codereview.chromium.org/1453193002/diff/280001/components/autofill/co... File components/autofill/core/browser/autofill_regex_constants.cc (right): https://codereview.chromium.org/1453193002/diff/280001/components/autofill/co... components/autofill/core/browser/autofill_regex_constants.cc:11: #define WORDBREAK "(\\A|\\z|\\PL)" On 2015/12/07 23:56:10, Evan Stade wrote: > could you add a comment explaining this is supposed to be the same as \b and why > we can't use \b. Also google style guide says to undef afterwards. Done. https://codereview.chromium.org/1453193002/diff/280001/components/autofill/co... components/autofill/core/browser/autofill_regex_constants.cc:130: ".*card|(card|cc).?name|cc.?full.?name" On 2015/12/07 23:49:25, Ilya Sherman wrote: > nit: Please wrap this as > > "card.?(holder|owner)|name.*" WORDBREAK "on" WORDBREAK ".*card" > "|(card|cc).?name|cc.?full.?name" Done. https://codereview.chromium.org/1453193002/diff/280001/components/autofill/co... components/autofill/core/browser/autofill_regex_constants.cc:138: "|持卡人姓名"; // zh-TW On 2015/12/07 23:49:25, Ilya Sherman wrote: > nit: Please revert these alignment changes. Sorry, that was clang format. Reverted.
https://codereview.chromium.org/1453193002/diff/300001/components/autofill/co... File components/autofill/core/browser/address_field.cc (right): https://codereview.chromium.org/1453193002/diff/300001/components/autofill/co... components/autofill/core/browser/address_field.cc:245: // Ignore spurious matches for "United States". where is this coming from? is this a new addition? Seems orthogonal enough to warrant its own change.
https://codereview.chromium.org/1453193002/diff/300001/components/autofill/co... File components/autofill/core/browser/address_field.cc (right): https://codereview.chromium.org/1453193002/diff/300001/components/autofill/co... components/autofill/core/browser/address_field.cc:245: // Ignore spurious matches for "United States". On 2015/12/08 19:34:56, Evan Stade wrote: > where is this coming from? is this a new addition? Seems orthogonal enough to > warrant its own change. It's coming from a change to one of the regexes, which used a negative lookbehind operator for this functionality. RE2 doesn't support negative lookbehind, AFAICT.
lgtm
The CQ bit was checked by tfarina@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1453193002/#ps300001 (title: "address reviews")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1453193002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1453193002/300001
Message was sent while issue was closed.
Description was changed from ========== autofill: switch autofill_regexes to RE2 library This moves it away from using regex library from icu to the in-house re2 library that is known to have a better performance and be much more lightweight and much less verbose to write code. BUG=470065 TEST=components_unittests --gtest_filter=*Autofill* R=vabr@chromium.org,isherman@chromium.org ========== to ========== autofill: switch autofill_regexes to RE2 library This moves it away from using regex library from icu to the in-house re2 library that is known to have a better performance and be much more lightweight and much less verbose to write code. BUG=470065 TEST=components_unittests --gtest_filter=*Autofill* R=vabr@chromium.org,isherman@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== autofill: switch autofill_regexes to RE2 library This moves it away from using regex library from icu to the in-house re2 library that is known to have a better performance and be much more lightweight and much less verbose to write code. BUG=470065 TEST=components_unittests --gtest_filter=*Autofill* R=vabr@chromium.org,isherman@chromium.org ========== to ========== autofill: switch autofill_regexes to RE2 library This moves it away from using regex library from icu to the in-house re2 library that is known to have a better performance and be much more lightweight and much less verbose to write code. BUG=470065 TEST=components_unittests --gtest_filter=*Autofill* R=vabr@chromium.org,isherman@chromium.org Committed: https://crrev.com/2df535152b8023dc3dcbc7dd8c7f1a1e3d959e13 Cr-Commit-Position: refs/heads/master@{#363835} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/2df535152b8023dc3dcbc7dd8c7f1a1e3d959e13 Cr-Commit-Position: refs/heads/master@{#363835}
Message was sent while issue was closed.
Reverting this now. Will also revert the other one that introduced the re2 dependency.
Message was sent while issue was closed.
A revert of this CL (patchset #16 id:300001) has been created in https://codereview.chromium.org/1518783002/ by tfarina@chromium.org. The reason for reverting is: This caused a memory perf regression on android. BUG=568134. |
