Add GetSitesWithData and FreeSiteList methods to PPP_Flash_BrowserOperations interface and hook the…

Add GetSitesWithData and FreeSiteList methods to PPP_Flash_BrowserOperations interface and hook them up in PepperFlashSettingsManager.


BUG=132409
TEST=none


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

[Bernhard Bauer, 2012-07-25 21:05:00]

Please review.

[brettw, 2012-07-25 23:17:03]

Just looked at the IDL so far.

https://chromiumcodereview.appspot.com/10825018/diff/1/ppapi/api/private/ppp_...
File ppapi/api/private/ppp_flash_browser_operations.idl (right):

https://chromiumcodereview.appspot.com/10825018/diff/1/ppapi/api/private/ppp_...
ppapi/api/private/ppp_flash_browser_operations.idl:48: * Returns a list of sites
that have stored data, for use with the
Can you put these new versions at the bottom?

I try to keep "point" revisions of interfaces the same as the previous version
but with additions at the bottom (so you can downcast safely).

https://chromiumcodereview.appspot.com/10825018/diff/1/ppapi/api/private/ppp_...
ppapi/api/private/ppp_flash_browser_operations.idl:61: [out] str_t[] sites);
We need to define how the memory management works here. We say that the plugin
and system don't necessarily share the same allocator, so we don't want this.
Probably this function should just return a PP_Var that's a string.

Can you mention how the return value is formatted? (i.e. how the sites are
separated).

https://chromiumcodereview.appspot.com/10825018/diff/1/ppapi/api/private/ppp_...
ppapi/api/private/ppp_flash_browser_operations.idl:69: void FreeSiteList([inout]
str_t[] sites);
Ditto with the return value instead of out param.

[yzshen1, 2012-07-25 23:56:23]

https://chromiumcodereview.appspot.com/10825018/diff/1/chrome/browser/pepper_...
File chrome/browser/pepper_flash_settings_manager.cc (right):

