TabSpecificContentSettings no longer knows about MediaStreamDeviceController

TabSpecificContentSettings no longer knows about MediaStreamDeviceController

To prepare TSCS for componentising, this CL removes the knowledge about MSDS out of TSCS, by pulling out MSDS-specific processing from TSCS to MSDS.
Some tests had to be changed in a non-trivial way to preserve the coverage, in particular, some TSCS unit-tests needed to become MSDS browsertests (as wiring up everything needed for MSDS would be too messy in a unit test).

BUG=387075, 395060
Committed: https://crrev.com/b3a93a6794477a270de3eadccc7d12485649b861
Cr-Commit-Position: refs/heads/master@{#302068}

[vabr (Chromium), 2014-10-28 15:51:57]

Patchset #1 (id:1) has been deleted

[vabr (Chromium), 2014-10-28 19:14:54]

Patchset #1 (id:20001) has been deleted

[vabr (Chromium), 2014-10-28 19:22:33]

vabr@chromium.org changed reviewers:
+ bauerb@chromium.org, xians@chromium.org

[vabr (Chromium), 2014-10-28 19:22:33]

Bernhard,
Could you please review this CL, especially the content_settings parts?

Shijing,
Could you please have a look at chrome/browser/media?

Thanks!
Vaclav

[vabr (Chromium), 2014-10-28 20:48:43]

https://codereview.chromium.org/678023002/diff/60001/chrome/browser/content_s...
File chrome/browser/content_settings/tab_specific_content_settings.cc (right):

https://codereview.chromium.org/678023002/diff/60001/chrome/browser/content_s...
chrome/browser/content_settings/tab_specific_content_settings.cc:543: bool
mic_blocked = (new_microphone_camera_state & MICROPHONE_BLOCKED) != 0;
This != 0 might seem unnecessary, but without it, the Win compiler throws the
performance warning C4800 about forcing an int into bool, which breaks the
compilation.

[Bernhard Bauer, 2014-10-29 09:35:18]

lgtm

https://codereview.chromium.org/678023002/diff/60001/chrome/browser/content_s...
File chrome/browser/content_settings/tab_specific_content_settings.cc (right):

https://codereview.chromium.org/678023002/diff/60001/chrome/browser/content_s...
chrome/browser/content_settings/tab_specific_content_settings.cc:543: bool
mic_blocked = (new_microphone_camera_state & MICROPHONE_BLOCKED) != 0;
On 2014/10/28 20:48:43, vabr (Chromium) wrote:
> This != 0 might seem unnecessary, but without it, the Win compiler throws the
> performance warning C4800 about forcing an int into bool, which breaks the
> compilation.

Yeah, direct assignment can be tricky (I remember something about assignment to
a smaller type truncating everything but the least significant byte, which would
make a non-zero value where the last byte is zero be interpreted as false).
Alternatives are a static_cast<bool> (I think), or double negation (`!!`).

https://codereview.chromium.org/678023002/diff/60001/chrome/browser/media/med...
File chrome/browser/media/media_stream_devices_controller.cc (right):

https://codereview.chromium.org/678023002/diff/60001/chrome/browser/media/med...
chrome/browser/media/media_stream_devices_controller.cc:106:
MediaStreamDevicesController::MediaStreamTypeSettingsMap::const_iterator it =
This seems like a good place for auto :)

[no longer working on chromium, 2014-10-29 10:09:43]

some nits, looks a good refactoring to TabSpecificContentSettings.

https://codereview.chromium.org/678023002/diff/60001/chrome/browser/media/med...
File chrome/browser/media/media_stream_devices_controller.cc (right):

https://codereview.chromium.org/678023002/diff/60001/chrome/browser/media/med...
chrome/browser/media/media_stream_devices_controller.cc:105:
web_contents->GetBrowserContext())->GetPrefs();
indentation.

https://codereview.chromium.org/678023002/diff/60001/chrome/browser/media/med...
chrome/browser/media/media_stream_devices_controller.cc:110:
selected_audio_device =
I think the correct format should be 
selected_audio_device = requested_audio_device.empty() ?
    prefs->GetString(prefs::kDefaultAudioCaptureDevice) :
    requested_audio_device;

