Raise an infobar and deny access to WebGL if a GPU reset was detected while a web page containing W…

Raise an infobar and deny access to WebGL if a GPU reset was detected while a web page containing WebGL content was visible.

The same mechanism will work for Pepper 3D but has not been hooked up for that API yet.

https://bugs.webkit.org/show_bug.cgi?id=101826 added the hooks out through the Chromium WebKit API.

BUG=125125
TEST=GPUCrashTest.ContextLossRaisesInfobar


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=168794

[jam, 2012-11-15 16:57:25]

https://codereview.chromium.org/11378008/diff/5001/chrome/test/gpu/gpu_crash_...
File chrome/test/gpu/gpu_crash_browsertest.cc (right):

https://codereview.chromium.org/11378008/diff/5001/chrome/test/gpu/gpu_crash_...
chrome/test/gpu/gpu_crash_browsertest.cc:87:
IN_PROC_BROWSER_TEST_F(GPUCrashTest, ContextLossRaisesInfobar) {
can you make this test be in content? it seems that the interesting bit here is
to check that WCD is notified of a webgl crash. so perhaps this test can create
a webcontents and set a custom webcontentsdelegate that looks for that
notification? that way you don't need to add a fortesting method to the public
api

https://codereview.chromium.org/11378008/diff/5001/content/browser/gpu/gpu_da...
File content/browser/gpu/gpu_data_manager_impl.h (right):

https://codereview.chromium.org/11378008/diff/5001/content/browser/gpu/gpu_da...
content/browser/gpu/gpu_data_manager_impl.h:231: mutable std::list<base::Time>
timestamps_of_gpu_resets_;
doesn't look like this needs to be mutable?

https://codereview.chromium.org/11378008/diff/5001/content/browser/renderer_h...
File content/browser/renderer_host/render_message_filter.cc (right):

https://codereview.chromium.org/11378008/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/render_message_filter.cc:1053:
content::GpuDataManagerImpl::DomainBlockStatus block_status =
nit: no "content::" here and below

https://codereview.chromium.org/11378008/diff/5001/content/browser/renderer_h...
File content/browser/renderer_host/render_message_filter.h (right):

https://codereview.chromium.org/11378008/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/render_message_filter.h:244: void
OnAre3DAPIsBlocked(int render_view_id,
nit: no blank line between previous method (this header doesn't follow the
convention for putting all IPC handlers together and not commmenting. i'll fix
this in a followup)

https://codereview.chromium.org/11378008/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/render_message_filter.h:246:
content::ThreeDAPIType requester,
nit: here and below, no need for "content::"

https://codereview.chromium.org/11378008/diff/5001/content/browser/web_conten...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/11378008/diff/5001/content/browser/web_conten...
content/browser/web_contents/web_contents_impl.cc:2361: }
this could be written as

FOR_EACH_OBSERVER(WebContentsObserver, observers_,
                  DidBlock3DAPIs(url, requester));

(the one that you copied from OnMessageReceived is more complicated because it
looks at the return value)

https://codereview.chromium.org/11378008/diff/5001/content/browser/web_conten...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/11378008/diff/5001/content/browser/web_conten...
content/browser/web_contents/web_contents_impl.h:183: void DidBlock3DAPIs(const
GURL& url, content::ThreeDAPIType requester);
nit: no need for content::

https://codereview.chromium.org/11378008/diff/5001/content/common/view_messag...
File content/common/view_messages.h (right):

https://codereview.chromium.org/11378008/diff/5001/content/common/view_messag...
content/common/view_messages.h:2416: GURL /* top origin url */,
nit: here and below, the convention is to put the parameter name in the
dispatching method inside the comment

https://codereview.chromium.org/11378008/diff/5001/content/public/browser/web...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/11378008/diff/5001/content/public/browser/web...
content/public/browser/web_contents_observer.h:155: // reset recently. If this
object handles the request, it should
nit: just stop this comment at the period. the content api shouldn't document
what chrome does in response to a call

https://codereview.chromium.org/11378008/diff/5001/content/public/common/thre...
File content/public/common/three_d_api_types.h (right):

https://codereview.chromium.org/11378008/diff/5001/content/public/common/thre...
content/public/common/three_d_api_types.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
nit: add this to content_common.gypi

https://codereview.chromium.org/11378008/diff/5001/content/renderer/render_vi...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/11378008/diff/5001/content/renderer/render_vi...
content/renderer/render_view_impl.h:658: // Indicates whether 3D APIs (Pepper
3D, WebGL) are blocked for the
nit: don't comment functions on overridden methods. i.e. this duplicates
comments that should be on the base class

