Add infobar for PPAPI broker usage.

Add infobar for PPAPI broker usage.

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

[Bernhard Bauer, 2012-08-10 12:27:36]

Hi Markus, 

please review the infobar and content settings parts. I'm going to add people
for the rest later. Also, I started a separate discussion about the strings over
on http://crbug.com/100960, so you can probably ignore their clumsy wording :)

[markusheintz_, 2012-08-13 06:09:06]

As discussed offline the new content setting needs also to be added to the
Website Setting UI. Could you file a bug for that? Then I would impl it.

Content Settings related parts LGTM with some comments.

http://codereview.chromium.org/10826164/diff/2043/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10826164/diff/2043/chrome/app/generated_resour...
chrome/app/generated_resources.grd:7262: +        Allow all sites to access the
PPAPI broker
I think these strings make no sense to users. But you already mentioned that you
started a discussion about the UI so I assume they will be changed later.

http://codereview.chromium.org/10826164/diff/2043/chrome/browser/content_sett...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

http://codereview.chromium.org/10826164/diff/2043/chrome/browser/content_sett...
chrome/browser/content_settings/host_content_settings_map.cc:361: case
CONTENT_SETTINGS_TYPE_PPAPI_BROKER:
nit: I wonder whether we could give this content setting a more descriptive name
e.g. CONTENT_SETTINGS_TYPE_UNSANDBOXED_PLUGINS? or something similar. Your call.

