Switch media stream permissions to use IsOriginSecure() instead of SchemeIsSecure().

Switch media stream permissions to use IsOriginSecure() instead of
SchemeIsSecure().

In particular, note that this means that media permissions are persisted on http://localhost.

The change also updates the old media stream permission tests (which
were no longer testing anything meaningful).

BUG=295723, 362214, 509844
Committed: https://crrev.com/2249c80e8c4b50793a8261c4f9eb3f879ddf4aff
Cr-Commit-Position: refs/heads/master@{#342458}

[lgarron, 2015-05-08 22:16:59]

lgarron@chromium.org changed reviewers:
+ tommi@chromium.org

[lgarron, 2015-05-08 22:17:00]

tommi@: These changes are the same ones from
https://codereview.chromium.org/1099453005/ moved into their own CL. Would you
mind reviewing here?

[tommi (sloooow) - chröme, 2015-05-10 05:54:56]

The CQ bit was checked by tommi@chromium.org

[tommi (sloooow) - chröme, 2015-05-10 05:54:57]

lgtm

[commit-bot: I haz the power, 2015-05-10 05:56:18]

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

[commit-bot: I haz the power, 2015-05-10 08:25:42]

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

[commit-bot: I haz the power, 2015-05-10 08:25:43]

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

[lgarron, 2015-05-11 19:53:25]

The CQ bit was checked by lgarron@chromium.org

[lgarron, 2015-05-11 19:53:26]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org
Link to the patchset: https://codereview.chromium.org/1132203002/#ps20001
(title: "Add explicit content:: namespace due to rebase.")

[commit-bot: I haz the power, 2015-05-11 19:54:37]

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

[commit-bot: I haz the power, 2015-05-11 20:01:29]

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

[commit-bot: I haz the power, 2015-05-11 20:01:31]

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_...)
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lgarron, 2015-05-11 22:50:08]

The CQ bit was checked by lgarron@chromium.org

[lgarron, 2015-05-11 22:50:08]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org
Link to the patchset: https://codereview.chromium.org/1132203002/#ps40001
(title: "Add explicit content:: namespace due to rebasing.")

[lgarron, 2015-05-11 22:51:50]

The CQ bit was unchecked by lgarron@chromium.org

[lgarron, 2015-05-11 22:51:51]

The CQ bit was checked by lgarron@chromium.org

[commit-bot: I haz the power, 2015-05-11 22:53:24]

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

[commit-bot: I haz the power, 2015-05-12 00:18:00]

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

[commit-bot: I haz the power, 2015-05-12 00:18:02]

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

[lgarron, 2015-05-12 01:42:08]

The CQ bit was checked by lgarron@chromium.org

[commit-bot: I haz the power, 2015-05-12 01:43:01]

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

[commit-bot: I haz the power, 2015-05-12 02:55:05]

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

[commit-bot: I haz the power, 2015-05-12 02:55:07]

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

[lgarron, 2015-05-12 18:40:50]

The CQ bit was checked by lgarron@chromium.org

[lgarron, 2015-05-12 18:41:07]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2015-05-12 18:41:37]

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

[commit-bot: I haz the power, 2015-05-12 20:00:41]

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

[commit-bot: I haz the power, 2015-05-12 20:00:43]

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

[lgarron, 2015-05-14 19:08:44]

The CQ bit was checked by lgarron@chromium.org

[lgarron, 2015-05-14 19:08:47]

The CQ bit was checked by lgarron@chromium.org

[commit-bot: I haz the power, 2015-05-14 19:09:35]

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

[commit-bot: I haz the power, 2015-05-14 20:23:34]

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

[commit-bot: I haz the power, 2015-05-14 20:23:35]

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

[lgarron, 2015-05-15 00:16:06]

On 2015/05/14 at 20:23:35, commit-bot wrote:
> Try jobs failed on following builders:
>   linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

Three tests fail on the bots:

