Add logic to block the use of client 3D APIs (WebGL, Pepper 3D) if context lost notifications are r…

Add logic to block the use of client 3D APIs (WebGL, Pepper 3D) if context lost notifications are received.

This CL only adds the core mechanism and unit tests. A follow-on CL will hook it up to WebGL.

BUG=125125
TEST=GpuDataManagerImplTest.*

[Ken Russell (switch to Gerrit), 2012-11-14 02:15:57]

zmo: please review.

joi: please do OWNERS review for content/public/browser/.

[Jói, 2012-11-14 08:50:31]

Adding jam@.

Does the logic using this API need to live outside of content?  If not, then
there is no need for a public API.

Cheers,
Jói

[Zhenyao Mo, 2012-11-14 16:35:57]

LGTM (please fix the mem leak in the tests)

http://codereview.chromium.org/11366237/diff/1/content/browser/gpu/gpu_data_m...
File content/browser/gpu/gpu_data_manager_impl.cc (right):

http://codereview.chromium.org/11366237/diff/1/content/browser/gpu/gpu_data_m...
content/browser/gpu/gpu_data_manager_impl.cc:356: const GURL& url) const {
nit: you don't need to put this in a new line.

http://codereview.chromium.org/11366237/diff/1/content/browser/gpu/gpu_data_m...
File content/browser/gpu/gpu_data_manager_impl.h (right):

http://codereview.chromium.org/11366237/diff/1/content/browser/gpu/gpu_data_m...
content/browser/gpu/gpu_data_manager_impl.h:160: std::string
GetDomainFromURL(const GURL& url) const;
nit: this can be a helper defined in anonymous namespace in the .cc file

http://codereview.chromium.org/11366237/diff/1/content/browser/gpu/gpu_data_m...
File content/browser/gpu/gpu_data_manager_impl_unittest.cc (right):

http://codereview.chromium.org/11366237/diff/1/content/browser/gpu/gpu_data_m...
content/browser/gpu/gpu_data_manager_impl_unittest.cc:339: GpuDataManagerImpl*
manager = new GpuDataManagerImpl();
You need to manually delete this manager to avoid mem leaking redness

http://codereview.chromium.org/11366237/diff/1/content/browser/gpu/gpu_data_m...
content/browser/gpu/gpu_data_manager_impl_unittest.cc:360: GpuDataManagerImpl*
manager = new GpuDataManagerImpl();
delete manager in the end

http://codereview.chromium.org/11366237/diff/1/content/browser/gpu/gpu_data_m...
content/browser/gpu/gpu_data_manager_impl_unittest.cc:389: GpuDataManagerImpl*
manager = new GpuDataManagerImpl();
delete manager in the end

http://codereview.chromium.org/11366237/diff/1/content/browser/gpu/gpu_data_m...
content/browser/gpu/gpu_data_manager_impl_unittest.cc:414: GpuDataManagerImpl*
manager = new GpuDataManagerImpl();
delete manager in the end

[Ken Russell (switch to Gerrit), 2012-11-14 18:13:52]

On 2012/11/14 08:50:31, Jói wrote:
> Adding jam@.
> 
> Does the logic using this API need to live outside of content?  If not, then
> there is no need for a public API.

Yes, the consuming logic lives in chrome/ . (It raises infobars, which are a
Chrome-only concept.)

[jam, 2012-11-14 18:21:57]

https://codereview.chromium.org/11366237/diff/1/content/public/browser/gpu_da...
File content/public/browser/gpu_data_manager.h (right):

https://codereview.chromium.org/11366237/diff/1/content/public/browser/gpu_da...
content/public/browser/gpu_data_manager.h:34: kKnownGuilty,
nit: here and below, please use chrome recommended style for enum values, i.e.
DOMAIN_GUILT_KNOWN, DOMAIN_BLOCK_STATUS_BLOCKED..

also, it's hard for content-api reviewers to review these changes without seeing
the chrome side. why are they not together? i.e. why is the test in chrome, if
it were in content (which it seems that it should, since content code should be
tested in content) then the ForTesting method can be hidden

[Ken Russell (switch to Gerrit), 2012-11-14 19:09:13]

New patch up. Please re-review.

https://codereview.chromium.org/11366237/diff/1/content/browser/gpu/gpu_data_...
File content/browser/gpu/gpu_data_manager_impl.cc (right):

https://codereview.chromium.org/11366237/diff/1/content/browser/gpu/gpu_data_...
content/browser/gpu/gpu_data_manager_impl.cc:356: const GURL& url) const {
On 2012/11/14 16:35:58, Zhenyao Mo wrote:
> nit: you don't need to put this in a new line.

Done.

https://codereview.chromium.org/11366237/diff/1/content/browser/gpu/gpu_data_...
File content/browser/gpu/gpu_data_manager_impl.h (right):

https://codereview.chromium.org/11366237/diff/1/content/browser/gpu/gpu_data_...
content/browser/gpu/gpu_data_manager_impl.h:160: std::string
GetDomainFromURL(const GURL& url) const;
On 2012/11/14 16:35:58, Zhenyao Mo wrote:
> nit: this can be a helper defined in anonymous namespace in the .cc file

I anticipate this may need to have state in the future, so am leaving it as a
member of GpuDataManagerImpl.

https://codereview.chromium.org/11366237/diff/1/content/browser/gpu/gpu_data_...
File content/browser/gpu/gpu_data_manager_impl_unittest.cc (right):

https://codereview.chromium.org/11366237/diff/1/content/browser/gpu/gpu_data_...
content/browser/gpu/gpu_data_manager_impl_unittest.cc:339: GpuDataManagerImpl*
manager = new GpuDataManagerImpl();
On 2012/11/14 16:35:58, Zhenyao Mo wrote:
> You need to manually delete this manager to avoid mem leaking redness

I see. I don't think this is maintainable so I've added a
ScopedGpuDataManagerImpl helper class to GpuDataManagerImplTest (needed because
GpuDataManagerImpl's destructor is private and scoped_ptr doesn't work with it)
and changed all the tests in this file to use it.

https://codereview.chromium.org/11366237/diff/1/content/public/browser/gpu_da...
File content/public/browser/gpu_data_manager.h (right):

https://codereview.chromium.org/11366237/diff/1/content/public/browser/gpu_da...
content/public/browser/gpu_data_manager.h:34: kKnownGuilty,
On 2012/11/14 18:21:57, John Abd-El-Malek wrote:
> nit: here and below, please use chrome recommended style for enum values, i.e.
> DOMAIN_GUILT_KNOWN, DOMAIN_BLOCK_STATUS_BLOCKED..

Will change -- sorry, I'd missed this in the Chromium style guide.

> also, it's hard for content-api reviewers to review these changes without
seeing
> the chrome side. why are they not together? i.e. why is the test in chrome, if
> it were in content (which it seems that it should, since content code should
be
> tested in content) then the ForTesting method can be hidden

I wanted to split off this patch to avoid having one gigantic patch to review.
The chrome side will be large enough as it is. The tests currently must live in
chrome because they cause infobars to be raised. Some other preexisting
chrome-side tests (gpu_crash_browsertest.cc) now need to suppress the raising of
infobars which is why the ForTesting method is needed.

[Zhenyao Mo, 2012-11-14 19:14:52]

Thanks.  LGTM again.

[Ken Russell (switch to Gerrit), 2012-11-15 21:22:57]

Based on @jam's feedback I've put up the entire set of changes, including this
one, in https://codereview.chromium.org/11378008/ . Closing this -- not going to
land it separately.