https://codereview.chromium.org/11378008/diff/5001/content/renderer/render_vi...
content/renderer/render_view_impl.h:661: virtual bool
allowWebGL(WebKit::WebFrame* frame, bool defaultValue) OVERRIDE;
nit: default_value per google style

[Ken Russell (switch to Gerrit), 2012-11-15 20:53:25]

https://codereview.chromium.org/11378008/diff/5001/chrome/test/gpu/gpu_crash_...
File chrome/test/gpu/gpu_crash_browsertest.cc (right):

https://codereview.chromium.org/11378008/diff/5001/chrome/test/gpu/gpu_crash_...
chrome/test/gpu/gpu_crash_browsertest.cc:87:
IN_PROC_BROWSER_TEST_F(GPUCrashTest, ContextLossRaisesInfobar) {
On 2012/11/15 16:57:25, John Abd-El-Malek wrote:
> can you make this test be in content? it seems that the interesting bit here
is
> to check that WCD is notified of a webgl crash. so perhaps this test can
create
> a webcontents and set a custom webcontentsdelegate that looks for that
> notification? that way you don't need to add a fortesting method to the public
> api

I will work on adding a copy of this test to content. The public ForTesting API
on GpuDataManager is not for this test; it is needed to avoid regressing the
test above, which currently runs on the GPU bots and is necessary to test an
important code path.

https://codereview.chromium.org/11378008/diff/5001/content/browser/gpu/gpu_da...
File content/browser/gpu/gpu_data_manager_impl.h (right):

https://codereview.chromium.org/11378008/diff/5001/content/browser/gpu/gpu_da...
content/browser/gpu/gpu_data_manager_impl.h:231: mutable std::list<base::Time>
timestamps_of_gpu_resets_;
On 2012/11/15 16:57:25, John Abd-El-Malek wrote:
> doesn't look like this needs to be mutable?

It does -- the const Are3DAPIsBlocked removes expired timestamps as a
non-user-observable side-effect.

https://codereview.chromium.org/11378008/diff/5001/content/browser/renderer_h...
File content/browser/renderer_host/render_message_filter.cc (right):

https://codereview.chromium.org/11378008/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/render_message_filter.cc:1053:
content::GpuDataManagerImpl::DomainBlockStatus block_status =
On 2012/11/15 16:57:25, John Abd-El-Malek wrote:
> nit: no "content::" here and below

Done.

https://codereview.chromium.org/11378008/diff/5001/content/browser/renderer_h...
File content/browser/renderer_host/render_message_filter.h (right):

https://codereview.chromium.org/11378008/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/render_message_filter.h:244: void
OnAre3DAPIsBlocked(int render_view_id,
On 2012/11/15 16:57:25, John Abd-El-Malek wrote:
> nit: no blank line between previous method (this header doesn't follow the
> convention for putting all IPC handlers together and not commmenting. i'll fix
> this in a followup)

Done.

https://codereview.chromium.org/11378008/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/render_message_filter.h:246:
content::ThreeDAPIType requester,
On 2012/11/15 16:57:25, John Abd-El-Malek wrote:
> nit: here and below, no need for "content::"

Done.

https://codereview.chromium.org/11378008/diff/5001/content/browser/web_conten...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/11378008/diff/5001/content/browser/web_conten...
content/browser/web_contents/web_contents_impl.cc:2361: }
On 2012/11/15 16:57:25, John Abd-El-Malek wrote:
> this could be written as
> 
> FOR_EACH_OBSERVER(WebContentsObserver, observers_,
>                   DidBlock3DAPIs(url, requester));
> 
> (the one that you copied from OnMessageReceived is more complicated because it
> looks at the return value)

Done.

https://codereview.chromium.org/11378008/diff/5001/content/browser/web_conten...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/11378008/diff/5001/content/browser/web_conten...
content/browser/web_contents/web_contents_impl.h:183: void DidBlock3DAPIs(const
GURL& url, content::ThreeDAPIType requester);
On 2012/11/15 16:57:25, John Abd-El-Malek wrote:
> nit: no need for content::

Done.

https://codereview.chromium.org/11378008/diff/5001/content/common/view_messag...
File content/common/view_messages.h (right):

https://codereview.chromium.org/11378008/diff/5001/content/common/view_messag...
content/common/view_messages.h:2416: GURL /* top origin url */,
On 2012/11/15 16:57:25, John Abd-El-Malek wrote:
> nit: here and below, the convention is to put the parameter name in the
> dispatching method inside the comment

Renamed some of the dispatching methods' parameter names and updated the
comments here.

https://codereview.chromium.org/11378008/diff/5001/content/public/browser/web...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/11378008/diff/5001/content/public/browser/web...
content/public/browser/web_contents_observer.h:155: // reset recently. If this
object handles the request, it should
On 2012/11/15 16:57:25, John Abd-El-Malek wrote:
> nit: just stop this comment at the period. the content api shouldn't document
> what chrome does in response to a call

Done.

https://codereview.chromium.org/11378008/diff/5001/content/public/common/thre...
File content/public/common/three_d_api_types.h (right):

https://codereview.chromium.org/11378008/diff/5001/content/public/common/thre...
content/public/common/three_d_api_types.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2012/11/15 16:57:25, John Abd-El-Malek wrote:
> nit: add this to content_common.gypi 

Done.

https://codereview.chromium.org/11378008/diff/5001/content/renderer/render_vi...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/11378008/diff/5001/content/renderer/render_vi...
content/renderer/render_view_impl.h:658: // Indicates whether 3D APIs (Pepper
3D, WebGL) are blocked for the
On 2012/11/15 16:57:25, John Abd-El-Malek wrote:
> nit: don't comment functions on overridden methods. i.e. this duplicates
> comments that should be on the base class

