Add bits to indicate which parts of GPUInfo are collected.

Add bits to indicate which parts of GPUInfo are collected.

Also the collection failure/success.

This is to investigate why on some GPU bots gpu info collection randomly fails.

BUG=407739
TEST=gpu bots, gpu_unittests
R=kbr@chromium.org,piman@chromium.org,jschuh@chromium.org,estade@chromium.org

Committed: https://crrev.com/84eae5e4c626a3b61b13b22fd13f859ec410583a
Cr-Commit-Position: refs/heads/master@{#293394}

[Zhenyao Mo, 2014-09-03 00:39:49]

zmo@chromium.org changed reviewers:
+ estade@chromium.org, jschuh@chromium.org, kbr@chromium.org, piman@chromium.org

[Zhenyao Mo, 2014-09-03 00:39:50]

Please review

[Evan Stade, 2014-09-03 00:48:11]

https://codereview.chromium.org/534043002/diff/20001/components/autofill/cont...
File components/autofill/content/browser/risk/fingerprint.cc (right):

https://codereview.chromium.org/534043002/diff/20001/components/autofill/cont...
components/autofill/content/browser/risk/fingerprint.cc:163:
content::GpuDataManager* const gpu_data_manager) {
gpu_data_manager should be const ref

https://codereview.chromium.org/534043002/diff/20001/content/public/browser/g...
File content/public/browser/gpu_data_manager.h (right):

https://codereview.chromium.org/534043002/diff/20001/content/public/browser/g...
content/public/browser/gpu_data_manager.h:66: virtual bool
IsCompleteGpuInfoAvailable(bool includes_extra) const = 0;
Every time this function is called, it's called with a constant value for the
parameter, never a variable. IsCompleteGpuInfoAvailable(true) isn't very self
documenting, instead I think you should add a new function
IsCompleteGpuInfoAvailableIncludingExtra().

[piman, 2014-09-03 00:50:55]

https://codereview.chromium.org/534043002/diff/20001/content/browser/gpu/gpu_...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/534043002/diff/20001/content/browser/gpu/gpu_...
content/browser/gpu/gpu_data_manager_impl_private.cc:386: if
(gpu_info_.dx_diagnostics_info_state == gpu::kCollectInfoNone)
did you mean && includes_extra here?

Otherwise, includes_extra is unused.

https://codereview.chromium.org/534043002/diff/20001/content/gpu/gpu_child_th...
File content/gpu/gpu_child_thread.cc (right):

https://codereview.chromium.org/534043002/diff/20001/content/gpu/gpu_child_th...
content/gpu/gpu_child_thread.cc:200: default:
nit: please enumerate all values and avoid the default: case, to catch potential
additions to the enum.

https://codereview.chromium.org/534043002/diff/20001/content/gpu/gpu_main.cc
File content/gpu/gpu_main.cc (right):

https://codereview.chromium.org/534043002/diff/20001/content/gpu/gpu_main.cc#...
content/gpu/gpu_main.cc:408: default:
nit: same here

https://codereview.chromium.org/534043002/diff/20001/gpu/config/gpu_info.h
File gpu/config/gpu_info.h (right):

https://codereview.chromium.org/534043002/diff/20001/gpu/config/gpu_info.h#ne...
gpu/config/gpu_info.h:33: kCollectInfoNotSupported = 2,
Do we really need to discriminate this from NonFatalFailure? I'm not sure what
"Not supported" means from the client point of view. Is it fatal (i.e. we should
disable the GPU) or not?

https://codereview.chromium.org/534043002/diff/20001/gpu/config/gpu_info_coll...
File gpu/config/gpu_info_collector_win.cc (right):

https://codereview.chromium.org/534043002/diff/20001/gpu/config/gpu_info_coll...
gpu/config/gpu_info_collector_win.cc:456: return found ? kCollectInfoSuccess ?
kCollectInfoNonFatalFailure;
typo?

[Zhenyao Mo, 2014-09-03 01:31:23]

https://codereview.chromium.org/534043002/diff/20001/components/autofill/cont...
File components/autofill/content/browser/risk/fingerprint.cc (right):

https://codereview.chromium.org/534043002/diff/20001/components/autofill/cont...
components/autofill/content/browser/risk/fingerprint.cc:163:
content::GpuDataManager* const gpu_data_manager) {
On 2014/09/03 00:48:11, Evan Stade wrote:
> gpu_data_manager should be const ref

Done.

Will the ref type works the same as pointer type, and call the right
implementation class instead of the base class function?

https://codereview.chromium.org/534043002/diff/20001/content/browser/gpu/gpu_...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/534043002/diff/20001/content/browser/gpu/gpu_...
content/browser/gpu/gpu_data_manager_impl_private.cc:386: if
(gpu_info_.dx_diagnostics_info_state == gpu::kCollectInfoNone)
On 2014/09/03 00:50:55, piman (OOO) wrote:
> did you mean && includes_extra here?
> 
> Otherwise, includes_extra is unused.

Per estatde's comment, I divide into two functions.

https://codereview.chromium.org/534043002/diff/20001/content/gpu/gpu_child_th...
File content/gpu/gpu_child_thread.cc (right):

https://codereview.chromium.org/534043002/diff/20001/content/gpu/gpu_child_th...
content/gpu/gpu_child_thread.cc:200: default:
On 2014/09/03 00:50:55, piman (OOO) wrote:
> nit: please enumerate all values and avoid the default: case, to catch
potential
> additions to the enum.

Done.

https://codereview.chromium.org/534043002/diff/20001/content/gpu/gpu_main.cc
File content/gpu/gpu_main.cc (right):

https://codereview.chromium.org/534043002/diff/20001/content/gpu/gpu_main.cc#...
content/gpu/gpu_main.cc:408: default:
On 2014/09/03 00:50:55, piman (OOO) wrote:
> nit: same here

Done.

https://codereview.chromium.org/534043002/diff/20001/content/public/browser/g...
File content/public/browser/gpu_data_manager.h (right):

https://codereview.chromium.org/534043002/diff/20001/content/public/browser/g...
content/public/browser/gpu_data_manager.h:66: virtual bool
IsCompleteGpuInfoAvailable(bool includes_extra) const = 0;
On 2014/09/03 00:48:11, Evan Stade wrote:
> Every time this function is called, it's called with a constant value for the
> parameter, never a variable. IsCompleteGpuInfoAvailable(true) isn't very self
> documenting, instead I think you should add a new function
> IsCompleteGpuInfoAvailableIncludingExtra().

Done.

https://codereview.chromium.org/534043002/diff/20001/gpu/config/gpu_info.h
File gpu/config/gpu_info.h (right):

https://codereview.chromium.org/534043002/diff/20001/gpu/config/gpu_info.h#ne...
gpu/config/gpu_info.h:33: kCollectInfoNotSupported = 2,
On 2014/09/03 00:50:55, piman (OOO) wrote:
> Do we really need to discriminate this from NonFatalFailure? I'm not sure what
> "Not supported" means from the client point of view. Is it fatal (i.e. we
should
> disable the GPU) or not?

Done.

https://codereview.chromium.org/534043002/diff/20001/gpu/config/gpu_info_coll...
File gpu/config/gpu_info_collector_win.cc (right):

https://codereview.chromium.org/534043002/diff/20001/gpu/config/gpu_info_coll...
gpu/config/gpu_info_collector_win.cc:456: return found ? kCollectInfoSuccess ?
kCollectInfoNonFatalFailure;
On 2014/09/03 00:50:55, piman (OOO) wrote:
> typo?

Done.

[piman, 2014-09-03 02:02:21]

lgtm

https://codereview.chromium.org/534043002/diff/40001/content/gpu/gpu_child_th...
File content/gpu/gpu_child_thread.cc (right):

https://codereview.chromium.org/534043002/diff/40001/content/gpu/gpu_child_th...
content/gpu/gpu_child_thread.cc:201: NOTIMPLEMENTED();
nit: NOTREACHED()? I don't think it can happen.

https://codereview.chromium.org/534043002/diff/40001/content/gpu/gpu_main.cc
File content/gpu/gpu_main.cc (right):

https://codereview.chromium.org/534043002/diff/40001/content/gpu/gpu_main.cc#...
content/gpu/gpu_main.cc:409: NOTIMPLEMENTED();
(same here)

[Zhenyao Mo, 2014-09-03 02:10:28]

https://codereview.chromium.org/534043002/diff/40001/content/gpu/gpu_child_th...
File content/gpu/gpu_child_thread.cc (right):

https://codereview.chromium.org/534043002/diff/40001/content/gpu/gpu_child_th...
content/gpu/gpu_child_thread.cc:201: NOTIMPLEMENTED();
On 2014/09/03 02:02:20, piman (OOO) wrote:
> nit: NOTREACHED()? I don't think it can happen.

Done.

https://codereview.chromium.org/534043002/diff/40001/content/gpu/gpu_main.cc
File content/gpu/gpu_main.cc (right):

https://codereview.chromium.org/534043002/diff/40001/content/gpu/gpu_main.cc#...
content/gpu/gpu_main.cc:409: NOTIMPLEMENTED();
On 2014/09/03 02:02:20, piman (OOO) wrote:
> (same here)

Done.

[Zhenyao Mo, 2014-09-03 19:52:50]

All bots are green now.  Please take a look.

estade: chrome/autofill owner
jschuh: gpu_messages.h owner

[Evan Stade, 2014-09-03 21:47:58]

lgtm

https://codereview.chromium.org/534043002/diff/20001/components/autofill/cont...
File components/autofill/content/browser/risk/fingerprint.cc (right):

https://codereview.chromium.org/534043002/diff/20001/components/autofill/cont...
components/autofill/content/browser/risk/fingerprint.cc:163:
content::GpuDataManager* const gpu_data_manager) {
On 2014/09/03 01:31:23, Zhenyao Mo wrote:
> On 2014/09/03 00:48:11, Evan Stade wrote:
> > gpu_data_manager should be const ref
> 
> Done.
> 
> Will the ref type works the same as pointer type, and call the right
> implementation class instead of the base class function?

yep

https://codereview.chromium.org/534043002/diff/80001/content/browser/gpu/gpu_...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/534043002/diff/80001/content/browser/gpu/gpu_...
content/browser/gpu/gpu_data_manager_impl_private.cc:382:
gpu_info_.context_info_state == gpu::kCollectInfoNone)
nit: curlies

