Pepper Flash settings integration: add UI for "deauthorize content licenses".

Pepper Flash settings integration: add UI for "deauthorize content licenses".

This CL adds to the "Clear browsing data" dialog a checkbox, which will only show up when Pepper Flash is in use.

BUG=112190
TEST=None


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

[yzshen1, 2012-05-16 21:29:50]

Hi, Brett and Chris.

Would you please take a look? Thanks!

Chris: things in chrome/browser/resources and chrome/browser/ui.
Brett: everything else.

[yzshen1, 2012-05-18 20:57:41]

Friendly ping.

On 2012/05/16 21:29:50, yzshen1 wrote:
> Hi, Brett and Chris.
> 
> Would you please take a look? Thanks!
> 
> Chris: things in chrome/browser/resources and chrome/browser/ui.
> Brett: everything else.

[csilv, 2012-05-18 21:21:41]

lgtm

chrome/browser/resources, chrome/browser/ui

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

http://codereview.chromium.org/10387161/diff/3003/chrome/app/generated_resour...
chrome/app/generated_resources.grd:6550: +        Deauthorize content licenses
I worry that this is going to be a confusing item for average users... the
wording is a bit technical.  You may want to confer with UI leads about the best
wording for this.

http://codereview.chromium.org/10387161/diff/3003/chrome/browser/resources/op...
File chrome/browser/resources/options2/clear_browser_data_overlay.html (right):

http://codereview.chromium.org/10387161/diff/3003/chrome/browser/resources/op...
chrome/browser/resources/options2/clear_browser_data_overlay.html:64: </div>
nit: line 64 is indented by 1 extra space

[yzshen1, 2012-05-18 21:30:01]

Thanks Chris!

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

http://codereview.chromium.org/10387161/diff/3003/chrome/app/generated_resour...
chrome/app/generated_resources.grd:6550: +        Deauthorize content licenses
The wording is from PM/UI leads and partners. (I agree it is not very
user-friendly, though. :))
http://code.google.com/p/chromium/issues/detail?id=112190#c7


On 2012/05/18 21:21:41, csilv wrote:
> I worry that this is going to be a confusing item for average users... the
> wording is a bit technical.  You may want to confer with UI leads about the
best
> wording for this.

http://codereview.chromium.org/10387161/diff/3003/chrome/browser/resources/op...
File chrome/browser/resources/options2/clear_browser_data_overlay.html (right):

http://codereview.chromium.org/10387161/diff/3003/chrome/browser/resources/op...
chrome/browser/resources/options2/clear_browser_data_overlay.html:64: </div>
On 2012/05/18 21:21:41, csilv wrote:
> nit: line 64 is indented by 1 extra space

Done.

[brettw, 2012-05-20 21:15:40]

http://codereview.chromium.org/10387161/diff/13001/chrome/browser/plugin_data...
File chrome/browser/plugin_data_remover_helper.h (right):

http://codereview.chromium.org/10387161/diff/13001/chrome/browser/plugin_data...
chrome/browser/plugin_data_remover_helper.h:17: // Helper class to
(asynchronously) update the preference specifying whether
I don't get the point of this class, and I don't see anything asynchronous in
it.

[yzshen1, 2012-05-21 16:58:44]

http://codereview.chromium.org/10387161/diff/13001/chrome/browser/plugin_data...
File chrome/browser/plugin_data_remover_helper.h (right):

http://codereview.chromium.org/10387161/diff/13001/chrome/browser/plugin_data...
chrome/browser/plugin_data_remover_helper.h:17: // Helper class to
(asynchronously) update the preference specifying whether
The 'async' part is moved into PluginStatusPrefSetter, which set several pref
values specified by PluginStatusPrefSetter::Client.

Do you think it is better to merge the two clients directly into
PluginStatusPrefSetter? (But we may want to keep the static IsSupported() here.)
Or do you suggest me to update the comments?

Thanks!
On 2012/05/20 21:15:40, brettw wrote:
> I don't get the point of this class, and I don't see anything asynchronous in
> it.

[brettw, 2012-05-21 17:45:33]

