Make kClearSiteDataOnExit work correctly for Flash in multi-profile.

Make kClearSiteDataOnExit work for Flash plugins in a multi-profile environment.

Because Flash data is global among all profiles (and all browsers) on a machine, we can't clear site data easily per profile on exit. Instead, if this flag is set, we run Flash instances in Incognito mode for profiles that want to leave no data stored on exit.

BUG=80794
TEST=

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

[Miranda Callahan, 2011-06-28 22:11:22]

Ok, after much poking about, I discovered the NPAPI specification -- I'm
guessing that NPNVprivateModeBool has to be set somewhere (in the
WebPluginDelegate?) to make this do the right thing.  Will poke around a bit
more -- if you think I'm almost there, I'll continue; if there's much more
plugin work that I'm not seeing due to my ignorance of plugins, perhaps the
Muenchener Nooglerin should take it over?

[bauerb at google, 2011-06-28 22:31:29]

On 2011/06/28 22:11:22, Miranda Callahan wrote:
> Ok, after much poking about, I discovered the NPAPI specification -- I'm
> guessing that NPNVprivateModeBool has to be set somewhere (in the
> WebPluginDelegate?) to make this do the right thing.  

We already do that if the incognito flag is set. What I meant was basically
setting the flag to true if we are in an incognito profile *or* if the
preference to clear data at shutdown is set. That way, we don't need any changes
on the plug-in side (which only needs to know about one flag, regardless of
where it comes from)


> Will poke around a bit
> more -- if you think I'm almost there, I'll continue; if there's much more
> plugin work that I'm not seeing due to my ignorance of plugins, perhaps the
> Muenchener Nooglerin should take it over?

[Miranda Callahan, 2011-06-28 22:34:31]

On 2011/06/28 22:31:29, please use chromium account wrote:
> On 2011/06/28 22:11:22, Miranda Callahan wrote:
> > Ok, after much poking about, I discovered the NPAPI specification -- I'm
> > guessing that NPNVprivateModeBool has to be set somewhere (in the
> > WebPluginDelegate?) to make this do the right thing.  
> 
> We already do that if the incognito flag is set. What I meant was basically
> setting the flag to true if we are in an incognito profile *or* if the
> preference to clear data at shutdown is set. That way, we don't need any
changes
> on the plug-in side (which only needs to know about one flag, regardless of
> where it comes from)

Ok, great -- sounds like it's simpler than I was making it out to be. Thanks for
the advice; will try to finish this later today or tomorrow, then.

[Jói, 2011-06-29 03:37:32]

http://codereview.chromium.org/7230052/diff/1/content/browser/renderer_host/r...
File content/browser/renderer_host/render_message_filter.cc (right):

http://codereview.chromium.org/7230052/diff/1/content/browser/renderer_host/r...
content/browser/renderer_host/render_message_filter.cc:17: #include
"chrome/browser/prefs/pref_service.h"
Please don't add a content->pref_service dependency, this dependency has been
removed from content. You may want to e.g. add a method to ContentBrowserClient
(see use of content::GetContentClient()->browser() below) or have the Chrome
side poke this setting somewhere into content whenever it changes.

[Miranda Callahan, 2011-06-29 19:55:31]

Piped the bool through a PrefMember in ProfileIOData that I moved to the
IOThread -- this seems to do the trick. Thanks to both of you for suggestions;
should be ready for a review.

http://codereview.chromium.org/7230052/diff/1/content/browser/renderer_host/r...
File content/browser/renderer_host/render_message_filter.cc (right):

http://codereview.chromium.org/7230052/diff/1/content/browser/renderer_host/r...
content/browser/renderer_host/render_message_filter.cc:17: #include
"chrome/browser/prefs/pref_service.h"
On 2011/06/29 03:37:32, Jói wrote:
> Please don't add a content->pref_service dependency, this dependency has been
> removed from content. You may want to e.g. add a method to
ContentBrowserClient
> (see use of content::GetContentClient()->browser() below) or have the Chrome
> side poke this setting somewhere into content whenever it changes.

Ok -- thanks for the suggestion, it seems to work well.

[Jói, 2011-06-30 12:09:16]

LGTM

[Bernhard Bauer, 2011-06-30 12:24:12]

http://codereview.chromium.org/7230052/diff/9029/content/browser/plugin_proce...
File content/browser/plugin_process_host.h (right):

http://codereview.chromium.org/7230052/diff/9029/content/browser/plugin_proce...
content/browser/plugin_process_host.h:48: virtual bool SaveLocalState() = 0;
I think we should collapse |OffTheRecord| and |SaveLocalState| already in the
PluginProcessHost::Client, as they both essentially tell the plug-in not to save
state (and whether this is because of incognito mode or a preference is a
chrome/ thing, not a content/ thing).

[Miranda Callahan, 2011-06-30 13:38:43]

http://codereview.chromium.org/7230052/diff/9029/content/browser/plugin_proce...
File content/browser/plugin_process_host.h (right):

http://codereview.chromium.org/7230052/diff/9029/content/browser/plugin_proce...
content/browser/plugin_process_host.h:48: virtual bool SaveLocalState() = 0;
On 2011/06/30 12:24:12, Bernhard Bauer wrote:
> I think we should collapse |OffTheRecord| and |SaveLocalState| already in the
> PluginProcessHost::Client, as they both essentially tell the plug-in not to
save
> state (and whether this is because of incognito mode or a preference is a
> chrome/ thing, not a content/ thing). 

Ha -- I woke up at four in the morning and had the same thought. Done.

[Bernhard Bauer, 2011-06-30 13:47:28]

LGTM with a cleanup nit:

http://codereview.chromium.org/7230052/diff/11026/chrome/browser/browser_proc...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/7230052/diff/11026/chrome/browser/browser_proc...
chrome/browser/browser_process_impl.cc:653: void
BrowserProcessImpl::WaitForPluginDataRemoverToFinish() {
This method (and the whole |plugin_data_remover_|) is now unnecessary as well.
Could you remove it? Thanks!

[Miranda Callahan, 2011-06-30 14:07:39]

Thanks!

http://codereview.chromium.org/7230052/diff/11026/chrome/browser/browser_proc...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/7230052/diff/11026/chrome/browser/browser_proc...
chrome/browser/browser_process_impl.cc:653: void
BrowserProcessImpl::WaitForPluginDataRemoverToFinish() {
On 2011/06/30 13:47:29, Bernhard Bauer wrote:
> This method (and the whole |plugin_data_remover_|) is now unnecessary as well.
> Could you remove it? Thanks!

Done.

[commit-bot: I haz the power, 2011-06-30 16:12:19]

Presubmit check for 7230052-10050 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
content/browser/mock_content_browser_client.h,content/browser/content_browser_client.h,content/browser/renderer_host/render_message_filter.cc,content/browser/renderer_host/render_message_filter.h,content/browser/plugin_process_host.cc,content/browser/mock_content_browser_client.cc

Presubmit checks took 1.0s to calculate.

[Miranda Callahan, 2011-06-30 16:19:23]

Brett, may I have your imprimatur?

[Miranda Callahan, 2011-06-30 17:03:52]

Darin, are you available to OWNER-review this change?

[darin (slow to review), 2011-06-30 17:45:50]

content/  <-- LGTM

[commit-bot: I haz the power, 2011-06-30 18:51:57]

Change committed as 91168