Retrieve Windows performance assessment information from results files.

Retrieve Windows performance assessment information from results files.


TBR=ben@chromium.org
BUG=chromium:122838, chromium:124325
TEST=Builds on all platforms, and chrome://gpu gives a correct rating after the user re-runs the WinSAT assessment.

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

[Zhenyao Mo, 2012-04-20 00:10:50]

https://chromiumcodereview.appspot.com/10128002/diff/16001/base/base.gypi
File base/base.gypi (right):

https://chromiumcodereview.appspot.com/10128002/diff/16001/base/base.gypi#new...
base/base.gypi:808: '../third_party/libxml/libxml.gyp:libxml',
This should be without ../

See the next line as an example

[vangelis, 2012-04-24 00:37:29]

https://chromiumcodereview.appspot.com/10128002/diff/16001/content/gpu/gpu_in...
File content/gpu/gpu_info_collector_win.cc (right):

https://chromiumcodereview.appspot.com/10128002/diff/16001/content/gpu/gpu_in...
content/gpu/gpu_info_collector_win.cc:53: std::string assessment) {
Watch indentation.

https://chromiumcodereview.appspot.com/10128002/diff/16001/content/gpu/gpu_in...
content/gpu/gpu_info_collector_win.cc:107: file_util::FileEnumerator
file_enumerator(
Given the flakiness of the winsat api, we should always use the xml file for
retrieving the data.

https://chromiumcodereview.appspot.com/10128002/diff/16001/content/gpu/gpu_in...
content/gpu/gpu_info_collector_win.cc:108:
FilePath(FILE_PATH_LITERAL("c:/Windows/Performance/WinSAT/DataStore/")),
You should probably use %windir% instead of hardcoding c:/windows .

https://chromiumcodereview.appspot.com/10128002/diff/16001/content/gpu/gpu_in...
content/gpu/gpu_info_collector_win.cc:128: stats.overall =
GetAssessmentScoreFromFile(&reader, "SystemScore");
The WinSAT xml schema is here:
http://msdn.microsoft.com/en-us/library/windows/desktop/aa969210(v=vs.85).aspx

It would be preferable to look for the <WinSPR> section first instead of doing
global searches for the stats.

https://chromiumcodereview.appspot.com/10128002/diff/16001/third_party/libxml...
File third_party/libxml/libxml.gyp (right):

https://chromiumcodereview.appspot.com/10128002/diff/16001/third_party/libxml...
third_party/libxml/libxml.gyp:21: 'toolsets': ['host', 'target'],
What's the purpose of this line?  I wonder if it is what's causing the trybot
build failures.

[dtu, 2012-05-01 00:32:15]

brettw said libxml_utils should not go into base because it will introduce a new
dependency there. Therefore, moved libxml_utils into third_party/libxml.

Note that moving libxml_utils is in this CL:
https://chromiumcodereview.appspot.com/10251004/

And the only files that are changed in this CL after the above is merged are the
ones under content.

https://chromiumcodereview.appspot.com/10128002/diff/16001/base/base.gypi
File base/base.gypi (right):

https://chromiumcodereview.appspot.com/10128002/diff/16001/base/base.gypi#new...
base/base.gypi:808: '../third_party/libxml/libxml.gyp:libxml',
On 2012/04/20 00:10:51, Zhenyao Mo wrote:
> This should be without ../
> 
> See the next line as an example

No, dynamic_annotations is in src/base/third_party/ but libxml is in
src/third_party/.

https://chromiumcodereview.appspot.com/10128002/diff/16001/content/gpu/gpu_in...
File content/gpu/gpu_info_collector_win.cc (right):

https://chromiumcodereview.appspot.com/10128002/diff/16001/content/gpu/gpu_in...
content/gpu/gpu_info_collector_win.cc:53: std::string assessment) {
On 2012/04/24 00:37:30, vangelis wrote:
> Watch indentation.

Done.

https://chromiumcodereview.appspot.com/10128002/diff/16001/content/gpu/gpu_in...
content/gpu/gpu_info_collector_win.cc:107: file_util::FileEnumerator
file_enumerator(
On 2012/04/24 00:37:30, vangelis wrote:
> Given the flakiness of the winsat api, we should always use the xml file for
> retrieving the data.

Done.

https://chromiumcodereview.appspot.com/10128002/diff/16001/content/gpu/gpu_in...
content/gpu/gpu_info_collector_win.cc:108:
FilePath(FILE_PATH_LITERAL("c:/Windows/Performance/WinSAT/DataStore/")),
On 2012/04/24 00:37:30, vangelis wrote:
> You should probably use %windir% instead of hardcoding c:/windows .

Done.

https://chromiumcodereview.appspot.com/10128002/diff/16001/content/gpu/gpu_in...
content/gpu/gpu_info_collector_win.cc:128: stats.overall =
GetAssessmentScoreFromFile(&reader, "SystemScore");
On 2012/04/24 00:37:30, vangelis wrote:
> The WinSAT xml schema is here:
>
http://msdn.microsoft.com/en-us/library/windows/desktop/aa969210%28v=vs.85%29...
> 
> It would be preferable to look for the <WinSPR> section first instead of doing
> global searches for the stats.  

Done.

https://chromiumcodereview.appspot.com/10128002/diff/16001/third_party/libxml...
File third_party/libxml/libxml.gyp (right):

https://chromiumcodereview.appspot.com/10128002/diff/16001/third_party/libxml...
third_party/libxml/libxml.gyp:21: 'toolsets': ['host', 'target'],
On 2012/04/24 00:37:30, vangelis wrote:
> What's the purpose of this line?  I wonder if it is what's causing the trybot
> build failures.

Previously the failure was showing for libxml.gyp when referenced from
base.gypi. Adding this line moved the error down the dependency tree by one
level.

[Zhenyao Mo, 2012-05-01 18:11:52]

Are there other examples that we put our wrapper code into the original package
in the third_party?  If not, maybe it's not a good idea to do this.

Overall looks good other wise with one nit.

https://chromiumcodereview.appspot.com/10128002/diff/65023/content/gpu/gpu_in...
File content/gpu/gpu_info_collector_win.cc (right):

https://chromiumcodereview.appspot.com/10128002/diff/65023/content/gpu/gpu_in...
content/gpu/gpu_info_collector_win.cc:78: if
(FilePath::CompareLessIgnoreCase(current_results.value(),
Can you add a comment here to explain how Windows naming the results files and
why CompareLessIgnoreCase will give us the most recent results?

Also, it's better putting the body of for-clause into { } when it's multiple
lines.

[dtu, 2012-05-02 23:58:01]

https://chromiumcodereview.appspot.com/10128002/diff/65023/content/gpu/gpu_in...
File content/gpu/gpu_info_collector_win.cc (right):

https://chromiumcodereview.appspot.com/10128002/diff/65023/content/gpu/gpu_in...
content/gpu/gpu_info_collector_win.cc:78: if
(FilePath::CompareLessIgnoreCase(current_results.value(),
On 2012/05/01 18:11:53, Zhenyao Mo wrote:
> Can you add a comment here to explain how Windows naming the results files and
> why CompareLessIgnoreCase will give us the most recent results?
> 
> Also, it's better putting the body of for-clause into { } when it's multiple
> lines.

Done.

[Zhenyao Mo, 2012-05-02 23:59:57]

LGTM once libxml_utils issue is agreed upon

[vangelis, 2012-05-03 20:45:36]

Looks good with the exception of one comment. What's the consensus  on
libxml_util?

https://chromiumcodereview.appspot.com/10128002/diff/79002/content/gpu/gpu_in...
File content/gpu/gpu_info_collector_win.cc (right):

https://chromiumcodereview.appspot.com/10128002/diff/79002/content/gpu/gpu_in...
content/gpu/gpu_info_collector_win.cc:102: reader.Read();  // Descend into
<WinSPR> element.
You should probably check here that you did indeed find the WinSPR element
before proceeding.

[Zhenyao Mo, 2012-05-14 17:20:01]

David, any progress on this CL?  We definitely need to land this as soon as
possible and merge it to M20 before Beta.  Otherwise we will encounter a lof of
crashes on Windows.

[dtu, 2012-05-15 21:59:57]

The libxml_utils change went in, and I've merged to include it.

http://codereview.chromium.org/10128002/diff/79002/content/gpu/gpu_info_colle...
File content/gpu/gpu_info_collector_win.cc (right):

http://codereview.chromium.org/10128002/diff/79002/content/gpu/gpu_info_colle...
content/gpu/gpu_info_collector_win.cc:102: reader.Read();  // Descend into
<WinSPR> element.
On 2012/05/03 20:45:37, vangelis wrote:
> You should probably check here that you did indeed find the WinSPR element
> before proceeding.

Done.

[dharani, 2012-05-15 22:37:57]

we need to get this change landed in trunk and merge it M20 asap. since this has
a fix for GPU's #1 crash, this should go in M20 before it goes to beta.

[dtu, 2012-05-16 00:47:25]

The try bots are passing, so it seems like it's ready. PTAL.

[vangelis, 2012-05-16 16:34:21]

I would like if possible to include some histograms as part of this:

1. The elapsed time to read the XML file (and possibly a different one for the
entire gpu info collection). This is going to affect startup times.
2. The stats values collected to make sure that everything works as expected in
the field.  

Otherwise the code looks good!

[vangelis, 2012-05-16 16:36:52]

http://codereview.chromium.org/10128002/diff/96004/content/gpu/gpu_info_colle...
File content/gpu/gpu_info_collector_win.cc (right):

http://codereview.chromium.org/10128002/diff/96004/content/gpu/gpu_info_colle...
content/gpu/gpu_info_collector_win.cc:143: gpu_info->performance_stats =
RetrieveGpuPerformanceStats();
We should call this on windows only.

[vangelis, 2012-05-16 18:45:08]

On 2012/05/16 16:36:52, vangelis wrote:
>
http://codereview.chromium.org/10128002/diff/96004/content/gpu/gpu_info_colle...
> File content/gpu/gpu_info_collector_win.cc (right):
> 
>
http://codereview.chromium.org/10128002/diff/96004/content/gpu/gpu_info_colle...
> content/gpu/gpu_info_collector_win.cc:143: gpu_info->performance_stats =
> RetrieveGpuPerformanceStats();
> We should call this on windows only.

In the interest of getting this checked in today, let's just take care of the
comment about calling the info collection on windows only and put the histograms
in place in a subsequent patch.

LGTM

[dtu, 2012-05-16 18:46:57]

http://codereview.chromium.org/10128002/diff/96004/content/gpu/gpu_info_colle...
File content/gpu/gpu_info_collector_win.cc (right):

http://codereview.chromium.org/10128002/diff/96004/content/gpu/gpu_info_colle...
content/gpu/gpu_info_collector_win.cc:143: gpu_info->performance_stats =
RetrieveGpuPerformanceStats();
On 2012/05/16 16:36:52, vangelis wrote:
> We should call this on windows only.

Isn't this entire file Windows-only?

[vangelis, 2012-05-16 19:06:45]

On 2012/05/16 18:46:57, Dave Tu wrote:
>
http://codereview.chromium.org/10128002/diff/96004/content/gpu/gpu_info_colle...
> File content/gpu/gpu_info_collector_win.cc (right):
> 
>
http://codereview.chromium.org/10128002/diff/96004/content/gpu/gpu_info_colle...
> content/gpu/gpu_info_collector_win.cc:143: gpu_info->performance_stats =
> RetrieveGpuPerformanceStats();
> On 2012/05/16 16:36:52, vangelis wrote:
> > We should call this on windows only.
> 
> Isn't this entire file Windows-only?

Oh, darn, of course it is...

[dtu, 2012-05-16 21:19:56]

Histograms added.

[dtu, 2012-05-16 23:00:50]

+ben, need OWNER for content, content/gpu.

[apatrick_chromium, 2012-05-16 23:48:36]

content/gpu/OWNERS LGTM

[Ben Goodger (Google), 2012-05-17 16:04:25]

replacing myself with jam