MediaStreamInfoBarTest.TestAcceptIsNotSticky
MediaStreamInfoBarTest.TestAcceptThenDenyWhichShouldBeSticky
WebRtcBrowserTest.RunsAudioVideoWebRTCCallInTwoTabs

After looking at them closely[1], I've found that all three of the tests need to
be updated. They all make an undocumented assumption that the origin used for
testing (http://127.0.0.1:{port} [2]) is not considered secure, and therefore
does not have a sticky "granted" permission.

In more detail: The tests have helper methods GetUserMedia(),
GetUserMediaAndAccept(), and GetUserMediaAndDeny(). The latter two wait for an
infobar before taking an action (when they reach GetUserMediaAndWaitForInfoBar()
in GetUserMediaWithSpecificConstraintsAndDeny()[3]), so they hang if the
permission is granted or denied without an infobar. The permission is
granted/denied without an infobar, which happens if a previous decision for the
permission on the same origin was "sticky". The permission is sticky for
"granted" on secure origins and "denied" on all origins. One change introduced
with this CL is that the default test server (localhost) is considered secure,
so that "granted" is now sticky in the tests.

MediaStreamInfoBarTest.TestAcceptIsNotSticky calls GetUserMediaAndAccept(),
expects it not to be sticky, and calls it again.
MediaStreamInfoBarTest.TestAcceptThenDenyWhichShouldBeSticky calls
GetUserMediaAndAccept(), expects it not to be sticky, and calls
GetUserMediaAndDeny().
WebRtcBrowserTest.RunsAudioVideoWebRTCCallInTwoTabs indirectly does the same as
GetUserMediaAndAccept().

The MediaStreamInfoBarTest tests seem to be written under the assumption that
they are testing non-secure origins. I think the most reasonable thing to do is
preserve the tests, and make them work on a non-secure origin. This is a bit
tricky, because you can't just change the default local test server not to be
localhost (I tried changing it from 127.0.0.1 to 129.0.0.1 just in case the test
harness could handle it, but that definitely didn't work).
meacer@ suggested using a mock host resolver (similar to
LoginPromptBrowserTest.BlockCrossdomainPromptForSubresources) to point a dummy
domain to localhost (which it seems won't cause IsLocalhost() to return true[5])
for this test; I'm trying that, but can't even get a hacky version to work – and
I'm still not sure where I'd introduce a way to set a dummy domain for the test
server, because it's also instantiated a few levels of abstraction away.

WebRtcBrowserTest.RunsAudioVideoWebRTCCallInTwoTabs should probably use a
version of WebRtcTestBase::OpenPageAndGetUserMediaInNewTab() [6] that doesn't
wait for an infobar. This is annoying because the part that waits for an infobar
(WebRtcTestBase::GetUserMediaWithSpecificConstraintsAndAccept() [7]) is three
helper methods deep. Options I see are:
1. Introduce an alternate chain that doesn't wait for an infobar.
2. Introduce flags along the chain telling it not to wait for an infobar.
3. Audit everything using the core helper method[7], and see if we can modify
the method not to wait for an infobar if there won't be one.

Any thoughts?

[1] And after realizing that the tests were actually failing, which wasn't
obvious to me on OSX 10.10, because these tests don't work at all – hence the
many CQ attempts.
[2]
https://chromium.googlesource.com/chromium/src/+/d6e2fd12f9d16a6913f129dce5eb...
[3]
https://chromium.googlesource.com/chromium/src/+/d6e2fd12f9d16a6913f129dce5eb...
[4]
https://chromium.googlesource.com/chromium/src/+/d6e2fd12f9d16a6913f129dce5eb...
[5]
https://chromium.googlesource.com/chromium/src/+/d6e2fd12f9d16a6913f129dce5eb...
[6]
https://chromium.googlesource.com/chromium/src/+/d6e2fd12f9d16a6913f129dce5eb...
[7]
https://chromium.googlesource.com/chromium/src/+/d6e2fd12f9d16a6913f129dce5eb...

[lgarron, 2015-07-28 18:32:41]

On 2015/05/15 at 00:16:06, lgarron wrote:
> On 2015/05/14 at 20:23:35, commit-bot wrote:
> > Try jobs failed on following builders:
> >   linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> 
> Three tests fail on the bots:
> 
> MediaStreamInfoBarTest.TestAcceptIsNotSticky
> MediaStreamInfoBarTest.TestAcceptThenDenyWhichShouldBeSticky
> WebRtcBrowserTest.RunsAudioVideoWebRTCCallInTwoTabs
> 
> After looking at them closely[1], I've found that all three of the tests need
to be updated. They all make an undocumented assumption that the origin used for
testing (http://127.0.0.1:{port} [2]) is not considered secure, and therefore
does not have a sticky "granted" permission.
> 
> In more detail: The tests have helper methods GetUserMedia(),
GetUserMediaAndAccept(), and GetUserMediaAndDeny(). The latter two wait for an
infobar before taking an action (when they reach GetUserMediaAndWaitForInfoBar()
in GetUserMediaWithSpecificConstraintsAndDeny()[3]), so they hang if the
permission is granted or denied without an infobar. The permission is
granted/denied without an infobar, which happens if a previous decision for the
permission on the same origin was "sticky". The permission is sticky for
"granted" on secure origins and "denied" on all origins. One change introduced
with this CL is that the default test server (localhost) is considered secure,
so that "granted" is now sticky in the tests.
> 
> MediaStreamInfoBarTest.TestAcceptIsNotSticky calls GetUserMediaAndAccept(),
expects it not to be sticky, and calls it again.
> MediaStreamInfoBarTest.TestAcceptThenDenyWhichShouldBeSticky calls
GetUserMediaAndAccept(), expects it not to be sticky, and calls
GetUserMediaAndDeny().
> WebRtcBrowserTest.RunsAudioVideoWebRTCCallInTwoTabs indirectly does the same
as GetUserMediaAndAccept().
> 
> The MediaStreamInfoBarTest tests seem to be written under the assumption that
they are testing non-secure origins. I think the most reasonable thing to do is
preserve the tests, and make them work on a non-secure origin. This is a bit
tricky, because you can't just change the default local test server not to be
localhost (I tried changing it from 127.0.0.1 to 129.0.0.1 just in case the test
harness could handle it, but that definitely didn't work).
> meacer@ suggested using a mock host resolver (similar to
LoginPromptBrowserTest.BlockCrossdomainPromptForSubresources) to point a dummy
domain to localhost (which it seems won't cause IsLocalhost() to return true[5])
for this test; I'm trying that, but can't even get a hacky version to work – and
I'm still not sure where I'd introduce a way to set a dummy domain for the test
server, because it's also instantiated a few levels of abstraction away.
> 
> WebRtcBrowserTest.RunsAudioVideoWebRTCCallInTwoTabs should probably use a
version of WebRtcTestBase::OpenPageAndGetUserMediaInNewTab() [6] that doesn't
wait for an infobar. This is annoying because the part that waits for an infobar
(WebRtcTestBase::GetUserMediaWithSpecificConstraintsAndAccept() [7]) is three
helper methods deep. Options I see are:
> 1. Introduce an alternate chain that doesn't wait for an infobar.
> 2. Introduce flags along the chain telling it not to wait for an infobar.
> 3. Audit everything using the core helper method[7], and see if we can modify
the method not to wait for an infobar if there won't be one.
> 
> Any thoughts?
> 
> [1] And after realizing that the tests were actually failing, which wasn't
obvious to me on OSX 10.10, because these tests don't work at all – hence the
many CQ attempts.
> [2]
https://chromium.googlesource.com/chromium/src/+/d6e2fd12f9d16a6913f129dce5eb...
> [3]
https://chromium.googlesource.com/chromium/src/+/d6e2fd12f9d16a6913f129dce5eb...
> [4]
https://chromium.googlesource.com/chromium/src/+/d6e2fd12f9d16a6913f129dce5eb...
> [5]
https://chromium.googlesource.com/chromium/src/+/d6e2fd12f9d16a6913f129dce5eb...
> [6]
https://chromium.googlesource.com/chromium/src/+/d6e2fd12f9d16a6913f129dce5eb...
> [7]
https://chromium.googlesource.com/chromium/src/+/d6e2fd12f9d16a6913f129dce5eb...

Ping. :-)

[lgarron, 2015-07-28 18:33:05]

On 2015/07/28 at 18:32:41, lgarron wrote:
> On 2015/05/15 at 00:16:06, lgarron wrote:
> > On 2015/05/14 at 20:23:35, commit-bot wrote:
> > > Try jobs failed on following builders:
> > >   linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> > 
> > Three tests fail on the bots:
> > 
> > MediaStreamInfoBarTest.TestAcceptIsNotSticky
> > MediaStreamInfoBarTest.TestAcceptThenDenyWhichShouldBeSticky
> > WebRtcBrowserTest.RunsAudioVideoWebRTCCallInTwoTabs
> > 
> > After looking at them closely[1], I've found that all three of the tests
need to be updated. They all make an undocumented assumption that the origin
used for testing (http://127.0.0.1:{port} [2]) is not considered secure, and
therefore does not have a sticky "granted" permission.
> > 
> > In more detail: The tests have helper methods GetUserMedia(),
GetUserMediaAndAccept(), and GetUserMediaAndDeny(). The latter two wait for an
infobar before taking an action (when they reach GetUserMediaAndWaitForInfoBar()
in GetUserMediaWithSpecificConstraintsAndDeny()[3]), so they hang if the
permission is granted or denied without an infobar. The permission is
granted/denied without an infobar, which happens if a previous decision for the
permission on the same origin was "sticky". The permission is sticky for
"granted" on secure origins and "denied" on all origins. One change introduced
with this CL is that the default test server (localhost) is considered secure,
so that "granted" is now sticky in the tests.
> > 
> > MediaStreamInfoBarTest.TestAcceptIsNotSticky calls GetUserMediaAndAccept(),
expects it not to be sticky, and calls it again.
> > MediaStreamInfoBarTest.TestAcceptThenDenyWhichShouldBeSticky calls
GetUserMediaAndAccept(), expects it not to be sticky, and calls
GetUserMediaAndDeny().
> > WebRtcBrowserTest.RunsAudioVideoWebRTCCallInTwoTabs indirectly does the same
as GetUserMediaAndAccept().
> > 
> > The MediaStreamInfoBarTest tests seem to be written under the assumption
that they are testing non-secure origins. I think the most reasonable thing to
do is preserve the tests, and make them work on a non-secure origin. This is a
bit tricky, because you can't just change the default local test server not to
be localhost (I tried changing it from 127.0.0.1 to 129.0.0.1 just in case the
test harness could handle it, but that definitely didn't work).
> > meacer@ suggested using a mock host resolver (similar to
LoginPromptBrowserTest.BlockCrossdomainPromptForSubresources) to point a dummy
domain to localhost (which it seems won't cause IsLocalhost() to return true[5])
for this test; I'm trying that, but can't even get a hacky version to work – and
I'm still not sure where I'd introduce a way to set a dummy domain for the test
server, because it's also instantiated a few levels of abstraction away.
> > 
> > WebRtcBrowserTest.RunsAudioVideoWebRTCCallInTwoTabs should probably use a
version of WebRtcTestBase::OpenPageAndGetUserMediaInNewTab() [6] that doesn't
wait for an infobar. This is annoying because the part that waits for an infobar
(WebRtcTestBase::GetUserMediaWithSpecificConstraintsAndAccept() [7]) is three
helper methods deep. Options I see are:
> > 1. Introduce an alternate chain that doesn't wait for an infobar.
> > 2. Introduce flags along the chain telling it not to wait for an infobar.
> > 3. Audit everything using the core helper method[7], and see if we can
modify the method not to wait for an infobar if there won't be one.
> > 
> > Any thoughts?
> > 
> > [1] And after realizing that the tests were actually failing, which wasn't
obvious to me on OSX 10.10, because these tests don't work at all – hence the
many CQ attempts.
> > [2]
https://chromium.googlesource.com/chromium/src/+/d6e2fd12f9d16a6913f129dce5eb...
> > [3]
https://chromium.googlesource.com/chromium/src/+/d6e2fd12f9d16a6913f129dce5eb...
> > [4]
https://chromium.googlesource.com/chromium/src/+/d6e2fd12f9d16a6913f129dce5eb...
> > [5]
https://chromium.googlesource.com/chromium/src/+/d6e2fd12f9d16a6913f129dce5eb...
> > [6]
https://chromium.googlesource.com/chromium/src/+/d6e2fd12f9d16a6913f129dce5eb...
> > [7]
https://chromium.googlesource.com/chromium/src/+/d6e2fd12f9d16a6913f129dce5eb...
> 
> Ping. :-)

Sorry, wrong bug. I'll have another patch up for this soon, though. :-)

[lgarron, 2015-08-04 01:22:38]

It's been a long delay (because I was prioritizing my DevTools work over this),
but I've finally finished rewriting the media stream permission tests in a way
I'm happy with.

tommi@: Could you take a look again?
felt@, raymes@: Would you mind sanity checking the updated tests?

[tommi (sloooow) - chröme, 2015-08-04 09:09:26]

lgtm

https://codereview.chromium.org/1132203002/diff/80001/chrome/browser/media/me...
File chrome/browser/media/media_stream_devices_controller.cc (right):

https://codereview.chromium.org/1132203002/diff/80001/chrome/browser/media/me...
chrome/browser/media/media_stream_devices_controller.cc:445:
IsUserAcceptAllowed(content_type))
nit: use {}

[felt, 2015-08-04 13:42:25]

felt@chromium.org changed reviewers:
+ felt@chromium.org

[felt, 2015-08-04 13:42:26]

https://codereview.chromium.org/1132203002/diff/100001/chrome/browser/media/c...
File chrome/browser/media/chrome_media_stream_infobar_browsertest.cc (right):

https://codereview.chromium.org/1132203002/diff/100001/chrome/browser/media/c...
chrome/browser/media/chrome_media_stream_infobar_browsertest.cc:165:
EXPECT_TRUE(GetUserMediaAndAccept(tab_contents, EXPECT_PROMPT_NOT_SHOWN));
This seems weird. The name of the method is GetUserMediaAndAccept, but you're
expecting that the prompt won't be shown, so there is nothing to accept? Could
you make a new method instead for the cases where a prompt is not supposed to be
shown?

[lgarron, 2015-08-04 18:02:41]

https://codereview.chromium.org/1132203002/diff/100001/chrome/browser/media/c...
File chrome/browser/media/chrome_media_stream_infobar_browsertest.cc (right):

https://codereview.chromium.org/1132203002/diff/100001/chrome/browser/media/c...
chrome/browser/media/chrome_media_stream_infobar_browsertest.cc:165:
EXPECT_TRUE(GetUserMediaAndAccept(tab_contents, EXPECT_PROMPT_NOT_SHOWN));
On 2015/08/04 at 13:42:26, felt wrote:
> This seems weird. The name of the method is GetUserMediaAndAccept, but you're
expecting that the prompt won't be shown, so there is nothing to accept? Could
you make a new method instead for the cases where a prompt is not supposed to be
shown?

Hmm, I didn't want to create 2 (or 3) methods for all the existing
GetUserMedia[WithSpecificConstrains][AndAcccept/AndDeny] helper methods. How
about calling them something like TryGetUserMedia...()?

[lgarron, 2015-08-04 20:15:44]

https://codereview.chromium.org/1132203002/diff/100001/chrome/browser/media/c...
File chrome/browser/media/chrome_media_stream_infobar_browsertest.cc (right):

https://codereview.chromium.org/1132203002/diff/100001/chrome/browser/media/c...
chrome/browser/media/chrome_media_stream_infobar_browsertest.cc:165:
EXPECT_TRUE(GetUserMediaAndAccept(tab_contents, EXPECT_PROMPT_NOT_SHOWN));
On 2015/08/04 at 18:02:41, lgarron wrote:
> On 2015/08/04 at 13:42:26, felt wrote:
> > This seems weird. The name of the method is GetUserMediaAndAccept, but
you're expecting that the prompt won't be shown, so there is nothing to accept?
Could you make a new method instead for the cases where a prompt is not supposed
to be shown?
> 
> Hmm, I didn't want to create 2 (or 3) methods for all the existing
GetUserMedia[WithSpecificConstrains][AndAcccept/AndDeny] helper methods. How
about calling them something like TryGetUserMedia...()?

After poking around a bit, I found that just adding two
GetUserMediaAndExpectAuto{Accept/Deny}WithoutPrompt() methods and doing the
prompt expectation checks in each helper method works quite well. Thanks for
pointing out the weirdness. :-)

[raymes, 2015-08-05 02:31:04]

raymes@chromium.org changed reviewers:
+ raymes@chromium.org

[raymes, 2015-08-05 02:31:05]

lgtm

https://codereview.chromium.org/1132203002/diff/180001/chrome/browser/media/c...
File chrome/browser/media/chrome_media_stream_infobar_browsertest.cc (right):

https://codereview.chromium.org/1132203002/diff/180001/chrome/browser/media/c...
chrome/browser/media/chrome_media_stream_infobar_browsertest.cc:36: //
https://www.chromium.org/Home/chromium-security/prefer-secure-origins-for-pow...
nit: should this be broken across lines to fit in 80chars?

https://codereview.chromium.org/1132203002/diff/180001/chrome/browser/media/m...
File chrome/browser/media/media_stream_device_permissions.cc (right):

https://codereview.chromium.org/1132203002/diff/180001/chrome/browser/media/m...
chrome/browser/media/media_stream_device_permissions.cc:57: if
(content::IsOriginSecure(origin))
You might get a merge conflict (or not) as I recently added an additional check
below this line:
  if (origin.SchemeIs(extensions::kExtensionScheme))
    return true;

I guess we can remove that check now?

[felt, 2015-08-06 18:40:13]

lgtm % nit (and make sure to fix raymes's comment about rebasing, too)

https://codereview.chromium.org/1132203002/diff/180001/chrome/browser/media/w...
File chrome/browser/media/webrtc_browsertest_base.cc (right):

https://codereview.chromium.org/1132203002/diff/180001/chrome/browser/media/w...
chrome/browser/media/webrtc_browsertest_base.cc:188: void
WebRtcTestBase::GetUserMediaAndExpectAutoAcceptWithoutPrompt(
This is kind of confusing, because you don't actually expect any prompt to
appear in the normal case yet are setting an autoresponder. Can you update the
comment to specify that the only reason for the PBM here is to document that it
does *not* appear?

[lgarron, 2015-08-06 22:10:09]

https://codereview.chromium.org/1132203002/diff/180001/chrome/browser/media/c...
File chrome/browser/media/chrome_media_stream_infobar_browsertest.cc (right):

https://codereview.chromium.org/1132203002/diff/180001/chrome/browser/media/c...
chrome/browser/media/chrome_media_stream_infobar_browsertest.cc:36: //
https://www.chromium.org/Home/chromium-security/prefer-secure-origins-for-pow...
On 2015/08/05 at 02:31:05, raymes wrote:
> nit: should this be broken across lines to fit in 80chars?

Splitting a URL breaks highlighting/copying/auto-linking. I've sidestepped the
point by using a shorter link straight to the part of the spec that talks about
localhost.

https://codereview.chromium.org/1132203002/diff/180001/chrome/browser/media/m...
File chrome/browser/media/media_stream_device_permissions.cc (right):

https://codereview.chromium.org/1132203002/diff/180001/chrome/browser/media/m...
chrome/browser/media/media_stream_device_permissions.cc:57: if
(content::IsOriginSecure(origin))
On 2015/08/05 at 02:31:05, raymes wrote:
> You might get a merge conflict (or not) as I recently added an additional
check below this line:
>   if (origin.SchemeIs(extensions::kExtensionScheme))
>     return true;
> 
> I guess we can remove that check now?

Done.

https://codereview.chromium.org/1132203002/diff/180001/chrome/browser/media/w...
File chrome/browser/media/webrtc_browsertest_base.cc (right):

https://codereview.chromium.org/1132203002/diff/180001/chrome/browser/media/w...
chrome/browser/media/webrtc_browsertest_base.cc:188: void
WebRtcTestBase::GetUserMediaAndExpectAutoAcceptWithoutPrompt(
On 2015/08/06 at 18:40:13, felt wrote:
> This is kind of confusing, because you don't actually expect any prompt to
appear in the normal case yet are setting an autoresponder. Can you update the
comment to specify that the only reason for the PBM here is to document that it
does *not* appear?

Done.

[lgarron, 2015-08-06 22:11:01]

The CQ bit was checked by lgarron@chromium.org

[lgarron, 2015-08-06 22:11:02]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org,
felt@chromium.org, raymes@chromium.org
Link to the patchset: https://codereview.chromium.org/1132203002/#ps200001
(title: "Address LGTM nits.")

[commit-bot: I haz the power, 2015-08-06 22:12:17]

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

[commit-bot: I haz the power, 2015-08-06 22:55:38]

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

[commit-bot: I haz the power, 2015-08-06 22:55:39]

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

[lgarron, 2015-08-06 23:06:25]

The CQ bit was checked by lgarron@chromium.org

[lgarron, 2015-08-06 23:06:25]

The patchset sent to the CQ was uploaded after l-g-t-m from felt@chromium.org,
raymes@chromium.org, tommi@chromium.org
Link to the patchset: https://codereview.chromium.org/1132203002/#ps220001
(title: "Fix incorrect test code that I accidentally committed.")

[commit-bot: I haz the power, 2015-08-06 23:07:14]

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

[commit-bot: I haz the power, 2015-08-07 01:20:06]

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

[commit-bot: I haz the power, 2015-08-07 01:20:08]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[lgarron, 2015-08-07 20:26:27]

The CQ bit was checked by lgarron@chromium.org

[commit-bot: I haz the power, 2015-08-07 20:27:18]

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

[commit-bot: I haz the power, 2015-08-07 22:18:42]

Committed patchset #11 (id:220001)

[commit-bot: I haz the power, 2015-08-07 22:19:26]

Patchset 11 (id:??) landed as
https://crrev.com/2249c80e8c4b50793a8261c4f9eb3f879ddf4aff
Cr-Commit-Position: refs/heads/master@{#342458}

[magjed_chromium, 2015-08-09 15:19:40]

A revert of this CL (patchset #11 id:220001) has been created in
https://codereview.chromium.org/1276373004/ by magjed@chromium.org.

The reason for reverting is: This CL broke
WebRtcWebcamBrowserTest.TestAcquiringAndReacquiringWebcam on all platforms,
failing on the introduced assert
'EXPECT_TRUE(permissionRequestObserver.request_shown());’ at
webrtc_browsertest_base.cc:149.

Link to bots:
https://build.chromium.org/p/chromium.webrtc/builders/Win7%20Tester
https://build.chromium.org/p/chromium.webrtc/builders/Linux%20Tester
https://build.chromium.org/p/chromium.webrtc/builders/Mac%20Tester

Stdio output:
[ RUN      ]
WebRtcWebcamBrowserTests/WebRtcWebcamBrowserTest.TestAcquiringAndReacquiringWebcam/0
[7391:1799:0807/160702:WARNING:vt_video_decode_accelerator.cc(194)] Failed to
initialize VideoToolbox framework. Hardware accelerated video decoding will be
disabled.
[7388:1799:0807/160703:INFO:CONSOLE(71)] "This appears to be Chrome", source:
http://127.0.0.1:50232/webrtc/adapter.js (71)
[7388:71959:0807/160703:WARNING:embedded_test_server.cc(258)] Request not
handled. Returning 404: /favicon.ico
[7388:1799:0807/160703:INFO:CONSOLE(13)] "Returning has-video-input-device to
test.", source: http://127.0.0.1:50232/webrtc/test_functions.js (13)
[7388:1799:0807/160703:INFO:CONSOLE(13)] "Requesting doGetUserMedia:
constraints:
{"video":{"mandatory":{"minWidth":640,"maxWidth":640,"minHeight":480,"maxHeight":480}}}",
source: http://127.0.0.1:50232/webrtc/test_functions.js (13)
[7388:1799:0807/160703:INFO:CONSOLE(13)] "Returning request-callback-granted to
test.", source: http://127.0.0.1:50232/webrtc/test_functions.js (13)
[7388:1799:0807/160703:INFO:CONSOLE(13)] "Returning ok-got-stream to test.",
source: http://127.0.0.1:50232/webrtc/test_functions.js (13)
[7388:1799:0807/160703:INFO:CONSOLE(13)] "Returning ok-started to test.",
source: http://127.0.0.1:50232/webrtc/test_functions.js (13)
[7388:1799:0807/160703:INFO:CONSOLE(13)] "Returning video-not-playing to test.",
source: http://127.0.0.1:50232/webrtc/test_functions.js (13)
[7388:1799:0807/160703:INFO:CONSOLE(13)] "Returning video-playing to test.",
source: http://127.0.0.1:50232/webrtc/test_functions.js (13)
[7388:1799:0807/160703:INFO:CONSOLE(13)] "Returning ok-640x480 to test.",
source: http://127.0.0.1:50232/webrtc/test_functions.js (13)
[7388:1799:0807/160704:INFO:CONSOLE(13)] "Returning ok-stopped to test.",
source: http://127.0.0.1:50232/webrtc/test_functions.js (13)
[7388:1799:0807/160704:INFO:CONSOLE(13)] "Requesting doGetUserMedia:
constraints:
{"video":{"mandatory":{"minWidth":640,"maxWidth":640,"minHeight":360,"maxHeight":360}}}",
source: http://127.0.0.1:50232/webrtc/test_functions.js (13)
[7388:1799:0807/160704:INFO:CONSOLE(13)] "Returning request-callback-granted to
test.", source: http://127.0.0.1:50232/webrtc/test_functions.js (13)
../../chrome/browser/media/webrtc_browsertest_base.cc:149: Failure
Value of: permissionRequestObserver.request_shown()
  Actual: false
Expected: true
[7388:1799:0807/160704:INFO:CONSOLE(13)] "Returning ok-got-stream to test.",
source: http://127.0.0.1:50232/webrtc/test_functions.js (13)
[7388:1799:0807/160704:INFO:CONSOLE(13)] "Returning ok-started to test.",
source: http://127.0.0.1:50232/webrtc/test_functions.js (13)
[7388:1799:0807/160704:INFO:CONSOLE(13)] "Returning video-not-playing to test.",
source: http://127.0.0.1:50232/webrtc/test_functions.js (13)
[7388:1799:0807/160704:INFO:CONSOLE(13)] "Returning video-playing to test.",
source: http://127.0.0.1:50232/webrtc/test_functions.js (13)
[7388:1799:0807/160704:INFO:CONSOLE(13)] "Returning ok-640x360 to test.",
source: http://127.0.0.1:50232/webrtc/test_functions.js (13)
[7388:1799:0807/160704:INFO:CONSOLE(13)] "Returning ok-stopped to test.",
source: http://127.0.0.1:50232/webrtc/test_functions.js (13)
[  FAILED  ]
WebRtcWebcamBrowserTests/WebRtcWebcamBrowserTest.TestAcquiringAndReacquiringWebcam/0,
where GetParam() = "force-qtkit" (2127 ms).

[lgarron, 2015-08-13 05:55:12]

Patchset #12 (id:240001) has been deleted