Add an interface for Flash to clear its data.

Add an interface for Flash to clear its data.

This mirrors NPP_ClearSiteData. I basically just hooked into the existing infrastructure in the browser process, and create a new plugin.

I changed the NPAPI IPC message to take the max age rather than compute it from the time so I did not have to duplicate the time computation code.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=132067

[Bernhard Bauer, 2012-04-05 10:01:38]

http://codereview.chromium.org/9981015/diff/2001/content/browser/plugin_data_...
File content/browser/plugin_data_remover_impl.h (right):

http://codereview.chromium.org/9981015/diff/2001/content/browser/plugin_data_...
content/browser/plugin_data_remover_impl.h:13: #include
"base/memory/scoped_ptr.h"
Nit: Is this necessary?

http://codereview.chromium.org/9981015/diff/2001/ppapi/api/private/ppp_flash_...
File ppapi/api/private/ppp_flash_clear_site_data.idl (right):

http://codereview.chromium.org/9981015/diff/2001/ppapi/api/private/ppp_flash_...
ppapi/api/private/ppp_flash_clear_site_data.idl:33: void ClearSiteData(str_t
site,
Should we return an error code here?

[brettw, 2012-04-05 14:47:23]

http://codereview.chromium.org/9981015/diff/2001/ppapi/api/private/ppp_flash_...
File ppapi/api/private/ppp_flash_clear_site_data.idl (right):

http://codereview.chromium.org/9981015/diff/2001/ppapi/api/private/ppp_flash_...
ppapi/api/private/ppp_flash_clear_site_data.idl:33: void ClearSiteData(str_t
site,
On 2012/04/05 10:01:38, Bernhard Bauer wrote:
> Should we return an error code here?

It seemed like Chrome didn't do anything with this value. Do you think it would
be useful?

[Bernhard Bauer, 2012-04-05 14:49:07]

http://codereview.chromium.org/9981015/diff/2001/ppapi/api/private/ppp_flash_...
File ppapi/api/private/ppp_flash_clear_site_data.idl (right):

http://codereview.chromium.org/9981015/diff/2001/ppapi/api/private/ppp_flash_...
ppapi/api/private/ppp_flash_clear_site_data.idl:33: void ClearSiteData(str_t
site,
On 2012/04/05 14:47:23, brettw wrote:
> On 2012/04/05 10:01:38, Bernhard Bauer wrote:
> > Should we return an error code here?
> 
> It seemed like Chrome didn't do anything with this value. Do you think it
would
> be useful?

It might be useful if we have UI to show errors in the Clear Browsing Data
dialog at some point.

[viettrungluu, 2012-04-05 17:22:23]

http://codereview.chromium.org/9981015/diff/2001/ppapi/api/private/ppp_flash_...
File ppapi/api/private/ppp_flash_clear_site_data.idl (right):

http://codereview.chromium.org/9981015/diff/2001/ppapi/api/private/ppp_flash_...
ppapi/api/private/ppp_flash_clear_site_data.idl:33: void ClearSiteData(str_t
site,
This function needs to specify where the Flash data to be cleared lives, given
that Flapper has no idea. Moreover, the location is actually under the profile
directory.[*]

[*] Who knows how Flapper handles multi-profile properly right now.

[Bernhard Bauer, 2012-04-05 18:49:36]

http://codereview.chromium.org/9981015/diff/2001/ppapi/api/private/ppp_flash_...
File ppapi/api/private/ppp_flash_clear_site_data.idl (right):

http://codereview.chromium.org/9981015/diff/2001/ppapi/api/private/ppp_flash_...
ppapi/api/private/ppp_flash_clear_site_data.idl:33: void ClearSiteData(str_t
site,
On 2012/04/05 17:22:24, viettrungluu wrote:
> This function needs to specify where the Flash data to be cleared lives, given
> that Flapper has no idea. Moreover, the location is actually under the profile
> directory.[*]
> 
> [*] Who knows how Flapper handles multi-profile properly right now.

I thought all versions of Flash, including Flapper, have been storing their data
in the same location
(http://en.wikipedia.org/wiki/Local_Shared_Object#File_locations )?

[viettrungluu, 2012-04-05 19:51:54]

On 2012/04/05 18:49:36, Bernhard Bauer wrote:
>
http://codereview.chromium.org/9981015/diff/2001/ppapi/api/private/ppp_flash_...
> File ppapi/api/private/ppp_flash_clear_site_data.idl (right):
> 
>
http://codereview.chromium.org/9981015/diff/2001/ppapi/api/private/ppp_flash_...
> ppapi/api/private/ppp_flash_clear_site_data.idl:33: void ClearSiteData(str_t
> site,
> On 2012/04/05 17:22:24, viettrungluu wrote:
> > This function needs to specify where the Flash data to be cleared lives,
given
> > that Flapper has no idea. Moreover, the location is actually under the
profile
> > directory.[*]
> > 
> > [*] Who knows how Flapper handles multi-profile properly right now.
> 
> I thought all versions of Flash, including Flapper, have been storing their
data
> in the same location
> (http://en.wikipedia.org/wiki/Local_Shared_Object#File_locations )?

No.

[viettrungluu, 2012-04-05 20:06:18]

Also, Brett, is there (or could you add) a way for Flapper to determine
(preferably in module initialization) if it's being loaded outside the sandbox
(i.e., in the broker)?

There's probably some things (in initialization) which should be skipped in that
case, and it would help to make things safer (e.g., we never want to create an
instance in that case).

[bauerb at google, 2012-04-05 20:06:23]

On Thu Apr 05 21:51:54 GMT+200 2012, <viettrungluu@chromium.org> wrote:

> On 2012/04/05 18:49:36, Bernhard Bauer wrote:
>
>
>
http://codereview.chromium.org/9981015/diff/2001/ppapi/api/private/ppp_flash_...
> > File ppapi/api/private/ppp_flash_clear_site_data.idl (right):
>
>
>
>
http://codereview.chromium.org/9981015/diff/2001/ppapi/api/private/ppp_flash_...
> > ppapi/api/private/ppp_flash_clear_site_data.idl:33: void
> > ClearSiteData(str_t
> > site,
> > On 2012/04/05 17:22:24, viettrungluu wrote:
> > > This function needs to specify where the Flash data to be cleared
> lives,
> given
> > > that Flapper has no idea. Moreover, the location is actually under the
> profile
> > > directory.[*]
> > >
> > > [*] Who knows how Flapper handles multi-profile properly right now.
>
> > I thought all versions of Flash, including Flapper, have been storing
> > their
> data
> > in the same location
> >
(http://en.wikipedia.org/wiki/Local_Shared_Object#File_locations<https://www.g...
>
> No.
>
So… where *does* Flapper store its LSOs then?

Bernhard.

http://codereview.chromium.org/9981015/<https://www.google.com/url?sa=D&q=htt...
>
>

[viettrungluu, 2012-04-06 02:54:42]

On 2012/04/05 20:06:23, please use chromium account wrote:
> On Thu Apr 05 21:51:54 GMT+200 2012, <mailto:viettrungluu@chromium.org> wrote:
> 
> > On 2012/04/05 18:49:36, Bernhard Bauer wrote:
> >
> >
> >
>
http://codereview.chromium.org/9981015/diff/2001/ppapi/api/private/ppp_flash_...>
> > > File ppapi/api/private/ppp_flash_clear_site_data.idl (right):
> >
> >
> >
> >
>
http://codereview.chromium.org/9981015/diff/2001/ppapi/api/private/ppp_flash_...>
> > > ppapi/api/private/ppp_flash_clear_site_data.idl:33: void
> > > ClearSiteData(str_t
> > > site,
> > > On 2012/04/05 17:22:24, viettrungluu wrote:
> > > > This function needs to specify where the Flash data to be cleared
> > lives,
> > given
> > > > that Flapper has no idea. Moreover, the location is actually under the
> > profile
> > > > directory.[*]
> > > >
> > > > [*] Who knows how Flapper handles multi-profile properly right now.
> >
> > > I thought all versions of Flash, including Flapper, have been storing
> > > their
> > data
> > > in the same location
> > >
>
(http://en.wikipedia.org/wiki/Local_Shared_Object#File_locations%3Chttps://www...
> >
> > No.
> >
> So… where *does* Flapper store its LSOs then?

As I said, in some subdirectory of the profile directory (whose name I don't
know off the top of my head).

> 
> Bernhard.
> 
>
http://codereview.chromium.org/9981015/%3Chttps://www.google.com/url?sa=D&q=h...>
> >
> >

[brettw, 2012-04-06 04:15:57]

Uploaded a new one with a profile path and a return value. I renamed the
interface as well.

I wasn't exactly sure what profile string I should use, so left it empty for
now, this will be easy to add in later.

[brettw, 2012-04-10 19:58:27]

ping

[Bernhard Bauer, 2012-04-10 20:13:43]

LGTM w/ nits.

Stupid question: If we have a per-profile directory for LSOs  in Flapper, how
does it know where to put them?

http://codereview.chromium.org/9981015/diff/8002/content/browser/plugin_data_...
File content/browser/plugin_data_remover_impl.cc (right):

http://codereview.chromium.org/9981015/diff/8002/content/browser/plugin_data_...
content/browser/plugin_data_remover_impl.cc:226: FilePath profile_path;  //
FIXME ========================================================
Can you make this a TODO?

http://codereview.chromium.org/9981015/diff/8002/ppapi/api/private/ppp_flash_...
File ppapi/api/private/ppp_flash_browser_operations.idl (right):

http://codereview.chromium.org/9981015/diff/8002/ppapi/api/private/ppp_flash_...
ppapi/api/private/ppp_flash_browser_operations.idl:21: * @profile_dir String
containing the directory of the profile. On UTF-16
Could we change this to mean the directory containing the LSOs instead of the
profile directory?

[brettw, 2012-04-10 22:25:31]

I actually implemented the directory stuff, I forgot I hadn't done that. It
involved some rejiggering of the contexts to get the profile in there.

I changed the spec of the API to give the Flash data dir.

[viettrungluu, 2012-04-11 02:48:10]

I reviewed this change backwards, so the context of my comments might make more
sense if you read from the bottom up.

But mostly looks good, with just some nits.

http://codereview.chromium.org/9981015/diff/15002/content/browser/plugin_data...
File content/browser/plugin_data_remover_impl.cc (right):

http://codereview.chromium.org/9981015/diff/15002/content/browser/plugin_data...
content/browser/plugin_data_remover_impl.cc:107: // Balanced in OnChannelOpened
or OnError. Exactly one them will eventually
Now there are two OnChannelOpened's!

http://codereview.chromium.org/9981015/diff/15002/content/browser/plugin_data...
content/browser/plugin_data_remover_impl.cc:171: virtual void OnChannelOpened(
Having two very similar |OnChannelOpened()|s is really rather confusing -- could
you do something about it?

Also, can the Pepper broker start fail?

http://codereview.chromium.org/9981015/diff/15002/content/browser/plugin_data...
File content/browser/plugin_data_remover_impl.h (right):

http://codereview.chromium.org/9981015/diff/15002/content/browser/plugin_data...
content/browser/plugin_data_remover_impl.h:13: #include
"base/memory/scoped_ptr.h"
Eh?

(Though I still wonder where base::Time is coming from.)

http://codereview.chromium.org/9981015/diff/15002/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_file_message_filter.h (right):

http://codereview.chromium.org/9981015/diff/15002/content/browser/renderer_ho...
content/browser/renderer_host/pepper_file_message_filter.h:47: // Returns the
name of the pepper data directory that we'll use. The argument
"that we'll use" ... for what? For "Pepper module local data", presumably.

http://codereview.chromium.org/9981015/diff/15002/content/plugin/plugin_chann...
File content/plugin/plugin_channel.h (right):

http://codereview.chromium.org/9981015/diff/15002/content/plugin/plugin_chann...
content/plugin/plugin_channel.h:1: // Copyright (c) 2011 The Chromium Authors.
All rights reserved.
2012, or is that automatically updated these days?

http://codereview.chromium.org/9981015/diff/15002/content/plugin/plugin_chann...
content/plugin/plugin_channel.h:87: uint64 max_age);
/me wonders where base::Time was declared. Oh well, problem solved.

http://codereview.chromium.org/9981015/diff/15002/content/ppapi_plugin/ppapi_...
File content/ppapi_plugin/ppapi_thread.cc (right):

http://codereview.chromium.org/9981015/diff/15002/content/ppapi_plugin/ppapi_...
content/ppapi_plugin/ppapi_thread.cc:24: #include
"ppapi/c/private/ppp_flash_browser_operations.h"
nit: alphabetical order

http://codereview.chromium.org/9981015/diff/15002/content/ppapi_plugin/ppapi_...
content/ppapi_plugin/ppapi_thread.cc:292: bool PpapiThread::ClearSiteData(const
FilePath& profile_path,
What I said about "profile path" (in the .idl file). It's kind of confusing.

http://codereview.chromium.org/9981015/diff/15002/content/ppapi_plugin/ppapi_...
File content/ppapi_plugin/ppapi_thread.h (right):

http://codereview.chromium.org/9981015/diff/15002/content/ppapi_plugin/ppapi_...
content/ppapi_plugin/ppapi_thread.h:13: #include "base/file_path.h"
You can just forward-declare FilePath.

OTOH, you should include <string>.

http://codereview.chromium.org/9981015/diff/15002/ppapi/api/private/ppp_flash...
File ppapi/api/private/ppp_flash_browser_operations.idl (right):

http://codereview.chromium.org/9981015/diff/15002/ppapi/api/private/ppp_flash...
ppapi/api/private/ppp_flash_browser_operations.idl:21: * @profile_dir String
containing the directory where the plugin data is
I think you should call it "plugin_data_dir" or something like that. Maybe the
comment should also indicate that it's an absolute path?

(Incidentally, does it have a trailing slash/backslash? Probably not, but it may
be best to be clear about this.)

http://codereview.chromium.org/9981015/diff/15002/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

http://codereview.chromium.org/9981015/diff/15002/ppapi/proxy/ppapi_messages....
ppapi/proxy/ppapi_messages.h:248: // process will respond with
PpapiHostMsg_ClearSiteDataResult.
Should you indicate that this is for Flash specifically?

[brettw, 2012-04-11 21:22:34]

Comments addressed, PTAL

https://chromiumcodereview.appspot.com/9981015/diff/15002/content/plugin/plug...
File content/plugin/plugin_channel.h (right):

https://chromiumcodereview.appspot.com/9981015/diff/15002/content/plugin/plug...
content/plugin/plugin_channel.h:1: // Copyright (c) 2011 The Chromium Authors.
All rights reserved.
CQ does it automatically. I don't care beyond that.

https://chromiumcodereview.appspot.com/9981015/diff/26002/ppapi/api/ppb_mouse...
File ppapi/api/ppb_mouse_cursor.idl (right):

https://chromiumcodereview.appspot.com/9981015/diff/26002/ppapi/api/ppb_mouse...
ppapi/api/ppb_mouse_cursor.idl:1: /* Copyright (c) 2012 The Chromium Authors.
All rights reserved.
I had to fix the line endings in this file to make the IDL generator happy.

[sky, 2012-04-11 23:03:09]

LGTM

[viettrungluu, 2012-04-11 23:25:26]

LGTM with two nits.

https://chromiumcodereview.appspot.com/9981015/diff/14047/content/ppapi_plugi...
File content/ppapi_plugin/ppapi_thread.h (right):

https://chromiumcodereview.appspot.com/9981015/diff/14047/content/ppapi_plugi...
content/ppapi_plugin/ppapi_thread.h:14: #include "base/file_path.h"
Nit: unneeded.

https://chromiumcodereview.appspot.com/9981015/diff/14047/ppapi/api/private/p...
File ppapi/api/private/ppp_flash_browser_operations.idl (right):

https://chromiumcodereview.appspot.com/9981015/diff/14047/ppapi/api/private/p...
ppapi/api/private/ppp_flash_browser_operations.idl:21: * @profile_dir String
containing the directory where the plugin data is
You forgot to update the docs.