add "always allow" option to the mediastream infobar and allow user to allow/not allow acces to devi

Feature1:
User goes to http://neave.com/webcam/html5/
The url displayed is the security origin, just like maps.google.com
User needs to allow or deny access to his camera and microphone. Some sites will require only the mic or only the webcam.
User can open an options drop down. The drop down needs to be clicked for every modification. A divide line separates microphones and webcams.
User can also decide to Always allow. The decision will also remember the devices chosen.
Details:
The "Always allow" option will remember both the decision and the devices chosen.
If "Always allow" is selected and Deny is pressed, access is denied. Upon the next visit, the infobar will be displayed again.

If a a website is Allowed and "Always allowed":
...And devices disappears from the computer. It will show the infobar and ask the user to choose devices.

Feature2:
Settings => Privacy => Content Settings

We want to add a new section called Media to cover the getUserMedia decisions.
And we are able to see and remove the exception lists there.

BUG=132075, 122764
TEST=go to https://apprtc.appspot.com; click "always allow this site to use this device" option; go to content setting/privacy/media, try ask/block settings and remove exceptions.

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

[no longer working on chromium, 2012-06-11 15:51:43]

Hi all,

Would you please help reviewing this CL? This is a key feature webrtc wants to
deliver to the users for M21, so it might be great if you can take a look at it
ASAP. And please just let me know if you can't make it, I will try to find
another review to cover those parts.

Please feel free to review everything. And here is some breakdowns in terms of
"Owners approval"
Tommi : chrome/browser/media
jochen : chrome/browser/content_settings
joaodasilva : chrome/browser/policy
bauerb : chrome/browser/prefs
estade : chrome/browser/resource
sky : chrome/browser/ui

Thanks a lot.

Review

[Bernhard Bauer, 2012-06-11 18:16:00]

It feels a bit weird that we store origins that are allowed to access media in
content settings, but a map from origin to media devices in preferences. Content
settings do support arbitrary values (not only allow/block/ask), so have you
thought about using just content settings? You could store a dictionary like
this as the content setting

 {
   "audio": "device name",
   "video": "device name"
 }

Then you would show the infobar if there is no setting or the device doesn't
exist anymore.

http://codereview.chromium.org/10537099/diff/1/chrome/browser/content_setting...
File chrome/browser/content_settings/content_settings_policy_provider.cc
(right):

http://codereview.chromium.org/10537099/diff/1/chrome/browser/content_setting...
chrome/browser/content_settings/content_settings_policy_provider.cc:35:
prefs::kManagedDefaultMediaStreamSetting,
This array is indexed by content setting, so you need to add this to the end.

http://codereview.chromium.org/10537099/diff/1/chrome/browser/media/media_str...
File chrome/browser/media/media_stream_devices_prefs.cc (right):

http://codereview.chromium.org/10537099/diff/1/chrome/browser/media/media_str...
chrome/browser/media/media_stream_devices_prefs.cc:23: if
(!user_prefs->FindPreference(prefs::kMediaStreamAudioDevicesWhitelist))
You shouldn't need to check whether the preference is already registered here.
If RegisterUserPrefs is called more than once on the same PrefService, you're
doing something wrong.

http://codereview.chromium.org/10537099/diff/1/chrome/browser/media/media_str...
chrome/browser/media/media_stream_devices_prefs.cc:87:
CONTENT_SETTINGS_TYPE_MEDIASTREAM, NULL) == CONTENT_SETTING_BLOCK);
This line is a bit awkwardly broken. You could extract the setting to a local
variable and compare that. Same below.

http://codereview.chromium.org/10537099/diff/1/chrome/browser/media/media_str...
chrome/browser/media/media_stream_devices_prefs.cc:115: if
(IsDevicePairedWithOrigin(pref_id, origin, dev->name))
You could simplify this by first getting the device name paired with |origin|
from preferences, then iterating over the device list to see if the paired
device shows up in there.

http://codereview.chromium.org/10537099/diff/1/chrome/browser/media/media_str...
chrome/browser/media/media_stream_devices_prefs.cc:127: if (!dict ||
dict->empty())
PrefService::GetDictionary will never return NULL if the preference is registed.

Also, the empty dictionary case should be covered by
GetStringWithoutPathExpansion below returning false.

