Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
588153003:
Remove MediaSettingChangedInfobar and show latest state in bubble (Closed)
DescriptionRemove MediaSettingChanged infobar and show the latest state of the media stream
settings in the content settings bubble.
The reload infobar is no longer displayed when the media stream content settings
change. Instead, a small hint is displayed in the bubble when it is re-opened
after changing a media stream setting.
This hint is always displayed when the camera/microphone permission changes.
When the media device preference changes, the hint is only displayed if there
was at least one active media capture.
Also added a bunch of tests for the media menu because they had no test coverage.
BUG=405522
TEST=ContentSettingBubbleModelTest.*Media*
Committed: https://crrev.com/1fb9a9bdf67911afcd8579c446daed6015a86103
Cr-Commit-Position: refs/heads/master@{#296913}
Patch Set 1 #
Total comments: 24
Patch Set 2 : Address nits, add tests, refactor to use bitmask instead of enum. #
Total comments: 24
Patch Set 3 : nits. Cleanup #Patch Set 4 : nits #Patch Set 5 : Remove redundant is_cam #
Total comments: 8
Patch Set 6 : Small nits #Patch Set 7 : Fix test failures #Patch Set 8 : Fix MSVC warning C4800 #Patch Set 9 : Fix MSVC warning C4800 #2 #Patch Set 10 : switch -> if #Messages
Total messages: 34 (12 generated)
rob@robwu.nl changed reviewers: + jochen@chromium.org, pkasting@chromium.org
Optional: We could perhaps simplify some of this code even more if e.g. the
MicrophoneCameraState enum was changed to a bitfield:
typedef uint32_t MicCamState;
const uint32_t kMicCamState_Mic = 1 << 0; // 1 if this applies to the mic
const uint32_t kMicCamState_Camera = 1 << 1; // 1 if this applies to the cam
const uint32_t kMicCamState_Block = 1 << 2; // 1 for "blocked", 0 otherwise
Now you could implement e.g. this:
bool TabSpecificContentSettings::IsMicrophoneCameraStateChanged() const {
bool mic_changed = (microphone_camera_state_ & kMicCamState_Mic) &&
((microphone_camera_state_ & kMicCamState_Block) ?
IsContentBlocked(CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC) :
IsContentAllowed(CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC));
bool camera_changed = (microphone_camera_state_ & kMicCamState_Camera) &&
((microphone_camera_state_ & kMicCamState_Block) ?
IsContentBlocked(CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA) :
IsContentAllowed(CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA));
return mic_changed || camera_changed;
}
Similarly, the part near the end of OnMediaStreamPermissionSet() could probably
be simplified even more than I've done below.
This would also probably scale better if we added more devices to this set than
a mic and a camera, since we wouldn't need explicit enum values for the cross
product of all devices.
https://codereview.chromium.org/588153003/diff/1/chrome/browser/content_setti...
File chrome/browser/content_settings/tab_specific_content_settings.cc (right):
https://codereview.chromium.org/588153003/diff/1/chrome/browser/content_setti...
chrome/browser/content_settings/tab_specific_content_settings.cc:287: // setting
to kick in without reloading the page, and the UI for media is
Nit: setting -> settings
"the UI for media" is a bit unclear as well.
https://codereview.chromium.org/588153003/diff/1/chrome/browser/content_setti...
chrome/browser/content_settings/tab_specific_content_settings.cc:288: // always
reflecting the newest permission setting.
Nit: is always reflecting -> always reflects
https://codereview.chromium.org/588153003/diff/1/chrome/browser/content_setti...
chrome/browser/content_settings/tab_specific_content_settings.cc:527:
prefs->GetString(prefs::kDefaultAudioCaptureDevice) :
Nit: I'd indent this line and the next 4 more (2 places)
https://codereview.chromium.org/588153003/diff/1/chrome/browser/content_setti...
chrome/browser/content_settings/tab_specific_content_settings.cc:529: switch
(it->second.permission) {
Nit: Shorter:
DCHECK_NE(MediaStreamDevicesController::MEDIA_NONE, it->second.permission);
const bool mic_allowed =
it->second.permission == MediaStreamDevicesController::MEDIA_ALLOWED;
content_allowed_[CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC] = mic_allowed;
content_blocked_[CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC] = !mic_allowed;
(Same comment below)
https://codereview.chromium.org/588153003/diff/1/chrome/browser/content_setti...
chrome/browser/content_settings/tab_specific_content_settings.cc:573: if
(IsContentAllowed(CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC) &&
Nit: Slightly shorter and does fewer redundant checks:
const ContentSettingsType camera = CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA;
if (IsContentAllowed(CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC))
microphone_camera_state_ = IsContentAllowed(camera) ?
MICROPHONE_CAMERA_ACCESSED : MICROPHONE_ACCESSED;
} else if (IsContentAllowed(camera)) {
microphone_camera_state_ = CAMERA_ACCESSED;
} else if (IsContentBlocked(CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC) {
microphone_camera_state_ = IsContentBlocked(camera) ?
MICROPHONE_CAMERA_BLOCKED : MICROPHONE_BLOCKED;
} else if (IsContentBlocked(camera)) {
microphone_camera_state_ = CAMERA_BLOCKED;
}
https://codereview.chromium.org/588153003/diff/1/chrome/browser/content_setti...
chrome/browser/content_settings/tab_specific_content_settings.cc:769: if
(setting == CONTENT_SETTING_BLOCK) {
Nit: Shorter:
content_allowed_[content_type] = setting == CONTENT_SETTING_ALLOW;
content_blocked_[content_type] = setting == CONTENT_SETTING_BLOCK;
DCHECK(content_allowed_[content_type] || content_blocked_[content_type] ||
(setting == CONTENT_SETTING_ASK));
(The final DCHECK is optional, I don't know whether it's really critical)
https://codereview.chromium.org/588153003/diff/1/chrome/browser/content_setti...
File chrome/browser/content_settings/tab_specific_content_settings.h (right):
https://codereview.chromium.org/588153003/diff/1/chrome/browser/content_setti...
chrome/browser/content_settings/tab_specific_content_settings.h:225: // Returns
whether the state of the camera and microphone settings have
Nit: Remove "the state of", or else change "have" to "has"
It's also slightly inconsistent that here we refer to the "state of the
settings" but elsewhere (e.g. down below) it's more of a reference to the "state
of the microphone and camera" (no "settings"). The state of the devices and the
state of our settings for the devices are two different things. Be clear and
consistent.
https://codereview.chromium.org/588153003/diff/1/chrome/browser/content_setti...
chrome/browser/content_settings/tab_specific_content_settings.h:430: // The most
selected devices at the the of the media stream request.
Nit: "most selected"?
https://codereview.chromium.org/588153003/diff/1/chrome/browser/ui/content_se...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):
https://codereview.chromium.org/588153003/diff/1/chrome/browser/ui/content_se...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:876: void
ContentSettingMediaStreamBubbleModel::SetCustomLink() {
Nit: Instead of having this function set the link itself and return void, why
not have this be a "should have custom link" oracle that returns bool?
This will allow you to break the code below into more discrete conditionals for
readability, and declare temps like |prefs| right above first use.
https://codereview.chromium.org/588153003/diff/1/chrome/browser/ui/content_se...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:889: //
TODO(robwu): Rename the next resource ID (remove "INFOBAR").
Why not go ahead and do that in this change? It's only going to affect another
file or two, isn't it?
https://codereview.chromium.org/588153003/diff/1/chrome/browser/ui/content_se...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:891:
IDS_MEDIASTREAM_SETTING_CHANGED_INFOBAR_MESSAGE));
Nit: Bizarre indenting (should be 4)
https://codereview.chromium.org/588153003/diff/1/chrome/browser/ui/content_se...
File chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc
(right):
https://codereview.chromium.org/588153003/diff/1/chrome/browser/ui/content_se...
chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc:238:
scoped_ptr<ContentSettingBubbleModel> content_setting_bubble_model(
Nit: Would it make sense to make this added code its own test instead of tacking
it onto the previous one? If it didn't result in extra setup boilerplate or
something, it would mean shorter, more focused individual tests.
Addressed all nits, added more tests and refactored to use bitmask instead of enums. https://codereview.chromium.org/588153003/diff/1/chrome/browser/content_setti... File chrome/browser/content_settings/tab_specific_content_settings.cc (right): https://codereview.chromium.org/588153003/diff/1/chrome/browser/content_setti... chrome/browser/content_settings/tab_specific_content_settings.cc:287: // setting to kick in without reloading the page, and the UI for media is On 2014/09/22 23:11:22, Peter Kasting wrote: > Nit: setting -> settings > > "the UI for media" is a bit unclear as well. I have just deleted this comment since it is no longer relevant. It was added in https://codereview.chromium.org/13375004/patch/6001/7001. The comment is certainly not relevant to the remaining media setting (CONTENT_SETTINGS_TYPE_PROTECTED_MEDIA_IDENTIFIER) because there is no "reload page" UI for it (the setting was added in https://codereview.chromium.org/download/issue23531021_266001.diff) https://codereview.chromium.org/588153003/diff/1/chrome/browser/content_setti... chrome/browser/content_settings/tab_specific_content_settings.cc:527: prefs->GetString(prefs::kDefaultAudioCaptureDevice) : On 2014/09/22 23:11:22, Peter Kasting wrote: > Nit: I'd indent this line and the next 4 more (2 places) Done. https://codereview.chromium.org/588153003/diff/1/chrome/browser/content_setti... chrome/browser/content_settings/tab_specific_content_settings.cc:529: switch (it->second.permission) { On 2014/09/22 23:11:23, Peter Kasting wrote: > Nit: Shorter: > > DCHECK_NE(MediaStreamDevicesController::MEDIA_NONE, it->second.permission); > const bool mic_allowed = > it->second.permission == MediaStreamDevicesController::MEDIA_ALLOWED; > content_allowed_[CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC] = mic_allowed; > content_blocked_[CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC] = !mic_allowed; > > (Same comment below) Done. https://codereview.chromium.org/588153003/diff/1/chrome/browser/content_setti... chrome/browser/content_settings/tab_specific_content_settings.cc:573: if (IsContentAllowed(CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC) && On 2014/09/22 23:11:23, Peter Kasting wrote: > Nit: Slightly shorter and does fewer redundant checks: > > const ContentSettingsType camera = CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA; > if (IsContentAllowed(CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC)) > microphone_camera_state_ = IsContentAllowed(camera) ? > MICROPHONE_CAMERA_ACCESSED : MICROPHONE_ACCESSED; > } else if (IsContentAllowed(camera)) { > microphone_camera_state_ = CAMERA_ACCESSED; > } else if (IsContentBlocked(CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC) { > microphone_camera_state_ = IsContentBlocked(camera) ? > MICROPHONE_CAMERA_BLOCKED : MICROPHONE_BLOCKED; > } else if (IsContentBlocked(camera)) { > microphone_camera_state_ = CAMERA_BLOCKED; > } Done. https://codereview.chromium.org/588153003/diff/1/chrome/browser/content_setti... chrome/browser/content_settings/tab_specific_content_settings.cc:769: if (setting == CONTENT_SETTING_BLOCK) { On 2014/09/22 23:11:22, Peter Kasting wrote: > Nit: Shorter: > > content_allowed_[content_type] = setting == CONTENT_SETTING_ALLOW; > content_blocked_[content_type] = setting == CONTENT_SETTING_BLOCK; > DCHECK(content_allowed_[content_type] || content_blocked_[content_type] || > (setting == CONTENT_SETTING_ASK)); > > (The final DCHECK is optional, I don't know whether it's really critical) Done. https://codereview.chromium.org/588153003/diff/1/chrome/browser/content_setti... File chrome/browser/content_settings/tab_specific_content_settings.h (right): https://codereview.chromium.org/588153003/diff/1/chrome/browser/content_setti... chrome/browser/content_settings/tab_specific_content_settings.h:225: // Returns whether the state of the camera and microphone settings have On 2014/09/22 23:11:23, Peter Kasting wrote: > Nit: Remove "the state of", or else change "have" to "has" > > It's also slightly inconsistent that here we refer to the "state of the > settings" but elsewhere (e.g. down below) it's more of a reference to the "state > of the microphone and camera" (no "settings"). The state of the devices and the > state of our settings for the devices are two different things. Be clear and > consistent. Done. https://codereview.chromium.org/588153003/diff/1/chrome/browser/content_setti... chrome/browser/content_settings/tab_specific_content_settings.h:430: // The most selected devices at the the of the media stream request. On 2014/09/22 23:11:23, Peter Kasting wrote: > Nit: "most selected"? Done. https://codereview.chromium.org/588153003/diff/1/chrome/browser/ui/content_se... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/588153003/diff/1/chrome/browser/ui/content_se... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:889: // TODO(robwu): Rename the next resource ID (remove "INFOBAR"). On 2014/09/22 23:11:23, Peter Kasting wrote: > Why not go ahead and do that in this change? It's only going to affect another > file or two, isn't it? Done. https://codereview.chromium.org/588153003/diff/1/chrome/browser/ui/content_se... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:891: IDS_MEDIASTREAM_SETTING_CHANGED_INFOBAR_MESSAGE)); On 2014/09/22 23:11:23, Peter Kasting wrote: > Nit: Bizarre indenting (should be 4) Done. https://codereview.chromium.org/588153003/diff/1/chrome/browser/ui/content_se... File chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc (right): https://codereview.chromium.org/588153003/diff/1/chrome/browser/ui/content_se... chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc:238: scoped_ptr<ContentSettingBubbleModel> content_setting_bubble_model( On 2014/09/22 23:11:23, Peter Kasting wrote: > Nit: Would it make sense to make this added code its own test instead of tacking > it onto the previous one? If it didn't result in extra setup boilerplate or > something, it would mean shorter, more focused individual tests. Done. I have added two tests. One of them is for the media permission changes, the other one is for media menu settings (which was not covered by any test before this patch).
https://codereview.chromium.org/588153003/diff/1/chrome/browser/content_setti... File chrome/browser/content_settings/tab_specific_content_settings.cc (right): https://codereview.chromium.org/588153003/diff/1/chrome/browser/content_setti... chrome/browser/content_settings/tab_specific_content_settings.cc:287: // setting to kick in without reloading the page, and the UI for media is On 2014/09/24 00:03:04, robwu wrote: > On 2014/09/22 23:11:22, Peter Kasting wrote: > > Nit: setting -> settings > > > > "the UI for media" is a bit unclear as well. > > I have just deleted this comment since it is no longer relevant. > It was added in https://codereview.chromium.org/13375004/patch/6001/7001. > The comment is certainly not relevant to the remaining media setting > (CONTENT_SETTINGS_TYPE_PROTECTED_MEDIA_IDENTIFIER) because there is no "reload > page" UI for it (the setting was added in > https://codereview.chromium.org/download/issue23531021_266001.diff) OK, but in that case, as a reader I'm still going to wonder why this particular type is special and needs its own handling, so there needs to be some ind of explanation here and below. https://codereview.chromium.org/588153003/diff/20001/chrome/browser/content_s... File chrome/browser/content_settings/tab_specific_content_settings.cc (right): https://codereview.chromium.org/588153003/diff/20001/chrome/browser/content_s... chrome/browser/content_settings/tab_specific_content_settings.cc:485: if (microphone_camera_state_ == MICROPHONE_CAMERA_NOT_ACCESSED) Nit: This conditional isn't really needed. https://codereview.chromium.org/588153003/diff/20001/chrome/browser/content_s... chrome/browser/content_settings/tab_specific_content_settings.cc:546: microphone_camera_state_ |= MICROPHONE_ACCESSED; Nit: Shorter: microphone_camera_state_ |= MICROPHONE_ACCESSED | (mic_allowed ? 0 : MICROPHONE_BLOCKED); (2 places) https://codereview.chromium.org/588153003/diff/20001/chrome/browser/content_s... File chrome/browser/content_settings/tab_specific_content_settings.h (right): https://codereview.chromium.org/588153003/diff/20001/chrome/browser/content_s... chrome/browser/content_settings/tab_specific_content_settings.h:49: // its corresponding *_ALLOWED flag must also be set. Was _ALLOWED here supposed to be _ACCESSED? If so, write it that way -- and that would completely change my understanding (encoded in my other comments) of what the values here mean. I was assuming that the ACCESSED is the opposite of BLOCKED, as opposed to BLOCKED implying ACCESSED. (The fact that I can be confused about something basic like this implies that the existing names and comments need to be clarified.) Perhaps this: Fields describing the current mic/camera state. If a page has attempted to access a device, the XXX_ACCESSED bit will be set. In that case, the value of the corresponding XXX_BLOCKED bit corresponds to whether the access was blocked or allowed. Otherwise, the XXX_BLOCKED bit is ignored. https://codereview.chromium.org/588153003/diff/20001/chrome/browser/content_s... chrome/browser/content_settings/tab_specific_content_settings.h:50: enum MicrophoneCameraState { Nit: Don't use an enum to declare bitmasks. Use constants instead, because this is semantically no longer an enumeration. https://codereview.chromium.org/588153003/diff/20001/chrome/browser/content_s... chrome/browser/content_settings/tab_specific_content_settings.h:223: unsigned GetMicrophoneCameraState() const; (Here and elsewhere) Use types like uint32_t instead of "unsigned" for bitfields, so it's clear how many bits are available for use. (Basically, "unsigned" should never be used in our codebase.) You should also typedef this, so that it's clear to readers that the code in question is dealing with these state values, not integers. https://codereview.chromium.org/588153003/diff/20001/chrome/browser/content_s... File chrome/browser/content_settings/tab_specific_content_settings_unittest.cc (right): https://codereview.chromium.org/588153003/diff/20001/chrome/browser/content_s... chrome/browser/content_settings/tab_specific_content_settings_unittest.cc:229: ASSERT_EQ(static_cast<unsigned>( Note: Following my suggestions in the header should eliminate the need for these casts. https://codereview.chromium.org/588153003/diff/20001/chrome/browser/ui/conten... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/588153003/diff/20001/chrome/browser/ui/conten... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:648: NOTREACHED(); Nit: Better than this NOTREACHED() would be to eliminate this conditional arm and preface the function with DCHECK_NE(MICROPHONE_CAMERA_NOT_ACCESSED, state_);. https://codereview.chromium.org/588153003/diff/20001/chrome/browser/ui/conten... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:687: else if (is_cam) Nit: This conditional is always true, remove it. (3 places) https://codereview.chromium.org/588153003/diff/20001/chrome/browser/ui/conten... File chrome/browser/ui/content_settings/content_setting_image_model.cc (right): https://codereview.chromium.org/588153003/diff/20001/chrome/browser/ui/conten... chrome/browser/ui/content_settings/content_setting_image_model.cc:266: else if (is_cam) Nit: This conditional is always true. Here's a shorter way of doing this: int id = IDS_CAMERA_BLOCKED; if (is_mic) id = is_cam ? IDS_MICROPHONE_CAMERA_BLOCKED : IDS_MICROPHONE_BLOCKED; set_tooltip(l10n_util::GetStringUTF8(id)); (2 places)
https://codereview.chromium.org/588153003/diff/1/chrome/browser/content_setti... File chrome/browser/content_settings/tab_specific_content_settings.cc (right): https://codereview.chromium.org/588153003/diff/1/chrome/browser/content_setti... chrome/browser/content_settings/tab_specific_content_settings.cc:287: // setting to kick in without reloading the page, and the UI for media is On 2014/09/24 01:33:00, Peter Kasting wrote: > On 2014/09/24 00:03:04, robwu wrote: > > On 2014/09/22 23:11:22, Peter Kasting wrote: > > > Nit: setting -> settings > > > > > > "the UI for media" is a bit unclear as well. > > > > I have just deleted this comment since it is no longer relevant. > > It was added in https://codereview.chromium.org/13375004/patch/6001/7001. > > The comment is certainly not relevant to the remaining media setting > > (CONTENT_SETTINGS_TYPE_PROTECTED_MEDIA_IDENTIFIER) because there is no "reload > > page" UI for it (the setting was added in > > https://codereview.chromium.org/download/issue23531021_266001.diff) > > OK, but in that case, as a reader I'm still going to wonder why this particular > type is special and needs its own handling, so there needs to be some ind of > explanation here and below. According to IsContentAllowed, this field is only significant for a handful of types. CONTENT_SETTINGS_TYPE_PROTECTED_MEDIA_IDENTIFIER is not among them. content_allowed_[] is also used internally within TabSpecificContentSettings::OnContentAllowed. If it is false, then the flag is flipped to true and a change notification is triggered. By setting content_allowed_[] to true in OnContentBlocked, the notification is suppressed. HOWEVER, the OnContentAllowed method also has a special case for CONTENT_SETTINGS_TYPE_PROTECTED_MEDIA_IDENTIFIER, where access_blocked_[] is used to check whether the permissions has changed. I have clarified the latter in a comment in OnContentAllowed, and added a comment about the questionable use of content_allowed_. I don't want to make any more logical changes to this part of the code, because the change/bugfix is not related to this patch. If what I found turns out to be a real bug, then I will address that in a separate patch. https://codereview.chromium.org/588153003/diff/20001/chrome/browser/content_s... File chrome/browser/content_settings/tab_specific_content_settings.cc (right): https://codereview.chromium.org/588153003/diff/20001/chrome/browser/content_s... chrome/browser/content_settings/tab_specific_content_settings.cc:485: if (microphone_camera_state_ == MICROPHONE_CAMERA_NOT_ACCESSED) On 2014/09/24 01:33:00, Peter Kasting wrote: > Nit: This conditional isn't really needed. It is a small optimization to fail fast. The user is often not using the camera or microphone. By failing fast, we don't create unnecessary references to PrefService and MediaStreamCaptureIndicator. https://codereview.chromium.org/588153003/diff/20001/chrome/browser/content_s... chrome/browser/content_settings/tab_specific_content_settings.cc:546: microphone_camera_state_ |= MICROPHONE_ACCESSED; On 2014/09/24 01:33:00, Peter Kasting wrote: > Nit: Shorter: > > microphone_camera_state_ |= > MICROPHONE_ACCESSED | (mic_allowed ? 0 : MICROPHONE_BLOCKED); > > (2 places) Done. https://codereview.chromium.org/588153003/diff/20001/chrome/browser/content_s... File chrome/browser/content_settings/tab_specific_content_settings.h (right): https://codereview.chromium.org/588153003/diff/20001/chrome/browser/content_s... chrome/browser/content_settings/tab_specific_content_settings.h:50: enum MicrophoneCameraState { On 2014/09/24 01:33:00, Peter Kasting wrote: > Nit: Don't use an enum to declare bitmasks. Use constants instead, because this > is semantically no longer an enumeration. Done. https://codereview.chromium.org/588153003/diff/20001/chrome/browser/content_s... chrome/browser/content_settings/tab_specific_content_settings.h:223: unsigned GetMicrophoneCameraState() const; On 2014/09/24 01:33:00, Peter Kasting wrote: > (Here and elsewhere) Use types like uint32_t instead of "unsigned" for > bitfields, so it's clear how many bits are available for use. (Basically, > "unsigned" should never be used in our codebase.) > > You should also typedef this, so that it's clear to readers that the code in > question is dealing with these state values, not integers. Done. https://codereview.chromium.org/588153003/diff/20001/chrome/browser/content_s... File chrome/browser/content_settings/tab_specific_content_settings_unittest.cc (right): https://codereview.chromium.org/588153003/diff/20001/chrome/browser/content_s... chrome/browser/content_settings/tab_specific_content_settings_unittest.cc:229: ASSERT_EQ(static_cast<unsigned>( On 2014/09/24 01:33:00, Peter Kasting wrote: > Note: Following my suggestions in the header should eliminate the need for these > casts. Done. though I had to add "| 0" in 5 places though, to solve errors like tab_specific_content_settings_unittest.cc:216: error: undefined reference to 'TabSpecificContentSettings::MICROPHONE_ACCESSED' https://codereview.chromium.org/588153003/diff/20001/chrome/browser/ui/conten... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/588153003/diff/20001/chrome/browser/ui/conten... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:648: NOTREACHED(); On 2014/09/24 01:33:00, Peter Kasting wrote: > Nit: Better than this NOTREACHED() would be to eliminate this conditional arm > and preface the function with DCHECK_NE(MICROPHONE_CAMERA_NOT_ACCESSED, > state_);. Done. https://codereview.chromium.org/588153003/diff/20001/chrome/browser/ui/conten... File chrome/browser/ui/content_settings/content_setting_image_model.cc (right): https://codereview.chromium.org/588153003/diff/20001/chrome/browser/ui/conten... chrome/browser/ui/content_settings/content_setting_image_model.cc:266: else if (is_cam) On 2014/09/24 01:33:00, Peter Kasting wrote: > Nit: This conditional is always true. Here's a shorter way of doing this: > > int id = IDS_CAMERA_BLOCKED; > if (is_mic) > id = is_cam ? IDS_MICROPHONE_CAMERA_BLOCKED : IDS_MICROPHONE_BLOCKED; > set_tooltip(l10n_util::GetStringUTF8(id)); > > (2 places) Nice. Done.
LGTM https://codereview.chromium.org/588153003/diff/20001/chrome/browser/content_s... File chrome/browser/content_settings/tab_specific_content_settings.cc (right): https://codereview.chromium.org/588153003/diff/20001/chrome/browser/content_s... chrome/browser/content_settings/tab_specific_content_settings.cc:485: if (microphone_camera_state_ == MICROPHONE_CAMERA_NOT_ACCESSED) On 2014/09/24 23:38:44, robwu wrote: > On 2014/09/24 01:33:00, Peter Kasting wrote: > > Nit: This conditional isn't really needed. > > It is a small optimization to fail fast. The user is often not using the camera > or microphone. By failing fast, we don't create unnecessary references to > PrefService and MediaStreamCaptureIndicator. We should only worry about optimizing that if the performance impact of creating those is meaningful. I can't imagine it is. Otherwise, it's just more code to read, and weakly implies that the below code doesn't handle this case correctly when in fact it does. Ultimately up to you -- I won't demand you remove this -- but I think it's better without it. https://codereview.chromium.org/588153003/diff/20001/chrome/browser/content_s... File chrome/browser/content_settings/tab_specific_content_settings_unittest.cc (right): https://codereview.chromium.org/588153003/diff/20001/chrome/browser/content_s... chrome/browser/content_settings/tab_specific_content_settings_unittest.cc:229: ASSERT_EQ(static_cast<unsigned>( On 2014/09/24 23:38:44, robwu wrote: > On 2014/09/24 01:33:00, Peter Kasting wrote: > > Note: Following my suggestions in the header should eliminate the need for > these > > casts. > > Done. though I had to add "| 0" in 5 places though, to solve errors like > tab_specific_content_settings_unittest.cc:216: error: undefined reference to > 'TabSpecificContentSettings::MICROPHONE_ACCESSED' This means that your constants have a declaration, but not a definition. Instead of using | 0 in various places, add to the corresponding .cc file for these constants: STATIC_CONST_MEMBER_DEFINITION const Type Foo::val1; STATIC_CONST_MEMBER_DEFINITION const Type Foo::val2; ... etc. This provides an appropriate definition for these constants. This shouldn't really be needed, I don't think, unless the macros in question are actually taking the address of their arguments, but MSVC in particular sometimes has issues here. https://codereview.chromium.org/588153003/diff/20001/chrome/browser/ui/conten... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/588153003/diff/20001/chrome/browser/ui/conten... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:687: else if (is_cam) On 2014/09/24 01:33:00, Peter Kasting wrote: > Nit: This conditional is always true, remove it. (3 places) Still would be good to do this.
rob@robwu.nl changed reviewers: + bauerb@chromium.org
+bauerb for OWNERS review of chrome/browser/content_settings/tab_specific_content*.cc https://codereview.chromium.org/588153003/diff/20001/chrome/browser/content_s... File chrome/browser/content_settings/tab_specific_content_settings.cc (right): https://codereview.chromium.org/588153003/diff/20001/chrome/browser/content_s... chrome/browser/content_settings/tab_specific_content_settings.cc:485: if (microphone_camera_state_ == MICROPHONE_CAMERA_NOT_ACCESSED) On 2014/09/25 00:53:25, Peter Kasting wrote: > On 2014/09/24 23:38:44, robwu wrote: > > On 2014/09/24 01:33:00, Peter Kasting wrote: > > > Nit: This conditional isn't really needed. > > > > It is a small optimization to fail fast. The user is often not using the > camera > > or microphone. By failing fast, we don't create unnecessary references to > > PrefService and MediaStreamCaptureIndicator. > > We should only worry about optimizing that if the performance impact of creating > those is meaningful. I can't imagine it is. Otherwise, it's just more code to > read, and weakly implies that the below code doesn't handle this case correctly > when in fact it does. > > Ultimately up to you -- I won't demand you remove this -- but I think it's > better without it. All right, removed. https://codereview.chromium.org/588153003/diff/20001/chrome/browser/content_s... File chrome/browser/content_settings/tab_specific_content_settings_unittest.cc (right): https://codereview.chromium.org/588153003/diff/20001/chrome/browser/content_s... chrome/browser/content_settings/tab_specific_content_settings_unittest.cc:229: ASSERT_EQ(static_cast<unsigned>( On 2014/09/25 00:53:25, Peter Kasting wrote: > On 2014/09/24 23:38:44, robwu wrote: > > On 2014/09/24 01:33:00, Peter Kasting wrote: > > > Note: Following my suggestions in the header should eliminate the need for > > these > > > casts. > > > > Done. though I had to add "| 0" in 5 places though, to solve errors like > > tab_specific_content_settings_unittest.cc:216: error: undefined reference to > > 'TabSpecificContentSettings::MICROPHONE_ACCESSED' > > This means that your constants have a declaration, but not a definition. > > Instead of using | 0 in various places, add to the corresponding .cc file for > these constants: > > STATIC_CONST_MEMBER_DEFINITION const Type Foo::val1; > STATIC_CONST_MEMBER_DEFINITION const Type Foo::val2; > ... etc. > > This provides an appropriate definition for these constants. This shouldn't > really be needed, I don't think, unless the macros in question are actually > taking the address of their arguments, but MSVC in particular sometimes has > issues here. The error actually showed up in GCC 4.9.1. I've put the definitions in the .cc file as suggested instead of |0 and the linker errors went away. https://codereview.chromium.org/588153003/diff/20001/chrome/browser/ui/conten... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/588153003/diff/20001/chrome/browser/ui/conten... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:687: else if (is_cam) On 2014/09/25 00:53:25, Peter Kasting wrote: > On 2014/09/24 01:33:00, Peter Kasting wrote: > > Nit: This conditional is always true, remove it. (3 places) > > Still would be good to do this. Was already done.
https://codereview.chromium.org/588153003/diff/20001/chrome/browser/ui/conten... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/588153003/diff/20001/chrome/browser/ui/conten... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:687: else if (is_cam) On 2014/09/25 08:57:57, robwu wrote: > On 2014/09/25 00:53:25, Peter Kasting wrote: > > On 2014/09/24 01:33:00, Peter Kasting wrote: > > > Nit: This conditional is always true, remove it. (3 places) > > > > Still would be good to do this. > > Was already done. Eh? It's still not done even in your latest patch set.
https://codereview.chromium.org/588153003/diff/20001/chrome/browser/ui/conten... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/588153003/diff/20001/chrome/browser/ui/conten... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:687: else if (is_cam) On 2014/09/25 09:00:06, Peter Kasting wrote: > On 2014/09/25 08:57:57, robwu wrote: > > On 2014/09/25 00:53:25, Peter Kasting wrote: > > > On 2014/09/24 01:33:00, Peter Kasting wrote: > > > > Nit: This conditional is always true, remove it. (3 places) > > > > > > Still would be good to do this. > > > > Was already done. > > Eh? It's still not done even in your latest patch set. Apologies, I confused this one with your other comment in content_setting_image_model.cc. I've corrected this in the last patch set.
lgtm https://codereview.chromium.org/588153003/diff/80001/chrome/browser/content_s... File chrome/browser/content_settings/tab_specific_content_settings.cc (right): https://codereview.chromium.org/588153003/diff/80001/chrome/browser/content_s... chrome/browser/content_settings/tab_specific_content_settings.cc:539: unsigned prev_microphone_camera_state = microphone_camera_state_; Unsigned what? I thought you had the typedef MicrophoneCameraState for this. https://codereview.chromium.org/588153003/diff/80001/chrome/browser/content_s... chrome/browser/content_settings/tab_specific_content_settings.cc:554: const bool mic_allowed = Nit: We usually don't bother with making purely local variables const. https://codereview.chromium.org/588153003/diff/80001/chrome/browser/ui/conten... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/588153003/diff/80001/chrome/browser/ui/conten... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:704: selected_item_ = Indent this four spaces. https://codereview.chromium.org/588153003/diff/80001/chrome/browser/ui/conten... File chrome/browser/ui/content_settings/content_setting_image_model.cc (right): https://codereview.chromium.org/588153003/diff/80001/chrome/browser/ui/conten... chrome/browser/ui/content_settings/content_setting_image_model.cc:249: content_settings->GetMicrophoneCameraState(); Nit: remove the extra space here.
The CQ bit was checked by rob@robwu.nl
https://codereview.chromium.org/588153003/diff/80001/chrome/browser/content_s... File chrome/browser/content_settings/tab_specific_content_settings.cc (right): https://codereview.chromium.org/588153003/diff/80001/chrome/browser/content_s... chrome/browser/content_settings/tab_specific_content_settings.cc:539: unsigned prev_microphone_camera_state = microphone_camera_state_; On 2014/09/25 16:18:27, Bernhard Bauer wrote: > Unsigned what? I thought you had the typedef MicrophoneCameraState for this. Done. https://codereview.chromium.org/588153003/diff/80001/chrome/browser/content_s... chrome/browser/content_settings/tab_specific_content_settings.cc:554: const bool mic_allowed = On 2014/09/25 16:18:27, Bernhard Bauer wrote: > Nit: We usually don't bother with making purely local variables const. Done. https://codereview.chromium.org/588153003/diff/80001/chrome/browser/ui/conten... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/588153003/diff/80001/chrome/browser/ui/conten... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:704: selected_item_ = On 2014/09/25 16:18:27, Bernhard Bauer wrote: > Indent this four spaces. Fixed. https://codereview.chromium.org/588153003/diff/80001/chrome/browser/ui/conten... File chrome/browser/ui/content_settings/content_setting_image_model.cc (right): https://codereview.chromium.org/588153003/diff/80001/chrome/browser/ui/conten... chrome/browser/ui/content_settings/content_setting_image_model.cc:249: content_settings->GetMicrophoneCameraState(); On 2014/09/25 16:18:27, Bernhard Bauer wrote: > Nit: remove the extra space here. Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/588153003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
The CQ bit was checked by rob@robwu.nl
The CQ bit was unchecked by rob@robwu.nl
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/588153003/120001
The CQ bit was checked by rob@robwu.nl
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/588153003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/65790)
The CQ bit was checked by rob@robwu.nl
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/588153003/140001
The CQ bit was checked by rob@robwu.nl
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/588153003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by rob@robwu.nl
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/588153003/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as 5650f22797a286677ec472e4de42ded8f2d5e67f
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/1fb9a9bdf67911afcd8579c446daed6015a86103 Cr-Commit-Position: refs/heads/master@{#296913} |
