[ios] Adds unittests for blocks and NSRegularExpression.

[ios] Adds unittests for blocks and NSRegularExpression.

BUG=662902
Committed: https://crrev.com/273e8fd9f475187d270bcd85f11806c1fa311556
Cr-Commit-Position: refs/heads/master@{#432178}

[rohitrao (ping after 24h), 2016-11-07 13:43:15]

The CQ bit was checked by rohitrao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-07 13:43:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rohitrao (ping after 24h), 2016-11-07 13:45:39]

The CQ bit was checked by rohitrao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-07 13:45:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rohitrao (ping after 24h), 2016-11-07 13:45:50]

rohitrao@chromium.org changed reviewers:
+ sdefresne@chromium.org

[sdefresne, 2016-11-07 13:54:58]

https://codereview.chromium.org/2480233002/diff/20001/ios/chrome/common/block...
File ios/chrome/common/block_unittest.mm (right):

https://codereview.chromium.org/2480233002/diff/20001/ios/chrome/common/block...
ios/chrome/common/block_unittest.mm:18: RefCountedObject(){};
Remove trailing semi-colon ...

https://codereview.chromium.org/2480233002/diff/20001/ios/chrome/common/block...
ios/chrome/common/block_unittest.mm:30: for (int ii = 1; ii < count; ii++)
nit: why "ii" and not "i"?

https://codereview.chromium.org/2480233002/diff/20001/ios/chrome/common/block...
ios/chrome/common/block_unittest.mm:37: virtual ~RefCountedObject(){};
Remove trailing semi-colon ...

https://codereview.chromium.org/2480233002/diff/20001/ios/chrome/common/ns_re...
File ios/chrome/common/ns_regular_expression_unittest.mm (right):

https://codereview.chromium.org/2480233002/diff/20001/ios/chrome/common/ns_re...
ios/chrome/common/ns_regular_expression_unittest.mm:57: NSError* error;
nit: NSError* error = nil;

https://codereview.chromium.org/2480233002/diff/20001/ios/chrome/common/ns_re...
ios/chrome/common/ns_regular_expression_unittest.mm:94:
ExpectRegexMatched(regex, @"ac", @"$0", @"ac");
This seems to contradict the comment above. Am I missing something?

Comment say that it should match "ab" or "c" but not "ac" but the test check
that it match "ab", "c" and "ac" but not "abc" nor "a" nor "b".

Can you fix the comment?

[rohitrao (ping after 24h), 2016-11-07 14:04:05]

+pkl can you comment on why "ac" matches that regular expression?

The block for '(?:ab)|(?:c)' below explicitly says that it should behave the
same way as 'ab|c', but the test for "ac" is expected to match one but not the
other.

https://codereview.chromium.org/2480233002/diff/20001/ios/chrome/common/block...
File ios/chrome/common/block_unittest.mm (right):

https://codereview.chromium.org/2480233002/diff/20001/ios/chrome/common/block...
ios/chrome/common/block_unittest.mm:18: RefCountedObject(){};
On 2016/11/07 13:54:57, sdefresne wrote:
> Remove trailing semi-colon ...

Done.

https://codereview.chromium.org/2480233002/diff/20001/ios/chrome/common/block...
ios/chrome/common/block_unittest.mm:30: for (int ii = 1; ii < count; ii++)
On 2016/11/07 13:54:57, sdefresne wrote:
> nit: why "ii" and not "i"?

I tend to use "ii" because it doesn't commonly occur in words, making it easier
to grep for.

https://codereview.chromium.org/2480233002/diff/20001/ios/chrome/common/block...
ios/chrome/common/block_unittest.mm:37: virtual ~RefCountedObject(){};
On 2016/11/07 13:54:57, sdefresne wrote:
> Remove trailing semi-colon ...

Done.

https://codereview.chromium.org/2480233002/diff/20001/ios/chrome/common/ns_re...
File ios/chrome/common/ns_regular_expression_unittest.mm (right):