http://codereview.chromium.org/10537099/diff/1/chrome/browser/media/media_str...
chrome/browser/media/media_stream_devices_prefs.cc:144: if (!whitelist) {
This will also never be NULL if the preference is registered.

http://codereview.chromium.org/10537099/diff/1/chrome/browser/media/media_str...
File chrome/browser/media/media_stream_devices_prefs.h (right):

http://codereview.chromium.org/10537099/diff/1/chrome/browser/media/media_str...
chrome/browser/media/media_stream_devices_prefs.h:72: // The profile which owns
this object.
That's not true. The infobar delegate owns this object.

http://codereview.chromium.org/10537099/diff/1/chrome/browser/resources/optio...
File chrome/browser/resources/options2/content_settings_exceptions_area.html
(right):

http://codereview.chromium.org/10537099/diff/1/chrome/browser/resources/optio...
chrome/browser/resources/options2/content_settings_exceptions_area.html:63: <div
contentType="fullscreen">	 66
The "66" doesn't seem like it belongs here?

http://codereview.chromium.org/10537099/diff/1/chrome/browser/resources/optio...
chrome/browser/resources/options2/content_settings_exceptions_area.html:79: <div
contentType="mediastream">
Nit: tab

http://codereview.chromium.org/10537099/diff/1/chrome/browser/ui/media_stream...
File chrome/browser/ui/media_stream_infobar_delegate.cc (right):

http://codereview.chromium.org/10537099/diff/1/chrome/browser/ui/media_stream...
chrome/browser/ui/media_stream_infobar_delegate.cc:82: // Users has blocked the
access to the media device in content settings,
Nit: "The user has blocked […]"

http://codereview.chromium.org/10537099/diff/1/chrome/browser/ui/webui/option...
File chrome/browser/ui/webui/options2/content_settings_handler2.cc (right):

http://codereview.chromium.org/10537099/diff/1/chrome/browser/ui/webui/option...
chrome/browser/ui/webui/options2/content_settings_handler2.cc:560: case
CONTENT_SETTINGS_TYPE_MEDIASTREAM:
You're removing a |break| here.

Also, doesn't this mean that we don't show exceptions for media stream access in
incognito mode?

[no longer working on chromium, 2012-06-11 19:13:30]

Hi Andrew,

We are trying to push this new UI feature out for M21, which has its cut in
a week.
And unfortunately Tommi is on vocation for the whole week, so I wonder if
you have
time to help reviewing this CL?

I would fully understand if you don't have time, please just let me know.

Thanks,

BR，
SX

On Mon, Jun 11, 2012 at 5:51 PM, <xians@chromium.org> wrote:

> Reviewers: tommi, jochen, Joao da Silva, Bernhard Bauer, sky, Evan Stade,
>
> Message:
>
> Hi all,
>
> Would you please help reviewing this CL? This is a key feature webrtc
> wants to
> deliver to the users for M21, so it might be great if you can take a look
> at it
> ASAP. And please just let me know if you can't make it, I will try to find
> another review to cover those parts.
>
> Please feel free to review everything. And here is some breakdowns in
> terms of
> "Owners approval"
> Tommi : chrome/browser/media
> jochen : chrome/browser/content_**settings
> joaodasilva : chrome/browser/policy
> bauerb : chrome/browser/prefs
> estade : chrome/browser/resource
> sky : chrome/browser/ui
>
> Thanks a lot.
>
> Review
>
> Description:
> Feature1:
> User goes to http://neave.com/webcam/html5/
> The url displayed is the security origin, just like maps.google.com
> User needs to allow or deny access to his camera and microphone. Some
> sites will
> require only the mic or only the webcam.
> User can open an options drop down. The drop down needs to be clicked for
> every
> modification. A divide line separates microphones and webcams.
> User can also decide to Always allow. The decision will also remember the
> devices chosen.
> Details:
> The "Always allow" option will remember both the decision and the devices
> chosen.
> If "Always allow" is selected and Deny is pressed, access is denied. Upon
> the
> next visit, the infobar will be displayed again.
>
> If a a website is Allowed and "Always allowed":
> ...And devices disappears from the computer. It will show the infobar and
> ask
> the user to choose devices.
>
> Feature2:
> Settings => Privacy => Content Settings
>
> We want to add a new section called Media to cover the getUserMedia
> decisions.
> And we are able to see and remove the exception lists there.
>
> BUG=132075
> TEST=
>
> Please review this at
https://chromiumcodereview.**appspot.com/10537099/<https://chromiumcodereview...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>  M chrome/app/chrome_command_ids.**h
>  M chrome/app/generated_**resources.grd
>  M chrome/app/policy/policy_**templates.json
>  M chrome/browser/automation/**testing_automation_provider.cc
>  M chrome/browser/chrome_content_**browser_client.cc
>  M chrome/browser/content_**settings/content_settings_**
> default_provider.cc
>  M chrome/browser/content_**settings/content_settings_**policy_provider.cc
>  M chrome/browser/content_**settings/content_settings_**utils.cc
>  M chrome/browser/content_**settings/host_content_**settings_map.cc
>  M chrome/browser/media/media_**stream_devices_menu_model.h
>  M chrome/browser/media/media_**stream_devices_menu_model.cc
>  A chrome/browser/media/media_**stream_devices_prefs.h
>  A chrome/browser/media/media_**stream_devices_prefs.cc
>  M chrome/browser/policy/**configuration_policy_handler_**list.cc
>  M chrome/browser/prefs/browser_**prefs.cc
>  M chrome/browser/prefs/pref_**model_associator.cc
>  M chrome/browser/resources/**options2/content_settings.html
>  M chrome/browser/resources/**options2/content_settings_**
> exceptions_area.html
>  M chrome/browser/resources/**options2/content_settings_**
> exceptions_area.js
>  M
chrome/browser/ui/cocoa/**infobars/media_stream_infobar_**controller.mm<http://media_stream_infobar_controller.mm>
>  M chrome/browser/ui/gtk/**infobars/media_stream_infobar_**gtk.cc
>  M chrome/browser/ui/media_**stream_infobar_delegate.h
>  M chrome/browser/ui/media_**stream_infobar_delegate.cc
>  M chrome/browser/ui/views/**infobars/media_stream_infobar.**cc
>  M chrome/browser/ui/webui/**options2/content_settings_**handler2.cc
>  M chrome/chrome_browser.gypi
>  M chrome/common/content_**settings_types.h
>  M chrome/common/pref_names.h
>  M chrome/common/pref_names.cc
>  M chrome/test/functional/policy_**test_cases.py
>  M chrome/tools/chromeactions.txt
>
>
>

[tommi (sloooow) - chröme, 2012-06-11 20:59:21]

good job xians - I briefly looked of the cl and agree with the comments so far
and added a few little things.  Since I'm on vacation please don't wait for my
review once you've got the goahead from all the other reviewers.  I might not be
immediately available.

https://chromiumcodereview.appspot.com/10537099/diff/1/chrome/app/policy/poli...
File chrome/app/policy/policy_templates.json (right):

https://chromiumcodereview.appspot.com/10537099/diff/1/chrome/app/policy/poli...
chrome/app/policy/policy_templates.json:1560: 'name': 'AskMediaStream',
PromptOnAccess?

https://chromiumcodereview.appspot.com/10537099/diff/1/chrome/app/policy/poli...
chrome/app/policy/policy_templates.json:1565: 'name': 'BlockMediaStream',
BlockAccess?

https://chromiumcodereview.appspot.com/10537099/diff/1/chrome/browser/media/m...
File chrome/browser/media/media_stream_devices_menu_model.h (right):

https://chromiumcodereview.appspot.com/10537099/diff/1/chrome/browser/media/m...
chrome/browser/media/media_stream_devices_menu_model.h:32: bool
IsAlwaysAllowChecked() { return always_allow_; }
bool always_allow() const?

https://chromiumcodereview.appspot.com/10537099/diff/1/chrome/browser/media/m...
chrome/browser/media/media_stream_devices_menu_model.h:54:
MediaStreamInfoBarDelegate* media_stream_delegate_;
document ownership

https://chromiumcodereview.appspot.com/10537099/diff/1/chrome/browser/media/m...
File chrome/browser/media/media_stream_devices_prefs.cc (right):

https://chromiumcodereview.appspot.com/10537099/diff/1/chrome/browser/media/m...
chrome/browser/media/media_stream_devices_prefs.cc:26: if
(!user_prefs->FindPreference(prefs::kMediaStreamVideoDevicesWhitelist))
{}

https://chromiumcodereview.appspot.com/10537099/diff/1/chrome/browser/media/m...
File chrome/browser/media/media_stream_devices_prefs.h (right):

https://chromiumcodereview.appspot.com/10537099/diff/1/chrome/browser/media/m...
chrome/browser/media/media_stream_devices_prefs.h:51: const
content::MediaStreamDevices& devices);
fix indent

https://chromiumcodereview.appspot.com/10537099/diff/1/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options2/content_settings_handler2.cc (right):

https://chromiumcodereview.appspot.com/10537099/diff/1/chrome/browser/ui/webu...
chrome/browser/ui/webui/options2/content_settings_handler2.cc:870:
UserMetricsAction("Options_DefaultMediaStreamSettingChanged"));
add a break here for consistency with all the other cases.

[Evan Stade, 2012-06-12 01:51:12]

this is kind of an "innovative" use of a <select> dropdown. Correct me if I'm
wrong, but the way the UI seems to work is that there's a dropdown which says
"Devices" while in the closed state, and in the open state you can select items
from 2 separate lists as well as check or uncheck a box. This just seems really
unintuitive to users who expect things that look like <select> to behave like
<select> (i.e. you can only choose one thing, and the closed-state text changes
to that one thing).

http://codereview.chromium.org/10537099/diff/1/chrome/browser/ui/media_stream...
File chrome/browser/ui/media_stream_infobar_delegate.cc (right):

http://codereview.chromium.org/10537099/diff/1/chrome/browser/ui/media_stream...
chrome/browser/ui/media_stream_infobar_delegate.cc:79: bool
MediaStreamInfoBarDelegate::ShouldShowInfoBar() {
it's unexpected that a function called ShouldShowInfoBar() would have the
audacity to Accept or Deny. I would expect it to simply check some state and
return true or false, not to take any action. (basically, it needs a better
name)

http://codereview.chromium.org/10537099/diff/1/chrome/browser/ui/media_stream...
chrome/browser/ui/media_stream_infobar_delegate.cc:145: bool
MediaStreamInfoBarDelegate::GetAlwaysAllowedDevices(
with the addition of these new methods, this class is looking less like an
infobar delegate and more like a controller...

[jochen (gone - plz use gerrit), 2012-06-12 15:01:29]

I defer my part to Bernhard

[no longer working on chromium, 2012-06-14 13:03:25]

Sorry for the delay on uploading a new version, we just had a day-trip summer
party yesterday.

For this version, I remove all the references and use only the content settings.
Added a new MediaStreamDevicesController and moved most of the code from the
delegate to the new controller.
I think I have also addressed all the comments.

Please take a second look.

FYI, I will rebase the code soon.

Thanks,
SX

http://codereview.chromium.org/10537099/diff/1/chrome/app/policy/policy_templ...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/10537099/diff/1/chrome/app/policy/policy_templ...
chrome/app/policy/policy_templates.json:1560: 'name': 'AskMediaStream',
On 2012/06/11 20:59:21, tommi wrote:
> PromptOnAccess?

Done.

http://codereview.chromium.org/10537099/diff/1/chrome/app/policy/policy_templ...
chrome/app/policy/policy_templates.json:1565: 'name': 'BlockMediaStream',
On 2012/06/11 20:59:21, tommi wrote:
> BlockAccess?

Done.

http://codereview.chromium.org/10537099/diff/1/chrome/browser/content_setting...
File chrome/browser/content_settings/content_settings_policy_provider.cc
(right):

http://codereview.chromium.org/10537099/diff/1/chrome/browser/content_setting...
chrome/browser/content_settings/content_settings_policy_provider.cc:35:
prefs::kManagedDefaultMediaStreamSetting,
On 2012/06/11 18:16:00, Bernhard Bauer wrote:
> This array is indexed by content setting, so you need to add this to the end.

Done.

http://codereview.chromium.org/10537099/diff/1/chrome/browser/media/media_str...
File chrome/browser/media/media_stream_devices_menu_model.h (right):

http://codereview.chromium.org/10537099/diff/1/chrome/browser/media/media_str...
chrome/browser/media/media_stream_devices_menu_model.h:32: bool
IsAlwaysAllowChecked() { return always_allow_; }
On 2012/06/11 20:59:21, tommi wrote:
> bool always_allow() const?

Done.

http://codereview.chromium.org/10537099/diff/1/chrome/browser/media/media_str...
chrome/browser/media/media_stream_devices_menu_model.h:54:
MediaStreamInfoBarDelegate* media_stream_delegate_;
On 2012/06/11 20:59:21, tommi wrote:
> document ownership

Uncleaned debugging code. Remove now.

http://codereview.chromium.org/10537099/diff/1/chrome/browser/media/media_str...
File chrome/browser/media/media_stream_devices_prefs.h (right):

http://codereview.chromium.org/10537099/diff/1/chrome/browser/media/media_str...
chrome/browser/media/media_stream_devices_prefs.h:51: const
content::MediaStreamDevices& devices);
On 2012/06/11 20:59:21, tommi wrote:
> fix indent

Done.

http://codereview.chromium.org/10537099/diff/1/chrome/browser/media/media_str...
chrome/browser/media/media_stream_devices_prefs.h:72: // The profile which owns
this object.
On 2012/06/11 18:16:00, Bernhard Bauer wrote:
> That's not true. The infobar delegate owns this object.

Remove this invalid comment.

http://codereview.chromium.org/10537099/diff/1/chrome/browser/resources/optio...
File chrome/browser/resources/options2/content_settings_exceptions_area.html
(right):

http://codereview.chromium.org/10537099/diff/1/chrome/browser/resources/optio...
chrome/browser/resources/options2/content_settings_exceptions_area.html:63: <div
contentType="fullscreen">	 66
On 2012/06/11 18:16:00, Bernhard Bauer wrote:
> The "66" doesn't seem like it belongs here?

Done.

http://codereview.chromium.org/10537099/diff/1/chrome/browser/resources/optio...
chrome/browser/resources/options2/content_settings_exceptions_area.html:79: <div
contentType="mediastream">
On 2012/06/11 18:16:00, Bernhard Bauer wrote:
> Nit: tab

Done.

http://codereview.chromium.org/10537099/diff/1/chrome/browser/ui/media_stream...
File chrome/browser/ui/media_stream_infobar_delegate.cc (right):

http://codereview.chromium.org/10537099/diff/1/chrome/browser/ui/media_stream...
chrome/browser/ui/media_stream_infobar_delegate.cc:79: bool
MediaStreamInfoBarDelegate::ShouldShowInfoBar() {
On 2012/06/12 01:51:12, Evan Stade wrote:
> it's unexpected that a function called ShouldShowInfoBar() would have the
> audacity to Accept or Deny. I would expect it to simply check some state and
> return true or false, not to take any action. (basically, it needs a better
> name)

The name has been changed to DismissInfoBarAndTakeActionOnSettings(), hope it is
fine.

http://codereview.chromium.org/10537099/diff/1/chrome/browser/ui/media_stream...
chrome/browser/ui/media_stream_infobar_delegate.cc:82: // Users has blocked the
access to the media device in content settings,
On 2012/06/11 18:16:00, Bernhard Bauer wrote:
> Nit: "The user has blocked […]"

Done.

http://codereview.chromium.org/10537099/diff/1/chrome/browser/ui/media_stream...
chrome/browser/ui/media_stream_infobar_delegate.cc:145: bool
MediaStreamInfoBarDelegate::GetAlwaysAllowedDevices(
On 2012/06/12 01:51:12, Evan Stade wrote:
> with the addition of these new methods, this class is looking less like an
> infobar delegate and more like a controller...

Added a MediaStreamDevicesController, moved most of the code from the delegate
to the new controller.

http://codereview.chromium.org/10537099/diff/1/chrome/browser/ui/webui/option...
File chrome/browser/ui/webui/options2/content_settings_handler2.cc (right):

http://codereview.chromium.org/10537099/diff/1/chrome/browser/ui/webui/option...
chrome/browser/ui/webui/options2/content_settings_handler2.cc:560: case
CONTENT_SETTINGS_TYPE_MEDIASTREAM:
On 2012/06/11 18:16:00, Bernhard Bauer wrote:
> You're removing a |break| here.
> 
> Also, doesn't this mean that we don't show exceptions for media stream access
in
> incognito mode?


Thanks, we should show the exceptions in incognito mode.
Remove and add back the break;

http://codereview.chromium.org/10537099/diff/1/chrome/browser/ui/webui/option...
chrome/browser/ui/webui/options2/content_settings_handler2.cc:870:
UserMetricsAction("Options_DefaultMediaStreamSettingChanged"));
On 2012/06/11 20:59:21, tommi wrote:
> add a break here for consistency with all the other cases.

Done.

[no longer working on chromium, 2012-06-14 15:25:14]

On 2012/06/12 01:51:12, Evan Stade wrote:
> this is kind of an "innovative" use of a <select> dropdown. Correct me if I'm
> wrong, but the way the UI seems to work is that there's a dropdown which says
> "Devices" while in the closed state, and in the open state you can select
items
> from 2 separate lists as well as check or uncheck a box. This just seems
really
> unintuitive to users who expect things that look like <select> to behave like
> <select> (i.e. you can only choose one thing, and the closed-state text
changes
> to that one thing).
> 

Thanks for input, we had quite some discussions on this problem before and are
not surprised by the question like this.
We did it this ways because we may have both audio and video devices, which may
have very long names. And we need users to grand permission to both devices to
initialize a call. So we can't make the <select> to behave like how it supposed
to be for our case.

One idea to improve it is to have a checkbox for each item, how do you think?

We may need another UI review process if we are going to change how it looks, so
hope it is fine to keep it this way for this CL.

BR,
/SX



>
http://codereview.chromium.org/10537099/diff/1/chrome/browser/ui/media_stream...
> File chrome/browser/ui/media_stream_infobar_delegate.cc (right):
> 
>
http://codereview.chromium.org/10537099/diff/1/chrome/browser/ui/media_stream...
> chrome/browser/ui/media_stream_infobar_delegate.cc:79: bool
> MediaStreamInfoBarDelegate::ShouldShowInfoBar() {
> it's unexpected that a function called ShouldShowInfoBar() would have the
> audacity to Accept or Deny. I would expect it to simply check some state and
> return true or false, not to take any action. (basically, it needs a better
> name)
> 
>
http://codereview.chromium.org/10537099/diff/1/chrome/browser/ui/media_stream...
> chrome/browser/ui/media_stream_infobar_delegate.cc:145: bool
> MediaStreamInfoBarDelegate::GetAlwaysAllowedDevices(
> with the addition of these new methods, this class is looking less like an
> infobar delegate and more like a controller...

[Ivan Korotkov, 2012-06-14 17:14:12]

I agree with Evan that "always allows" as a select option is pretty confusing.
Why not make it another button on infobar, i.e. "Allow always", "Allow once",
"Deny"?

http://codereview.chromium.org/10537099/diff/8004/chrome/browser/media/media_...
File chrome/browser/media/media_stream_devices_controller.cc (right):

http://codereview.chromium.org/10537099/diff/8004/chrome/browser/media/media_...
chrome/browser/media/media_stream_devices_controller.cc:54: static const char
kAudioKey[] = "audio";
This can be moved to the anonymous namespace instead of being static.

http://codereview.chromium.org/10537099/diff/8004/chrome/browser/media/media_...
chrome/browser/media/media_stream_devices_controller.cc:184: bool
MediaStreamDevicesController::IsInternalContent() {
Please rename to something like IsAutoAllowed/ShouldAlwaysAllow

http://codereview.chromium.org/10537099/diff/8004/chrome/browser/media/media_...
chrome/browser/media/media_stream_devices_controller.cc:237: DictionaryValue*
value_dict = static_cast<DictionaryValue*>(value.get());
This can be made const

http://codereview.chromium.org/10537099/diff/8004/chrome/browser/media/media_...
File chrome/browser/media/media_stream_devices_controller.h (right):

http://codereview.chromium.org/10537099/diff/8004/chrome/browser/media/media_...
chrome/browser/media/media_stream_devices_controller.h:86:
content::MediaResponseCallback callback_;
Please rename to something more meaningful, like response_callback_

http://codereview.chromium.org/10537099/diff/8004/chrome/browser/media/media_...
File chrome/browser/media/media_stream_devices_menu_model.cc (right):

http://codereview.chromium.org/10537099/diff/8004/chrome/browser/media/media_...
chrome/browser/media/media_stream_devices_menu_model.cc:60: return
(selected_command_id_audio_ == command_id ||
No need in parens.

http://codereview.chromium.org/10537099/diff/8004/chrome/browser/media/media_...
chrome/browser/media/media_stream_devices_menu_model.cc:107: if (!audio &&
!video)
Can that ever happen?

http://codereview.chromium.org/10537099/diff/8004/chrome/browser/media/media_...
File chrome/browser/media/media_stream_devices_menu_model.h (right):

http://codereview.chromium.org/10537099/diff/8004/chrome/browser/media/media_...
chrome/browser/media/media_stream_devices_menu_model.h:33: bool
IsAlwaysAllowChecked() const { return always_allow_; }
If this method is non-virtual, it should be named always_allow()

http://codereview.chromium.org/10537099/diff/8004/chrome/browser/resources/op...
File chrome/browser/resources/options2/content_settings_exceptions_area.js
(right):

http://codereview.chromium.org/10537099/diff/8004/chrome/browser/resources/op...
chrome/browser/resources/options2/content_settings_exceptions_area.js:445: //
Editing notifications, geolocation and mediastream is disabled for now.
What about fullscreen? I suggest a more generic message like "Editin these
settings..."

http://codereview.chromium.org/10537099/diff/8004/chrome/browser/ui/media_str...
File chrome/browser/ui/media_stream_infobar_delegate.h (right):

http://codereview.chromium.org/10537099/diff/8004/chrome/browser/ui/media_str...
chrome/browser/ui/media_stream_infobar_delegate.h:32: bool has_video() const;
If it's easy to rename them to HasVideo/Audio, please do so.

http://codereview.chromium.org/10537099/diff/8004/chrome/browser/ui/media_str...
chrome/browser/ui/media_stream_infobar_delegate.h:49: void Accept(const
std::string& audio_id, const std::string& video_id,
Should be one line per argument.

[no longer working on chromium, 2012-06-15 16:52:06]

To Ivan and Evan,

The "always allow" is "always allow this site to use this microphone and/or
camera" for short. It is a bit similar to how the translate infobar looks like.

One of the reason why we don't want a "always allow" button on the infobar is
that we would like to put it under options, then the users know which devices
will be used for the future.

To Bernhard,
You may have already known, I just had a discussion with the chrome-privacy@,
the conclusion is that:
# We should block all the access to the media devices if the users choose the
block it, regardless on the "allowed" exceptions users make before.
# About the internal URLs like chrome://url, it is fine to always allow them no
matter what the users set, but my team decided to put the users first, at least
for now.

Thanks,

BR,
/SX

http://codereview.chromium.org/10537099/diff/8004/chrome/browser/media/media_...
File chrome/browser/media/media_stream_devices_controller.cc (right):

http://codereview.chromium.org/10537099/diff/8004/chrome/browser/media/media_...
chrome/browser/media/media_stream_devices_controller.cc:54: static const char
kAudioKey[] = "audio";
On 2012/06/14 17:14:12, Ivan Korotkov wrote:
> This can be moved to the anonymous namespace instead of being static.

Done.

http://codereview.chromium.org/10537099/diff/8004/chrome/browser/media/media_...
chrome/browser/media/media_stream_devices_controller.cc:184: bool
MediaStreamDevicesController::IsInternalContent() {
On 2012/06/14 17:14:12, Ivan Korotkov wrote:
> Please rename to something like IsAutoAllowed/ShouldAlwaysAllow

Done.

http://codereview.chromium.org/10537099/diff/8004/chrome/browser/media/media_...
chrome/browser/media/media_stream_devices_controller.cc:237: DictionaryValue*
value_dict = static_cast<DictionaryValue*>(value.get());
On 2012/06/14 17:14:12, Ivan Korotkov wrote:
> This can be made const

Done.

http://codereview.chromium.org/10537099/diff/8004/chrome/browser/media/media_...
File chrome/browser/media/media_stream_devices_controller.h (right):

http://codereview.chromium.org/10537099/diff/8004/chrome/browser/media/media_...
chrome/browser/media/media_stream_devices_controller.h:86:
content::MediaResponseCallback callback_;
On 2012/06/14 17:14:12, Ivan Korotkov wrote:
> Please rename to something more meaningful, like response_callback_

Since there is only one type of callback used by this class, I would like to
keep it simple and consistent with the naming style like request_, hope it is
fine to leave it this way.

http://codereview.chromium.org/10537099/diff/8004/chrome/browser/media/media_...
File chrome/browser/media/media_stream_devices_menu_model.cc (right):

http://codereview.chromium.org/10537099/diff/8004/chrome/browser/media/media_...
chrome/browser/media/media_stream_devices_menu_model.cc:60: return
(selected_command_id_audio_ == command_id ||
On 2012/06/14 17:14:12, Ivan Korotkov wrote:
> No need in parens.

I tried, but it looks pretty odd since it does not fit in one line. Hope it is
fine to keep it this way.

http://codereview.chromium.org/10537099/diff/8004/chrome/browser/media/media_...
chrome/browser/media/media_stream_devices_menu_model.cc:107: if (!audio &&
!video)
On 2012/06/14 17:14:12, Ivan Korotkov wrote:
> Can that ever happen?

Good question, we just had our lately UI discussion and decided not to show the
infobar when there is no devices. And this should be handled by the controller.

http://codereview.chromium.org/10537099/diff/8004/chrome/browser/media/media_...
File chrome/browser/media/media_stream_devices_menu_model.h (right):

http://codereview.chromium.org/10537099/diff/8004/chrome/browser/media/media_...
chrome/browser/media/media_stream_devices_menu_model.h:33: bool
IsAlwaysAllowChecked() const { return always_allow_; }
On 2012/06/14 17:14:12, Ivan Korotkov wrote:
> If this method is non-virtual, it should be named always_allow()

Done.

http://codereview.chromium.org/10537099/diff/8004/chrome/browser/resources/op...
File chrome/browser/resources/options2/content_settings_exceptions_area.js
(right):

http://codereview.chromium.org/10537099/diff/8004/chrome/browser/resources/op...
chrome/browser/resources/options2/content_settings_exceptions_area.js:445: //
Editing notifications, geolocation and mediastream is disabled for now.
On 2012/06/14 17:14:12, Ivan Korotkov wrote:
> What about fullscreen? I suggest a more generic message like "Editin these
> settings..."

Done.

http://codereview.chromium.org/10537099/diff/8004/chrome/browser/ui/media_str...
File chrome/browser/ui/media_stream_infobar_delegate.h (right):

http://codereview.chromium.org/10537099/diff/8004/chrome/browser/ui/media_str...
chrome/browser/ui/media_stream_infobar_delegate.h:32: bool has_video() const;
On 2012/06/14 17:14:12, Ivan Korotkov wrote:
> If it's easy to rename them to HasVideo/Audio, please do so.

Done.

http://codereview.chromium.org/10537099/diff/8004/chrome/browser/ui/media_str...
chrome/browser/ui/media_stream_infobar_delegate.h:49: void Accept(const
std::string& audio_id, const std::string& video_id,
On 2012/06/14 17:14:12, Ivan Korotkov wrote:
> Should be one line per argument.

Done.

[Evan Stade, 2012-06-15 18:51:25]

http://codereview.chromium.org/10537099/diff/16001/chrome/browser/resources/o...
File chrome/browser/resources/options2/content_settings.html (right):

http://codereview.chromium.org/10537099/diff/16001/chrome/browser/resources/o...
chrome/browser/resources/options2/content_settings.html:301: <h3
i18n-content="mediastream_tab_label"></h3>
these i18n var names should be camel case. Also, please keep it consistent
whether "mediastream" is one word or two (looks like it should be two:
mediaStream)

I realize that much of this file fails to follow this advice, but it should.

http://codereview.chromium.org/10537099/diff/16001/chrome/browser/resources/o...
File chrome/browser/resources/options2/content_settings_exceptions_area.js
(right):

http://codereview.chromium.org/10537099/diff/16001/chrome/browser/resources/o...
chrome/browser/resources/options2/content_settings_exceptions_area.js:117:
this.contentType == 'mediastream') {
why isn't fullscreen in this list?

[no longer working on chromium, 2012-06-15 21:04:47]

http://codereview.chromium.org/10537099/diff/16001/chrome/browser/resources/o...
File chrome/browser/resources/options2/content_settings.html (right):

http://codereview.chromium.org/10537099/diff/16001/chrome/browser/resources/o...
chrome/browser/resources/options2/content_settings.html:301: <h3
i18n-content="mediastream_tab_label"></h3>
On 2012/06/15 18:51:25, Evan Stade wrote:
> these i18n var names should be camel case. Also, please keep it consistent
> whether "mediastream" is one word or two (looks like it should be two:
> mediaStream)
> 
> I realize that much of this file fails to follow this advice, but it should.

I checked the naming style for the existing code, but could not find any one
camelcase, and I use mediastream because of similar case like mouselock. Could
you please confirm it?

In case mediastream is a bad name anyway, I can consider changing it to media.

http://codereview.chromium.org/10537099/diff/16001/chrome/browser/resources/o...
File chrome/browser/resources/options2/content_settings_exceptions_area.js
(right):

http://codereview.chromium.org/10537099/diff/16001/chrome/browser/resources/o...
chrome/browser/resources/options2/content_settings_exceptions_area.js:117:
this.contentType == 'mediastream') {
On 2012/06/15 18:51:25, Evan Stade wrote:
> why isn't fullscreen in this list?

Not sure, shall we add it?

[Evan Stade, 2012-06-15 21:33:32]

http://codereview.chromium.org/10537099/diff/16001/chrome/browser/resources/o...
File chrome/browser/resources/options2/content_settings.html (right):

http://codereview.chromium.org/10537099/diff/16001/chrome/browser/resources/o...
chrome/browser/resources/options2/content_settings.html:301: <h3
i18n-content="mediastream_tab_label"></h3>
On 2012/06/15 21:04:47, xians1 wrote:
> On 2012/06/15 18:51:25, Evan Stade wrote:
> > these i18n var names should be camel case. Also, please keep it consistent
> > whether "mediastream" is one word or two (looks like it should be two:
> > mediaStream)
> > 
> > I realize that much of this file fails to follow this advice, but it should.
> 
> I checked the naming style for the existing code, but could not find any one
> camelcase, and I use mediastream because of similar case like mouselock. Could
> you please confirm it?
> 
> In case mediastream is a bad name anyway, I can consider changing it to media.

examples of i18n-content="camelCase" in this file: L293, L288, L284, L273, etc.

mouselock is bad, it should be mouseLock. It's not uncommon to have
inconsistencies like this (SpellCheck vs. Spellcheck, AutoFill vs Autofill,
etc.), but they are all terribly annoying. I don't really care between
MediaStream/mediaStream/media_stream and Mediastream/mediastream/mediastream,
but it should be consistent.

http://codereview.chromium.org/10537099/diff/16001/chrome/browser/resources/o...
File chrome/browser/resources/options2/content_settings_exceptions_area.js
(right):

http://codereview.chromium.org/10537099/diff/16001/chrome/browser/resources/o...
chrome/browser/resources/options2/content_settings_exceptions_area.js:117:
this.contentType == 'mediastream') {
On 2012/06/15 21:04:47, xians1 wrote:
> On 2012/06/15 18:51:25, Evan Stade wrote:
> > why isn't fullscreen in this list?
> 
> Not sure, shall we add it?

could you find whoever added it to the list down below and ping them?

[no longer working on chromium, 2012-06-18 10:22:49]

https://chromiumcodereview.appspot.com/10537099/diff/16001/chrome/browser/res...
File chrome/browser/resources/options2/content_settings.html (right):

https://chromiumcodereview.appspot.com/10537099/diff/16001/chrome/browser/res...
chrome/browser/resources/options2/content_settings.html:301: <h3
i18n-content="mediastream_tab_label"></h3>
On 2012/06/15 21:33:32, Evan Stade wrote:
> On 2012/06/15 21:04:47, xians1 wrote:
> > On 2012/06/15 18:51:25, Evan Stade wrote:
> > > these i18n var names should be camel case. Also, please keep it consistent
> > > whether "mediastream" is one word or two (looks like it should be two:
> > > mediaStream)
> > > 
> > > I realize that much of this file fails to follow this advice, but it
should.
> > 
> > I checked the naming style for the existing code, but could not find any one
> > camelcase, and I use mediastream because of similar case like mouselock.
Could
> > you please confirm it?
> > 
> > In case mediastream is a bad name anyway, I can consider changing it to
media.
> 
> examples of i18n-content="camelCase" in this file: L293, L288, L28i18n var
names4, L273, etc.
> 
> mouselock is bad, it should be mouseLock. It's not uncommon to have
> inconsistencies like this (SpellCheck vs. Spellcheck, AutoFill vs Autofill,
> etc.), but they are all terribly annoying. I don't really care between
> MediaStream/mediaStream/media_stream and Mediastream/mediastream/mediastream,
> but it should be consistent.


Thanks for pointing out, this file is indeed misled, and hard to find a
consistent style. I changed them to camelcase, please double check.

https://chromiumcodereview.appspot.com/10537099/diff/16001/chrome/browser/res...
File chrome/browser/resources/options2/content_settings_exceptions_area.js
(right):

https://chromiumcodereview.appspot.com/10537099/diff/16001/chrome/browser/res...
chrome/browser/resources/options2/content_settings_exceptions_area.js:117:
this.contentType == 'mediastream') {
On 2012/06/15 21:33:32, Evan Stade wrote:
> On 2012/06/15 21:04:47, xians1 wrote:
> > On 2012/06/15 18:51:25, Evan Stade wrote:
> > > why isn't fullscreen in this list?
> > 
> > Not sure, shall we add it?
> 
> could you find whoever added it to the list down below and ping them?

I will ping the guy.

[no longer working on chromium, 2012-06-18 16:43:19]

After discussing with the privacy team and Bernhard, I changed the behavior to:
If a user grants a permission to always allow this page to access specific A/V
devices, the page is allowed to do so even if the user switches the media
section setting to "Do not any site to access my devices". 
The reason here is to be consistent with other sections, like Location,
Notifications, etc.

Please take another look.

Thanks,

BR,
/SX

[Bernhard Bauer, 2012-06-18 17:38:14]

https://chromiumcodereview.appspot.com/10537099/diff/8004/chrome/browser/cont...
File chrome/browser/content_settings/host_content_settings_map.cc (left):

https://chromiumcodereview.appspot.com/10537099/diff/8004/chrome/browser/cont...
chrome/browser/content_settings/host_content_settings_map.cc:188:
DCHECK(!ContentTypeHasCompoundValue(content_type));
Are you sure about this? This method is supposed to return a ContentSetting, so
it can't be called for a content type where the value can't be coerced to a
content setting. We might need to add a GetDefaultWebsiteSetting method that
doesn't do the coercion.

https://chromiumcodereview.appspot.com/10537099/diff/8004/chrome/browser/cont...
chrome/browser/content_settings/host_content_settings_map.cc:258:
DCHECK(!ContentTypeHasCompoundValue(content_type));
In this case it's easier; we can replace calls to SetDefaultContentSetting()
with SetWebsiteSetting(ContentSettingsPattern::Wildcard(), ...).

https://chromiumcodereview.appspot.com/10537099/diff/8004/chrome/browser/cont...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

https://chromiumcodereview.appspot.com/10537099/diff/8004/chrome/browser/cont...
chrome/browser/content_settings/host_content_settings_map.cc:484: ContentSetting
setting_value = CONTENT_SETTING_DEFAULT;
Can you add a TODO(bauerb) that we should return rules as a list of values, not
content settings?

https://chromiumcodereview.appspot.com/10537099/diff/8004/chrome/browser/medi...
File chrome/browser/media/media_stream_devices_controller.cc (right):

https://chromiumcodereview.appspot.com/10537099/diff/8004/chrome/browser/medi...
chrome/browser/media/media_stream_devices_controller.cc:54: static const char
kAudioKey[] = "audio";
On 2012/06/15 16:52:07, xians1 wrote:
> On 2012/06/14 17:14:12, Ivan Korotkov wrote:
> > This can be moved to the anonymous namespace instead of being static.
> 
> Done.

Sorry I'm late here, but it actually doesn't matter; const variables
automatically get internal linkage :)

https://chromiumcodereview.appspot.com/10537099/diff/8004/chrome/browser/medi...
chrome/browser/media/media_stream_devices_controller.cc:116:
MediaStreamDevicesController::GetAudioDevices() const {
I think we don't indent the method name if we have to break the signature after
the return type.

https://chromiumcodereview.appspot.com/10537099/diff/8004/chrome/browser/medi...
chrome/browser/media/media_stream_devices_controller.cc:174: if (device_it !=
request_->devices.end()) {
You could negative the condition and early-return.

https://chromiumcodereview.appspot.com/10537099/diff/8004/chrome/browser/medi...
chrome/browser/media/media_stream_devices_controller.cc:204: if
(!audio_device.empty()) {
You might want to leave out the braces. Personally, I don't mind (and the style
guide does allow it); other people might.

https://chromiumcodereview.appspot.com/10537099/diff/8004/chrome/browser/medi...
chrome/browser/media/media_stream_devices_controller.cc:234: if (!value.get() ||
!value->IsType(Value::TYPE_DICTIONARY))
Add a NOTREACHED() if we get an unexpected type back?

https://chromiumcodereview.appspot.com/10537099/diff/8004/chrome/browser/medi...
chrome/browser/media/media_stream_devices_controller.cc:238: if
(value_dict->empty())
This isn't reeaaally necessary. If the dictionary is empty, both checks below
will simply fail gracefully.

https://chromiumcodereview.appspot.com/10537099/diff/8004/chrome/browser/medi...
chrome/browser/media/media_stream_devices_controller.cc:274: std::string
MediaStreamDevicesController::GetFirstDevice(
I think this returns a device ID? Could you change the name to GetFirstDeviceId
or something to make that clear?

https://chromiumcodereview.appspot.com/10537099/diff/8004/chrome/browser/medi...
File chrome/browser/media/media_stream_devices_controller.h (right):

https://chromiumcodereview.appspot.com/10537099/diff/8004/chrome/browser/medi...
chrome/browser/media/media_stream_devices_controller.h:79: Profile* profile_;
Could you add a comment that the owner of this class should make sure that it
doesn't outlive the profile?

https://chromiumcodereview.appspot.com/10537099/diff/24004/chrome/browser/con...
File chrome/browser/content_settings/content_settings_pref_provider.cc (right):

https://chromiumcodereview.appspot.com/10537099/diff/24004/chrome/browser/con...
chrome/browser/content_settings/content_settings_pref_provider.cc:364: if
(content_type == CONTENT_SETTINGS_TYPE_MEDIASTREAM) {
Hm. I think we should make the ContentTypeHasCompoundValue() method from
host_content_settings_map.cc publicly accessible and use it here, to make it
easier to add other compound content types.

I'm also not sure whether the PrefProvider needs to verify that the value is a
non-empty dictionary; it seems the client of the HostContentSettingsMap could do
that as well (I try to keep the content_settings/ code as agnostic as possible
of the actual content settings).

Then, if we don't have any special treatment for
CONTENT_SETTINGS_TYPE_MEDIASTREAM anymore, it might be a bit nicer if you pull
out the call to value_map_.SetValue() and just pass in a different Value based
on whether the content type is compound.

https://chromiumcodereview.appspot.com/10537099/diff/24004/chrome/browser/ui/...
File chrome/browser/ui/browser.cc (right):

https://chromiumcodereview.appspot.com/10537099/diff/24004/chrome/browser/ui/...
chrome/browser/ui/browser.cc:3715: for (; i < infobar_helper->infobar_count() &&
!old_infobar; ++i) {
Inline the declaration of |i| into the for-loop?

https://chromiumcodereview.appspot.com/10537099/diff/24004/chrome/browser/ui/...
chrome/browser/ui/browser.cc:3716: old_infobar =
infobar_helper->GetInfoBarDelegateAt(i)->
If you |break| here soon as |old_infobar| is not NULL, the for loop becomes a
bit more idiomatic.

https://chromiumcodereview.appspot.com/10537099/diff/24004/chrome/browser/ui/...
File chrome/browser/ui/webui/options2/content_settings_handler2.cc (right):

https://chromiumcodereview.appspot.com/10537099/diff/24004/chrome/browser/ui/...
chrome/browser/ui/webui/options2/content_settings_handler2.cc:1175: } else {
Would you mind doing a small bit of cleanup here? All these if-clauses could
return afterwards instead of having else's, so we could get rid off all the
indentation.

https://chromiumcodereview.appspot.com/10537099/diff/24004/chrome/browser/ui/...
chrome/browser/ui/webui/options2/content_settings_handler2.cc:1181: //
SetWebsiteSetting instead of SetContentSetting.
I think you *can* use SetWebsiteSetting for simple content types as well, so you
could just always call SetWebsiteSetting here (NULL is converted to
CONTENT_SETTING_DEFAULT).

[Evan Stade, 2012-06-18 19:12:58]

my part lgtm

https://chromiumcodereview.appspot.com/10537099/diff/24004/chrome/browser/res...
File chrome/browser/resources/options2/content_settings_exceptions_area.js
(right):

https://chromiumcodereview.appspot.com/10537099/diff/24004/chrome/browser/res...
chrome/browser/resources/options2/content_settings_exceptions_area.js:16: *     
    in the select.
I believe the indentation was correct before.

[Ivan Korotkov, 2012-06-19 09:22:04]

My comments and OOBE perspective LGTM

[no longer working on chromium, 2012-06-19 12:23:16]

https://chromiumcodereview.appspot.com/10537099/diff/8004/chrome/browser/cont...
File chrome/browser/content_settings/host_content_settings_map.cc (left):

https://chromiumcodereview.appspot.com/10537099/diff/8004/chrome/browser/cont...
chrome/browser/content_settings/host_content_settings_map.cc:188:
DCHECK(!ContentTypeHasCompoundValue(content_type));
On 2012/06/18 17:38:14, Bernhard Bauer wrote:
> Are you sure about this? This method is supposed to return a ContentSetting,
so
> it can't be called for a content type where the value can't be coerced to a
> content setting. We might need to add a GetDefaultWebsiteSetting method that
> doesn't do the coercion.

I assume the default values are only "ask/allow/block" for all the content
types, regardless if they are using compound value or not.
Take MediaStream as example, it uses a dictionary value {audio: name, video:
name}, but for its default value, it only has "ask/block".

I think we should allow a type using compound value and also contentsetting
value for its default setting.

Furthermore, I don't think this DCHECK adds much here since
content_settings::ValueToContentSetting() will hit a DCHECK anyway.

https://chromiumcodereview.appspot.com/10537099/diff/8004/chrome/browser/cont...
chrome/browser/content_settings/host_content_settings_map.cc:258:
DCHECK(!ContentTypeHasCompoundValue(content_type));
On 2012/06/18 17:38:14, Bernhard Bauer wrote:
> In this case it's easier; we can replace calls to SetDefaultContentSetting()
> with SetWebsiteSetting(ContentSettingsPattern::Wildcard(), ...).

See reply above.

https://chromiumcodereview.appspot.com/10537099/diff/8004/chrome/browser/cont...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

https://chromiumcodereview.appspot.com/10537099/diff/8004/chrome/browser/cont...
chrome/browser/content_settings/host_content_settings_map.cc:484: ContentSetting
setting_value = CONTENT_SETTING_DEFAULT;
On 2012/06/18 17:38:14, Bernhard Bauer wrote:
> Can you add a TODO(bauerb) that we should return rules as a list of values,
not
> content settings?

Done.

https://chromiumcodereview.appspot.com/10537099/diff/8004/chrome/browser/medi...
File chrome/browser/media/media_stream_devices_controller.cc (right):

https://chromiumcodereview.appspot.com/10537099/diff/8004/chrome/browser/medi...
chrome/browser/media/media_stream_devices_controller.cc:54: static const char
kAudioKey[] = "audio";
On 2012/06/18 17:38:14, Bernhard Bauer wrote:
> On 2012/06/15 16:52:07, xians1 wrote:
> > On 2012/06/14 17:14:12, Ivan Korotkov wrote:
> > > This can be moved to the anonymous namespace instead of being static.
> > 
> > Done.
> 
> Sorry I'm late here, but it actually doesn't matter; const variables
> automatically get internal linkage :)

:) thanks. It is more like a matter of taste. Both are fine to me, let me know
if you want to to revert to the static const.

https://chromiumcodereview.appspot.com/10537099/diff/8004/chrome/browser/medi...
chrome/browser/media/media_stream_devices_controller.cc:116:
MediaStreamDevicesController::GetAudioDevices() const {
On 2012/06/18 17:38:14, Bernhard Bauer wrote:
> I think we don't indent the method name if we have to break the signature
after
> the return type.

Done.

https://chromiumcodereview.appspot.com/10537099/diff/8004/chrome/browser/medi...
chrome/browser/media/media_stream_devices_controller.cc:174: if (device_it !=
request_->devices.end()) {
On 2012/06/18 17:38:14, Bernhard Bauer wrote:
> You could negative the condition and early-return.

Done.

https://chromiumcodereview.appspot.com/10537099/diff/8004/chrome/browser/medi...
chrome/browser/media/media_stream_devices_controller.cc:204: if
(!audio_device.empty()) {
On 2012/06/18 17:38:14, Bernhard Bauer wrote:
> You might want to leave out the braces. Personally, I don't mind (and the
style
> guide does allow it); other people might.

Done.

https://chromiumcodereview.appspot.com/10537099/diff/8004/chrome/browser/medi...
chrome/browser/media/media_stream_devices_controller.cc:234: if (!value.get() ||
!value->IsType(Value::TYPE_DICTIONARY))
On 2012/06/18 17:38:14, Bernhard Bauer wrote:
> Add a NOTREACHED() if we get an unexpected type back?

We can't put a NOTREACHED() here, if it can't find a valid value for the
|security_origin|, it will return a TYPE_BOOLEAN type here.

https://chromiumcodereview.appspot.com/10537099/diff/8004/chrome/browser/medi...
chrome/browser/media/media_stream_devices_controller.cc:238: if
(value_dict->empty())
On 2012/06/18 17:38:14, Bernhard Bauer wrote:
> This isn't reeaaally necessary. If the dictionary is empty, both checks below
> will simply fail gracefully.

I was thinking about adding a DCHECK here, but not sure if it is the right thing
to do, how do you think?
If it is a valid case, should we early return?

https://chromiumcodereview.appspot.com/10537099/diff/8004/chrome/browser/medi...
chrome/browser/media/media_stream_devices_controller.cc:274: std::string
MediaStreamDevicesController::GetFirstDevice(
On 2012/06/18 17:38:14, Bernhard Bauer wrote:
> I think this returns a device ID? Could you change the name to
GetFirstDeviceId
> or something to make that clear?

Done.

https://chromiumcodereview.appspot.com/10537099/diff/8004/chrome/browser/medi...
File chrome/browser/media/media_stream_devices_controller.h (right):

https://chromiumcodereview.appspot.com/10537099/diff/8004/chrome/browser/medi...
chrome/browser/media/media_stream_devices_controller.h:79: Profile* profile_;
On 2012/06/18 17:38:14, Bernhard Bauer wrote:
> Could you add a comment that the owner of this class should make sure that it
> doesn't outlive the profile?

Done.

https://chromiumcodereview.appspot.com/10537099/diff/24004/chrome/browser/con...
File chrome/browser/content_settings/content_settings_pref_provider.cc (right):

https://chromiumcodereview.appspot.com/10537099/diff/24004/chrome/browser/con...
chrome/browser/content_settings/content_settings_pref_provider.cc:364: if
(content_type == CONTENT_SETTINGS_TYPE_MEDIASTREAM) {
On 2012/06/18 17:38:15, Bernhard Bauer wrote:
> Hm. I think we should make the ContentTypeHasCompoundValue() method from
> host_content_settings_map.cc publicly accessible and use it here, to make it
> easier to add other compound content types.
> 
> I'm also not sure whether the PrefProvider needs to verify that the value is a
> non-empty dictionary; it seems the client of the HostContentSettingsMap could
do
> that as well (I try to keep the content_settings/ code as agnostic as possible
> of the actual content settings).
> 
> Then, if we don't have any special treatment for
> CONTENT_SETTINGS_TYPE_MEDIASTREAM anymore, it might be a bit nicer if you pull
> out the call to value_map_.SetValue() and just pass in a different Value based
> on whether the content type is compound.

I make the ContentTypeHasCompoundValue() a static function to be easily publicly
accessed by other classes.

I think it is fine to add a DCHECK() since the empty dictionary is an unexpected
value, and if it does happens, a DCHECK will help detecting it.

Done with moving the SetValue outside.

https://chromiumcodereview.appspot.com/10537099/diff/24004/chrome/browser/res...
File chrome/browser/resources/options2/content_settings_exceptions_area.js
(right):

https://chromiumcodereview.appspot.com/10537099/diff/24004/chrome/browser/res...
chrome/browser/resources/options2/content_settings_exceptions_area.js:16: *     
    in the select.
On 2012/06/18 19:12:58, Evan Stade wrote:
> I believe the indentation was correct before.

Sorry, mistake made by eclipse, correct now.

https://chromiumcodereview.appspot.com/10537099/diff/24004/chrome/browser/ui/...
File chrome/browser/ui/browser.cc (right):

https://chromiumcodereview.appspot.com/10537099/diff/24004/chrome/browser/ui/...
chrome/browser/ui/browser.cc:3715: for (; i < infobar_helper->infobar_count() &&
!old_infobar; ++i) {
On 2012/06/18 17:38:15, Bernhard Bauer wrote:
> Inline the declaration of |i| into the for-loop?

Done.

https://chromiumcodereview.appspot.com/10537099/diff/24004/chrome/browser/ui/...
chrome/browser/ui/browser.cc:3716: old_infobar =
infobar_helper->GetInfoBarDelegateAt(i)->
On 2012/06/18 17:38:15, Bernhard Bauer wrote:
> If you |break| here soon as |old_infobar| is not NULL, the for loop becomes a
> bit more idiomatic.

Done.

https://chromiumcodereview.appspot.com/10537099/diff/24004/chrome/browser/ui/...
File chrome/browser/ui/webui/options2/content_settings_handler2.cc (right):

https://chromiumcodereview.appspot.com/10537099/diff/24004/chrome/browser/ui/...
chrome/browser/ui/webui/options2/content_settings_handler2.cc:1175: } else {
On 2012/06/18 17:38:15, Bernhard Bauer wrote:
> Would you mind doing a small bit of cleanup here? All these if-clauses could
> return afterwards instead of having else's, so we could get rid off all the
> indentation.

Sure, no problem at all.
I found the function gets long anyway, so I break it down into some
sub-functions instead, hope it is fine.

https://chromiumcodereview.appspot.com/10537099/diff/24004/chrome/browser/ui/...
chrome/browser/ui/webui/options2/content_settings_handler2.cc:1181: //
SetWebsiteSetting instead of SetContentSetting.
On 2012/06/18 17:38:15, Bernhard Bauer wrote:
> I think you *can* use SetWebsiteSetting for simple content types as well, so
you
> could just always call SetWebsiteSetting here (NULL is converted to
> CONTENT_SETTING_DEFAULT).

Done.

[Joao da Silva, 2012-06-19 14:28:08]

The enterprise team was at a summit last week, sorry for the late review. This
is quite a big CL and could have been 3 smaller ones for ease of reviewing.

Policy part lgtm after fixing the template. Please consider the other comments
too.

https://chromiumcodereview.appspot.com/10537099/diff/34002/chrome/app/policy/...
File chrome/app/policy/policy_templates.json (right):

https://chromiumcodereview.appspot.com/10537099/diff/34002/chrome/app/policy/...
chrome/app/policy/policy_templates.json:1570: 'supported_on': ['chrome.*:10-',
'chrome_os:0.11-'],
Please update the version. I guess you want chrome 22 here.

https://chromiumcodereview.appspot.com/10537099/diff/34002/chrome/app/policy/...
chrome/app/policy/policy_templates.json:1577: If this policy is left not set,
'AskMediaStream' will be used and the user will be able to change it.''',
Did you mean 'PromptOnAccess' instead of 'AskMediaStream' in this description?

https://chromiumcodereview.appspot.com/10537099/diff/34002/chrome/browser/con...
File chrome/browser/content_settings/content_settings_pref_provider.cc (right):

https://chromiumcodereview.appspot.com/10537099/diff/34002/chrome/browser/con...
chrome/browser/content_settings/content_settings_pref_provider.cc:372: if
(settings_dictionary->GetDictionaryWithoutPathExpansion(
This "if" is sufficient, and the 2 ifs above it aren't necessary.

I noticed patch set 2 was doing that, but I don't see why it has been changed.
Either version is fine, I'd just prefer less code if possible :-)

https://chromiumcodereview.appspot.com/10537099/diff/34002/chrome/browser/med...
File chrome/browser/media/media_stream_devices_controller.cc (right):

https://chromiumcodereview.appspot.com/10537099/diff/34002/chrome/browser/med...
chrome/browser/media/media_stream_devices_controller.cc:253: return;
Replace these with value->GetAsDictionary() to avoid the cast.

https://chromiumcodereview.appspot.com/10537099/diff/34002/chrome/browser/med...
File chrome/browser/media/media_stream_devices_controller.h (right):

https://chromiumcodereview.appspot.com/10537099/diff/34002/chrome/browser/med...
chrome/browser/media/media_stream_devices_controller.h:11: #include
"base/callback.h"
Needed?

https://chromiumcodereview.appspot.com/10537099/diff/34002/chrome/browser/med...
chrome/browser/media/media_stream_devices_controller.h:12: #include
"chrome/browser/media/media_stream_devices_controller.h"
Self-include?

https://chromiumcodereview.appspot.com/10537099/diff/34002/chrome/browser/med...
chrome/browser/media/media_stream_devices_controller.h:13: #include
"content/public/browser/content_browser_client.h"
Needed?

https://chromiumcodereview.appspot.com/10537099/diff/34002/chrome/browser/med...
chrome/browser/media/media_stream_devices_controller.h:14: #include
"content/public/browser/web_contents_delegate.h"
Needed?

https://chromiumcodereview.appspot.com/10537099/diff/34002/chrome/browser/med...
chrome/browser/media/media_stream_devices_controller.h:17: class
ContentSettingsPattern;
Needed?

https://chromiumcodereview.appspot.com/10537099/diff/34002/chrome/browser/med...
chrome/browser/media/media_stream_devices_controller.h:89: };
DISALLOW_COPY_AND_ASSIGN(MediaStreamDevicesController);

https://chromiumcodereview.appspot.com/10537099/diff/34002/chrome/browser/med...
File chrome/browser/media/media_stream_devices_menu_model.cc (right):

https://chromiumcodereview.appspot.com/10537099/diff/34002/chrome/browser/med...
chrome/browser/media/media_stream_devices_menu_model.cc:38: 
Nit: remove this newline

https://chromiumcodereview.appspot.com/10537099/diff/34002/chrome/browser/ui/...
File chrome/browser/ui/media_stream_infobar_delegate.cc (right):

https://chromiumcodereview.appspot.com/10537099/diff/34002/chrome/browser/ui/...
chrome/browser/ui/media_stream_infobar_delegate.cc:8: #include <functional>
Are these 2 needed?

https://chromiumcodereview.appspot.com/10537099/diff/34002/chrome/browser/ui/...
chrome/browser/ui/media_stream_infobar_delegate.cc:11: #include
"base/memory/scoped_ptr.h"
Needed?

https://chromiumcodereview.appspot.com/10537099/diff/34002/chrome/browser/ui/...
File chrome/browser/ui/media_stream_infobar_delegate.h (right):

https://chromiumcodereview.appspot.com/10537099/diff/34002/chrome/browser/ui/...
chrome/browser/ui/media_stream_infobar_delegate.h:12: #include
"chrome/browser/media/media_stream_devices_controller.h"
MediaStreamDevicesController can be forward-declared in this file.

https://chromiumcodereview.appspot.com/10537099/diff/34002/chrome/browser/ui/...
chrome/browser/ui/media_stream_infobar_delegate.h:14: class TabContents;
Needed?

https://chromiumcodereview.appspot.com/10537099/diff/34002/chrome/browser/ui/...
File chrome/browser/ui/webui/options2/content_settings_handler2.cc (right):

https://chromiumcodereview.appspot.com/10537099/diff/34002/chrome/browser/ui/...
chrome/browser/ui/webui/options2/content_settings_handler2.cc:702: case
CONTENT_SETTINGS_TYPE_MEDIASTREAM:
break; before this line? If this is on purpose, then add a comment like

// fallthrough:

https://chromiumcodereview.appspot.com/10537099/diff/34002/chrome/browser/ui/...
File chrome/browser/ui/webui/options2/content_settings_handler2.h (right):

https://chromiumcodereview.appspot.com/10537099/diff/34002/chrome/browser/ui/...
chrome/browser/ui/webui/options2/content_settings_handler2.h:106: void
RemoveGeolocationException(const ListValue* args, size_t arg_index);
Nit: indent

https://chromiumcodereview.appspot.com/10537099/diff/34002/chrome/chrome_brow...
File chrome/chrome_browser.gypi (right):

https://chromiumcodereview.appspot.com/10537099/diff/34002/chrome/chrome_brow...
chrome/chrome_browser.gypi:1359:
'browser/media/media_stream_devices_controller.h',
Nit: alphabetical order

https://chromiumcodereview.appspot.com/10537099/diff/34002/chrome/common/pref...
File chrome/common/pref_names.cc (right):

https://chromiumcodereview.appspot.com/10537099/diff/34002/chrome/common/pref...
chrome/common/pref_names.cc:1830: "mediastream.default_content_settings";
I'd make this string follow the same pattern as the ones above, and make it
"profile.managed_default_content_settings.mediastream"

[Bernhard Bauer, 2012-06-19 14:33:34]

https://chromiumcodereview.appspot.com/10537099/diff/8004/chrome/browser/cont...
File chrome/browser/content_settings/host_content_settings_map.cc (left):

https://chromiumcodereview.appspot.com/10537099/diff/8004/chrome/browser/cont...
chrome/browser/content_settings/host_content_settings_map.cc:188:
DCHECK(!ContentTypeHasCompoundValue(content_type));
On 2012/06/19 12:23:16, xians1 wrote:
> On 2012/06/18 17:38:14, Bernhard Bauer wrote:
> > Are you sure about this? This method is supposed to return a ContentSetting,
> so
> > it can't be called for a content type where the value can't be coerced to a
> > content setting. We might need to add a GetDefaultWebsiteSetting method that
> > doesn't do the coercion.
> 
> I assume the default values are only "ask/allow/block" for all the content
> types, regardless if they are using compound value or not.
> Take MediaStream as example, it uses a dictionary value {audio: name, video:
> name}, but for its default value, it only has "ask/block".
> 
> I think we should allow a type using compound value and also contentsetting
> value for its default setting.

Hm, ok. I guess it's fair to say that a content type using compound values can
have arbitrary values.

> Furthermore, I don't think this DCHECK adds much here since
> content_settings::ValueToContentSetting() will hit a DCHECK anyway.

https://chromiumcodereview.appspot.com/10537099/diff/8004/chrome/browser/medi...
File chrome/browser/media/media_stream_devices_controller.cc (right):

https://chromiumcodereview.appspot.com/10537099/diff/8004/chrome/browser/medi...
chrome/browser/media/media_stream_devices_controller.cc:238: if
(value_dict->empty())
On 2012/06/19 12:23:16, xians1 wrote:
> On 2012/06/18 17:38:14, Bernhard Bauer wrote:
> > This isn't reeaaally necessary. If the dictionary is empty, both checks
below
> > will simply fail gracefully.
> 
> I was thinking about adding a DCHECK here, but not sure if it is the right
thing
> to do, how do you think?
> If it is a valid case, should we early return?

The easiest thing would be to do nothing at all. The checks below will do the
Right Thing(tm).

https://chromiumcodereview.appspot.com/10537099/diff/24004/chrome/browser/con...
File chrome/browser/content_settings/content_settings_pref_provider.cc (right):

https://chromiumcodereview.appspot.com/10537099/diff/24004/chrome/browser/con...
chrome/browser/content_settings/content_settings_pref_provider.cc:364: if
(content_type == CONTENT_SETTINGS_TYPE_MEDIASTREAM) {
On 2012/06/19 12:23:17, xians1 wrote:
> On 2012/06/18 17:38:15, Bernhard Bauer wrote:
> > Hm. I think we should make the ContentTypeHasCompoundValue() method from
> > host_content_settings_map.cc publicly accessible and use it here, to make it
> > easier to add other compound content types.
> > 
> > I'm also not sure whether the PrefProvider needs to verify that the value is
a
> > non-empty dictionary; it seems the client of the HostContentSettingsMap
could
> do
> > that as well (I try to keep the content_settings/ code as agnostic as
possible
> > of the actual content settings).
> > 
> > Then, if we don't have any special treatment for
> > CONTENT_SETTINGS_TYPE_MEDIASTREAM anymore, it might be a bit nicer if you
pull
> > out the call to value_map_.SetValue() and just pass in a different Value
based
> > on whether the content type is compound.
> 
> I make the ContentTypeHasCompoundValue() a static function to be easily
publicly
> accessed by other classes.
> 
> I think it is fine to add a DCHECK() since the empty dictionary is an
unexpected
> value, and if it does happens, a DCHECK will help detecting it.

Right, but it's only unexpected to a client that uses the MEDIASTREAM content
type. Other content types might presumably allow empty dictionaries. If you want
to verify the value we're reading from prefs, you could call
HostContentSettingsMap::IsValueAllowedForType() and do the check for a non-empty
dictionary there. That would also verify the value if we're changing a setting.

> Done with moving the SetValue outside.

https://chromiumcodereview.appspot.com/10537099/diff/24004/chrome/browser/ui/...
File chrome/browser/ui/webui/options2/content_settings_handler2.cc (right):

https://chromiumcodereview.appspot.com/10537099/diff/24004/chrome/browser/ui/...
chrome/browser/ui/webui/options2/content_settings_handler2.cc:1175: } else {
On 2012/06/19 12:23:17, xians1 wrote:
> On 2012/06/18 17:38:15, Bernhard Bauer wrote:
> > Would you mind doing a small bit of cleanup here? All these if-clauses could
> > return afterwards instead of having else's, so we could get rid off all the
> > indentation.
> 
> Sure, no problem at all.
> I found the function gets long anyway, so I break it down into some
> sub-functions instead, hope it is fine.

Nice, thanks!

https://chromiumcodereview.appspot.com/10537099/diff/34002/chrome/browser/med...
File chrome/browser/media/media_stream_devices_controller.cc (right):

https://chromiumcodereview.appspot.com/10537099/diff/34002/chrome/browser/med...
chrome/browser/media/media_stream_devices_controller.cc:246: &info));
You can pass in NULL if you don't need |info|.

https://chromiumcodereview.appspot.com/10537099/diff/34002/chrome/browser/med...
chrome/browser/media/media_stream_devices_controller.cc:247: if (!value.get() ||
!value->IsType(Value::TYPE_DICTIONARY))
I think |value| can't be NULL, right?

https://chromiumcodereview.appspot.com/10537099/diff/34002/chrome/browser/med...
chrome/browser/media/media_stream_devices_controller.cc:268: return;
Nit: Unnecessary :)

[no longer working on chromium, 2012-06-20 17:24:38]

Hello,

Sorry for the delay on uploading a new version, we stockholm just had a water
leak on the server room, and the internet was shutdown.

I am really sorry that I accidentally deleted the previous patchset when I tried
to download the patch, so I lost the corresponding comments from Joao and
Bernhard.

Could you please take another look, the difference between patch7 and patch6
contains the changes addressing the recent comments from Joao and Bernhard.

Thanks,
BR,
-SX

https://chromiumcodereview.appspot.com/10537099/diff/34036/chrome/app/policy/...
File chrome/app/policy/policy_templates.json (right):

https://chromiumcodereview.appspot.com/10537099/diff/34036/chrome/app/policy/...
chrome/app/policy/policy_templates.json:1577: If this policy is left not set,
'PromptOnAccess' will be used and the user will be able to change it.''',
To joaodasilva: Done with correcting the version number and the comment.

https://chromiumcodereview.appspot.com/10537099/diff/34036/chrome/browser/con...
File chrome/browser/content_settings/content_settings_pref_provider.cc (right):

https://chromiumcodereview.appspot.com/10537099/diff/34036/chrome/browser/con...
chrome/browser/content_settings/content_settings_pref_provider.cc:367:
DictionaryValue* setting = NULL;
To joaodasilva: Thanks for pointing out, I was not sure two solutions worked the
same way. Clean it up by using only GetDictionaryWithoutPathExpansion.

https://chromiumcodereview.appspot.com/10537099/diff/34036/chrome/browser/con...
chrome/browser/content_settings/content_settings_pref_provider.cc:371:
DCHECK(!setting->empty());
To bauerb: I am a bit not sure if we should allow empty dictionary as value
here. But if this gets true, the developer needs to remove the DCHECK and handle
it thoroughly.
Currently IsValueAllowedForType only takes care of the arbitrary values, if I
add support to compound values, I will do no more than checking if the value is
empty there, please let me know if you still want it.

https://chromiumcodereview.appspot.com/10537099/diff/34036/chrome/browser/med...
File chrome/browser/media/media_stream_devices_controller.cc (right):

https://chromiumcodereview.appspot.com/10537099/diff/34036/chrome/browser/med...
chrome/browser/media/media_stream_devices_controller.cc:245: NULL));
To bauerb: Done with using NULL for the &info, and remove the unnecessary return
below.

https://chromiumcodereview.appspot.com/10537099/diff/34036/chrome/browser/med...
chrome/browser/media/media_stream_devices_controller.cc:246: if (!value.get()) {
To bauerb: Done with adding a NOTREACHED();

https://chromiumcodereview.appspot.com/10537099/diff/34036/chrome/browser/med...
chrome/browser/media/media_stream_devices_controller.cc:252: if
(!value->GetAsDictionary(&value_dict) || value_dict->empty())
To joaodasilva: Done with replaying static_cast with GetAsDictionary().

https://chromiumcodereview.appspot.com/10537099/diff/34036/chrome/browser/med...
File chrome/browser/media/media_stream_devices_controller.h (right):

https://chromiumcodereview.appspot.com/10537099/diff/34036/chrome/browser/med...
chrome/browser/media/media_stream_devices_controller.h:11: #include
"content/browser/renderer_host/render_view_host_delegate.h"
To joaodasilva: Done with cleaning up the #includes.

https://chromiumcodereview.appspot.com/10537099/diff/34036/chrome/browser/ui/...
File chrome/browser/ui/media_stream_infobar_delegate.cc (right):

https://chromiumcodereview.appspot.com/10537099/diff/34036/chrome/browser/ui/...
chrome/browser/ui/media_stream_infobar_delegate.cc:8: #include
"googleurl/src/gurl.h"
To  joaodasilva: Done with cleaning up the #includes.

https://chromiumcodereview.appspot.com/10537099/diff/34036/chrome/browser/ui/...
File chrome/browser/ui/media_stream_infobar_delegate.h (right):

https://chromiumcodereview.appspot.com/10537099/diff/34036/chrome/browser/ui/...
chrome/browser/ui/media_stream_infobar_delegate.h:12: #include
"chrome/browser/media/media_stream_devices_controller.h"
To joaodasilva: We need to include this header file because it has a member
variable scoped_ptr<MediaStreamDevicesController> controller_.

https://chromiumcodereview.appspot.com/10537099/diff/34036/chrome/chrome_brow...
File chrome/chrome_browser.gypi (right):

https://chromiumcodereview.appspot.com/10537099/diff/34036/chrome/chrome_brow...
chrome/chrome_browser.gypi:1358:
'browser/media/media_stream_devices_controller.cc',
To joaodasilva: Done with correcting the order.

https://chromiumcodereview.appspot.com/10537099/diff/34036/chrome/common/pref...
File chrome/common/pref_names.cc (right):

https://chromiumcodereview.appspot.com/10537099/diff/34036/chrome/common/pref...
chrome/common/pref_names.cc:1826:
"profile.managed_default_content_settings.media_stream";
To joaodasilva: Done with using the same naming style as others.

[Bernhard Bauer, 2012-06-20 18:33:27]

https://chromiumcodereview.appspot.com/10537099/diff/34036/chrome/browser/con...
File chrome/browser/content_settings/content_settings_pref_provider.cc (right):

https://chromiumcodereview.appspot.com/10537099/diff/34036/chrome/browser/con...
chrome/browser/content_settings/content_settings_pref_provider.cc:371:
DCHECK(!setting->empty());
On 2012/06/20 17:24:38, xians1 wrote:
> To bauerb: I am a bit not sure if we should allow empty dictionary as value
> here. But if this gets true, the developer needs to remove the DCHECK and
handle
> it thoroughly.
> Currently IsValueAllowedForType only takes care of the arbitrary values, if I
> add support to compound values, I will do no more than checking if the value
is
> empty there, please let me know if you still want it.

That's fine with me, my point was that we should do verification of the value
depending on the content type (in which case we can check for a non-empty
dictionary for MEDIASTREAM), but not have it inside of PrefProvider (to keep
that class content-type-agnostic).

[no longer working on chromium, 2012-06-21 19:46:17]

On 2012/06/20 18:33:27, Bernhard Bauer wrote:
>
https://chromiumcodereview.appspot.com/10537099/diff/34036/chrome/browser/con...
> File chrome/browser/content_settings/content_settings_pref_provider.cc
(right):
> 
>
https://chromiumcodereview.appspot.com/10537099/diff/34036/chrome/browser/con...
> chrome/browser/content_settings/content_settings_pref_provider.cc:371:
> DCHECK(!setting->empty());
> On 2012/06/20 17:24:38, xians1 wrote:
> > To bauerb: I am a bit not sure if we should allow empty dictionary as value
> > here. But if this gets true, the developer needs to remove the DCHECK and
> handle
> > it thoroughly.
> > Currently IsValueAllowedForType only takes care of the arbitrary values, if
I
> > add support to compound values, I will do no more than checking if the value
> is
> > empty there, please let me know if you still want it.
> 
> That's fine with me, my point was that we should do verification of the value
> depending on the content type (in which case we can check for a non-empty
> dictionary for MEDIASTREAM), but not have it inside of PrefProvider (to keep
> that class content-type-agnostic).

So if you don't mind, I will leave it now and add a TODO instead.
Could you please take another look again? I want to land it ASAP. Thanks.

[Bernhard Bauer, 2012-06-22 08:16:39]

LGTM

[Joao da Silva, 2012-06-22 16:58:04]

lgtm

[commit-bot: I haz the power, 2012-06-25 12:44:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xians@chromium.org/10537099/61002

[commit-bot: I haz the power, 2012-06-25 12:44:59]

Failed to apply patch for
chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa_unittest.mm:
While running patch -p1 --forward --force;
patching file
chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa_unittest.mm
Hunk #1 FAILED at 62.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa_unittest.mm.rej

[commit-bot: I haz the power, 2012-06-25 14:13:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xians@chromium.org/10537099/64002

[commit-bot: I haz the power, 2012-06-25 14:13:40]

Presubmit check for 10537099-64002 failed and returned exit status 1.

Running presubmit commit checks ...
Finished checking
/b/commit-queue/workdir/chromium/chrome/app/policy/policy_templates.json. 0
errors, 0 warnings.

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui
    chrome/browser/ui/cocoa/infobars

Presubmit checks took 3.1s to calculate.

[sky, 2012-06-25 14:19:42]

LGTM

https://chromiumcodereview.appspot.com/10537099/diff/64002/chrome/browser/ui/...
File chrome/browser/ui/media_stream_infobar_delegate.h (right):

https://chromiumcodereview.appspot.com/10537099/diff/64002/chrome/browser/ui/...
chrome/browser/ui/media_stream_infobar_delegate.h:23:
MediaStreamDevicesController* controller);
Document this takes ownership of |controller|.

[commit-bot: I haz the power, 2012-06-25 14:29:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xians@chromium.org/10537099/65005

[commit-bot: I haz the power, 2012-06-25 17:00:58]

Change committed as 143918