Add a JS API for detecting WebGL availability.

Add a JS API for detecting WebGL availability.

Move GpuInfoUpdate to UI thread to avoid potential racing.

BUG=104768
TEST=ExtensionWebstorePrivateBundleTest.GetWebGLStatus
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113688

[Zhenyao Mo, 2011-12-02 19:50:44]

Ken, can you review the GpuDataManager part?
Mihai, can you review the extension API part?

The added browser tests pass on my local Mac.

[Ken Russell (switch to Gerrit), 2011-12-03 02:59:41]

http://codereview.chromium.org/8772031/diff/5008/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_webstore_private_api.cc (right):

http://codereview.chromium.org/8772031/diff/5008/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_webstore_private_api.cc:542:
manager->AddObserver(this);
I don't fully understand the multithreading in this code, but is it possible for
the full GPU information to become available between the time that
complete_gpu_info_available() is called above, and when AddObserver is called
here? If so, then is it possible the observer will never be called because it is
added after the event occurs of the full GPU info becoming available?

http://codereview.chromium.org/8772031/diff/5008/content/browser/gpu/gpu_data...
File content/browser/gpu/gpu_data_manager.h (right):

http://codereview.chromium.org/8772031/diff/5008/content/browser/gpu/gpu_data...
content/browser/gpu/gpu_data_manager.h:229: bool complete_gpu_info_available_;
I don't fully understand the threading model in this code, but is this variable
written and tested by different threads?

[Aaron Boodman, 2011-12-03 04:40:34]

Is this supposed to be available to all web pages? or just app pages? Because
the chrome.webstorePrivate namespace is only exposed to the web store, not any
other kind of web page.

[Zhenyao Mo, 2011-12-03 07:13:18]

On 2011/12/03 04:40:34, Aaron Boodman wrote:
> Is this supposed to be available to all web pages? or just app pages? Because
> the chrome.webstorePrivate namespace is only exposed to the web store, not any
> other kind of web page.

this api is for webstore only.

[Zhenyao Mo, 2011-12-03 07:13:27]

http://codereview.chromium.org/8772031/diff/5008/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_webstore_private_api.cc (right):

http://codereview.chromium.org/8772031/diff/5008/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_webstore_private_api.cc:542:
manager->AddObserver(this);
On 2011/12/03 02:59:42, kbr wrote:
> I don't fully understand the multithreading in this code, but is it possible
for
> the full GPU information to become available between the time that
> complete_gpu_info_available() is called above, and when AddObserver is called
> here? If so, then is it possible the observer will never be called because it
is
> added after the event occurs of the full GPU info becoming available?

Ah you are right.  I need to think more on this.

[Zhenyao Mo, 2011-12-05 23:45:25]

Addressed the potential racing issue by moving everything to UI thread.  The
webstore api is called from UI thread, so if I move UpdateGpuInfo to
GpuProcessHostUIShim, then there will be no racing.

Also, use osmesa for the tests because on Linux, we need to launch GPU process
to make sure WebGL is allowed, and try bots fail creating gl context if not
running on top of osmesa.

Please review.

[Mihai Parparita -not on Chrome, 2011-12-06 02:41:54]

On Fri, Dec 2, 2011 at 11:13 PM, <zmo@chromium.org> wrote:

> On 2011/12/03 04:40:34, Aaron Boodman wrote:
>
>> Is this supposed to be available to all web pages? or just app pages?
>> Because
>> the chrome.webstorePrivate namespace is only exposed to the web store,
>> not any
>> other kind of web page.
>>
>
> this api is for webstore only.
>
>
http://codereview.chromium.**org/8772031/<http://codereview.chromium.org/8772...
>

The specific usecase is the webstore wanting to have a fast way of
detecting if WebGL is available, so that users can be warned if they try to
install an app that requires it. All regular web pages can do right now is
try to create a WebGL context, which is slow (since it requires starting
the GPU process).

