Don't blacklist accelerated 2d canvas if WinStats are unavailable.

Don't blacklist accelerated 2d canvas if WinStats are unavailable.

BUG=349628
TEST=gpu_unittests
R=kbr@chromium.org

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

[Zhenyao Mo, 2014-03-05 23:56:04]

Please review.

[Ken Russell (switch to Gerrit), 2014-03-05 23:59:23]

The code changes LGTM.

I'd feel more comfortable about committing this if you could describe how it was
tested.

https://codereview.chromium.org/183883026/diff/1/gpu/config/software_renderin...
File gpu/config/software_rendering_list_json.cc (right):

https://codereview.chromium.org/183883026/diff/1/gpu/config/software_renderin...
gpu/config/software_rendering_list_json.cc:351: "perf_overall": {
Is there any way to enter a comment for this? (i.e. "comment": "don't blacklist
accelerated 2D canvas if WinSAT scores haven't been gathered")

[Zhenyao Mo, 2014-03-06 00:04:38]

On 2014/03/05 23:59:23, Ken Russell wrote:
> The code changes LGTM.
> 
> I'd feel more comfortable about committing this if you could describe how it
was
> tested.
> 
>
https://codereview.chromium.org/183883026/diff/1/gpu/config/software_renderin...
> File gpu/config/software_rendering_list_json.cc (right):
> 
>
https://codereview.chromium.org/183883026/diff/1/gpu/config/software_renderin...
> gpu/config/software_rendering_list_json.cc:351: "perf_overall": {
> Is there any way to enter a comment for this? (i.e. "comment": "don't
blacklist
> accelerated 2D canvas if WinSAT scores haven't been gathered")

What is the comment intended?  We can just add it to the .cc file as a c++
comment, if it's for developers to see.  However, if it is for users to see,
then we have to put it as part of the entry description.

As for testing, I only ran through gpu_unitests to make sure data is valid.  As
for whether it has the desired effects on a real machine, since the blacklist
logic with exceptions has been well tested, I am quite confident it will just
work (and we can confirm that once this lands and the new win bot turns green).

[Ken Russell (switch to Gerrit), 2014-03-06 00:38:21]

On 2014/03/06 00:04:38, Zhenyao Mo wrote:
> What is the comment intended?  We can just add it to the .cc file as a c++
> comment, if it's for developers to see.  However, if it is for users to see,
> then we have to put it as part of the entry description.

The comment would be intended for other developers modifying the blacklist in
the future.

Maybe the link to the bug report is sufficient. Don't spend too much time on it.

> As for testing, I only ran through gpu_unitests to make sure data is valid. 
As
> for whether it has the desired effects on a real machine, since the blacklist
> logic with exceptions has been well tested, I am quite confident it will just
> work (and we can confirm that once this lands and the new win bot turns
green).

I was hoping it would have been possible to test this locally on Windows by
deleting the WinSAT assessment and seeing that accelerated 2D canvas is enabled.
If that's too difficult, never mind, let's just land this and watch the bot.

[Zhenyao Mo, 2014-03-06 01:15:13]

https://codereview.chromium.org/183883026/diff/1/gpu/config/software_renderin...
File gpu/config/software_rendering_list_json.cc (right):

https://codereview.chromium.org/183883026/diff/1/gpu/config/software_renderin...
gpu/config/software_rendering_list_json.cc:351: "perf_overall": {
On 2014/03/05 23:59:23, Ken Russell wrote:
> Is there any way to enter a comment for this? (i.e. "comment": "don't
blacklist
> accelerated 2D canvas if WinSAT scores haven't been gathered")

Done.

[Zhenyao Mo, 2014-03-06 01:15:26]

The CQ bit was checked by zmo@chromium.org

[commit-bot: I haz the power, 2014-03-06 01:21:56]

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

[Ken Russell (switch to Gerrit), 2014-03-06 01:21:57]

https://codereview.chromium.org/183883026/diff/20001/gpu/config/software_rend...
File gpu/config/software_rendering_list_json.cc (right):

https://codereview.chromium.org/183883026/diff/20001/gpu/config/software_rend...
gpu/config/software_rendering_list_json.cc:351: // been gathered.
Careful! Comments like these aren't valid JSON. Does this still parse? I don't
know exactly how this file is set up.

[Zhenyao Mo, 2014-03-06 01:25:02]

On 2014/03/06 01:21:57, Ken Russell wrote:
>
https://codereview.chromium.org/183883026/diff/20001/gpu/config/software_rend...
> File gpu/config/software_rendering_list_json.cc (right):
> 
>
https://codereview.chromium.org/183883026/diff/20001/gpu/config/software_rend...
> gpu/config/software_rendering_list_json.cc:351: // been gathered.
> Careful! Comments like these aren't valid JSON. Does this still parse? I don't
> know exactly how this file is set up.

It is OK I think.  We use macro to construct the extra long json string.  So the
comments will just be left out.

[Ken Russell (switch to Gerrit), 2014-03-06 01:29:27]

On 2014/03/06 01:25:02, Zhenyao Mo wrote:
> On 2014/03/06 01:21:57, Ken Russell wrote:
> >
>
https://codereview.chromium.org/183883026/diff/20001/gpu/config/software_rend...
> > File gpu/config/software_rendering_list_json.cc (right):
> > 
> >
>
https://codereview.chromium.org/183883026/diff/20001/gpu/config/software_rend...
> > gpu/config/software_rendering_list_json.cc:351: // been gathered.
> > Careful! Comments like these aren't valid JSON. Does this still parse? I
don't
> > know exactly how this file is set up.
> 
> It is OK I think.  We use macro to construct the extra long json string.  So
the
> comments will just be left out.

Are you sure the C++ style comment won't end up being concatenated and
accidentally comment out the entire rest of the file beyond it?

[Zhenyao Mo, 2014-03-06 01:31:27]

On 2014/03/06 01:29:27, Ken Russell wrote:
> On 2014/03/06 01:25:02, Zhenyao Mo wrote:
> > On 2014/03/06 01:21:57, Ken Russell wrote:
> > >
> >
>
https://codereview.chromium.org/183883026/diff/20001/gpu/config/software_rend...
> > > File gpu/config/software_rendering_list_json.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/183883026/diff/20001/gpu/config/software_rend...
> > > gpu/config/software_rendering_list_json.cc:351: // been gathered.
> > > Careful! Comments like these aren't valid JSON. Does this still parse? I
> don't
> > > know exactly how this file is set up.
> > 
> > It is OK I think.  We use macro to construct the extra long json string.  So
> the
> > comments will just be left out.
> 
> Are you sure the C++ style comment won't end up being concatenated and
> accidentally comment out the entire rest of the file beyond it?

No that's not the case.  That will end up a invalid json file and will fail our
gpu_unittests.

[commit-bot: I haz the power, 2014-03-06 02:19:33]

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

[commit-bot: I haz the power, 2014-03-06 02:19:33]

Retried try job too often on win for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[Zhenyao Mo, 2014-03-06 02:24:24]

Committed patchset #2 manually as r255228 (presubmit successful).