Fix for closing the desktop sharing notification bar when the shared window is closed

Fix for closing the desktop sharing notification bar when the shared window is closed.
Previously MediaStreamManager is not notified when a video capturing device has stopped due to error (e.g. the shared window is closed), so the notification UI is still shown when the stream already stopped.
This change fixes it by populating the state to MediaStreamManager through VideoCaptureHost --> VidoeCaptureManager --> MediaStreamProviderListner.

BUG=360181
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267045

[jiayl, 2014-04-22 23:29:34]

PTAL. I'll add unit tests if the design looks good in general.

[perkj_chrome, 2014-04-23 11:30:42]

Adding Miguel since he has been working most on this lately.
Can you take a look?

For example, when a USB audio or video device is removed- the device monitor
notifies the MediaStreamManager that do a new enumeration of available devices.
Can screen share do the same thing?

https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/media_stream_provider.h (right):

https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_provider.h:55: virtual void
Aborted(MediaStreamType stream_type, int capture_session_id) = 0;
Why not use Closed?

[jiayl, 2014-04-23 15:42:55]

On 2014/04/23 11:30:42, perkj wrote:
> Adding Miguel since he has been working most on this lately.
> Can you take a look?
> 
> For example, when a USB audio or video device is removed- the device monitor
> notifies the MediaStreamManager that do a new enumeration of available
devices.
> Can screen share do the same thing?
> 

The existing way to report an error from desktop capture device is is well
defined, i.e. VideoCaptureDevice::Client::OnError. 
I don't think it's favorable to add another monitoring mechanism.

>
https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
> File content/browser/renderer_host/media/media_stream_provider.h (right):
> 
>
https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
> content/browser/renderer_host/media/media_stream_provider.h:55: virtual void
> Aborted(MediaStreamType stream_type, int capture_session_id) = 0;
> Why not use Closed?

[jiayl, 2014-04-23 15:43:06]

https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/media_stream_provider.h (right):

https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_provider.h:55: virtual void
Aborted(MediaStreamType stream_type, int capture_session_id) = 0;
On 2014/04/23 11:30:43, perkj wrote:
> Why not use Closed? 

Closed is called when a MediaStreamProvider::Close call is completed, not
suitable for error report.

[mcasas, 2014-04-24 07:43:23]

On 2014/04/23 15:43:06, jiayl wrote:
>
https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
> File content/browser/renderer_host/media/media_stream_provider.h (right):
> 
>
https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
> content/browser/renderer_host/media/media_stream_provider.h:55: virtual void
> Aborted(MediaStreamType stream_type, int capture_session_id) = 0;
> On 2014/04/23 11:30:43, perkj wrote:
> > Why not use Closed? 
> 
> Closed is called when a MediaStreamProvider::Close call is completed, not
> suitable for error report.


Correct typos in CL description (i.e. s/notificaiton/notification/g).

DeviceMonitorMac registers as a Mac-observer of
device notification changes (connected, disconnected 
and suspended). When a change is observed, DMM 
triggers SystemMonitor [1]. MediaStreamManager is
registered with it as a listener of such SM notifications
on IO thread [2].


[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/de...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

[perkj_chrome, 2014-04-24 10:00:57]

ok- In that case the code seems ok to me but Mcasas, please help review.

jiayl, will you add a test?

https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.h:160: OVERRIDE;
nit: Don't break line just before OVERRIDE.

[mcasas, 2014-04-24 11:09:46]

Please fix CL description typos.

https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:1629: }
Is not common in this file, but should we add NOTREACHED() here? I.e. an
Aborted() request on nonexistent |capture_session_id|, is it valid (tearing
down infra f.i.) or is a problem?

https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.h:160: OVERRIDE;
I feel undecided if Aborted() is a good name for the window being closed. When
the window is closed, is this a normal circumstance? Then Aborted() sounds
harsh, but is not an Error(), right? What about stream is Finished(),
Stopped()...?

https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/media_stream_provider.h (right):

https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_provider.h:55: virtual void
Aborted(MediaStreamType stream_type, int capture_session_id) = 0;
On 2014/04/23 15:43:06, jiayl wrote:
> On 2014/04/23 11:30:43, perkj wrote:
> > Why not use Closed? 
> 
> Closed is called when a MediaStreamProvider::Close call is completed, not
> suitable for error report.

See other comments on naming.

https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/video_capture_host.cc (right):

https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_host.cc:180: 
All platforms/implementations will basically pass by a cycle of
AbortCaptureForDeviceError() before doing the expected 
StopCaptureForClient().

We could instead add a param to VideoCaptureDevice::Client::OnError()
to distinguish... ?

