Add PP_FlashLSORestrictions to the list of settings supported by the Flash_GetSetting Pepper API.

Add PP_FlashLSORestrictions to the list of settings supported by the Flash_GetSetting Pepper API.


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

[Bernhard Bauer, 2012-07-31 00:49:21]

Hi Trung and Brett, 

could one of you review the Pepper side of this CL?

John, could you take a look at content/?

Thanks!

[jam, 2012-07-31 01:58:05]

content/public lgtm, i defer to brett for the rest

[viettrungluu, 2012-07-31 04:54:56]

http://codereview.chromium.org/10831045/diff/15005/ppapi/api/private/ppb_flas...
File ppapi/api/private/ppb_flash.idl (right):

http://codereview.chromium.org/10831045/diff/15005/ppapi/api/private/ppb_flas...
ppapi/api/private/ppb_flash.idl:17: M22_1 = 12.6
There's no need to rev the API when adding settings, as long as it fails
gracefully (and it's not essential that the plugin know whether or not that
setting is supported).

Moreover, if you add another version, you have to update the C++ wrapper.

[Bernhard Bauer, 2012-07-31 05:22:40]

https://chromiumcodereview.appspot.com/10831045/diff/15005/ppapi/api/private/...
File ppapi/api/private/ppb_flash.idl (right):

https://chromiumcodereview.appspot.com/10831045/diff/15005/ppapi/api/private/...
ppapi/api/private/ppb_flash.idl:17: M22_1 = 12.6
On 2012/07/31 04:54:56, viettrungluu wrote:
> There's no need to rev the API when adding settings, as long as it fails
> gracefully (and it's not essential that the plugin know whether or not that
> setting is supported).
> 
> Moreover, if you add another version, you have to update the C++ wrapper.

Right. That was leftover from an old version where I did make API changes.

[viettrungluu, 2012-08-01 04:06:41]

Read my comments below from the bottom up.

But is it my imagination or are you bouncing to the renderer process, which then
bounces to the browser? Can't you directly to the browser?

http://codereview.chromium.org/10831045/diff/17001/ppapi/proxy/ppb_flash_prox...
File ppapi/proxy/ppb_flash_proxy.cc (right):