https://codereview.chromium.org/678023002/diff/60001/chrome/browser/media/med...
chrome/browser/media/media_stream_devices_controller.cc:125:
selected_video_device =
ditto

https://codereview.chromium.org/678023002/diff/60001/chrome/browser/media/med...
File chrome/browser/media/media_stream_devices_controller_browsertest.cc
(right):

https://codereview.chromium.org/678023002/diff/60001/chrome/browser/media/med...
chrome/browser/media/media_stream_devices_controller_browsertest.cc:1: //
Copyright (c) 2014 The Chromium Authors. All rights reserved.
nit, remove (c)

https://codereview.chromium.org/678023002/diff/60001/chrome/browser/media/med...
chrome/browser/media/media_stream_devices_controller_browsertest.cc:61:
GetWebContents()->GetBrowserContext())->GetPrefs();
nit, fix the indentation.

[no longer working on chromium, 2014-10-29 10:10:07]

lgtm after you address the nits.

[vabr (Chromium), 2014-10-29 10:30:23]

Thanks both of you for the review!

Shijing,
I pushed back on one of the formatting comments (lines with "PrefService*
prefs"), please see below and let me know what you think.

Cheers,
Vaclav

https://codereview.chromium.org/678023002/diff/60001/chrome/browser/content_s...
File chrome/browser/content_settings/tab_specific_content_settings.cc (right):

https://codereview.chromium.org/678023002/diff/60001/chrome/browser/content_s...
chrome/browser/content_settings/tab_specific_content_settings.cc:543: bool
mic_blocked = (new_microphone_camera_state & MICROPHONE_BLOCKED) != 0;
On 2014/10/29 09:35:17, Bernhard Bauer wrote:
> On 2014/10/28 20:48:43, vabr (Chromium) wrote:
> > This != 0 might seem unnecessary, but without it, the Win compiler throws
the
> > performance warning C4800 about forcing an int into bool, which breaks the
> > compilation.
> 
> Yeah, direct assignment can be tricky (I remember something about assignment
to
> a smaller type truncating everything but the least significant byte, which
would
> make a non-zero value where the last byte is zero be interpreted as false).
If that happens, that would be bad. The C4800 warning is called a "performance
warning" on the MS page http://msdn.microsoft.com/en-us/library/b6801kcy.aspx,
so I guess speed is the concern here rather than correctness of the implicit
cast.

> Alternatives are a static_cast<bool> (I think), or double negation (`!!`).
According to http://msdn.microsoft.com/en-us/library/b6801kcy.aspx, casting
should not work. Double negation probably should, but I (subjectively) prefer
the "!=0" suggested on the linked page, because "!!" are easy for me to confuse
with "!".

https://codereview.chromium.org/678023002/diff/60001/chrome/browser/media/med...
File chrome/browser/media/media_stream_devices_controller.cc (right):

https://codereview.chromium.org/678023002/diff/60001/chrome/browser/media/med...
chrome/browser/media/media_stream_devices_controller.cc:105:
web_contents->GetBrowserContext())->GetPrefs();
On 2014/10/29 10:09:42, xians1 wrote:
> indentation.

I believe this is indented correctly.
See "Arguments may optionally all be placed on subsequent lines with a four
space indent" in
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Calls.

Also, this is the result of "git cl format".

https://codereview.chromium.org/678023002/diff/60001/chrome/browser/media/med...
chrome/browser/media/media_stream_devices_controller.cc:106:
MediaStreamDevicesController::MediaStreamTypeSettingsMap::const_iterator it =
On 2014/10/29 09:35:17, Bernhard Bauer wrote:
> This seems like a good place for auto :)

Auto-awesome! :)

Done.