On Mon, May 21, 2012 at 9:58 AM,  <yzshen@chromium.org> wrote:
>
>
http://codereview.chromium.org/10387161/diff/13001/chrome/browser/plugin_data...
> File chrome/browser/plugin_data_remover_helper.h (right):
>
>
http://codereview.chromium.org/10387161/diff/13001/chrome/browser/plugin_data...
> chrome/browser/plugin_data_remover_helper.h:17: // Helper class to
> (asynchronously) update the preference specifying whether
> The 'async' part is moved into PluginStatusPrefSetter, which set several
> pref values specified by PluginStatusPrefSetter::Client.
>
> Do you think it is better to merge the two clients directly into
> PluginStatusPrefSetter? (But we may want to keep the static
> IsSupported() here.)
> Or do you suggest me to update the comments?

I'm not sure what the best approach is. Is there any reason this
shouldn't be in the PluginStatusPrefSetter? It looks like both of
these files are pretty small and simple.

Brett

[yzshen1, 2012-05-21 19:07:23]

On 2012/05/21 17:45:33, brettw wrote:
> On Mon, May 21, 2012 at 9:58 AM,  <mailto:yzshen@chromium.org> wrote:
> >
> >
>
http://codereview.chromium.org/10387161/diff/13001/chrome/browser/plugin_data...
> > File chrome/browser/plugin_data_remover_helper.h (right):
> >
> >
>
http://codereview.chromium.org/10387161/diff/13001/chrome/browser/plugin_data...
> > chrome/browser/plugin_data_remover_helper.h:17: // Helper class to
> > (asynchronously) update the preference specifying whether
> > The 'async' part is moved into PluginStatusPrefSetter, which set several
> > pref values specified by PluginStatusPrefSetter::Client.
> >
> > Do you think it is better to merge the two clients directly into
> > PluginStatusPrefSetter? (But we may want to keep the static
> > IsSupported() here.)
> > Or do you suggest me to update the comments?
> 
> I'm not sure what the best approach is. Is there any reason this
> shouldn't be in the PluginStatusPrefSetter? It looks like both of
> these files are pretty small and simple.
> 
> Brett

Done.

There is no good reason to keep registration of pref members outside of
PluginStatusPrefSetter. It is only used by core_options_handler2. So I have made
changes as you suggested.

Thanks!

[yzshen1, 2012-05-21 19:11:00]

Hi, Scott.

Would you please review chrome/browser/ui/browser.cc?
Thanks!
On 2012/05/21 19:07:23, yzshen1 wrote:
> On 2012/05/21 17:45:33, brettw wrote:
> > On Mon, May 21, 2012 at 9:58 AM,  <mailto:yzshen@chromium.org> wrote:
> > >
> > >
> >
>
http://codereview.chromium.org/10387161/diff/13001/chrome/browser/plugin_data...
> > > File chrome/browser/plugin_data_remover_helper.h (right):
> > >
> > >
> >
>
http://codereview.chromium.org/10387161/diff/13001/chrome/browser/plugin_data...
> > > chrome/browser/plugin_data_remover_helper.h:17: // Helper class to
> > > (asynchronously) update the preference specifying whether
> > > The 'async' part is moved into PluginStatusPrefSetter, which set several
> > > pref values specified by PluginStatusPrefSetter::Client.
> > >
> > > Do you think it is better to merge the two clients directly into
> > > PluginStatusPrefSetter? (But we may want to keep the static
> > > IsSupported() here.)
> > > Or do you suggest me to update the comments?
> > 
> > I'm not sure what the best approach is. Is there any reason this
> > shouldn't be in the PluginStatusPrefSetter? It looks like both of
> > these files are pretty small and simple.
> > 
> > Brett
> 
> Done.
> 
> There is no good reason to keep registration of pref members outside of
> PluginStatusPrefSetter. It is only used by core_options_handler2. So I have
made
> changes as you suggested.
> 
> Thanks!

[sky, 2012-05-21 20:47:50]

