Pepper Flash settings integration - camera and microphone.

Pepper Flash settings integration - camera and microphone.

BUG=112190
TEST=None

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

[yzshen1, 2012-06-04 19:43:12]

Hi, Brett and Chris.

Would you please take a look? Thanks!

Chris: chrome/browser/resources/ and chrome/browser/browser/ui/.

Brett: Everything else. Although this part of change is fairly big, it is very
mechanical. All it does is sending three kinds of requests
(GetPermissionSettings/SetDefaultPermission/SetSitePermission) to the broker
side and get the results.

[brettw, 2012-06-04 21:53:31]

lgtm

http://codereview.chromium.org/10479015/diff/4002/chrome/browser/pepper_flash...
File chrome/browser/pepper_flash_settings_manager.cc (right):

http://codereview.chromium.org/10479015/diff/4002/chrome/browser/pepper_flash...
chrome/browser/pepper_flash_settings_manager.cc:378: LOG(ERROR) << "Couldn't
send GetPermissionSettings message";
I think you can change these to DLOG (I'd probably change the existing ones as
well). I don't think pipe errors are the types of things we need to be logging
in release builds.

[yzshen1, 2012-06-04 22:05:00]

http://codereview.chromium.org/10479015/diff/4002/chrome/browser/pepper_flash...
File chrome/browser/pepper_flash_settings_manager.cc (right):

http://codereview.chromium.org/10479015/diff/4002/chrome/browser/pepper_flash...
chrome/browser/pepper_flash_settings_manager.cc:378: LOG(ERROR) << "Couldn't
send GetPermissionSettings message";
Done. Thanks!
On 2012/06/04 21:53:31, brettw wrote:
> I think you can change these to DLOG (I'd probably change the existing ones as
> well). I don't think pipe errors are the types of things we need to be logging
> in release builds.

[csilv, 2012-06-05 00:40:20]

lgtm

http://codereview.chromium.org/10479015/diff/1022/chrome/browser/resources/op...
File chrome/browser/resources/options2/content_settings.html (right):

http://codereview.chromium.org/10479015/diff/1022/chrome/browser/resources/op...
chrome/browser/resources/options2/content_settings.html:260: <h3
i18n-content="pepper_flash_cameramic_tab_label"></h3>
nit: Per style guide, use javaScriptCase for i18n-content.

http://codereview.chromium.org/10479015/diff/1022/chrome/browser/resources/op...
chrome/browser/resources/options2/content_settings.html:264: <input type="radio"
name="pepper_flash_cameramic" value="ask">
nit: probably better to use dash instead of underscore for the name attributes,
more consistent with other attributes (id, class).  Not required.

http://codereview.chromium.org/10479015/diff/1022/chrome/browser/resources/op...
File chrome/browser/resources/options2/content_settings.js (right):

http://codereview.chromium.org/10479015/diff/1022/chrome/browser/resources/op...
chrome/browser/resources/options2/content_settings.js:99:
$('pepper-flash-cameramic-exceptions-div').style.display = 'none';
$('pepper-flash-cameramic-section').hidden = true;
$('pepper-flash-cameramic-exceptions-div').hidden = true;

http://codereview.chromium.org/10479015/diff/1022/chrome/browser/resources/op...
chrome/browser/resources/options2/content_settings.js:235:
$('pepper-flash-cameramic-exceptions-div').style.display = '';
$('pepper-flash-cameramic-section').hidden = false;
$('pepper-flash-cameramic-exceptions-div').hidden = false;

http://codereview.chromium.org/10479015/diff/1022/chrome/browser/resources/op...
File chrome/browser/resources/options2/content_settings_exceptions_area.html
(right):

http://codereview.chromium.org/10479015/diff/1022/chrome/browser/resources/op...
chrome/browser/resources/options2/content_settings_exceptions_area.html:80:
contentType="pepper_flash_cameramic" class="pepper-flash-settings">
nit: preferable to use dashes instead of underscore for contentType value.

http://codereview.chromium.org/10479015/diff/1022/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options2/content_settings_handler2.cc (right):

http://codereview.chromium.org/10479015/diff/1022/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/content_settings_handler2.cc:401: // Pepper
Flash camera and microphone fileter.
nit: "filter"

[yzshen1, 2012-06-05 17:28:43]

Thanks, Chris!

WRT the naming issue, please see my comments. I would be happy to make the
change once I understand what is the best way to do it.

http://codereview.chromium.org/10479015/diff/1022/chrome/browser/resources/op...
File chrome/browser/resources/options2/content_settings.html (right):

