Implement CancelUserMediaRequest for the browser part

Implement CancelUserMediaRequest for the browser part

BUG=126081
TEST=content_unittests, manual test


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

[Jeremy Mao, 2012-06-05 09:41:33]

Hi scherkus and wjia,

Please take a look at this when you are free, this patch will remove the pending
ui request when the user reload current page.

Thanks & Best regards
Jeremy

[wjia(left Chromium), 2012-06-06 06:18:21]

Thanks for the patch!

http://codereview.chromium.org/10534004/diff/1/content/browser/renderer_host/...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

http://codereview.chromium.org/10534004/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/media/media_stream_manager.cc:217: void
MediaStreamManager::CancelGeneratedStream(const std::string& label) {
This would be MediaStreamManager::CancelGenerateStream.

Since this function could be called at any time during a stream is generated and
there are multiple steps to generate a stream, it would be great to take care
all cases. For example, this function is called when user has granted access
permission and device managers are trying to open devices. This patch takes care
of the case in which user has not granted access permission yet.

http://codereview.chromium.org/10534004/diff/1/content/renderer/media/media_s...
File content/renderer/media/media_stream_impl.cc (right):

http://codereview.chromium.org/10534004/diff/1/content/renderer/media/media_s...
content/renderer/media/media_stream_impl.cc:265: if (it ==
user_media_requests_.end()) {
Since the request could be asynchronously canceled, it's needed to call
media_stream_dispatcher_->StopStream(label) here to avoid memory/device leak.

http://codereview.chromium.org/10534004/diff/1/content/renderer/media/media_s...
content/renderer/media/media_stream_impl.cc:360:
WebKit::WebNavigationTypeReload) {
Should this be done for all navigation types?

[tommi (sloooow) - chröme, 2012-06-06 09:29:58]

lgtm with wjia's comments addressed.

http://codereview.chromium.org/10534004/diff/6002/content/browser/renderer_ho...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

http://codereview.chromium.org/10534004/diff/6002/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:230: if (it->in_use
== true) {
if (it->in_use) {

http://codereview.chromium.org/10534004/diff/6002/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:240: if (it->in_use
== true) {
ditto

[Jeremy Mao, 2012-06-06 09:33:10]

On 2012/06/06 06:18:21, wjia wrote:
> Thanks for the patch!
> 
>
http://codereview.chromium.org/10534004/diff/1/content/browser/renderer_host/...
> File content/browser/renderer_host/media/media_stream_manager.cc (right):
> 
>
http://codereview.chromium.org/10534004/diff/1/content/browser/renderer_host/...
> content/browser/renderer_host/media/media_stream_manager.cc:217: void
> MediaStreamManager::CancelGeneratedStream(const std::string& label) {
> This would be MediaStreamManager::CancelGenerateStream.
Done.
> 
> Since this function could be called at any time during a stream is generated
and
> there are multiple steps to generate a stream, it would be great to take care
> all cases. For example, this function is called when user has granted access
> permission and device managers are trying to open devices. This patch takes
care
> of the case in which user has not granted access permission yet.
Done.
> 
>
http://codereview.chromium.org/10534004/diff/1/content/renderer/media/media_s...
> File content/renderer/media/media_stream_impl.cc (right):
> 
>
http://codereview.chromium.org/10534004/diff/1/content/renderer/media/media_s...
> content/renderer/media/media_stream_impl.cc:265: if (it ==
> user_media_requests_.end()) {
> Since the request could be asynchronously canceled, it's needed to call
> media_stream_dispatcher_->StopStream(label) here to avoid memory/device leak.
Done.
> 
>
http://codereview.chromium.org/10534004/diff/1/content/renderer/media/media_s...
> content/renderer/media/media_stream_impl.cc:360:
> WebKit::WebNavigationTypeReload) {
> Should this be done for all navigation types?
Done.

[Jeremy Mao, 2012-06-06 09:35:49]

On 2012/06/06 09:29:58, tommi wrote:
> lgtm with wjia's comments addressed.
> 
>
http://codereview.chromium.org/10534004/diff/6002/content/browser/renderer_ho...
> File content/browser/renderer_host/media/media_stream_manager.cc (right):
> 
>
http://codereview.chromium.org/10534004/diff/6002/content/browser/renderer_ho...
> content/browser/renderer_host/media/media_stream_manager.cc:230: if
(it->in_use
> == true) {
> if (it->in_use) {
Done.
> 
>
http://codereview.chromium.org/10534004/diff/6002/content/browser/renderer_ho...
> content/browser/renderer_host/media/media_stream_manager.cc:240: if
(it->in_use
> == true) {
> ditto
Done.

[commit-bot: I haz the power, 2012-06-06 09:39:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yujie.mao@intel.com/10534004/4002

[commit-bot: I haz the power, 2012-06-06 10:47:06]

Change committed as 140738

[wjia(left Chromium), 2012-06-06 13:40:24]

You can do a follow up patch.

http://codereview.chromium.org/10534004/diff/4002/content/browser/renderer_ho...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

http://codereview.chromium.org/10534004/diff/4002/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:230: if (it->in_use)
{
This check is not needed. |in_use| is a signal for "opened". We have to call
"Close()" as long as we have called "Open()".

The |state| would be per device. But currently it's per device type. That's a
different problem.

http://codereview.chromium.org/10534004/diff/4002/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:240: if (it->in_use)
{
ditto.

[Jeremy Mao, 2012-06-07 03:40:17]

Thanks for making it clear, I will upload another patch to fix this issue.
On 2012/06/06 13:40:24, wjia wrote:
> You can do a follow up patch. 
> 
>
http://codereview.chromium.org/10534004/diff/4002/content/browser/renderer_ho...
> File content/browser/renderer_host/media/media_stream_manager.cc (right):
> 
>
http://codereview.chromium.org/10534004/diff/4002/content/browser/renderer_ho...
> content/browser/renderer_host/media/media_stream_manager.cc:230: if
(it->in_use)
> {
> This check is not needed. |in_use| is a signal for "opened". We have to call
> "Close()" as long as we have called "Open()".
> 
> The |state| would be per device. But currently it's per device type. That's a
> different problem.
> 
>
http://codereview.chromium.org/10534004/diff/4002/content/browser/renderer_ho...
> content/browser/renderer_host/media/media_stream_manager.cc:240: if
(it->in_use)
> {
> ditto.