Handle failures for GPU info collection

content/renderer/renderer_blink_platform_impl.cc

Index: content/renderer/renderer_blink_platform_impl.cc
diff --git a/content/renderer/renderer_blink_platform_impl.cc b/content/renderer/renderer_blink_platform_impl.cc
index 9b5d79121e925bd0e21584aeeb210ec30418bee5..90ab6b8608ad78429ff887728927733260ceee2f 100644
--- a/content/renderer/renderer_blink_platform_impl.cc
+++ b/content/renderer/renderer_blink_platform_impl.cc
@@ -979,11 +979,33 @@ RendererBlinkPlatformImpl::createOffscreenGraphicsContext3D(
 
   if (gpu_channel_host.get() && gl_info) {
     const gpu::GPUInfo& gpu_info = gpu_channel_host->gpu_info();
-    gl_info->vendorInfo.assign(blink::WebString::fromUTF8(gpu_info.gl_vendor));
-    gl_info->rendererInfo.assign(
-        blink::WebString::fromUTF8(gpu_info.gl_renderer));
-    gl_info->driverVersion.assign(
-        blink::WebString::fromUTF8(gpu_info.gl_version));
+    if (gpu_info.basic_info_state != gpu::kCollectInfoSuccess) {

[no sievers, 2014/12/12 18:41:26]

I think you want to check |context_info_state| instead since we are interested
in the GL strings here.

[Ken Russell (switch to Gerrit), 2014/12/12 22:43:05]

Agreed; |context_info_state| is what should be checked here.

[sivag, 2014/12/15 13:36:03]

My intention here is to check, whether the GPUInfo is populated or not.

context_info_state can give whether a context creation is success or failure.
The context validity is already checked @1024

But even in context_info_state success, there are cases where empty strings for
GPU infor driver, renderer, vendor are returned (in case of mac), this can be
determined only by basic_info_state fatal/non-fatal reasons which i wanted to
categorize and populate in webglcontextcreationerror status message.

Please correct me if i am wrong.

Though i can validate the info strings just by checking for empty strings, but
what i wanted here is the proper reason why glstrings are not populated.

[no sievers, 2014/12/15 21:18:20]

|basic_info_state| is for CollectBasicGraphicsInfo(), which generally does not
include GL strings, which need a context to be queried. Therefore
|context_info_state| is set in CollectGraphicsInfoGL() which is the info you are
using here.

|context_info_state| has values to tell you exactly what happened, i.e. was not
initialized (kCollectInfoNone), was initialized (kCollectInfoSucces), could not
be initialized (kCollectInfoFatalFailure), could be partially initialized
(kCollectInfoNonFatalFailure).

If the value of the status field does not match what happened in the
initialization, then that needs to be fixed somewhere in that code. But what you
are adding here in the renderer should be added under the assumption that the
status field does make sense.

+      switch (gpu_info.basic_info_state) {
+        // TODO (sikugu): On Mac gpu's this info is not collected, some tests
+        // fail.Need to check the root cause. Other references:crbug.com/222934

[no sievers, 2014/12/12 18:41:26]

nit: space before "Need".. also just say TODO(sikugu): ... (crbug.com/222934)

[sivag, 2014/12/15 13:36:03]

Done.

+        case gpu::kCollectInfoNonFatalFailure:
+          gl_info->basicInfoCollectionFailure.assign(blink::WebString::fromUTF8(

[no sievers, 2014/12/12 18:41:26]

Looks like kCollectInfoNonFatalFailure gets set when an implementation failed to
parse GL_VERSION (i.e. it wasn't in the format that the code expected, and they
all pretty much seem to have specific expectations). GL_VENDOR and GL_RENDERER
might still be valid, because that happens first in the shared code in
CollectGraphicsInfoGL() which then calls the platform-specific
CollectDriverInfoGL().

To not regress anything here you might want to still assign the fields as before
in case of a non-fatal failure.

[sivag, 2014/12/15 13:36:03]

GL_VENDOR, GL_RENDERER, GL_VERSION all these strings are empty in case of mac
gpu, which i need to investigate.

[Zhenyao Mo, 2014/12/15 18:57:12]

I agree with sievers.  You should set the three string values if the state is
not kCollectInfoFatalFailure.  NonFatalFailure can be (now or in the future) the
case partial info collection fails, and you can still benefit from the ones that
are available.

[sivag, 2014/12/16 12:51:28]

Done.

+              "GPUInfoCollectionFailure: Not able to fetch GPU info."));
+          break;
+        case gpu::kCollectInfoFatalFailure:
+          gl_info->contextInfoCollectionFailure.assign(
+              blink::WebString::fromUTF8(
+                  "GPUInfoCollectionFailure: GPU initialization Failed. GPU "
+                  "Info "
+                  "not "

[no sievers, 2014/12/12 18:41:26]

nit: weird formatting, can fit in one line

[sivag, 2014/12/15 13:36:03]

Done.

+                  "collected."));
+          break;
+        default:
+          NOTREACHED();

[no sievers, 2014/12/12 18:46:26]

So we will be hitting this? Because you commented that it ends up being
kCollectInfoNone somewhere.

[Ken Russell (switch to Gerrit), 2014/12/12 22:43:05]

I had the same question.

[sivag, 2014/12/15 13:36:03]

It works fine with browser build.
kCollectInfoNone is never hit. kCollectInfoSuccess is returned.

Tools/Scripts/run-webkit-tests --debug
LayoutTests/fast/canvas/webgl/webgl-error-response.html

While running webkit layout_tests with test_runner this is getting hit.
The gpu info collection flow is different here?

[Zhenyao Mo, 2014/12/15 18:57:12]

layout tests run with content_shell, where this GpuDataManagerImpl::Initialize()
may not be in the start up path.  You need to verify, because that changed a few
times, so I am no longer 100% sure.

[sivag, 2014/12/16 12:51:28]

Done.

+      };
+    } else {
+      gl_info->vendorInfo.assign(
+          blink::WebString::fromUTF8(gpu_info.gl_vendor));
+      gl_info->rendererInfo.assign(
+          blink::WebString::fromUTF8(gpu_info.gl_renderer));
+      gl_info->driverVersion.assign(
+          blink::WebString::fromUTF8(gpu_info.gl_version));
+    }
   }
 
   WebGraphicsContext3DCommandBufferImpl::SharedMemoryLimits limits;