Done.

https://codereview.chromium.org/11378008/diff/5001/content/renderer/render_vi...
content/renderer/render_view_impl.h:661: virtual bool
allowWebGL(WebKit::WebFrame* frame, bool defaultValue) OVERRIDE;
On 2012/11/15 16:57:25, John Abd-El-Malek wrote:
> nit: default_value per google style

Done.

[jam, 2012-11-15 21:11:38]

lgtm

https://codereview.chromium.org/11378008/diff/5001/content/browser/gpu/gpu_da...
File content/browser/gpu/gpu_data_manager_impl.h (right):

https://codereview.chromium.org/11378008/diff/5001/content/browser/gpu/gpu_da...
content/browser/gpu/gpu_data_manager_impl.h:231: mutable std::list<base::Time>
timestamps_of_gpu_resets_;
On 2012/11/15 20:53:25, kbr wrote:
> On 2012/11/15 16:57:25, John Abd-El-Malek wrote:
> > doesn't look like this needs to be mutable?
> 
> It does -- the const Are3DAPIsBlocked removes expired timestamps as a
> non-user-observable side-effect.

ah I somehow missed that one when looking, nvm

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

zmo: please review. This CL replaces https://codereview.chromium.org/11366237/ .
The logic from that CL is unchanged, but most of the new APIs were removed from
GpuDataManager and placed in the implementation.

OWNERS reviews:

pkasting => chrome/browser/api/infobars
ben => chrome , chrome/browser
palmer => content/common/view_messages.h

[jam, 2012-11-15 21:27:06]

On 2012/11/15 21:25:04, kbr wrote:
> zmo: please review. This CL replaces https://codereview.chromium.org/11366237/
.
> The logic from that CL is unchanged, but most of the new APIs were removed
from
> GpuDataManager and placed in the implementation.
> 
> OWNERS reviews:
> 
> pkasting => chrome/browser/api/infobars
> ben => chrome , chrome/browser
btw to save you (and ben) time, I already looked at all the files in chrome so
lgtm for those too (i'm an owner, although usually not a reviewer, for
src\chrome)

[Peter Kasting, 2012-11-15 21:59:59]

LGTM with a number of cleanups to make the infobar you're defining consistent
with our other infobars.