https://chromiumcodereview.appspot.com/10387161/diff/7027/chrome/browser/ui/b...
File chrome/browser/ui/browser.cc (right):

https://chromiumcodereview.appspot.com/10387161/diff/7027/chrome/browser/ui/b...
chrome/browser/ui/browser.cc:2387:
prefs->RegisterBooleanPref(prefs::kDeauthorizeContentLicenses,
Why is this in browser and not some where closer to pepper/flash?

[yzshen1, 2012-05-21 20:53:19]

https://chromiumcodereview.appspot.com/10387161/diff/7027/chrome/browser/ui/b...
File chrome/browser/ui/browser.cc (right):

https://chromiumcodereview.appspot.com/10387161/diff/7027/chrome/browser/ui/b...
chrome/browser/ui/browser.cc:2387:
prefs->RegisterBooleanPref(prefs::kDeauthorizeContentLicenses,
On 2012/05/21 20:47:50, sky wrote:
> Why is this in browser and not some where closer to pepper/flash?

This option item is displayed in the "Clear browsing data" dialog along with the
six prefs above (line 2369-2386). So I thought it makes sense to put them
together. Similarly kPepperFlashSettingsEnabled is used together with
kClearPluginLSODataEnabled, so I put it in this file as well.

Do you think that is okay? Thanks!

[sky, 2012-05-21 20:56:38]

On Mon, May 21, 2012 at 1:53 PM,  <yzshen@chromium.org> wrote:
>
>
https://chromiumcodereview.appspot.com/10387161/diff/7027/chrome/browser/ui/b...
> File chrome/browser/ui/browser.cc (right):
>
>
https://chromiumcodereview.appspot.com/10387161/diff/7027/chrome/browser/ui/b...
> chrome/browser/ui/browser.cc:2387:
> prefs->RegisterBooleanPref(prefs::kDeauthorizeContentLicenses,
> On 2012/05/21 20:47:50, sky wrote:
>>
>> Why is this in browser and not some where closer to pepper/flash?
>
>
> This option item is displayed in the "Clear browsing data" dialog along
> with the six prefs above (line 2369-2386). So I thought it makes sense
> to put them together. Similarly kPepperFlashSettingsEnabled is used
> together with kClearPluginLSODataEnabled, so I put it in this file as
> well.
>
> Do you think that is okay? Thanks!
>
> https://chromiumcodereview.appspot.com/10387161/

Prefs should be registered close to the classes that actually use
them. I don't think thats browser.

  -Scott

[yzshen1, 2012-05-21 21:02:37]

On 2012/05/21 20:56:38, sky wrote:
> On Mon, May 21, 2012 at 1:53 PM,  <mailto:yzshen@chromium.org> wrote:
> >
> >
>
https://chromiumcodereview.appspot.com/10387161/diff/7027/chrome/browser/ui/b...
> > File chrome/browser/ui/browser.cc (right):
> >
> >
>
https://chromiumcodereview.appspot.com/10387161/diff/7027/chrome/browser/ui/b...
> > chrome/browser/ui/browser.cc:2387:
> > prefs->RegisterBooleanPref(prefs::kDeauthorizeContentLicenses,
> > On 2012/05/21 20:47:50, sky wrote:
> >>
> >> Why is this in browser and not some where closer to pepper/flash?
> >
> >
> > This option item is displayed in the "Clear browsing data" dialog along
> > with the six prefs above (line 2369-2386). So I thought it makes sense
> > to put them together. Similarly kPepperFlashSettingsEnabled is used
> > together with kClearPluginLSODataEnabled, so I put it in this file as
> > well.
> >
> > Do you think that is okay? Thanks!
> >
> > https://chromiumcodereview.appspot.com/10387161/
> 
> Prefs should be registered close to the classes that actually use
> them. I don't think thats browser.
> 
>   -Scott

[I am happy to make the change. Just try to understand your point better.]
It seems things like kDeleteBrowsingHistory or kClearPluginLSODataEnabled are
not used in browser either. Is this something that we would like to change?

[sky, 2012-05-21 21:23:33]

On Mon, May 21, 2012 at 2:02 PM,  <yzshen@chromium.org> wrote:
> On 2012/05/21 20:56:38, sky wrote:
>
>> On Mon, May 21, 2012 at 1:53 PM,  <mailto:yzshen@chromium.org> wrote:
>> >
>> >
>
>
>
https://chromiumcodereview.appspot.com/10387161/diff/7027/chrome/browser/ui/b...
>>
>> > File chrome/browser/ui/browser.cc (right):
>> >
>> >
>
>
>
https://chromiumcodereview.appspot.com/10387161/diff/7027/chrome/browser/ui/b...
>>
>> > chrome/browser/ui/browser.cc:2387:
>> > prefs->RegisterBooleanPref(prefs::kDeauthorizeContentLicenses,
>> > On 2012/05/21 20:47:50, sky wrote:
>> >>
>> >> Why is this in browser and not some where closer to pepper/flash?
>> >
>> >
>> > This option item is displayed in the "Clear browsing data" dialog along
>> > with the six prefs above (line 2369-2386). So I thought it makes sense
>> > to put them together. Similarly kPepperFlashSettingsEnabled is used
>> > together with kClearPluginLSODataEnabled, so I put it in this file as
>> > well.
>> >
>> > Do you think that is okay? Thanks!
>> >
>> > https://chromiumcodereview.appspot.com/10387161/
>
>
>> Prefs should be registered close to the classes that actually use
>> them. I don't think thats browser.
>
>
>>   -Scott
>
>
> [I am happy to make the change. Just try to understand your point better.]
> It seems things like kDeleteBrowsingHistory or kClearPluginLSODataEnabled
> are
> not used in browser either. Is this something that we would like to change?

Definitely. For a certain set of things Browser has become a dumpy
ground and clearly needs some refactoring.

  -Scott

[yzshen1, 2012-05-21 22:14:41]

Thanks for your explanation, Scott!
Please take another look.

https://chromiumcodereview.appspot.com/10387161/diff/7027/chrome/browser/ui/b...
File chrome/browser/ui/browser.cc (right):

https://chromiumcodereview.appspot.com/10387161/diff/7027/chrome/browser/ui/b...
chrome/browser/ui/browser.cc:2387:
prefs->RegisterBooleanPref(prefs::kDeauthorizeContentLicenses,
On 2012/05/21 20:53:20, yzshen1 wrote:
> On 2012/05/21 20:47:50, sky wrote:
> > Why is this in browser and not some where closer to pepper/flash?
> 
> This option item is displayed in the "Clear browsing data" dialog along with
the
> six prefs above (line 2369-2386). So I thought it makes sense to put them
> together. Similarly kPepperFlashSettingsEnabled is used together with
> kClearPluginLSODataEnabled, so I put it in this file as well.
> 
> Do you think that is okay? Thanks!

Done.

[sky, 2012-05-21 22:39:17]

LGTM

https://chromiumcodereview.appspot.com/10387161/diff/12003/chrome/browser/pep...
File chrome/browser/pepper_flash_settings_manager.h (right):

https://chromiumcodereview.appspot.com/10387161/diff/12003/chrome/browser/pep...
chrome/browser/pepper_flash_settings_manager.h:26: };
private:
DISALLOW_IMPLICIT_CONSTRUCTORS

[yzshen1, 2012-05-22 19:39:25]

Hi, Brett.

Would you please take another look?

https://chromiumcodereview.appspot.com/10387161/diff/12003/chrome/browser/pep...
File chrome/browser/pepper_flash_settings_manager.h (right):

https://chromiumcodereview.appspot.com/10387161/diff/12003/chrome/browser/pep...
chrome/browser/pepper_flash_settings_manager.h:26: };
On 2012/05/21 22:39:17, sky wrote:
> private:
> DISALLOW_IMPLICIT_CONSTRUCTORS

Done.

[brettw, 2012-05-25 21:54:45]

lgtm

[commit-bot: I haz the power, 2012-05-25 21:57:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/10387161/2027

[commit-bot: I haz the power, 2012-05-25 23:37:06]

Change committed as 139153