Document and test case sensitivity in URL matcher attributes

Document and test case sensitivity in URL matcher attributes


BUG=160702
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175762

[vabr (Chromium), 2013-01-07 18:33:48]

Hi Dominic,

PTAL.

Note that the error message requested in
http://code.google.com/p/chromium/issues/detail?id=160702#c7 will come in a
separate CL.

Thanks,

Vaclav

https://codereview.chromium.org/11776024/diff/2001/chrome/common/extensions/m...
File chrome/common/extensions/matcher/url_matcher_factory_unittest.cc (right):

https://codereview.chromium.org/11776024/diff/2001/chrome/common/extensions/m...
chrome/common/extensions/matcher/url_matcher_factory_unittest.cc:141: };
No DISALLOW_COPY_AND_ASSIGN here, because a public copy constructor is needed
for the definition of |case_tests| below to compile. The copy-con is never used,
though.

[battre, 2013-01-07 19:13:00]

lgtm

https://codereview.chromium.org/11776024/diff/2001/chrome/common/extensions/m...
File chrome/common/extensions/matcher/url_matcher_factory_unittest.cc (right):

https://codereview.chromium.org/11776024/diff/2001/chrome/common/extensions/m...
chrome/common/extensions/matcher/url_matcher_factory_unittest.cc:141: };
On 2013/01/07 18:33:48, vabr (Chromium) wrote:
> No DISALLOW_COPY_AND_ASSIGN here, because a public copy constructor is needed
> for the definition of |case_tests| below to compile. The copy-con is never
used,
> though.

How about adding a comment of the style

// Allow implicit copy and assign because ...

https://codereview.chromium.org/11776024/diff/2001/chrome/common/extensions/m...
chrome/common/extensions/matcher/url_matcher_factory_unittest.cc:163:
error.clear();
nit: I would remove this line.

https://codereview.chromium.org/11776024/diff/2001/chrome/common/extensions/m...
chrome/common/extensions/matcher/url_matcher_factory_unittest.cc:241: for
(size_t i = 0; i < ARRAYSIZE_UNSAFE(case_tests); ++i)
Tip: You may add a line like the following:
SCOPED_TRACE(std::string("Iteration: ") + i);
to generate better failure messages, see
http://code.google.com/p/googletest/wiki/AdvancedGuide

[commit-bot: I haz the power, 2013-01-07 19:13:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vabr@chromium.org/11776024/2001

[commit-bot: I haz the power, 2013-01-07 19:13:46]

Presubmit check for 11776024-2001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/common/extensions/api

Presubmit checks took 6.8s to calculate.

[battre, 2013-01-07 19:14:45]

oh, looks like I accidentally pressed "Quick LGTM & CQ" instead of "Quick LGTM".
Sorry.

[vabr (Chromium), 2013-01-08 16:52:47]

Thanks, Dominic.

Mike,
By following strange chain of intra-OWNERS redirections, I derived that you can
give me a rubber stamp for the JSON change (just the first file).
Please, take a look.

Thanks,
Vaclav

https://codereview.chromium.org/11776024/diff/2001/chrome/common/extensions/m...
File chrome/common/extensions/matcher/url_matcher_factory_unittest.cc (right):

https://codereview.chromium.org/11776024/diff/2001/chrome/common/extensions/m...
chrome/common/extensions/matcher/url_matcher_factory_unittest.cc:141: };
On 2013/01/07 19:13:01, battre wrote:
> On 2013/01/07 18:33:48, vabr (Chromium) wrote:
> > No DISALLOW_COPY_AND_ASSIGN here, because a public copy constructor is
needed
> > for the definition of |case_tests| below to compile. The copy-con is never
> used,
> > though.
> 
> How about adding a comment of the style
> 
> // Allow implicit copy and assign because ...

Done.

https://codereview.chromium.org/11776024/diff/2001/chrome/common/extensions/m...
chrome/common/extensions/matcher/url_matcher_factory_unittest.cc:163:
error.clear();
On 2013/01/07 19:13:01, battre wrote:
> nit: I would remove this line.

Done.

https://codereview.chromium.org/11776024/diff/2001/chrome/common/extensions/m...
chrome/common/extensions/matcher/url_matcher_factory_unittest.cc:241: for
(size_t i = 0; i < ARRAYSIZE_UNSAFE(case_tests); ++i)
TIL, thanks!
Done.

On 2013/01/07 19:13:01, battre wrote:
> Tip: You may add a line like the following:
> SCOPED_TRACE(std::string("Iteration: ") + i);
> to generate better failure messages, see
> http://code.google.com/p/googletest/wiki/AdvancedGuide

[Mike West, 2013-01-08 16:55:42]

On 2013/01/08 16:52:47, vabr (Chromium) wrote:
> Thanks, Dominic.
> 
> Mike,
> By following strange chain of intra-OWNERS redirections, I derived that you
can
> give me a rubber stamp for the JSON change (just the first file).
> Please, take a look.

Only for documentation, which, happily, this CL consists of!

LGTM. Thanks! :)

-mike

[commit-bot: I haz the power, 2013-01-09 06:45:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vabr@chromium.org/11776024/14001

[commit-bot: I haz the power, 2013-01-09 10:19:25]

Change committed as 175762