https://codereview.chromium.org/11378008/diff/14001/chrome/browser/three_d_ap...
File chrome/browser/three_d_api_observer.cc (right):

https://codereview.chromium.org/11378008/diff/14001/chrome/browser/three_d_ap...
chrome/browser/three_d_api_observer.cc:17: // ThreeDAPIInfoBarDelegate
Tiny nit: This is somewhat unusual for a divider style -- most of the dividers
I've seen put the class name followed on the same line by hyphens.

https://codereview.chromium.org/11378008/diff/14001/chrome/browser/three_d_ap...
chrome/browser/three_d_api_observer.cc:20: // with client 3D APIs and GPU
resets.
Does this comment really belong here in the infobar code?

https://codereview.chromium.org/11378008/diff/14001/chrome/browser/three_d_ap...
chrome/browser/three_d_api_observer.cc:24: static void Show(
This method doesn't seem to be defined or used.

https://codereview.chromium.org/11378008/diff/14001/chrome/browser/three_d_ap...
chrome/browser/three_d_api_observer.cc:29: InfoBarTabHelper* helper,
Nit: Please take an InfoBarService* instead of an InfoBarTabHelper*.

https://codereview.chromium.org/11378008/diff/14001/chrome/browser/three_d_ap...
chrome/browser/three_d_api_observer.cc:32: virtual ~ThreeDAPIInfoBarDelegate();
Nit: Please make everything but the constructor private

https://codereview.chromium.org/11378008/diff/14001/chrome/browser/three_d_ap...
chrome/browser/three_d_api_observer.cc:34: // InfoBarDelegate:
Nit: You don't directly inherit from InfoBarDelegate, so put these in with the
ConfirmInfoBarDelegate overrides, at the top of the list.

https://codereview.chromium.org/11378008/diff/14001/chrome/browser/three_d_ap...
chrome/browser/three_d_api_observer.cc:46: virtual gfx::Image* GetIcon() const
OVERRIDE;
Nit: This should be placed between EqualsDelegate() and
AsThreeDAPIInfoBarDelegate().

https://codereview.chromium.org/11378008/diff/14001/chrome/browser/three_d_ap...
chrome/browser/three_d_api_observer.cc:78:
ThreeDAPIInfoBarDelegate::AsThreeDAPIInfoBarDelegate() {
Nit: Indent 4

https://codereview.chromium.org/11378008/diff/14001/chrome/browser/three_d_ap...
chrome/browser/three_d_api_observer.cc:94: api_name);
Nit: Shorter and simpler:

  return l10n_util::GetStringFUTF16(
      IDS_3D_APIS_BLOCKED_TEXT,
      l10n_util::GetStringUTF16(
          (requester_ == content::THREE_D_API_TYPE_WEBGL) ?
          IDS_3D_APIS_WEBGL_NAME : IDS_3D_APIS_PEPPER_3D_NAME));

https://codereview.chromium.org/11378008/diff/14001/chrome/browser/three_d_ap...
chrome/browser/three_d_api_observer.cc:98: return BUTTON_OK | BUTTON_CANCEL;
This is the default behavior, don't override this function.

https://codereview.chromium.org/11378008/diff/14001/chrome/browser/three_d_ap...
chrome/browser/three_d_api_observer.cc:112: }
Nit: Our infobar code explicitly assumes that BUTTON_OK and BUTTON_CANCEL are
the only choices, so we always write this function like:

  return l10n_util::GetStringUTF16((button == BUTTON_OK) ?
      IDS_3D_APIS_BLOCKED_OK_BUTTON_LABEL :
      IDS_3D_APIS_BLOCKED_TRY_AGAIN_BUTTON_LABEL);

https://codereview.chromium.org/11378008/diff/14001/chrome/browser/three_d_ap...
chrome/browser/three_d_api_observer.cc:128: return string16();
This is the default behavior, don't override this function.

https://codereview.chromium.org/11378008/diff/14001/chrome/browser/three_d_ap...
chrome/browser/three_d_api_observer.cc:133: return false;
You have no link, don't override this function.

https://codereview.chromium.org/11378008/diff/14001/chrome/browser/three_d_ap...
chrome/browser/three_d_api_observer.cc:137: return NULL;
This is the default behavior, don't override this function.