http://codereview.chromium.org/10826164/diff/2043/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/10826164/diff/2043/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:264: class PpapiBrokerPermissionInfoBarDelegate :
public ConfirmInfoBarDelegate {
nit: I wonder whether this class should live in it's own file(s).

http://codereview.chromium.org/10826164/diff/2043/chrome/test/ppapi/ppapi_bro...
File chrome/test/ppapi/ppapi_browsertest.cc (right):

http://codereview.chromium.org/10826164/diff/2043/chrome/test/ppapi/ppapi_bro...
chrome/test/ppapi/ppapi_browsertest.cc:128: 
nit: pls remove empty line.

[Bernhard Bauer, 2012-08-13 10:51:46]

On 2012/08/13 06:09:06, markusheintz_ wrote:
> As discussed offline the new content setting needs also to be added to the
> Website Setting UI. Could you file a bug for that? Then I would impl it.

Done: http://crbug.com/142298

http://codereview.chromium.org/10826164/diff/2043/chrome/browser/content_sett...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

http://codereview.chromium.org/10826164/diff/2043/chrome/browser/content_sett...
chrome/browser/content_settings/host_content_settings_map.cc:361: case
CONTENT_SETTINGS_TYPE_PPAPI_BROKER:
On 2012/08/13 06:09:06, markusheintz_ wrote:
> nit: I wonder whether we could give this content setting a more descriptive
name
> e.g. CONTENT_SETTINGS_TYPE_UNSANDBOXED_PLUGINS? or something similar. Your
call.

At least internally, I'd like to keep the term "PPAPI broker", as that is
exactly what it is (unsandboxed plug-in could mean NPAPI plug-in, for example,
or even an unsandboxed Pepper plug-in).

http://codereview.chromium.org/10826164/diff/2043/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/10826164/diff/2043/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:264: class PpapiBrokerPermissionInfoBarDelegate :
public ConfirmInfoBarDelegate {
On 2012/08/13 06:09:06, markusheintz_ wrote:
> nit: I wonder whether this class should live in it's own file(s).

Yeah, I'm not sure either. I'm slightly leaning towards keeping it in here,
because that makes it clear that the class is only meant to be used by
RequestPpapiBrokerPermissionHelper(). WDYT?

http://codereview.chromium.org/10826164/diff/2043/chrome/test/ppapi/ppapi_bro...
File chrome/test/ppapi/ppapi_browsertest.cc (right):

http://codereview.chromium.org/10826164/diff/2043/chrome/test/ppapi/ppapi_bro...
chrome/test/ppapi/ppapi_browsertest.cc:128: 
On 2012/08/13 06:09:06, markusheintz_ wrote:
> nit: pls remove empty line.

Done.

[Bernhard Bauer, 2012-08-14 10:14:40]

OWNERS, if you could be so kind?

James: chrome/browser/resources/options2/ and chrome/browser/ui/webui/options2/
Scott: chrome/ and chrome/browser/ui/
Trung: ppapi/ and chrome/test/ppapi/

Thanks!

[James Hawkins, 2012-08-14 14:19:42]

options2 LGTM with nit.

http://codereview.chromium.org/10826164/diff/4070/chrome/browser/resources/op...
File chrome/browser/resources/options2/content_settings.html (right):

http://codereview.chromium.org/10826164/diff/4070/chrome/browser/resources/op...
chrome/browser/resources/options2/content_settings.html:318:
i18n-content="manage_exceptions"></button>
nit: i18n-content should be javaScriptCase.

[sky, 2012-08-14 15:52:12]

http://codereview.chromium.org/10826164/diff/2051/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/10826164/diff/2051/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:272: class PpapiBrokerPermissionInfoBarDelegate :
public ConfirmInfoBarDelegate {
Move this into its own .h/.cc

http://codereview.chromium.org/10826164/diff/2051/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:294: GURL url_;
Make members const where applicable.

http://codereview.chromium.org/10826164/diff/2051/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:299: };
DISALLOW_...

http://codereview.chromium.org/10826164/diff/2051/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:1105: void
Browser::RequestPpapiBrokerPermissionHelper(
Why does this need to live in Browser?

[Bernhard Bauer, 2012-08-14 23:36:24]

http://codereview.chromium.org/10826164/diff/4070/chrome/browser/resources/op...
File chrome/browser/resources/options2/content_settings.html (right):

http://codereview.chromium.org/10826164/diff/4070/chrome/browser/resources/op...
chrome/browser/resources/options2/content_settings.html:318:
i18n-content="manage_exceptions"></button>
On 2012/08/14 14:19:42, James Hawkins wrote:
> nit: i18n-content should be javaScriptCase.

Done.

http://codereview.chromium.org/10826164/diff/2051/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/10826164/diff/2051/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:272: class PpapiBrokerPermissionInfoBarDelegate :
public ConfirmInfoBarDelegate {
On 2012/08/14 15:52:12, sky wrote:
> Move this into its own .h/.cc

Done.

http://codereview.chromium.org/10826164/diff/2051/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:294: GURL url_;
On 2012/08/14 15:52:12, sky wrote:
> Make members const where applicable.

Done.

http://codereview.chromium.org/10826164/diff/2051/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:299: };
On 2012/08/14 15:52:12, sky wrote:
> DISALLOW_...

Done.

http://codereview.chromium.org/10826164/diff/2051/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:1105: void
Browser::RequestPpapiBrokerPermissionHelper(
On 2012/08/14 15:52:12, sky wrote:
> Why does this need to live in Browser?

Browser is the "standard" implementation of WebContentsDelegate for this
infobar. Other implementations (ExternalTabContainerWin) delegate to this one.
This is modeled after similar infobars like the one for media access above  or
the one for protocol handlers.

[sky, 2012-08-15 02:29:34]

http://codereview.chromium.org/10826164/diff/2051/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/10826164/diff/2051/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:1105: void
Browser::RequestPpapiBrokerPermissionHelper(
On 2012/08/14 23:36:24, Bernhard Bauer wrote:
> On 2012/08/14 15:52:12, sky wrote:
> > Why does this need to live in Browser?
> 
> Browser is the "standard" implementation of WebContentsDelegate for this
> infobar. Other implementations (ExternalTabContainerWin) delegate to this one.
> This is modeled after similar infobars like the one for media access above  or
> the one for protocol handlers.

Who is calling it directly in browser.cc though? I don't see it on tip of tree,
so maybe it moved?

[Bernhard Bauer, 2012-08-15 09:36:53]

http://codereview.chromium.org/10826164/diff/2051/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/10826164/diff/2051/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:1105: void
Browser::RequestPpapiBrokerPermissionHelper(
On 2012/08/15 02:29:34, sky wrote:
> On 2012/08/14 23:36:24, Bernhard Bauer wrote:
> > On 2012/08/14 15:52:12, sky wrote:
> > > Why does this need to live in Browser?
> > 
> > Browser is the "standard" implementation of WebContentsDelegate for this
> > infobar. Other implementations (ExternalTabContainerWin) delegate to this
one.
> > This is modeled after similar infobars like the one for media access above 
or
> > the one for protocol handlers.
> 
> Who is calling it directly in browser.cc though? I don't see it on tip of
tree,
> so maybe it moved?

Browser::RequestPpapiBrokerPermission() does, which isn't on trunk yet (it's in
http://codereview.chromium.org/10854040/, which this CL depends on. I'm trying
to pipeline them).

[sky, 2012-08-15 15:12:49]

Ah, thanks for the clarification. In that case can the static
RequestPpapiBrokerPermissionHelper be moved else where? I'm requesting
this as until recently browser.cc was ~5k lines. We're trying to keep
browser from being a dumping ground by moving independent
functionality out of it. The code you're adding doesn't depend up
browser and so could easily be moved.

Thanks,

  -Scott

On Wed, Aug 15, 2012 at 2:36 AM,  <bauerb@chromium.org> wrote:
>
>
http://codereview.chromium.org/10826164/diff/2051/chrome/browser/ui/browser.cc
> File chrome/browser/ui/browser.cc (right):
>
>
http://codereview.chromium.org/10826164/diff/2051/chrome/browser/ui/browser.c...
> chrome/browser/ui/browser.cc:1105: void
> Browser::RequestPpapiBrokerPermissionHelper(
> On 2012/08/15 02:29:34, sky wrote:
>>
>> On 2012/08/14 23:36:24, Bernhard Bauer wrote:
>> > On 2012/08/14 15:52:12, sky wrote:
>> > > Why does this need to live in Browser?
>> >
>> > Browser is the "standard" implementation of WebContentsDelegate for
>
> this
>>
>> > infobar. Other implementations (ExternalTabContainerWin) delegate to
>
> this one.
>>
>> > This is modeled after similar infobars like the one for media access
>
> above  or
>>
>> > the one for protocol handlers.
>
>
>> Who is calling it directly in browser.cc though? I don't see it on tip
>
> of tree,
>>
>> so maybe it moved?
>
>
> Browser::RequestPpapiBrokerPermission() does, which isn't on trunk yet
> (it's in http://codereview.chromium.org/10854040/, which this CL depends
> on. I'm trying to pipeline them).
>
> http://codereview.chromium.org/10826164/

[Bernhard Bauer, 2012-08-15 19:54:36]

On 2012/08/15 15:12:49, sky wrote:
> Ah, thanks for the clarification. In that case can the static
> RequestPpapiBrokerPermissionHelper be moved else where? I'm requesting
> this as until recently browser.cc was ~5k lines. We're trying to keep
> browser from being a dumping ground by moving independent
> functionality out of it. The code you're adding doesn't depend up
> browser and so could easily be moved.

Sure, done.

[sky, 2012-08-15 21:10:03]

Excellent. Thank you. LGTM

[viettrungluu, 2012-08-20 21:06:29]

+brettw for his opinion on adding gmock to ppapi_browsertest.

[brettw, 2012-08-20 22:09:10]

I kind of hate gmock. Is there a good way to test this without using gmock?

[Bernhard Bauer, 2012-08-20 22:18:25]

Sure, I can react to the info bar manually. It just seemed like this is one
of the occasions where gmock actually fits the bill.
On Aug 21, 2012 12:09 AM, <brettw@chromium.org> wrote:

> I kind of hate gmock. Is there a good way to test this without using gmock?
>
>
http://codereview.chromium.**org/10826164/<http://codereview.chromium.org/108...
>

[Bernhard Bauer, 2012-08-21 11:50:17]

On 2012/08/20 22:09:10, brettw wrote:
> I kind of hate gmock. Is there a good way to test this without using gmock?

Done, and rebased on the latest patch set of
https://chromiumcodereview.appspot.com/10854040/.

[Bernhard Bauer, 2012-08-23 22:27:30]

Brett, are you okay with the ppapi/ changes?

[Bernhard Bauer, 2012-09-02 21:11:27]

On 2012/08/23 22:27:30, Bernhard Bauer wrote:
> Brett, are you okay with the ppapi/ changes?

Brett, friendly ping? Yours is the only review missing to land this.

[brettw, 2012-09-04 04:58:00]

LGTM. Sorry this got lost. I didn't check the code details.

[commit-bot: I haz the power, 2012-09-04 06:01:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/10826164/6052

[commit-bot: I haz the power, 2012-09-04 06:01:43]

Failed to apply patch for chrome/test/ppapi/ppapi_test.h:
While running patch -p1 --forward --force;
patching file chrome/test/ppapi/ppapi_test.h
Hunk #2 succeeded at 39 (offset 16 lines).
Hunk #3 FAILED at 72.
Hunk #4 succeeded at 123 (offset -12 lines).
1 out of 4 hunks FAILED -- saving rejects to file
chrome/test/ppapi/ppapi_test.h.rej

[commit-bot: I haz the power, 2012-09-04 07:50:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/10826164/23001

[commit-bot: I haz the power, 2012-09-04 10:28:03]

Change committed as 154742