https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_host.cc:190: }
This is just a copy-modify-1-word of DeleteVideoCaptureControllerOnIOThread()
l.355, please refactor those two pieces of code.

https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_manager.cc:350:
std::map<media::VideoCaptureSessionId, MediaStreamDevice>::iterator it;
nit: This data type std::map<media::VideoCaptureSessionId, MediaStreamDevice>
is broadly used in VCM, what about a private typedef for it? I 
understand it'd cover other parts of the code, too.

https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_manager.cc:356: 
nit: no blank line?

https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_manager.cc:357:
listener_->Aborted(it->second.type, it->first);
What happens if it->second.{id,type} has not been found?

https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_manager.cc:359:
StopCaptureForClient(controller, client_id, client_handler);
StopCaptureForClient() has overlapping boilerplate code, i.e. DCHECKs
and GetDeviceEntryForController()+check. Can we not reorganise it
somehow?

https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/video_capture_manager.h (right):

https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_manager.h:88: // happened . If
this is the last client of the device, the |controller| and
No blank before '.'

[jiayl, 2014-04-24 18:11:13]

comments addressed and a test added. PTAL.

https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:1629: }
On 2014/04/24 11:09:47, mcasas wrote:
> Is not common in this file, but should we add NOTREACHED() here? I.e. an
> Aborted() request on nonexistent |capture_session_id|, is it valid (tearing
> down infra f.i.) or is a problem?

Done.

https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.h:160: OVERRIDE;
Closing the window is not the expected way to stop the capturing. The expected
ways are (1) click the "stop sharing" button in the notification UI (2) call
stream.stop() from JS.
Plus, closing the window is only one scenario that triggers the OnError callback
of VideoCaptureController::Client. For example, it can be triggered when
capturing certain application window on Mac. 
So I think Aborted is better. It also clearly indicates that the capturing has
been aborted, making it more explicit than "Error" (e.g. the impl may choose to
ignore the error).

On 2014/04/24 11:09:47, mcasas wrote:
> I feel undecided if Aborted() is a good name for the window being closed. When
> the window is closed, is this a normal circumstance? Then Aborted() sounds
> harsh, but is not an Error(), right? What about stream is Finished(),
> Stopped()...?

https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/media_stream_provider.h (right):

https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_provider.h:55: virtual void
Aborted(MediaStreamType stream_type, int capture_session_id) = 0;
On 2014/04/24 11:09:47, mcasas wrote:
> On 2014/04/23 15:43:06, jiayl wrote:
> > On 2014/04/23 11:30:43, perkj wrote:
> > > Why not use Closed? 
> > 
> > Closed is called when a MediaStreamProvider::Close call is completed, not
> > suitable for error report.
> 
> See other comments on naming.

Done.

https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/video_capture_host.cc (right):

https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_host.cc:180: 
Not sure I understand you. Does the refactored code still have the problem?
Could you elaborate?

On 2014/04/24 11:09:47, mcasas wrote:
> All platforms/implementations will basically pass by a cycle of
> AbortCaptureForDeviceError() before doing the expected 
> StopCaptureForClient().
> 
> We could instead add a param to VideoCaptureDevice::Client::OnError()
> to distinguish... ?

https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_host.cc:190: }
On 2014/04/24 11:09:47, mcasas wrote:
> This is just a copy-modify-1-word of DeleteVideoCaptureControllerOnIOThread()
> l.355, please refactor those two pieces of code.

Done.

https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_manager.cc:350:
std::map<media::VideoCaptureSessionId, MediaStreamDevice>::iterator it;
On 2014/04/24 11:09:47, mcasas wrote:
> nit: This data type std::map<media::VideoCaptureSessionId, MediaStreamDevice>
> is broadly used in VCM, what about a private typedef for it? I 
> understand it'd cover other parts of the code, too.

Done.

https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_manager.cc:356: 
On 2014/04/24 11:09:47, mcasas wrote:
> nit: no blank line?

Done.

https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_manager.cc:357:
listener_->Aborted(it->second.type, it->first);
On 2014/04/24 11:09:47, mcasas wrote:
> What happens if it->second.{id,type} has not been found?

Done.

https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_manager.cc:359:
StopCaptureForClient(controller, client_id, client_handler);
On 2014/04/24 11:09:47, mcasas wrote:
> StopCaptureForClient() has overlapping boilerplate code, i.e. DCHECKs
> and GetDeviceEntryForController()+check. Can we not reorganise it
> somehow?

Done.

https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/video_capture_manager.h (right):

https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_manager.h:88: // happened . If
this is the last client of the device, the |controller| and
On 2014/04/24 11:09:47, mcasas wrote:
> No blank before '.'