However, really your infobar needs to have an icon.  It is an error for infobars
to not have an icon.  Please consult the UI designers to obtain an appropriate
one.

https://codereview.chromium.org/11378008/diff/14001/chrome/browser/three_d_ap...
chrome/browser/three_d_api_observer.cc:152:
InfoBarTabHelper::FromWebContents(web_contents());
Nit: Use InfoBarService* infobar_service = ... here.

[palmer, 2012-11-15 22:25:30]

LGTM from an IPC security perspective.

[Zhenyao Mo, 2012-11-15 22:38:45]

LGTM

One suggestion though: should we also add a commandline switch to disable the
infobar?  In case somehow it becomes annoying, for testing for example.  maybe
in another CL.

[Peter Kasting, 2012-11-15 22:40:15]

On 2012/11/15 22:38:45, Zhenyao Mo wrote:
> One suggestion though: should we also add a commandline switch to disable the
> infobar?

I'm biased against switches unless we really need them and they're temporary. 
Switches are like adding options, they impose costs.  And they don't tend to get
cleaned up.  Let's not do this unless we must.

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

@pkasting: thanks for your review.

https://codereview.chromium.org/11378008/diff/14001/chrome/browser/three_d_ap...
File chrome/browser/three_d_api_observer.cc (right):

https://codereview.chromium.org/11378008/diff/14001/chrome/browser/three_d_ap...
chrome/browser/three_d_api_observer.cc:17: // ThreeDAPIInfoBarDelegate
On 2012/11/15 21:59:59, Peter Kasting wrote:
> Tiny nit: This is somewhat unusual for a divider style -- most of the dividers
> I've seen put the class name followed on the same line by hyphens.

OK, changed.

https://codereview.chromium.org/11378008/diff/14001/chrome/browser/three_d_ap...
chrome/browser/three_d_api_observer.cc:20: // with client 3D APIs and GPU
resets.
On 2012/11/15 21:59:59, Peter Kasting wrote:
> Does this comment really belong here in the infobar code?

I really want it tied closely to the code, and have expanded the comment a
little with the removal of GetLinkText().

https://codereview.chromium.org/11378008/diff/14001/chrome/browser/three_d_ap...
chrome/browser/three_d_api_observer.cc:24: static void Show(
On 2012/11/15 21:59:59, Peter Kasting wrote:
> This method doesn't seem to be defined or used.

Oops. Thanks for catching this. Removed.

https://codereview.chromium.org/11378008/diff/14001/chrome/browser/three_d_ap...
chrome/browser/three_d_api_observer.cc:29: InfoBarTabHelper* helper,
On 2012/11/15 21:59:59, Peter Kasting wrote:
> Nit: Please take an InfoBarService* instead of an InfoBarTabHelper*.

Done.

https://codereview.chromium.org/11378008/diff/14001/chrome/browser/three_d_ap...
chrome/browser/three_d_api_observer.cc:32: virtual ~ThreeDAPIInfoBarDelegate();
On 2012/11/15 21:59:59, Peter Kasting wrote:
> Nit: Please make everything but the constructor private

Done.

https://codereview.chromium.org/11378008/diff/14001/chrome/browser/three_d_ap...
chrome/browser/three_d_api_observer.cc:34: // InfoBarDelegate:
On 2012/11/15 21:59:59, Peter Kasting wrote:
> Nit: You don't directly inherit from InfoBarDelegate, so put these in with the
> ConfirmInfoBarDelegate overrides, at the top of the list.

Done.

https://codereview.chromium.org/11378008/diff/14001/chrome/browser/three_d_ap...
chrome/browser/three_d_api_observer.cc:46: virtual gfx::Image* GetIcon() const
OVERRIDE;
On 2012/11/15 21:59:59, Peter Kasting wrote:
> Nit: This should be placed between EqualsDelegate() and
> AsThreeDAPIInfoBarDelegate().

Done.

https://codereview.chromium.org/11378008/diff/14001/chrome/browser/three_d_ap...
chrome/browser/three_d_api_observer.cc:78:
ThreeDAPIInfoBarDelegate::AsThreeDAPIInfoBarDelegate() {
On 2012/11/15 21:59:59, Peter Kasting wrote:
> Nit: Indent 4

The C++ style guide doesn't require this here; further, emacs won't auto-indent
here and there are other examples in the code base like
src/content/renderer/pepper/pepper_plugin_delegate_impl.cc which have both the
return type and method name on the first column. Leaving this as is.

https://codereview.chromium.org/11378008/diff/14001/chrome/browser/three_d_ap...
chrome/browser/three_d_api_observer.cc:94: api_name);
On 2012/11/15 21:59:59, Peter Kasting wrote:
> Nit: Shorter and simpler:
> 
>   return l10n_util::GetStringFUTF16(
>       IDS_3D_APIS_BLOCKED_TEXT,
>       l10n_util::GetStringUTF16(
>           (requester_ == content::THREE_D_API_TYPE_WEBGL) ?
>           IDS_3D_APIS_WEBGL_NAME : IDS_3D_APIS_PEPPER_3D_NAME));

It's desired for the build to break here if another API type is added to the
enum. Leaving this as is.

https://codereview.chromium.org/11378008/diff/14001/chrome/browser/three_d_ap...
chrome/browser/three_d_api_observer.cc:98: return BUTTON_OK | BUTTON_CANCEL;
On 2012/11/15 21:59:59, Peter Kasting wrote:
> This is the default behavior, don't override this function.

Oops, thanks. Done.

https://codereview.chromium.org/11378008/diff/14001/chrome/browser/three_d_ap...
chrome/browser/three_d_api_observer.cc:112: }
On 2012/11/15 21:59:59, Peter Kasting wrote:
> Nit: Our infobar code explicitly assumes that BUTTON_OK and BUTTON_CANCEL are
> the only choices, so we always write this function like:
> 
>   return l10n_util::GetStringUTF16((button == BUTTON_OK) ?
>       IDS_3D_APIS_BLOCKED_OK_BUTTON_LABEL :
>       IDS_3D_APIS_BLOCKED_TRY_AGAIN_BUTTON_LABEL);