https://codereview.chromium.org/678023002/diff/60001/chrome/browser/media/med...
chrome/browser/media/media_stream_devices_controller.cc:110:
selected_audio_device =
On 2014/10/29 10:09:42, xians1 wrote:
> I think the correct format should be 
> selected_audio_device = requested_audio_device.empty() ?
>     prefs->GetString(prefs::kDefaultAudioCaptureDevice) :
>     requested_audio_device;
> 

I'm yielding, because of consistency with the rest of the file.
But please note that clang-format will keep trying to change that, during all
future refactorings involving automated formatting.

Since the style guide does not seem to indicate what is the preferred way of
wrapping for the ternary operator (while it says the binary operators should go
before line breaks), it might be worth considering sticking to the clang-format
to keep indenting to automated tools.

Done.

https://codereview.chromium.org/678023002/diff/60001/chrome/browser/media/med...
chrome/browser/media/media_stream_devices_controller.cc:125:
selected_video_device =
On 2014/10/29 10:09:42, xians1 wrote:
> ditto

Done.

https://codereview.chromium.org/678023002/diff/60001/chrome/browser/media/med...
File chrome/browser/media/media_stream_devices_controller_browsertest.cc
(right):

https://codereview.chromium.org/678023002/diff/60001/chrome/browser/media/med...
chrome/browser/media/media_stream_devices_controller_browsertest.cc:1: //
Copyright (c) 2014 The Chromium Authors. All rights reserved.
On 2014/10/29 10:09:42, xians1 wrote:
> nit, remove (c)

Done.

https://codereview.chromium.org/678023002/diff/60001/chrome/browser/media/med...
chrome/browser/media/media_stream_devices_controller_browsertest.cc:61:
GetWebContents()->GetBrowserContext())->GetPrefs();
On 2014/10/29 10:09:42, xians1 wrote:
> nit, fix the indentation.

Please see my comment at the similar place in
media_stream_devices_controller.cc.

[Bernhard Bauer, 2014-10-29 13:56:59]

https://codereview.chromium.org/678023002/diff/60001/chrome/browser/content_s...
File chrome/browser/content_settings/tab_specific_content_settings.cc (right):

https://codereview.chromium.org/678023002/diff/60001/chrome/browser/content_s...
chrome/browser/content_settings/tab_specific_content_settings.cc:543: bool
mic_blocked = (new_microphone_camera_state & MICROPHONE_BLOCKED) != 0;
On 2014/10/29 10:30:21, vabr (Chromium) wrote:
> On 2014/10/29 09:35:17, Bernhard Bauer wrote:
> > On 2014/10/28 20:48:43, vabr (Chromium) wrote:
> > > This != 0 might seem unnecessary, but without it, the Win compiler throws
> the
> > > performance warning C4800 about forcing an int into bool, which breaks the
> > > compilation.
> > 
> > Yeah, direct assignment can be tricky (I remember something about assignment
> to
> > a smaller type truncating everything but the least significant byte, which
> would
> > make a non-zero value where the last byte is zero be interpreted as false).
> If that happens, that would be bad. The C4800 warning is called a "performance
> warning" on the MS page http://msdn.microsoft.com/en-us/library/b6801kcy.aspx,
> so I guess speed is the concern here rather than correctness of the implicit
> cast.
> 
> > Alternatives are a static_cast<bool> (I think), or double negation (`!!`).
> According to http://msdn.microsoft.com/en-us/library/b6801kcy.aspx, casting
> should not work. Double negation probably should, but I (subjectively) prefer
> the "!=0" suggested on the linked page, because "!!" are easy for me to
confuse
> with "!".

TBH, the whole thing seems a bit weird. It's supposedly about the additional
code for the implicit cast, but there are other implicit conversions that have a
much bigger overhead (like the implicit string constructor for a const char*, in
particular for an empty string), and for which the compiler doesn't warn. And
also, if you add an explicit cast, there shouldn't be a reason to warn about
implicitly generated code, as you are explicitly stating that you want to
convert the value to a bool. But I guess that's MSVS for you...

[vabr (Chromium), 2014-10-30 09:37:19]

Shijing,
If you find time to consider my reply to the only style comment I pushed back
against, let me know. Otherwise I'll land tonight, and will be happy to do a
follow-up CL to address any further comments you might have.