https://chromiumcodereview.appspot.com/10825018/diff/1/chrome/browser/pepper_...
chrome/browser/pepper_flash_settings_manager.cc:792: void
PepperFlashSettingsManager::Core::OnGetSitesWithDataResult(
Please keep method definitions in the same order as declarations.

https://chromiumcodereview.appspot.com/10825018/diff/1/chrome/browser/pepper_...
chrome/browser/pepper_flash_settings_manager.cc:798: 
nit: it seems good to have the same DLOG_IF as other methods here.

https://chromiumcodereview.appspot.com/10825018/diff/1/chrome/browser/pepper_...
chrome/browser/pepper_flash_settings_manager.cc:820: DLOG_IF(ERROR, !success) <<
"SetSitePermission returned error";
Incorrect output.

https://chromiumcodereview.appspot.com/10825018/diff/1/content/ppapi_plugin/b...
File content/ppapi_plugin/broker_process_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/10825018/diff/1/content/ppapi_plugin/b...
content/ppapi_plugin/broker_process_dispatcher.cc:227: if
(flash_browser_operations_1_2_) {
Once we have 1.3 interface, we should try to use it if it is available, and then
fallback to 1.2/1.0. (We should do it for all methods below.)

https://chromiumcodereview.appspot.com/10825018/diff/1/ppapi/api/private/ppp_...
File ppapi/api/private/ppp_flash_browser_operations.idl (right):

https://chromiumcodereview.appspot.com/10825018/diff/1/ppapi/api/private/ppp_...
ppapi/api/private/ppp_flash_browser_operations.idl:61: [out] str_t[] sites);
On 2012/07/25 23:17:03, brettw wrote:
> We need to define how the memory management works here. We say that the plugin
> and system don't necessarily share the same allocator, so we don't want this.
> Probably this function should just return a PP_Var that's a string.

We couldn't use PP_Var in this interface, because the broker process doesn't
have var tracker.

> 
> Can you mention how the return value is formatted? (i.e. how the sites are
> separated).

https://chromiumcodereview.appspot.com/10825018/diff/1/ppapi/proxy/ppapi_mess...
File ppapi/proxy/ppapi_messages.h (right):

https://chromiumcodereview.appspot.com/10825018/diff/1/ppapi/proxy/ppapi_mess...
ppapi/proxy/ppapi_messages.h:292: // Request a list of sites that have data
stored from the plugin. The plugin
nit: Request -> Requests

[Bernhard Bauer, 2012-07-26 00:18:53]

Thanks for the reviews!

https://chromiumcodereview.appspot.com/10825018/diff/1/chrome/browser/pepper_...
File chrome/browser/pepper_flash_settings_manager.cc (right):

https://chromiumcodereview.appspot.com/10825018/diff/1/chrome/browser/pepper_...
chrome/browser/pepper_flash_settings_manager.cc:792: void
PepperFlashSettingsManager::Core::OnGetSitesWithDataResult(
On 2012/07/25 23:56:23, yzshen1 wrote:
> Please keep method definitions in the same order as declarations.

Done.

https://chromiumcodereview.appspot.com/10825018/diff/1/chrome/browser/pepper_...
chrome/browser/pepper_flash_settings_manager.cc:798: 
On 2012/07/25 23:56:23, yzshen1 wrote:
> nit: it seems good to have the same DLOG_IF as other methods here.

Hm. Having no sites with LSO data seems like a valid thing to happen. I'm not
sure if we need an additional parameter that indicates whether there was an
error or the list really is just empty?

https://chromiumcodereview.appspot.com/10825018/diff/1/chrome/browser/pepper_...
chrome/browser/pepper_flash_settings_manager.cc:820: DLOG_IF(ERROR, !success) <<
"SetSitePermission returned error";
On 2012/07/25 23:56:24, yzshen1 wrote:
> Incorrect output.

Oops. Fixed.

https://chromiumcodereview.appspot.com/10825018/diff/1/content/ppapi_plugin/b...
File content/ppapi_plugin/broker_process_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/10825018/diff/1/content/ppapi_plugin/b...
content/ppapi_plugin/broker_process_dispatcher.cc:227: if
(flash_browser_operations_1_2_) {
On 2012/07/25 23:56:24, yzshen1 wrote:
> Once we have 1.3 interface, we should try to use it if it is available, and
then
> fallback to 1.2/1.0. (We should do it for all methods below.)

Done.

https://chromiumcodereview.appspot.com/10825018/diff/1/ppapi/api/private/ppp_...
File ppapi/api/private/ppp_flash_browser_operations.idl (right):

https://chromiumcodereview.appspot.com/10825018/diff/1/ppapi/api/private/ppp_...
ppapi/api/private/ppp_flash_browser_operations.idl:48: * Returns a list of sites
that have stored data, for use with the
On 2012/07/25 23:17:03, brettw wrote:
> Can you put these new versions at the bottom?
> 
> I try to keep "point" revisions of interfaces the same as the previous version
> but with additions at the bottom (so you can downcast safely).

Done.

https://chromiumcodereview.appspot.com/10825018/diff/1/ppapi/api/private/ppp_...
ppapi/api/private/ppp_flash_browser_operations.idl:61: [out] str_t[] sites);
On 2012/07/25 23:17:03, brettw wrote:
> We need to define how the memory management works here. We say that the plugin
> and system don't necessarily share the same allocator, so we don't want this.

The caller passes in the list of sites to FreeSiteList() afterwards to free it.
Allocation and deallocation are both done by Flash.

> Probably this function should just return a PP_Var that's a string.
> 
> Can you mention how the return value is formatted? (i.e. how the sites are
> separated).

The outparam is an array of char*'s (which makes it a char***), not a single
string.

https://chromiumcodereview.appspot.com/10825018/diff/1/ppapi/proxy/ppapi_mess...
File ppapi/proxy/ppapi_messages.h (right):

https://chromiumcodereview.appspot.com/10825018/diff/1/ppapi/proxy/ppapi_mess...
ppapi/proxy/ppapi_messages.h:292: // Request a list of sites that have data
stored from the plugin. The plugin
On 2012/07/25 23:56:24, yzshen1 wrote:
> nit: Request -> Requests

Done.

[yzshen1, 2012-07-26 00:38:42]

LGTM with one more comment. (Please wait for other reviewers' LGTMs.)

Thanks!

https://chromiumcodereview.appspot.com/10825018/diff/1/chrome/browser/pepper_...
File chrome/browser/pepper_flash_settings_manager.cc (right):

https://chromiumcodereview.appspot.com/10825018/diff/1/chrome/browser/pepper_...
chrome/browser/pepper_flash_settings_manager.cc:798: 
Ah, okay, that is fine.
I didn't notice that there isn't a |success| parameter.

On 2012/07/26 00:18:53, Bernhard Bauer wrote:
> On 2012/07/25 23:56:23, yzshen1 wrote:
> > nit: it seems good to have the same DLOG_IF as other methods here.
> 
> Hm. Having no sites with LSO data seems like a valid thing to happen. I'm not
> sure if we need an additional parameter that indicates whether there was an
> error or the list really is just empty?

https://chromiumcodereview.appspot.com/10825018/diff/10001/content/ppapi_plug...
File content/ppapi_plugin/broker_process_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/10825018/diff/10001/content/ppapi_plug...
content/ppapi_plugin/broker_process_dispatcher.cc:304: if (sites.empty())
the two if-blocks share a lot of code, could you please de-dup?

[Bernhard Bauer, 2012-07-26 00:58:39]

https://chromiumcodereview.appspot.com/10825018/diff/10001/content/ppapi_plug...
File content/ppapi_plugin/broker_process_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/10825018/diff/10001/content/ppapi_plug...
content/ppapi_plugin/broker_process_dispatcher.cc:304: if (sites.empty())
On 2012/07/26 00:38:42, yzshen1 wrote:
> the two if-blocks share a lot of code, could you please de-dup?

Done.

[Bernhard Bauer, 2012-07-26 21:44:03]

Brett, ping? :)

John, can I get a rubberstamp for the small change to PluginDataRemoverImpl?

Thanks!

[jam, 2012-07-27 02:03:12]

On 2012/07/26 21:44:03, Bernhard Bauer wrote:
> Brett, ping? :)
> 
> John, can I get a rubberstamp for the small change to PluginDataRemoverImpl?

since brett is reviewing this change, and he's an owners in browser, you don't
need me

[brettw, 2012-07-27 21:30:30]

lgtm

[commit-bot: I haz the power, 2012-07-27 21:33:03]

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

[commit-bot: I haz the power, 2012-07-27 22:46:54]

Change committed as 148839