Have the Chromium GPU folks heard of other WebGL users wanting this
functionality (e.g. from MapsGL?). It might make sense to expose it to more
pages down the line (though I'm fine with it being a webstore-only API for
now).

Mihai

[Mihai Parparita -not on Chrome, 2011-12-06 02:58:11]

http://codereview.chromium.org/8772031/diff/12022/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_webstore_private_api.cc (right):

http://codereview.chromium.org/8772031/diff/12022/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webstore_private_api.cc:515:
result_.reset(Value::CreateStringValue(
Rather than repeating the result creation code twice, can you move it to a
helper CreateResult method?

http://codereview.chromium.org/8772031/diff/12022/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webstore_private_api.cc:519: Release();
The convention in extensions code is to add comments next to Release/AddRef
calls indicating where they get balanced (see uses elsewhere in this file).

http://codereview.chromium.org/8772031/diff/12022/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webstore_private_api.cc:534: finalized =
true;
This confused me initially, perhaps add comments to IsWebGLAllowed indicating
that a false return value is always valid, but a true one is only valid if GPU
info has been loaded?

http://codereview.chromium.org/8772031/diff/12022/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_webstore_private_apitest.cc (right):

http://codereview.chromium.org/8772031/diff/12022/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webstore_private_apitest.cc:243: // Tests
getWebGLStatus function when webgl is allowed.
Nit: Capitalize WebGL (applies below too).

http://codereview.chromium.org/8772031/diff/12022/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webstore_private_apitest.cc:247:
RunPageTest(GetTestServerURL("get_webgl_status_allowed.html").spec()));
For testing single function behavior, rather than using api tests and
RunPageTest, the preferred new way is to use RunFunctionAndReturnError to invoke
the ExtensionFunction implementation directly. See extension_tabs_test.cc or
socket_apitest.cc for samples of how it's used.

http://codereview.chromium.org/8772031/diff/12022/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/8772031/diff/12022/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:8101: "type": "string",
The type of this should be "enum", with the possible values.

[Zhenyao Mo, 2011-12-06 20:00:15]

http://codereview.chromium.org/8772031/diff/12022/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_webstore_private_api.cc (right):

http://codereview.chromium.org/8772031/diff/12022/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webstore_private_api.cc:515:
result_.reset(Value::CreateStringValue(
On 2011/12/06 02:58:11, Mihai Parparita wrote:
> Rather than repeating the result creation code twice, can you move it to a
> helper CreateResult method?

Done.

http://codereview.chromium.org/8772031/diff/12022/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webstore_private_api.cc:519: Release();
On 2011/12/06 02:58:11, Mihai Parparita wrote:
> The convention in extensions code is to add comments next to Release/AddRef
> calls indicating where they get balanced (see uses elsewhere in this file).

Done.

http://codereview.chromium.org/8772031/diff/12022/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webstore_private_api.cc:534: finalized =
true;
On 2011/12/06 02:58:11, Mihai Parparita wrote:
> This confused me initially, perhaps add comments to IsWebGLAllowed indicating
> that a false return value is always valid, but a true one is only valid if GPU
> info has been loaded?

Done.  Added comment in .h file

http://codereview.chromium.org/8772031/diff/12022/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_webstore_private_apitest.cc (right):

http://codereview.chromium.org/8772031/diff/12022/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webstore_private_apitest.cc:243: // Tests
getWebGLStatus function when webgl is allowed.
On 2011/12/06 02:58:11, Mihai Parparita wrote:
> Nit: Capitalize WebGL (applies below too).

Done.

http://codereview.chromium.org/8772031/diff/12022/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webstore_private_apitest.cc:247:
RunPageTest(GetTestServerURL("get_webgl_status_allowed.html").spec()));
On 2011/12/06 02:58:11, Mihai Parparita wrote:
> For testing single function behavior, rather than using api tests and
> RunPageTest, the preferred new way is to use RunFunctionAndReturnError to
invoke
> the ExtensionFunction implementation directly. See extension_tabs_test.cc or
> socket_apitest.cc for samples of how it's used.

I tried, but it didn't work because RunFunctionAndReturnError or
RunFunctionAndReturnResult expect a sync function, so it expects HasResponse()
to be false, where in this function, HasResponse() is true.

http://codereview.chromium.org/8772031/diff/12022/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/8772031/diff/12022/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:8101: "type": "string",
On 2011/12/06 02:58:11, Mihai Parparita wrote:
> The type of this should be "enum", with the possible values.

Done.

[Zhenyao Mo, 2011-12-06 20:19:57]

http://codereview.chromium.org/8772031/diff/12022/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_webstore_private_apitest.cc (right):

http://codereview.chromium.org/8772031/diff/12022/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webstore_private_apitest.cc:247:
RunPageTest(GetTestServerURL("get_webgl_status_allowed.html").spec()));
On 2011/12/06 20:00:15, Zhenyao Mo wrote:
> On 2011/12/06 02:58:11, Mihai Parparita wrote:
> > For testing single function behavior, rather than using api tests and
> > RunPageTest, the preferred new way is to use RunFunctionAndReturnError to
> invoke
> > the ExtensionFunction implementation directly. See extension_tabs_test.cc or
> > socket_apitest.cc for samples of how it's used.
> 
> I tried, but it didn't work because RunFunctionAndReturnError or
> RunFunctionAndReturnResult expect a sync function, so it expects HasResponse()
> to be false, where in this function, HasResponse() is true.

Never mind.  It's a bug in my own code.  I switched to
RunFunctionAndReturnResult.

[Ken Russell (switch to Gerrit), 2011-12-06 22:42:04]

LGTM to me overall. One question related to the tests, but the GpuDataManager
side looks fine.

http://codereview.chromium.org/8772031/diff/24014/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_webstore_private_apitest.cc (right):

http://codereview.chromium.org/8772031/diff/24014/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webstore_private_apitest.cc:249:
GetWebGLStatusAllowed) {
Are these tests run in the same process, and if so, isn't there a dependency in
the order of running these tests? Since this one doesn't initialize the
blacklist in the GpuDataManager, it expects to run before the one following it.
Could you try switching their order in the file and see if this starts failing?
If it does then more setup code is needed.

[Zhenyao Mo, 2011-12-06 22:43:27]

On 2011/12/06 22:42:04, kbr wrote:
> LGTM to me overall. One question related to the tests, but the GpuDataManager
> side looks fine.
> 
>
http://codereview.chromium.org/8772031/diff/24014/chrome/browser/extensions/e...
> File chrome/browser/extensions/extension_webstore_private_apitest.cc (right):
> 
>
http://codereview.chromium.org/8772031/diff/24014/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_webstore_private_apitest.cc:249:
> GetWebGLStatusAllowed) {
> Are these tests run in the same process, and if so, isn't there a dependency
in
> the order of running these tests? Since this one doesn't initialize the
> blacklist in the GpuDataManager, it expects to run before the one following
it.
> Could you try switching their order in the file and see if this starts
failing?
> If it does then more setup code is needed.

I verified this before: each browser test run with a different process id.

[Mihai Parparita -not on Chrome, 2011-12-07 00:01:38]

http://codereview.chromium.org/8772031/diff/24014/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_webstore_private_api.cc (right):

http://codereview.chromium.org/8772031/diff/24014/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webstore_private_api.cc:529: #if
defined(OS_LINUX)
Can you comment why Linux gets this special treatment? Better yet, can this
knowledge be in GpuDataManager (i.e. should complete_gpu_info_available always
return true on Mac and Windows?)

http://codereview.chromium.org/8772031/diff/24014/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_webstore_private_api.h (right):

http://codereview.chromium.org/8772031/diff/24014/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webstore_private_api.h:167: virtual void
OnGpuInfoUpdate() OVERRIDE;
A comment indicating that this implements GpuDataManager::Observer would be
consistent with the rest of the functions in this file.

http://codereview.chromium.org/8772031/diff/24014/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webstore_private_api.h:173: void
CreateResult(bool webgl_allowed);
CreateResult and IsWebGLAllowed should be private.

http://codereview.chromium.org/8772031/diff/24014/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_webstore_private_apitest.cc (right):

http://codereview.chromium.org/8772031/diff/24014/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webstore_private_apitest.cc:96: // In linux,
we need to launch GPU process to decide if WebGL is allowed.
Can you move this to a separate subclass that you use for the GPU function
tests? Also, that subclass doesn't need to extend ExtensionApiTest, it can just
use InProcessBrowserTest.

http://codereview.chromium.org/8772031/diff/24014/chrome/test/data/extensions...
File
chrome/test/data/extensions/api_test/webstore_private/get_webgl_status_allowed.html
(right):

http://codereview.chromium.org/8772031/diff/24014/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/webstore_private/get_webgl_status_allowed.html:1:
<script src="common.js"></script>
This file (and the other test file) can be reverted now.

[Zhenyao Mo, 2011-12-07 17:26:49]

http://codereview.chromium.org/8772031/diff/24014/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_webstore_private_api.cc (right):

http://codereview.chromium.org/8772031/diff/24014/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webstore_private_api.cc:529: #if
defined(OS_LINUX)
On 2011/12/07 00:01:38, Mihai Parparita wrote:
> Can you comment why Linux gets this special treatment? Better yet, can this
> knowledge be in GpuDataManager (i.e. should complete_gpu_info_available always
> return true on Mac and Windows?) 

On Mac/Windows, gpu info is also incomplete without gpu process launching.  Only
that so far the partial information is always enough to make the final blacklist
decision.  So it's rather a hack here to speed up this GetWebGLStatusFunction in
Mac and Win.  I feel it's better to just add a comment to explain the situation
here.

http://codereview.chromium.org/8772031/diff/24014/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_webstore_private_api.h (right):

http://codereview.chromium.org/8772031/diff/24014/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webstore_private_api.h:167: virtual void
OnGpuInfoUpdate() OVERRIDE;
On 2011/12/07 00:01:38, Mihai Parparita wrote:
> A comment indicating that this implements GpuDataManager::Observer would be
> consistent with the rest of the functions in this file.

Done.

http://codereview.chromium.org/8772031/diff/24014/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webstore_private_api.h:173: void
CreateResult(bool webgl_allowed);
On 2011/12/07 00:01:38, Mihai Parparita wrote:
> CreateResult and IsWebGLAllowed should be private.

Done.

http://codereview.chromium.org/8772031/diff/24014/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_webstore_private_apitest.cc (right):

http://codereview.chromium.org/8772031/diff/24014/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webstore_private_apitest.cc:96: // In linux,
we need to launch GPU process to decide if WebGL is allowed.
On 2011/12/07 00:01:38, Mihai Parparita wrote:
> Can you move this to a separate subclass that you use for the GPU function
> tests? Also, that subclass doesn't need to extend ExtensionApiTest, it can
just
> use InProcessBrowserTest.

Done.

http://codereview.chromium.org/8772031/diff/24014/chrome/test/data/extensions...
File
chrome/test/data/extensions/api_test/webstore_private/get_webgl_status_allowed.html
(right):

http://codereview.chromium.org/8772031/diff/24014/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/webstore_private/get_webgl_status_allowed.html:1:
<script src="common.js"></script>
On 2011/12/07 00:01:38, Mihai Parparita wrote:
> This file (and the other test file) can be reverted now.

Done.

[Zhenyao Mo, 2011-12-08 19:21:43]

Ping: Mihai, the bots are all green, please have another look.

[Mihai Parparita -not on Chrome, 2011-12-08 22:30:40]

LGTM

http://codereview.chromium.org/8772031/diff/29001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_webstore_private_apitest.cc (right):

http://codereview.chromium.org/8772031/diff/29001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webstore_private_apitest.cc:96: // In linux,
we need to launch GPU process to decide if WebGL is allowed.
This can be removed, since you're no longer testing GPU functionality with 
ExtensionWebstorePrivateApiTest.