Bernhard,
Yes, I also don't see the point with the C4800 warning and resistance against
explicit casts. At least it definitely makes their help site popular. :)

Cheers,
Vaclav

[no longer working on chromium, 2014-10-30 09:59:19]

HI vabr, please see reply in line. They are nits, so I don't have strong opinion
at any of them, you can make the final call on which format you want.

https://codereview.chromium.org/678023002/diff/60001/chrome/browser/media/med...
File chrome/browser/media/media_stream_devices_controller.cc (right):

https://codereview.chromium.org/678023002/diff/60001/chrome/browser/media/med...
chrome/browser/media/media_stream_devices_controller.cc:105:
web_contents->GetBrowserContext())->GetPrefs();
On 2014/10/29 10:30:21, vabr (Chromium) wrote:
> On 2014/10/29 10:09:42, xians1 wrote:
> > indentation.
> 
> I believe this is indented correctly.
> See "Arguments may optionally all be placed on subsequent lines with a four
> space indent" in
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Calls.
> 
> Also, this is the result of "git cl format".

In some cases git cl format is not inline with the chromium style, though most
of them are nits.
The existing chromium code usually goes for four indentation in the new line,
you can find lots of examples, ex:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/his...

As they are nits, probably they don't matter in most of the cases. But I have to
point out that the script is not that smart, I have been running into cases that
the script is wrong. The idea is that we would like to use one style instead of
two. That says, I would be happy to adapt to clang-format when there is a
refactoring doing automated formatting on the existing code.

[vabr (Chromium), 2014-10-30 10:28:40]

Thank you, Shijing!

Given your response, I followed your guidance for the remaining two places as
well, with a verbose comment :).

Since all is addressed now, I'll land this CL.

Thank you both for your reviews.
Vaclav

https://codereview.chromium.org/678023002/diff/60001/chrome/browser/media/med...
File chrome/browser/media/media_stream_devices_controller.cc (right):

https://codereview.chromium.org/678023002/diff/60001/chrome/browser/media/med...
chrome/browser/media/media_stream_devices_controller.cc:105:
web_contents->GetBrowserContext())->GetPrefs();
On 2014/10/30 09:59:19, xians1 wrote:
> On 2014/10/29 10:30:21, vabr (Chromium) wrote:
> > On 2014/10/29 10:09:42, xians1 wrote:
> > > indentation.
> > 
> > I believe this is indented correctly.
> > See "Arguments may optionally all be placed on subsequent lines with a four
> > space indent" in
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Calls.
> > 
> > Also, this is the result of "git cl format".
> 
> In some cases git cl format is not inline with the chromium style, though most
> of them are nits.

My point here was that this particular formatting was inline with chromium
style, as documented by the link in my original comment.

> The existing chromium code usually goes for four indentation in the new line,
> you can find lots of examples, ex:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/his...

This is true, and because of the "be consistent" clause, and out of respect to
OWNERS view :), I followed your advice here.
Done.

> 
> As they are nits, probably they don't matter in most of the cases. But I have
to
> point out that the script is not that smart, I have been running into cases
that
> the script is wrong. The idea is that we would like to use one style instead
of
> two.

Exactly! :) And because the style guide is often left intentionally relaxed,
permitting different styles, it would be convenient to have a canonical format
provided by an automated tool, as long as the tool does not violate the style
guide.

BTW., if you see the script violate the style guide, that's a bug and should be
reported.

> That says, I would be happy to adapt to clang-format when there is a
> refactoring doing automated formatting on the existing code.

This sounds very reasonable. Until then, I'm happy to stick to the consistency
rule.

[vabr (Chromium), 2014-10-30 10:28:59]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2014-10-30 10:29:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/678023002/120001

[commit-bot: I haz the power, 2014-10-30 11:14:24]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2014-10-30 11:14:58]

Patchset 5 (id:??) landed as
https://crrev.com/b3a93a6794477a270de3eadccc7d12485649b861
Cr-Commit-Position: refs/heads/master@{#302068}