Bug fix: stop mirroring session when tab is closed.

Bug fix: stop mirroring session when tab is closed.
Fix bug caused by cl: https://codereview.chromium.org/2365223002.
|running_callback_| is used as a callback when status changes, and
should not be reset if the capture is successfully started.

BUG=655240
Committed: https://crrev.com/a8e9c8591d8d5026c32d6d93bf11f0ff5096eee9
Cr-Commit-Position: refs/heads/master@{#425124}

[xjz, 2016-10-12 19:00:37]

Patchset #1 (id:1) has been deleted

[xjz, 2016-10-12 19:01:59]

The CQ bit was checked by xjz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-12 19:02:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xjz, 2016-10-12 19:06:13]

Description was changed from

==========
Bug fix: stop mirroring session when tab is closed.
Fix bug caused by cl: https://codereview.chromium.org/2365223002.
|running_callback_| is used as a callback when status changes, and
should not be reset if the capture is successfully started.

BUG=655240
==========

to

==========
Bug fix: stop mirroring session when tab is closed.
Fix bug caused by cl: https://codereview.chromium.org/2365223002.
|running_callback_| is used as a callback when status changes, and
should not be reset if the capture is successfully started.

BUG=655240
==========

[xjz, 2016-10-12 19:06:14]

xjz@chromium.org changed reviewers:
+ mcasas@chromium.org

[xjz, 2016-10-12 20:25:04]

PTAL

[commit-bot: I haz the power, 2016-10-12 20:27:53]

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

[commit-bot: I haz the power, 2016-10-12 20:27:54]

Dry run: This issue passed the CQ dry run.

[mcasas, 2016-10-13 13:48:44]

lgtm with a nit

https://codereview.chromium.org/2408143004/diff/20001/content/renderer/media/...
File content/renderer/media/media_stream_video_capturer_source.cc (right):

https://codereview.chromium.org/2408143004/diff/20001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:237:
RunningCallback running_callback_;
nit: the name is misleading, maybe we could
call if 
StateUpdateCB state_update_callback_; ,
or
IsRunningCallback is_running_callback_;
Doesn't feel entirely right though... I'll be OK
with whatever you do.

[xjz, 2016-10-13 18:03:03]

Patchset #2 (id:40001) has been deleted

[xjz, 2016-10-13 18:04:15]

The CQ bit was checked by xjz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 18:05:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xjz, 2016-10-13 18:11:39]

Thanks for review!

https://codereview.chromium.org/2408143004/diff/20001/content/renderer/media/...
File content/renderer/media/media_stream_video_capturer_source.cc (right):

https://codereview.chromium.org/2408143004/diff/20001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:237:
RunningCallback running_callback_;
On 2016/10/13 13:48:43, mcasas wrote:
> nit: the name is misleading, maybe we could
> call if 
> StateUpdateCB state_update_callback_; ,
> or
> IsRunningCallback is_running_callback_;
> Doesn't feel entirely right though... I'll be OK
> with whatever you do.

Yes, the name sounds a little confusing. But it seems that currently only this
LocalVideoCapturerSource uses this callback to notify that when capture stops or
error happens. For other VideoCapturerSource, this callback is just used as a
StartCapture callback. I don't have a better idea for this naming issue. So keep
it as is for now, and add a comment here to explain when it will be called.

[commit-bot: I haz the power, 2016-10-13 18:46:57]

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

[commit-bot: I haz the power, 2016-10-13 18:46:58]

Dry run: This issue passed the CQ dry run.

[xjz, 2016-10-13 19:16:39]

The CQ bit was checked by xjz@chromium.org

[xjz, 2016-10-13 19:16:39]

The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org
Link to the patchset: https://codereview.chromium.org/2408143004/#ps60001
(title: "Addressed comments.")

[commit-bot: I haz the power, 2016-10-13 19:17:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-13 19:22:52]

Description was changed from

==========
Bug fix: stop mirroring session when tab is closed.
Fix bug caused by cl: https://codereview.chromium.org/2365223002.
|running_callback_| is used as a callback when status changes, and
should not be reset if the capture is successfully started.

BUG=655240
==========

to