https://codereview.chromium.org/2480233002/diff/20001/ios/chrome/common/ns_re...
ios/chrome/common/ns_regular_expression_unittest.mm:57: NSError* error;
On 2016/11/07 13:54:57, sdefresne wrote:
> nit: NSError* error = nil;

Done.

[rohitrao (ping after 24h), 2016-11-08 17:53:19]

rohitrao@chromium.org changed reviewers:
+ pkl@chromium.org

[rohitrao (ping after 24h), 2016-11-08 17:53:20]

+pkl for real

pkl@ can you comment on why "ac" matches that regular expression?

The block for '(?:ab)|(?:c)' below explicitly says that it should behave the
same way as 'ab|c', but the test for "ac" is expected to match one but not the
other.

[pkl (ping after 24h if needed), 2016-11-08 23:54:13]

lgtm

https://codereview.chromium.org/2480233002/diff/40001/ios/chrome/common/ns_re...
File ios/chrome/common/ns_regular_expression_unittest.mm (right):

https://codereview.chromium.org/2480233002/diff/40001/ios/chrome/common/ns_re...
ios/chrome/common/ns_regular_expression_unittest.mm:94:
ExpectRegexMatched(regex, @"ac", @"$0", @"ac");
I think this may be where ICU Regular Expressions and NS Regular Expressions
differs.

Looking at the old test on
https://chromereviews.googleplex.com/9582014/diff/7001/ios_internal/chrome/co...,
I can see that RegEx "ab|c" does not match "ac", i.e.
  ExpectRegexNotMatched(regex.get(), @"ac", @"$0");

It's likely that the migration from ICU to NS we fixed the test to match
NSRegularExpression behavior but failed to update the comments.

The comments and test code in this CL are both correct.

[rohitrao (ping after 24h), 2016-11-15 14:04:38]

https://codereview.chromium.org/2480233002/diff/40001/ios/chrome/common/ns_re...
File ios/chrome/common/ns_regular_expression_unittest.mm (right):

https://codereview.chromium.org/2480233002/diff/40001/ios/chrome/common/ns_re...
ios/chrome/common/ns_regular_expression_unittest.mm:94:
ExpectRegexMatched(regex, @"ac", @"$0", @"ac");
On 2016/11/08 23:54:13, pklpkl wrote:
> I think this may be where ICU Regular Expressions and NS Regular Expressions
> differs.
> 
> Looking at the old test on
>
https://chromereviews.googleplex.com/9582014/diff/7001/ios_internal/chrome/co...,
> I can see that RegEx "ab|c" does not match "ac", i.e.
>   ExpectRegexNotMatched(regex.get(), @"ac", @"$0");
> 
> It's likely that the migration from ICU to NS we fixed the test to match
> NSRegularExpression behavior but failed to update the comments.
> 
> The comments and test code in this CL are both correct.

Acknowledged.

[rohitrao (ping after 24h), 2016-11-15 14:04:42]

The CQ bit was checked by rohitrao@chromium.org

[rohitrao (ping after 24h), 2016-11-15 14:04:42]

The patchset sent to the CQ was uploaded after l-g-t-m from pkl@chromium.org
Link to the patchset: https://codereview.chromium.org/2480233002/#ps60001
(title: "Review.")

[commit-bot: I haz the power, 2016-11-15 14:05:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-15 15:08:28]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-15 15:10:22]

Description was changed from

==========
[ios] Adds unittests for blocks and NSRegularExpression.

BUG=662902
==========

to

==========
[ios] Adds unittests for blocks and NSRegularExpression.

BUG=662902

Committed: https://crrev.com/273e8fd9f475187d270bcd85f11806c1fa311556
Cr-Commit-Position: refs/heads/master@{#432178}
==========

[commit-bot: I haz the power, 2016-11-15 15:10:24]

Patchset 4 (id:??) landed as
https://crrev.com/273e8fd9f475187d270bcd85f11806c1fa311556
Cr-Commit-Position: refs/heads/master@{#432178}