http://codereview.chromium.org/10479015/diff/1022/chrome/browser/resources/op...
chrome/browser/resources/options2/content_settings.html:260: <h3
i18n-content="pepper_flash_cameramic_tab_label"></h3>
[ I actually noticed the name inconsistency. But after some consideration, I
chose the current approach. Here are the reasons. :( ]

It seems there are far more names_like_this for i18n-content in this file
Is that something that we would like to change in the future?

Besides, some js code actually expects name_like_this: showList in
content_settings_exceptions_area.js appends '_header' to a type name and make it
the text content.

So unless we change the js code, we will have to name at least the header as
<something>_header. In order to make it consistent, I decided to name the
content type as pepper_flash_cameramic (so that the header will be named
pepper_flash_cameramic_header).

This also explain why I named the input on line 264 using underscore. Because it
is the content type.
 
On 2012/06/05 00:40:21, csilv wrote:
> nit: Per style guide, use javaScriptCase for i18n-content.

http://codereview.chromium.org/10479015/diff/1022/chrome/browser/resources/op...
chrome/browser/resources/options2/content_settings.html:264: <input type="radio"
name="pepper_flash_cameramic" value="ask">
Please see my comment above.

On 2012/06/05 00:40:21, csilv wrote:
> nit: probably better to use dash instead of underscore for the name
attributes,
> more consistent with other attributes (id, class).  Not required.

http://codereview.chromium.org/10479015/diff/1022/chrome/browser/resources/op...
File chrome/browser/resources/options2/content_settings.js (right):

http://codereview.chromium.org/10479015/diff/1022/chrome/browser/resources/op...
chrome/browser/resources/options2/content_settings.js:99:
$('pepper-flash-cameramic-exceptions-div').style.display = 'none';
I tried hidden before.
It worked with the section, but not the exceptions-dev.
But I didn't looked into why it didn't work.

Do you think I should make hidden work for the exceptions-div as well? (Would be
happy to do it if you think so.)
On 2012/06/05 00:40:21, csilv wrote:
> $('pepper-flash-cameramic-section').hidden = true;
> $('pepper-flash-cameramic-exceptions-div').hidden = true;

http://codereview.chromium.org/10479015/diff/1022/chrome/browser/resources/op...
chrome/browser/resources/options2/content_settings.js:235:
$('pepper-flash-cameramic-exceptions-div').style.display = '';
Please see my comment above.

On 2012/06/05 00:40:21, csilv wrote:
> $('pepper-flash-cameramic-section').hidden = false;
> $('pepper-flash-cameramic-exceptions-div').hidden = false;

http://codereview.chromium.org/10479015/diff/1022/chrome/browser/resources/op...
File chrome/browser/resources/options2/content_settings_exceptions_area.html
(right):

http://codereview.chromium.org/10479015/diff/1022/chrome/browser/resources/op...
chrome/browser/resources/options2/content_settings_exceptions_area.html:80:
contentType="pepper_flash_cameramic" class="pepper-flash-settings">
Please see my comments in content_settings.html.

On 2012/06/05 00:40:21, csilv wrote:
> nit: preferable to use dashes instead of underscore for contentType value.

http://codereview.chromium.org/10479015/diff/1022/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options2/content_settings_handler2.cc (right):

http://codereview.chromium.org/10479015/diff/1022/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/content_settings_handler2.cc:401: // Pepper
Flash camera and microphone fileter.
On 2012/06/05 00:40:21, csilv wrote:
> nit: "filter"

Done.

[csilv, 2012-06-05 18:09:27]

http://codereview.chromium.org/10479015/diff/1022/chrome/browser/resources/op...
File chrome/browser/resources/options2/content_settings.html (right):

http://codereview.chromium.org/10479015/diff/1022/chrome/browser/resources/op...
chrome/browser/resources/options2/content_settings.html:260: <h3
i18n-content="pepper_flash_cameramic_tab_label"></h3>
On 2012/06/05 17:28:43, yzshen1 wrote:
> [ I actually noticed the name inconsistency. But after some consideration, I
> chose the current approach. Here are the reasons. :( ]

Your reasons are valid, but we shouldn't make the problem worse by adding to the
inconsistency.  While this file has underscore naming, most web-ui files do not.
 (For example, see BrowserOptionsHandler::GetLocalizedValues() in
browser_options_handler2.cc).  You don't need to change existing lines, but
let's follow the style guide for new lines (and hopefully someone will fix the
incorrect ones later.
(http://www.chromium.org/developers/web-development-style-guide) And yes, the
code that appends "_header" should ideally be fixed (but not for this CL).

http://codereview.chromium.org/10479015/diff/1022/chrome/browser/resources/op...
File chrome/browser/resources/options2/content_settings.js (right):

http://codereview.chromium.org/10479015/diff/1022/chrome/browser/resources/op...
chrome/browser/resources/options2/content_settings.js:99:
$('pepper-flash-cameramic-exceptions-div').style.display = 'none';
On 2012/06/05 17:28:43, yzshen1 wrote:
> I tried hidden before.
> It worked with the section, but not the exceptions-dev.
> But I didn't looked into why it didn't work.

That's okay, I'm fine with not using hidden for the reason you stated.  It would
be nice to figure out why it's not working, but not necessary for this CL.

[yzshen, 2012-06-05 18:22:13]

Thanks, Chris!

I will change the names and update the CL soon.
(Fixing broken tests now.)

On Tue, Jun 5, 2012 at 11:09 AM, <csilv@chromium.org> wrote:

>
> http://codereview.chromium.**org/10479015/diff/1022/chrome/**
>
browser/resources/options2/**content_settings.html<http://codereview.chromium.org/10479015/diff/1022/chrome/browser/resources/options2/content_settings.html>
> File chrome/browser/resources/**options2/content_settings.html (right):
>
> http://codereview.chromium.**org/10479015/diff/1022/chrome/**
>
browser/resources/options2/**content_settings.html#**newcode260<http://codereview.chromium.org/10479015/diff/1022/chrome/browser/resources/options2/content_settings.html#newcode260>
> chrome/browser/resources/**options2/content_settings.**html:260: <h3
> i18n-content="pepper_flash_**cameramic_tab_label"></h3>
> On 2012/06/05 17:28:43, yzshen1 wrote:
>
>> [ I actually noticed the name inconsistency. But after some
>>
> consideration, I
>
>> chose the current approach. Here are the reasons. :( ]
>>
>
> Your reasons are valid, but we shouldn't make the problem worse by
> adding to the inconsistency.  While this file has underscore naming,
> most web-ui files do not.  (For example, see
> BrowserOptionsHandler::**GetLocalizedValues() in
> browser_options_handler2.cc).  You don't need to change existing lines,
> but let's follow the style guide for new lines (and hopefully someone
> will fix the incorrect ones later.
>
(http://www.chromium.org/**developers/web-development-**style-guide<http://www...)
> And
> yes, the code that appends "_header" should ideally be fixed (but not
> for this CL).
>
>
> http://codereview.chromium.**org/10479015/diff/1022/chrome/**
>
browser/resources/options2/**content_settings.js<http://codereview.chromium.org/10479015/diff/1022/chrome/browser/resources/options2/content_settings.js>
> File chrome/browser/resources/**options2/content_settings.js (right):
>
> http://codereview.chromium.**org/10479015/diff/1022/chrome/**
>
browser/resources/options2/**content_settings.js#newcode99<http://codereview.chromium.org/10479015/diff/1022/chrome/browser/resources/options2/content_settings.js#newcode99>
> chrome/browser/resources/**options2/content_settings.js:**99:
> $('pepper-flash-cameramic-**exceptions-div').style.display = 'none';
> On 2012/06/05 17:28:43, yzshen1 wrote:
>
>> I tried hidden before.
>> It worked with the section, but not the exceptions-dev.
>> But I didn't looked into why it didn't work.
>>
>
> That's okay, I'm fine with not using hidden for the reason you stated.
> It would be nice to figure out why it's not working, but not necessary
> for this CL.
>
>
http://codereview.chromium.**org/10479015/<http://codereview.chromium.org/104...
>



-- 
Best regards,
Yuzhu Shen.

[yzshen1, 2012-06-05 19:57:57]

http://codereview.chromium.org/10479015/diff/1022/chrome/browser/resources/op...
File chrome/browser/resources/options2/content_settings.html (right):

http://codereview.chromium.org/10479015/diff/1022/chrome/browser/resources/op...
chrome/browser/resources/options2/content_settings.html:260: <h3
i18n-content="pepper_flash_cameramic_tab_label"></h3>
On 2012/06/05 18:09:27, csilv wrote:
> On 2012/06/05 17:28:43, yzshen1 wrote:
> > [ I actually noticed the name inconsistency. But after some consideration, I
> > chose the current approach. Here are the reasons. :( ]
> 
> Your reasons are valid, but we shouldn't make the problem worse by adding to
the
> inconsistency.  While this file has underscore naming, most web-ui files do
not.
>  (For example, see BrowserOptionsHandler::GetLocalizedValues() in
> browser_options_handler2.cc).  You don't need to change existing lines, but
> let's follow the style guide for new lines (and hopefully someone will fix the
> incorrect ones later.
> (http://www.chromium.org/developers/web-development-style-guide) And yes, the
> code that appends "_header" should ideally be fixed (but not for this CL).

Done.

[csilv, 2012-06-05 20:02:42]

SLGTM

Thank you for the extra effort :-)