Fix machine_model behaviors in gpu blacklist.

Fix machine_model behaviors in gpu blacklist.

We made the assumption that GPUInfo.machine_model is (name, version).

This is true for mac, where this machine_model was originally designed for.
Now we also use it on Android. However, the whole machine_model should just
be retreated as one name string.

Now on Android Nexus, we have "Nexus 5", "Nexus 7", so they are processed
like on Mac, which is incorrect.

I believe a few android entries in the software_rendering_list_json.cc
are not active right now due to the (name, version).

BUG=364743
TEST=gpu_unittests
R=kbr@chromium.org, tsepez@chromium.org, vrk@chromium.org
TBR=pfeldman@chromium.org

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

[Zhenyao Mo, 2014-04-17 22:03:25]

kbr: please take a look.  We need to fix this for the gpu_rasterization
blacklisting.

[Ken Russell (switch to Gerrit), 2014-04-17 22:24:57]

Please file a bug about this issue and reference it from the CL. More comments
below.

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

https://codereview.chromium.org/241793002/diff/60001/gpu/config/gpu_control_l...
gpu/config/gpu_control_list.cc:1142: #if defined(OS_MACOSX)
Making this code conditionally compiled is really unfortunate. It means that not
all of the tests can be run on all platforms. It also means that the non-Mac
code violates the spec of machine_model in gpu_info.h. I think a better approach
would be to change the rules.

Currently, the "real" model identifier on Mac OS uses a comma as a separator:
for example, "MacBookPro10,1". Currently, lower-level code parses this and turns
it into a space-separated string. This conversion is basically only done for the
GPU blacklist.

I'd like to propose that we change the rules about machine_model in gpu_info.h
so that:

1) the name can contain spaces
2) the name and version are comma separated

and then change the code which reports the machine identifier on Mac OS so that
it reassembles the string to use a comma separator rather than a space
separator.

Then in gpu_info.h it should be very clearly documented that this is intended to
allow the model version on Mac OS to be parsed out and treated as the version in
the blacklist, while on Android devices like "Nexus 5", that's the entire
machine_model.

What do you think?

[Zhenyao Mo, 2014-04-17 22:56:41]

On 2014/04/17 22:24:57, Ken Russell wrote:
> Please file a bug about this issue and reference it from the CL. More comments
> below.
> 
>
https://codereview.chromium.org/241793002/diff/60001/gpu/config/gpu_control_l...
> File gpu/config/gpu_control_list.cc (right):
> 
>
https://codereview.chromium.org/241793002/diff/60001/gpu/config/gpu_control_l...
> gpu/config/gpu_control_list.cc:1142: #if defined(OS_MACOSX)
> Making this code conditionally compiled is really unfortunate. It means that
not
> all of the tests can be run on all platforms. It also means that the non-Mac
> code violates the spec of machine_model in gpu_info.h. I think a better
approach
> would be to change the rules.
> 
> Currently, the "real" model identifier on Mac OS uses a comma as a separator:
> for example, "MacBookPro10,1". Currently, lower-level code parses this and
turns
> it into a space-separated string. This conversion is basically only done for
the
> GPU blacklist.
> 
> I'd like to propose that we change the rules about machine_model in gpu_info.h
> so that:
> 
> 1) the name can contain spaces
> 2) the name and version are comma separated
> 
> and then change the code which reports the machine identifier on Mac OS so
that
> it reassembles the string to use a comma separator rather than a space
> separator.
> 
> Then in gpu_info.h it should be very clearly documented that this is intended
to
> allow the model version on Mac OS to be parsed out and treated as the version
in
> the blacklist, while on Android devices like "Nexus 5", that's the entire
> machine_model.
> 
> What do you think?

What about beside machine_mode, we add a machine_model_version?

So on mac, we use machine_model_version, and on other platforms, we just don't
use it.

On mac we still have to pre-process a little bit, but that's all the less-than
straight-forward part.

[Ken Russell (switch to Gerrit), 2014-04-17 23:10:41]

On 2014/04/17 22:56:41, Zhenyao Mo wrote:
> What about beside machine_mode, we add a machine_model_version?
> 
> So on mac, we use machine_model_version, and on other platforms, we just don't
> use it.
> 
> On mac we still have to pre-process a little bit, but that's all the less-than
> straight-forward part.

That sounds fine too.

As we discussed offline, please document this very clearly in gpu_info.h,
including examples, and refer to gpu_control_list_format.txt pointing out what
kinds of blacklist entries can refer to these machine models.