https://codereview.chromium.org/534043002/diff/80001/content/browser/gpu/gpu_...
content/browser/gpu/gpu_data_manager_impl_private.cc:387: bool
GpuDataManagerImplPrivate::IsCompleteGpuInfoAvailable() const {
 {
#if defined(OS_WIN)
  ...
#endif
  return IsEssentialGpuInfoAvailable();
}

https://codereview.chromium.org/534043002/diff/80001/content/public/browser/g...
File content/public/browser/gpu_data_manager.h (right):

https://codereview.chromium.org/534043002/diff/80001/content/public/browser/g...
content/public/browser/gpu_data_manager.h:64: virtual bool
IsEssentialGpuInfoAvailable() const = 0;
\n

[Zhenyao Mo, 2014-09-03 21:56:10]

jschuh: ping.

https://codereview.chromium.org/534043002/diff/80001/content/browser/gpu/gpu_...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/534043002/diff/80001/content/browser/gpu/gpu_...
content/browser/gpu/gpu_data_manager_impl_private.cc:382:
gpu_info_.context_info_state == gpu::kCollectInfoNone)
On 2014/09/03 21:47:58, Evan Stade wrote:
> nit: curlies

Done.

https://codereview.chromium.org/534043002/diff/80001/content/browser/gpu/gpu_...
content/browser/gpu/gpu_data_manager_impl_private.cc:387: bool
GpuDataManagerImplPrivate::IsCompleteGpuInfoAvailable() const {
On 2014/09/03 21:47:58, Evan Stade wrote:
>  {
> #if defined(OS_WIN)
>   ...
> #endif
>   return IsEssentialGpuInfoAvailable();
> }

Done.

https://codereview.chromium.org/534043002/diff/80001/content/public/browser/g...
File content/public/browser/gpu_data_manager.h (right):

https://codereview.chromium.org/534043002/diff/80001/content/public/browser/g...
content/public/browser/gpu_data_manager.h:64: virtual bool
IsEssentialGpuInfoAvailable() const = 0;
On 2014/09/03 21:47:58, Evan Stade wrote:
> \n

Done.

[Ken Russell (switch to Gerrit), 2014-09-03 22:22:01]

LGTM with one comment.

https://codereview.chromium.org/534043002/diff/80001/gpu/config/gpu_info.h
File gpu/config/gpu_info.h (right):

https://codereview.chromium.org/534043002/diff/80001/gpu/config/gpu_info.h#ne...
gpu/config/gpu_info.h:34: kCollectInfoFatalFailure = 3
Something to consider for a follow-on CL: perhaps this enum could contain ranges
of fatal and non-fatal failures and there could be an API for categorizing a
particular code. I wonder whether the current statuses will provide enough
information to track down exactly what part of GPU info collection failed.

[Zhenyao Mo, 2014-09-03 22:26:29]

On 2014/09/03 22:22:01, Ken Russell wrote:
> LGTM with one comment.
> 
> https://codereview.chromium.org/534043002/diff/80001/gpu/config/gpu_info.h
> File gpu/config/gpu_info.h (right):
> 
>
https://codereview.chromium.org/534043002/diff/80001/gpu/config/gpu_info.h#ne...
> gpu/config/gpu_info.h:34: kCollectInfoFatalFailure = 3
> Something to consider for a follow-on CL: perhaps this enum could contain
ranges
> of fatal and non-fatal failures and there could be an API for categorizing a
> particular code. I wonder whether the current statuses will provide enough
> information to track down exactly what part of GPU info collection failed.

Good suggestion.  Will follow up on this.

[Zhenyao Mo, 2014-09-04 20:55:42]

zmo@chromium.org changed reviewers:
+ jln@chromium.org
- jschuh@chromium.org

[Zhenyao Mo, 2014-09-04 20:56:07]

+jln: gpu_messages.h

[Zhenyao Mo, 2014-09-04 21:41:25]

zmo@chromium.org changed reviewers:
+ palmer@chromium.org, tsepez@chromium.org

[Zhenyao Mo, 2014-09-04 21:41:26]

Also adds tsepez and palmer.  One of you please take a look at gpu_messages.h

[Tom Sepez, 2014-09-04 21:48:36]

Messages LGTM.

[Zhenyao Mo, 2014-09-04 21:49:26]

The CQ bit was checked by zmo@chromium.org

[palmer, 2014-09-04 21:52:14]

LGTM

[commit-bot: I haz the power, 2014-09-04 21:53:13]

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

[commit-bot: I haz the power, 2014-09-05 01:37:29]

Committed patchset #6 (id:100001) as 96f8f48a12152b590cd5682d0a7e97e2a49f893f

[commit-bot: I haz the power, 2014-09-10 03:35:15]

Patchset 6 (id:??) landed as
https://crrev.com/84eae5e4c626a3b61b13b22fd13f859ec410583a
Cr-Commit-Position: refs/heads/master@{#293394}