Done.

https://codereview.chromium.org/11378008/diff/14001/chrome/browser/three_d_ap...
chrome/browser/three_d_api_observer.cc:128: return string16();
On 2012/11/15 21:59:59, Peter Kasting wrote:
> This is the default behavior, don't override this function.

Done.

https://codereview.chromium.org/11378008/diff/14001/chrome/browser/three_d_ap...
chrome/browser/three_d_api_observer.cc:133: return false;
On 2012/11/15 21:59:59, Peter Kasting wrote:
> You have no link, don't override this function.

Done.

https://codereview.chromium.org/11378008/diff/14001/chrome/browser/three_d_ap...
chrome/browser/three_d_api_observer.cc:137: return NULL;
On 2012/11/15 21:59:59, Peter Kasting wrote:
> This is the default behavior, don't override this function.
> 
> However, really your infobar needs to have an icon.  It is an error for
infobars
> to not have an icon.  Please consult the UI designers to obtain an appropriate
> one.

Done.

https://codereview.chromium.org/11378008/diff/14001/chrome/browser/three_d_ap...
chrome/browser/three_d_api_observer.cc:152:
InfoBarTabHelper::FromWebContents(web_contents());
On 2012/11/15 21:59:59, Peter Kasting wrote:
> Nit: Use InfoBarService* infobar_service = ... here.

Done, thanks. Also updated the #include above.

[Ken Russell (switch to Gerrit), 2012-11-15 23:00:33]

On 2012/11/15 22:40:15, Peter Kasting wrote:
> On 2012/11/15 22:38:45, Zhenyao Mo wrote:
> > One suggestion though: should we also add a commandline switch to disable
the
> > infobar?
> 
> I'm biased against switches unless we really need them and they're temporary. 
> Switches are like adding options, they impose costs.  And they don't tend to
get
> cleaned up.  Let's not do this unless we must.

I am also strongly inclined not to provide an option to disable the infobar. A
mechanism has been added to disable it in tests, and used in GPUCrashTest.Kill.

[Peter Kasting, 2012-11-15 23:41:24]

https://codereview.chromium.org/11378008/diff/14001/chrome/browser/three_d_ap...
File chrome/browser/three_d_api_observer.cc (right):

