Fix the links in the media bubble pointing to the media UI.

Fix the links in the media bubble pointing to the media UI.

This bug is a result of the fact that the media settings bubble still uses the deprecated MEDIASTREAM content setting. It will need refactoring. But as a quick remedy, we will make the bubble point to camera exceptions, microphone exceptions, or both; depending on whether the website accessed camera, microphone, or both.

BUG=515376, 509962
Committed: https://crrev.com/b5e676b98055f298c6787bc49ef46fcd9eba9570
Cr-Commit-Position: refs/heads/master@{#341510}

[msramek, 2015-07-30 15:55:58]

msramek@chromium.org changed reviewers:
+ msw@chromium.org

[msramek, 2015-07-30 15:55:58]

Hi Mike,

could you PTAL at this?

Context: We split the media section in chrome://settings/content to two separate
sections for microphone and camera. However, a webpage can request mic and
camera permissions together, and then we show one bubble for both settings. This
mismatch led to the bug
https://code.google.com/p/chromium/issues/detail?id=515376

The bubble should be eventually refactored as we try to completely remove the
MEDIASTREAM content setting, but I want to make sure that this isn't a release
blocker first.

Thanks,
Martin

[msramek, 2015-07-30 16:45:49]

msramek@chromium.org changed reviewers:
+ bauerb@chromium.org

[msramek, 2015-07-30 16:45:49]

+Bernhard PTAL as well for resources/options.

https://codereview.chromium.org/1266583003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/options/content_settings.html (right):

https://codereview.chromium.org/1266583003/diff/20001/chrome/browser/resource...
chrome/browser/resources/options/content_settings.html:397: <section
id="media-stream-mic">
Currently, we just point to the microphone section of default settings, so
perhaps adding an ID just here would be enough. But we could also add IDs to all
the sections (it's not used in the code; but I like to be able to point to a
specific point in the settings with a link).

[msw, 2015-07-30 17:15:27]

Please include 509962 in your BUG= here. Just minor nits.

https://codereview.chromium.org/1266583003/diff/20001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_content_setting_bubble_model_delegate.cc (right):

https://codereview.chromium.org/1266583003/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser_content_setting_bubble_model_delegate.cc:50: // and
microphone, point them to the default settings instead
nit: "instead of"?

https://codereview.chromium.org/1266583003/diff/20001/chrome/browser/ui/chrom...
File chrome/browser/ui/chrome_pages.cc (right):

https://codereview.chromium.org/1266583003/diff/20001/chrome/browser/ui/chrom...
chrome/browser/ui/chrome_pages.cc:298: void ShowContentSettings(Browser*
browser,
It might make more sense to rename this ShowContentSettingsExceptions and just
name the below ShowContentSettings, but you don't need to do that [in this CL].

https://codereview.chromium.org/1266583003/diff/20001/chrome/browser/ui/conte...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

https://codereview.chromium.org/1266583003/diff/20001/chrome/browser/ui/conte...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:717: bool
is_mic = (state_ & TabSpecificContentSettings::MICROPHONE_ACCESSED) != 0;
nit: These could be helper functions used instead of repeated lengthy checks.

https://codereview.chromium.org/1266583003/diff/20001/chrome/browser/ui/conte...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:880: // to
change the text if only one media type was accessed.
nit: s/text/link/

https://codereview.chromium.org/1266583003/diff/20001/chrome/browser/ui/conte...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:887: ?
l10n_util::GetStringUTF8(
nit: put the trinary within l10n_util::GetStringUTF8(

[Bernhard Bauer, 2015-07-31 08:16:32]

LGTM with Mike's comments addressed.

[msramek, 2015-07-31 12:34:52]

https://codereview.chromium.org/1266583003/diff/20001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_content_setting_bubble_model_delegate.cc (right):

https://codereview.chromium.org/1266583003/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser_content_setting_bubble_model_delegate.cc:50: // and
microphone, point them to the default settings instead
On 2015/07/30 17:15:26, msw wrote:
> nit: "instead of"?

Done.

https://codereview.chromium.org/1266583003/diff/20001/chrome/browser/ui/chrom...
File chrome/browser/ui/chrome_pages.cc (right):

https://codereview.chromium.org/1266583003/diff/20001/chrome/browser/ui/chrom...
chrome/browser/ui/chrome_pages.cc:298: void ShowContentSettings(Browser*
browser,
On 2015/07/30 17:15:27, msw wrote:
> It might make more sense to rename this ShowContentSettingsExceptions and just
> name the below ShowContentSettings, but you don't need to do that [in this
CL].

Exactly. I just didn't want to touch all the callsites right now. I'll do that
in a separate CL.

https://codereview.chromium.org/1266583003/diff/20001/chrome/browser/ui/conte...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

https://codereview.chromium.org/1266583003/diff/20001/chrome/browser/ui/conte...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:717: bool
is_mic = (state_ & TabSpecificContentSettings::MICROPHONE_ACCESSED) != 0;
On 2015/07/30 17:15:27, msw wrote:
> nit: These could be helper functions used instead of repeated lengthy checks.

That's one of the things I wanted to leave for that future refactoring :) I
replaced the occurrences with helper functions, but now it's a bit inconsistent
that we check which devices were accessed using the helper functions, but which
ones were blocked using the mask.

I think I'll take the same approach to refactoring this as in
ContentSettingsHandler::MediaSettingsInfo. But let me do that in a follow-up CL
:)

https://codereview.chromium.org/1266583003/diff/20001/chrome/browser/ui/conte...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:880: // to
change the text if only one media type was accessed.
On 2015/07/30 17:15:27, msw wrote:
> nit: s/text/link/

Actually, I wanted to emphasize that here we're changing the text of the link,
as opposed to the internal url it points to.

https://codereview.chromium.org/1266583003/diff/20001/chrome/browser/ui/conte...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:887: ?
l10n_util::GetStringUTF8(
On 2015/07/30 17:15:27, msw wrote:
> nit: put the trinary within l10n_util::GetStringUTF8(

Done.

[msramek, 2015-07-31 12:36:29]

On 2015/07/30 17:15:27, msw wrote:
> Please include 509962 in your BUG= here. Just minor nits.
> 

Done.

[msramek, 2015-07-31 16:05:00]

https://codereview.chromium.org/1266583003/diff/20001/chrome/browser/ui/chrom...
File chrome/browser/ui/chrome_pages.cc (right):

https://codereview.chromium.org/1266583003/diff/20001/chrome/browser/ui/chrom...
chrome/browser/ui/chrome_pages.cc:298: void ShowContentSettings(Browser*
browser,
On 2015/07/31 12:34:52, msramek wrote:
> On 2015/07/30 17:15:27, msw wrote:
> > It might make more sense to rename this ShowContentSettingsExceptions and
just
> > name the below ShowContentSettings, but you don't need to do that [in this
> CL].
> 
> Exactly. I just didn't want to touch all the callsites right now. I'll do that
> in a separate CL.

Oh wait, that was the only callsite. I must have accidentally checked that for
some other function. So let's do it right now.

[msw, 2015-07-31 17:36:05]

lgtm with const helper function nit

https://codereview.chromium.org/1266583003/diff/20001/chrome/browser/ui/conte...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

https://codereview.chromium.org/1266583003/diff/20001/chrome/browser/ui/conte...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:880: // to
change the text if only one media type was accessed.
On 2015/07/31 12:34:52, msramek wrote:
> On 2015/07/30 17:15:27, msw wrote:
> > nit: s/text/link/
> 
> Actually, I wanted to emphasize that here we're changing the text of the link,
> as opposed to the internal url it points to.

Acknowledged.

https://codereview.chromium.org/1266583003/diff/60001/chrome/browser/ui/conte...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

https://codereview.chromium.org/1266583003/diff/60001/chrome/browser/ui/conte...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:586: bool
MicrophoneAccessed();
nit const here and below.

[msramek, 2015-08-03 08:43:04]

Thanks!

https://codereview.chromium.org/1266583003/diff/60001/chrome/browser/ui/conte...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

https://codereview.chromium.org/1266583003/diff/60001/chrome/browser/ui/conte...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:586: bool
MicrophoneAccessed();
On 2015/07/31 17:36:04, msw wrote:
> nit const here and below.

Done.

[msramek, 2015-08-03 08:43:09]

The CQ bit was checked by msramek@chromium.org

[msramek, 2015-08-03 08:43:09]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
msw@chromium.org
Link to the patchset: https://codereview.chromium.org/1266583003/#ps70001
(title: "Added 'const' to helper functions.")

[commit-bot: I haz the power, 2015-08-03 08:43:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1266583003/70001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1266583003/70001

[commit-bot: I haz the power, 2015-08-03 08:55:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-03 08:55:24]

Try jobs failed on following builders:
  linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msramek, 2015-08-03 09:06:54]

The CQ bit was checked by msramek@chromium.org

[msramek, 2015-08-03 09:06:55]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
msw@chromium.org
Link to the patchset: https://codereview.chromium.org/1266583003/#ps90001
(title: "The definitions too.")

[commit-bot: I haz the power, 2015-08-03 09:07:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1266583003/90001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1266583003/90001

[commit-bot: I haz the power, 2015-08-03 11:11:54]

Committed patchset #6 (id:90001)

[commit-bot: I haz the power, 2015-08-03 11:12:27]

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