[Zhenyao Mo, 2014-04-18 00:50:48]

Revised, please take another look.

[Zhenyao Mo, 2014-04-18 00:52:37]

scherkus: chrome/browsr/media owner

[Zhenyao Mo, 2014-04-18 00:53:51]

pfeldman: chrome/browser/devtools OWNER

[Zhenyao Mo, 2014-04-18 00:55:27]

tsepez: content/common/gpu/gpu_messages.h OWNER review

[Ken Russell (switch to Gerrit), 2014-04-18 01:29:12]

LGTM. One question. No need to change the code in response.

https://codereview.chromium.org/241793002/diff/110001/gpu/config/gpu_control_...
File gpu/config/gpu_control_list_format.txt (right):

https://codereview.chromium.org/241793002/diff/110001/gpu/config/gpu_control_...
gpu/config/gpu_control_list_format.txt:42: //     any characters.
On further thought: do you think we are going to regret not making the machine
model an array of structs, each of which contains both a name and version? It
doesn't make sense to have one version apply to multiple machine names, so the
only situation where the version can be effectively used right now is on the Mac
where we apply blacklist entries to a single name but a group of versions.

[Zhenyao Mo, 2014-04-18 16:47:02]

On 2014/04/18 01:29:12, Ken Russell wrote:
> LGTM. One question. No need to change the code in response.
> 
>
https://codereview.chromium.org/241793002/diff/110001/gpu/config/gpu_control_...
> File gpu/config/gpu_control_list_format.txt (right):
> 
>
https://codereview.chromium.org/241793002/diff/110001/gpu/config/gpu_control_...
> gpu/config/gpu_control_list_format.txt:42: //     any characters.
> On further thought: do you think we are going to regret not making the machine
> model an array of structs, each of which contains both a name and version? It
> doesn't make sense to have one version apply to multiple machine names, so the
> only situation where the version can be effectively used right now is on the
Mac
> where we apply blacklist entries to a single name but a group of versions.

I thought of this. However, doing this will make an entry with multiple models
very ugly - almost as ugly as before.

One thing we can do is to add a check to make sure when there is a
machine_model_version, we will only have one string in machine_model_name list,
i.e., we forbid multiple models with one version.

[Tom Sepez, 2014-04-18 17:45:56]

Messages LGTM.

[Ken Russell (switch to Gerrit), 2014-04-20 00:09:35]

On 2014/04/18 16:47:02, Zhenyao Mo wrote:
> On 2014/04/18 01:29:12, Ken Russell wrote:
> > LGTM. One question. No need to change the code in response.
> > 
> >
>
https://codereview.chromium.org/241793002/diff/110001/gpu/config/gpu_control_...
> > File gpu/config/gpu_control_list_format.txt (right):
> > 
> >
>
https://codereview.chromium.org/241793002/diff/110001/gpu/config/gpu_control_...
> > gpu/config/gpu_control_list_format.txt:42: //     any characters.
> > On further thought: do you think we are going to regret not making the
machine
> > model an array of structs, each of which contains both a name and version?
It
> > doesn't make sense to have one version apply to multiple machine names, so
the
> > only situation where the version can be effectively used right now is on the
> Mac
> > where we apply blacklist entries to a single name but a group of versions.
> 
> I thought of this. However, doing this will make an entry with multiple models
> very ugly - almost as ugly as before.
> 
> One thing we can do is to add a check to make sure when there is a
> machine_model_version, we will only have one string in machine_model_name
list,
> i.e., we forbid multiple models with one version.

Not necessary. That will make the behavior even more difficult to explain, and
the documentation more complex.

[Zhenyao Mo, 2014-04-21 18:11:52]

Adding a few devtools owners: one of you please review the devtools change.

[vrk (LEFT CHROMIUM), 2014-04-21 19:07:53]

lgtm

[vrk (LEFT CHROMIUM), 2014-04-21 19:08:25]

On 2014/04/21 19:07:53, Victoria Kirst wrote:
> lgtm

clarifying: lgtm for media :)

[Zhenyao Mo, 2014-04-21 20:34:29]

Thanks.

devtools change is trivia, so I am landing this with TBR to unblock enabling gpu
rasterization on a few Android devices.

[Zhenyao Mo, 2014-04-21 20:54:20]

Committed patchset #6 manually as r265062 (presubmit successful).