==========
Bug fix: stop mirroring session when tab is closed.
Fix bug caused by cl: https://codereview.chromium.org/2365223002.
|running_callback_| is used as a callback when status changes, and
should not be reset if the capture is successfully started.

BUG=655240
==========

[commit-bot: I haz the power, 2016-10-13 19:22:53]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2016-10-13 19:25:03]

Description was changed from

==========
Bug fix: stop mirroring session when tab is closed.
Fix bug caused by cl: https://codereview.chromium.org/2365223002.
|running_callback_| is used as a callback when status changes, and
should not be reset if the capture is successfully started.

BUG=655240
==========

to

==========
Bug fix: stop mirroring session when tab is closed.
Fix bug caused by cl: https://codereview.chromium.org/2365223002.
|running_callback_| is used as a callback when status changes, and
should not be reset if the capture is successfully started.

BUG=655240

Committed: https://crrev.com/a8e9c8591d8d5026c32d6d93bf11f0ff5096eee9
Cr-Commit-Position: refs/heads/master@{#425124}
==========

[commit-bot: I haz the power, 2016-10-13 19:25:04]

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

[miu, 2016-10-18 22:22:30]

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

[miu, 2016-10-18 22:22:31]

https://codereview.chromium.org/2408143004/diff/60001/content/renderer/media/...
File content/renderer/media/media_stream_video_capturer_source.cc (right):

https://codereview.chromium.org/2408143004/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:369: // Not
applicable to reporting on device start success/failure.
nit: // Not applicable to reporting on device starts or errors.

https://codereview.chromium.org/2408143004/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:494: void
MediaStreamVideoCapturerSource::OnStarted(bool result) {
The original bug was most likely caused because this method is mis-named (or
maybe it was re-purposed but without also making a name change). Can you please
fix this?

Suggestion: void OnRunStateChanged(bool is_running)

https://codereview.chromium.org/2408143004/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:495:
OnStartDone(result ? MEDIA_DEVICE_OK : MEDIA_DEVICE_TRACK_START_FAILURE);
This is also misleading, but I think there's more than a naming problem here. It
seems like this whole method should be:

  void MediaStreamVideoCapturerSource::OnRunStateChanged(bool is_running) {
    if (is_capture_starting_) {
      // Note: |is_capture_starting_| was set to true by StartSourceImpl().
      OnStartDone(is_running ? MEDIA_DEVICE_OK :
MEDIA_DEVICE_TRACK_START_FAILURE);
      is_capture_starting_ = false;
    } else if (!is_running) {
      StopSource();
    }
  }

[xjz, 2016-10-19 18:12:09]

Addressed miu@'s comments in the follow-up CL:
https://chromiumcodereview.appspot.com/2432373003.
PTAL

https://codereview.chromium.org/2408143004/diff/60001/content/renderer/media/...
File content/renderer/media/media_stream_video_capturer_source.cc (right):

https://codereview.chromium.org/2408143004/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:369: // Not
applicable to reporting on device start success/failure.
On 2016/10/18 22:22:30, miu wrote:
> nit: // Not applicable to reporting on device starts or errors.

Done.

https://codereview.chromium.org/2408143004/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:494: void
MediaStreamVideoCapturerSource::OnStarted(bool result) {
On 2016/10/18 22:22:30, miu wrote:
> The original bug was most likely caused because this method is mis-named (or
> maybe it was re-purposed but without also making a name change). Can you
please
> fix this?
> 
> Suggestion: void OnRunStateChanged(bool is_running)

Done.

https://codereview.chromium.org/2408143004/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:495:
OnStartDone(result ? MEDIA_DEVICE_OK : MEDIA_DEVICE_TRACK_START_FAILURE);
On 2016/10/18 22:22:30, miu wrote:
> This is also misleading, but I think there's more than a naming problem here.
It
> seems like this whole method should be:
> 
>   void MediaStreamVideoCapturerSource::OnRunStateChanged(bool is_running) {
>     if (is_capture_starting_) {
>       // Note: |is_capture_starting_| was set to true by StartSourceImpl().
>       OnStartDone(is_running ? MEDIA_DEVICE_OK :
> MEDIA_DEVICE_TRACK_START_FAILURE);
>       is_capture_starting_ = false;
>     } else if (!is_running) {
>       StopSource();
>     }
>   }

Done.