Use RE string pattern matching for blacklist strings.

Use RE string pattern matching for blacklist strings.

Including cpu_brand, gl_vendor, gl_renderer, gl_extension, driver_vendor,
and machine_model_name.

BUG=396578
TBR=piman@chromium.org
TEST=gpu_unittests

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

[Zhenyao Mo, 2014-08-09 00:22:06]

kbr: PTAL

[Ken Russell (switch to Gerrit), 2014-08-09 00:57:02]

On the whole this LGTM -- it's a good code cleanup -- but either the
documentation needs to be made more clear about the semantics of the patterns in
the blacklists, or the patterns themselves need to be updated. See below.

https://codereview.chromium.org/452293002/diff/40001/gpu/config/gpu_control_l...
File gpu/config/gpu_control_list.cc (right):

https://codereview.chromium.org/452293002/diff/40001/gpu/config/gpu_control_l...
gpu/config/gpu_control_list.cc:666:
!entry->AddMachineModelName(base::StringToLowerASCII(model_name))) {
I don't think that converting a regexp pattern to lowercase is guaranteed to
work correctly. See comment below.

https://codereview.chromium.org/452293002/diff/40001/gpu/config/gpu_control_l...
File gpu/config/gpu_control_list_format.txt (right):

https://codereview.chromium.org/452293002/diff/40001/gpu/config/gpu_control_l...
gpu/config/gpu_control_list_format.txt:83: //
https://code.google.com/p/re2/wiki/Syntax.
As mentioned above, a tighter definition of how the matching is done is needed.
I'm pretty sure it isn't correct in all cases to simply convert a regexp pattern
to lower case via ASCII rules. At a minimum, it needs to be documented that both
the string pattern, and the value it matches against, are converted to lower
case. Perhaps it would be better to just use re's functionality for doing
case-insensitive matches. This would mean that all of the regexps would need to
be prefixed with (I think)

  (?i:re)

This would certainly make the various blacklists more verbose, so it's your
decision what to do here.

[Zhenyao Mo, 2014-08-09 01:24:00]

https://codereview.chromium.org/452293002/diff/40001/gpu/config/gpu_control_l...
File gpu/config/gpu_control_list.cc (right):

https://codereview.chromium.org/452293002/diff/40001/gpu/config/gpu_control_l...
gpu/config/gpu_control_list.cc:666:
!entry->AddMachineModelName(base::StringToLowerASCII(model_name))) {
On 2014/08/09 00:57:02, Ken Russell wrote:
> I don't think that converting a regexp pattern to lowercase is guaranteed to
> work correctly. See comment below.

Done.

https://codereview.chromium.org/452293002/diff/40001/gpu/config/gpu_control_l...
File gpu/config/gpu_control_list_format.txt (right):

https://codereview.chromium.org/452293002/diff/40001/gpu/config/gpu_control_l...
gpu/config/gpu_control_list_format.txt:83: //
https://code.google.com/p/re2/wiki/Syntax.
On 2014/08/09 00:57:02, Ken Russell wrote:
> As mentioned above, a tighter definition of how the matching is done is
needed.
> I'm pretty sure it isn't correct in all cases to simply convert a regexp
pattern
> to lower case via ASCII rules. At a minimum, it needs to be documented that
both
> the string pattern, and the value it matches against, are converted to lower
> case. Perhaps it would be better to just use re's functionality for doing
> case-insensitive matches. This would mean that all of the regexps would need
to
> be prefixed with (I think)
> 
>   (?i:re)
> 
> This would certainly make the various blacklists more verbose, so it's your
> decision what to do here.

I think allow the full power of RE is a better option. However, we don't need to
set all entries to be case insensitive.  I think many of them should be
case-sensitive matching.  Only one needs to be case insensitive.

[Zhenyao Mo, 2014-08-11 21:59:27]

The CQ bit was checked by zmo@chromium.org

[commit-bot: I haz the power, 2014-08-11 22:02:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zmo@chromium.org/452293002/80001

[commit-bot: I haz the power, 2014-08-12 01:26:26]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-12 01:47:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-12 01:47:33]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Zhenyao Mo, 2014-08-12 14:42:17]

piman: gpu/ owner.

[Zhenyao Mo, 2014-08-12 21:04:17]

All owners are out of office, and the gpu/ build file change is trivia.  TBR
this time to land this CL to unblock others.

[Zhenyao Mo, 2014-08-12 21:04:22]

The CQ bit was checked by zmo@chromium.org

[commit-bot: I haz the power, 2014-08-12 21:08:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zmo@chromium.org/452293002/80001

[commit-bot: I haz the power, 2014-08-12 21:36:20]

Change committed as 289067