https://codereview.chromium.org/11378008/diff/14001/chrome/browser/three_d_ap...
chrome/browser/three_d_api_observer.cc:20: // with client 3D APIs and GPU
resets.
On 2012/11/15 22:59:23, kbr wrote:
> On 2012/11/15 21:59:59, Peter Kasting wrote:
> > Does this comment really belong here in the infobar code?
> 
> I really want it tied closely to the code, and have expanded the comment a
> little with the removal of GetLinkText().

Ah.  It makes more sense now, thanks.

https://codereview.chromium.org/11378008/diff/14001/chrome/browser/three_d_ap...
chrome/browser/three_d_api_observer.cc:94: api_name);
On 2012/11/15 22:59:23, kbr wrote:
> On 2012/11/15 21:59:59, Peter Kasting wrote:
> > Nit: Shorter and simpler:
> > 
> >   return l10n_util::GetStringFUTF16(
> >       IDS_3D_APIS_BLOCKED_TEXT,
> >       l10n_util::GetStringUTF16(
> >           (requester_ == content::THREE_D_API_TYPE_WEBGL) ?
> >           IDS_3D_APIS_WEBGL_NAME : IDS_3D_APIS_PEPPER_3D_NAME));
> 
> It's desired for the build to break here if another API type is added to the
> enum. Leaving this as is.

Is the build guaranteed to break on all our targets?  Or should we instead
change this to "default: NOTREACHED()"?  (The Google style guide says the
compiler will warn you, but it also only has to contend with one toolchain.)

[Ken Russell (switch to Gerrit), 2012-11-16 02:26:58]

https://codereview.chromium.org/11378008/diff/14001/chrome/browser/three_d_ap...
File chrome/browser/three_d_api_observer.cc (right):

https://codereview.chromium.org/11378008/diff/14001/chrome/browser/three_d_ap...
chrome/browser/three_d_api_observer.cc:94: api_name);
On 2012/11/15 23:41:24, Peter Kasting wrote:
> On 2012/11/15 22:59:23, kbr wrote:
> > On 2012/11/15 21:59:59, Peter Kasting wrote:
> > > Nit: Shorter and simpler:
> > > 
> > >   return l10n_util::GetStringFUTF16(
> > >       IDS_3D_APIS_BLOCKED_TEXT,
> > >       l10n_util::GetStringUTF16(
> > >           (requester_ == content::THREE_D_API_TYPE_WEBGL) ?
> > >           IDS_3D_APIS_WEBGL_NAME : IDS_3D_APIS_PEPPER_3D_NAME));
> > 
> > It's desired for the build to break here if another API type is added to the
> > enum. Leaving this as is.
> 
> Is the build guaranteed to break on all our targets?  Or should we instead
> change this to "default: NOTREACHED()"?  (The Google style guide says the
> compiler will warn you, but it also only has to contend with one toolchain.)

It definitely breaks with clang, so there is no way a breaking change in this
area would get into the source tree and stay there. (I ran into the build
failure on Linux earlier when I had a "last value" entry in the enum.)

[Peter Kasting, 2012-11-16 02:44:50]

On 2012/11/16 02:26:58, kbr wrote:
> > Is the build guaranteed to break on all our targets?  Or should we instead
> > change this to "default: NOTREACHED()"?  (The Google style guide says the
> > compiler will warn you, but it also only has to contend with one toolchain.)
> 
> It definitely breaks with clang, so there is no way a breaking change in this
> area would get into the source tree and stay there. (I ran into the build
> failure on Linux earlier when I had a "last value" entry in the enum.)

Fair enough!  LGTM (again)

[Ken Russell (switch to Gerrit), 2012-11-20 01:15:11]

For the record: the try job failures on the Linux GPU bot are caused by the
screen saver activating on that machine, and I believe the Win GPU bot try
failures are unrelated. Submitting this patch to the CQ now.

[commit-bot: I haz the power, 2012-11-20 01:16:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kbr@chromium.org/11378008/8006

[commit-bot: I haz the power, 2012-11-20 02:31:19]

Retried try job too often for step(s) interactive_ui_tests

[commit-bot: I haz the power, 2012-11-20 12:51:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kbr@chromium.org/11378008/8006

[commit-bot: I haz the power, 2012-11-20 13:14:15]

Change committed as 168794