http://codereview.chromium.org/10831045/diff/17001/ppapi/proxy/ppb_flash_prox...
ppapi/proxy/ppb_flash_proxy.cc:309: case PP_FLASHSETTING_LSORESTRICTIONS: {
Maybe you should point out explicitly that you need to go to the renderer for
this.

http://codereview.chromium.org/10831045/diff/17001/webkit/plugins/ppapi/ppb_f...
File webkit/plugins/ppapi/ppb_flash_impl.cc (right):

http://codereview.chromium.org/10831045/diff/17001/webkit/plugins/ppapi/ppb_f...
webkit/plugins/ppapi/ppb_flash_impl.cc:257: case
PP_FLASHSETTING_LSORESTRICTIONS: {
Why exactly do we need to support this in-process? (Or will the answer become
obvious when I look at another file?)

[Bernhard Bauer, 2012-08-01 05:17:39]

On 2012/08/01 04:06:41, viettrungluu wrote:
> Read my comments below from the bottom up.
> 
> But is it my imagination or are you bouncing to the renderer process, which
then
> bounces to the browser? Can't you directly to the browser?

Yes, we're going through the renderer process. If I would directly go to the
browser, I'd still need to get the plug-in and document URLs, which IIUC each
require a roundtrip to the renderer themselves, so I'm not sure that would
improve things?

http://codereview.chromium.org/10831045/diff/17001/webkit/plugins/ppapi/ppb_f...
File webkit/plugins/ppapi/ppb_flash_impl.cc (right):

http://codereview.chromium.org/10831045/diff/17001/webkit/plugins/ppapi/ppb_f...
webkit/plugins/ppapi/ppb_flash_impl.cc:257: case
PP_FLASHSETTING_LSORESTRICTIONS: {
On 2012/08/01 04:06:41, viettrungluu wrote:
> Why exactly do we need to support this in-process? (Or will the answer become
> obvious when I look at another file?)

This is proxied from the plug-in process.

[viettrungluu, 2012-08-01 13:52:37]

On 2012/08/01 05:17:39, Bernhard Bauer wrote:
> On 2012/08/01 04:06:41, viettrungluu wrote:
> > Read my comments below from the bottom up.
> > 
> > But is it my imagination or are you bouncing to the renderer process, which
> then
> > bounces to the browser? Can't you directly to the browser?
> 
> Yes, we're going through the renderer process. If I would directly go to the
> browser, I'd still need to get the plug-in and document URLs, which IIUC each
> require a roundtrip to the renderer themselves, so I'm not sure that would
> improve things?

I suppose we could have the plugin process store the plugin URL, though I guess
caching the document URL isn't valid (since the plugin could theoretically be
moved between documents -- bets on that working correctly? ha!). So I guess it's
okay, maybe.

> 
>
http://codereview.chromium.org/10831045/diff/17001/webkit/plugins/ppapi/ppb_f...
> File webkit/plugins/ppapi/ppb_flash_impl.cc (right):
> 
>
http://codereview.chromium.org/10831045/diff/17001/webkit/plugins/ppapi/ppb_f...
> webkit/plugins/ppapi/ppb_flash_impl.cc:257: case
> PP_FLASHSETTING_LSORESTRICTIONS: {
> On 2012/08/01 04:06:41, viettrungluu wrote:
> > Why exactly do we need to support this in-process? (Or will the answer
become
> > obvious when I look at another file?)
> 
> This is proxied from the plug-in process.

[viettrungluu, 2012-08-01 13:57:13]

I guess I'm mostly okay with this, other than the oddity of the two bool out
params (instead of an enum return value), but have brettw take a look at this
too.

http://codereview.chromium.org/10831045/diff/12022/chrome/browser/chrome_cont...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/10831045/diff/12022/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:964: ProfileIOData* io_data =
ProfileIOData::FromResourceContext(context);
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); ?

http://codereview.chromium.org/10831045/diff/12022/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:972: ProfileIOData* io_data =
ProfileIOData::FromResourceContext(context);
" ?

http://codereview.chromium.org/10831045/diff/12022/content/public/browser/con...
File content/public/browser/content_browser_client.h (right):

http://codereview.chromium.org/10831045/diff/12022/content/public/browser/con...
content/public/browser/content_browser_client.h:211: // access should be
blocked.
Maybe you should indicate the behavior of the default implementation?

http://codereview.chromium.org/10831045/diff/12022/content/public/browser/con...
content/public/browser/content_browser_client.h:219: // only.
"

http://codereview.chromium.org/10831045/diff/12022/webkit/plugins/ppapi/plugi...
File webkit/plugins/ppapi/plugin_delegate.h (right):

http://codereview.chromium.org/10831045/diff/12022/webkit/plugins/ppapi/plugi...
webkit/plugins/ppapi/plugin_delegate.h:671: bool* block_access,
I find it odd that you use two bool out parameters, instead of a single enum
(which you could have as a return value).

[Bernhard Bauer, 2012-08-02 03:54:07]

http://codereview.chromium.org/10831045/diff/12022/chrome/browser/chrome_cont...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/10831045/diff/12022/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:964: ProfileIOData* io_data =
ProfileIOData::FromResourceContext(context);
On 2012/08/01 13:57:14, viettrungluu wrote:
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); ?

Done. We should probably have that check in ProfileIOData though, for
simplicity.

http://codereview.chromium.org/10831045/diff/12022/content/public/browser/con...
File content/public/browser/content_browser_client.h (right):

http://codereview.chromium.org/10831045/diff/12022/content/public/browser/con...
content/public/browser/content_browser_client.h:211: // access should be
blocked.
On 2012/08/01 13:57:14, viettrungluu wrote:
> Maybe you should indicate the behavior of the default implementation?

Done.

http://codereview.chromium.org/10831045/diff/12022/webkit/plugins/ppapi/plugi...
File webkit/plugins/ppapi/plugin_delegate.h (right):

http://codereview.chromium.org/10831045/diff/12022/webkit/plugins/ppapi/plugi...
webkit/plugins/ppapi/plugin_delegate.h:671: bool* block_access,
On 2012/08/01 13:57:14, viettrungluu wrote:
> I find it odd that you use two bool out parameters, instead of a single enum
> (which you could have as a return value).

I mostly do it because I need to plumb that stuff through a lot of places, and
it's difficult to find a good place for an enum about plug-in local data
restrictions that is accessible from all these places.

[viettrungluu, 2012-08-02 13:19:33]

http://codereview.chromium.org/10831045/diff/12022/webkit/plugins/ppapi/plugi...
File webkit/plugins/ppapi/plugin_delegate.h (right):

http://codereview.chromium.org/10831045/diff/12022/webkit/plugins/ppapi/plugi...
webkit/plugins/ppapi/plugin_delegate.h:671: bool* block_access,
On 2012/08/02 03:54:08, Bernhard Bauer wrote:
> On 2012/08/01 13:57:14, viettrungluu wrote:
> > I find it odd that you use two bool out parameters, instead of a single enum
> > (which you could have as a return value).
> 
> I mostly do it because I need to plumb that stuff through a lot of places, and
> it's difficult to find a good place for an enum about plug-in local data
> restrictions that is accessible from all these places.

Really?  You could use the real PP_WhateverItWas enum (without breaking any
dependency rules). (It's only the impls of this class that'll need to know about
it.)

[Bernhard Bauer, 2012-08-02 18:28:35]

http://codereview.chromium.org/10831045/diff/12022/webkit/plugins/ppapi/plugi...
File webkit/plugins/ppapi/plugin_delegate.h (right):

http://codereview.chromium.org/10831045/diff/12022/webkit/plugins/ppapi/plugi...
webkit/plugins/ppapi/plugin_delegate.h:671: bool* block_access,
On 2012/08/02 13:19:33, viettrungluu wrote:
> On 2012/08/02 03:54:08, Bernhard Bauer wrote:
> > On 2012/08/01 13:57:14, viettrungluu wrote:
> > > I find it odd that you use two bool out parameters, instead of a single
enum
> > > (which you could have as a return value).
> > 
> > I mostly do it because I need to plumb that stuff through a lot of places,
and
> > it's difficult to find a good place for an enum about plug-in local data
> > restrictions that is accessible from all these places.
> 
> Really?  You could use the real PP_WhateverItWas enum (without breaking any
> dependency rules). (It's only the impls of this class that'll need to know
about
> it.)

Yeah, to break out of content/ I use individual methods anyway, so the enum
doesn't need to be visible there. Done.

[brettw, 2012-08-02 20:05:40]

lgtm

https://chromiumcodereview.appspot.com/10831045/diff/34/content/common/pepper...
File content/common/pepper_messages.h (right):

https://chromiumcodereview.appspot.com/10831045/diff/34/content/common/pepper...
content/common/pepper_messages.h:28: GURL /* document_url */,
Style nit: check indentation.

[Bernhard Bauer, 2012-08-02 20:27:22]

https://chromiumcodereview.appspot.com/10831045/diff/34/content/common/pepper...
File content/common/pepper_messages.h (right):

https://chromiumcodereview.appspot.com/10831045/diff/34/content/common/pepper...
content/common/pepper_messages.h:28: GURL /* document_url */,
On 2012/08/02 20:05:40, brettw wrote:
> Style nit: check indentation.

Done.

[commit-bot: I haz the power, 2012-08-02 20:29:54]

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

[commit-bot: I haz the power, 2012-08-02 21:50:21]

Change committed as 149711