Done.

[perkj_chrome, 2014-04-25 14:36:45]

lgtm if the below is addressed. 

If you can, please wait for mcasas review.

https://codereview.chromium.org/248113003/diff/20001/content/browser/media/ca...
File content/browser/media/capture/desktop_capture_device_uma_types.h (right):

https://codereview.chromium.org/248113003/diff/20001/content/browser/media/ca...
content/browser/media/capture/desktop_capture_device_uma_types.h:12: //
DESKTOP_CAPTURE_COUNTER_BOUNDARY.
This does not seem to belong to this cl.

https://codereview.chromium.org/248113003/diff/20001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://codereview.chromium.org/248113003/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.h:160: int
capture_session_id) OVERRIDE;
nit: indentation

https://codereview.chromium.org/248113003/diff/20001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_manager.h (right):

https://codereview.chromium.org/248113003/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager.h:153: void
StopCaptureForClientInternal(
This method doesnt seem to exist?

[mcasas, 2014-04-25 17:18:35]

Looks better, please check in particular media_stream_manager.cc:1616

https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/video_capture_host.cc (right):

https://codereview.chromium.org/248113003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_host.cc:180: 
I think is gone.

On 2014/04/24 18:11:13, jiayl wrote:
> Not sure I understand you. Does the refactored code still have the problem?
> Could you elaborate?
> 
> On 2014/04/24 11:09:47, mcasas wrote:
> > All platforms/implementations will basically pass by a cycle of
> > AbortCaptureForDeviceError() before doing the expected 
> > StopCaptureForClient().
> > 
> > We could instead add a param to VideoCaptureDevice::Client::OnError()
> > to distinguish... ?
>

https://codereview.chromium.org/248113003/diff/20001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/248113003/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:1616: for
(DeviceRequests::iterator request_it = requests_.begin();
Why not calling directly StopDevice() ? Seems that you're walking the request
list and the devices associated just to call that method, where the same double
search pattern exists.

https://codereview.chromium.org/248113003/diff/20001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_host.cc (right):

https://codereview.chromium.org/248113003/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_host.cc:180: 
ultra-nit: Remove empty line.

https://codereview.chromium.org/248113003/diff/20001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_host.h (right):

https://codereview.chromium.org/248113003/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_host.h:174: // Delete the
controller and notifies the VideoCaptureManager. |on_error| is
s/Delete/Deletes/

"comments should be descriptive ("Opens the file") rather than imperative ("Open
the file");"

Same in l.169 etc.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

[jiayl, 2014-04-28 18:40:08]

All comments resolved. PTAL.

[mcasas, 2014-04-29 09:51:06]

lgtm

[jiayl, 2014-04-29 15:54:24]

The CQ bit was checked by jiayl@chromium.org

[commit-bot: I haz the power, 2014-04-29 15:54:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jiayl@chromium.org/248113003/40001

[jiayl, 2014-04-29 15:54:52]

The CQ bit was unchecked by jiayl@chromium.org

[jiayl, 2014-04-29 15:54:57]

The CQ bit was checked by jiayl@chromium.org

[commit-bot: I haz the power, 2014-04-29 15:55:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jiayl@chromium.org/248113003/40001

[commit-bot: I haz the power, 2014-04-29 17:54:13]

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

[commit-bot: I haz the power, 2014-04-29 17:54:14]

Try jobs failed on following builders:
  mac_chromium_rel on tryserver.chromium

[jiayl, 2014-04-29 18:01:59]

The CQ bit was checked by jiayl@chromium.org

[commit-bot: I haz the power, 2014-04-29 18:05:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jiayl@chromium.org/248113003/40001

[commit-bot: I haz the power, 2014-04-29 18:53:00]

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

[commit-bot: I haz the power, 2014-04-29 18:53:01]

Try jobs failed on following builders:
  mac_chromium_rel on tryserver.chromium

[jiayl, 2014-04-29 19:02:20]

The CQ bit was checked by jiayl@chromium.org

[commit-bot: I haz the power, 2014-04-29 19:03:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jiayl@chromium.org/248113003/40001

[commit-bot: I haz the power, 2014-04-29 22:01:55]

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

[commit-bot: I haz the power, 2014-04-29 22:01:56]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
  linux_chromium_rel on tryserver.chromium

[jiayl, 2014-04-29 22:04:24]

The CQ bit was checked by jiayl@chromium.org

[commit-bot: I haz the power, 2014-04-29 22:09:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jiayl@chromium.org/248113003/40001

[commit-bot: I haz the power, 2014-04-30 02:17:24]

Change committed as 267045