[RemotePlayback] Stop remote playback when disableRemotePlayback is set

[RemotePlayback] Stop remote playback when disableRemotePlayback is set

When disableRemotePlayback attribute is added,
per spec we need to stop remote playback.

BUG=659669
TEST=manual (suggestions for reasonable tests are welcome)

Committed: https://crrev.com/8d8c53b95cb9bd0d927c2b31137b54e73a9dc089
Cr-Commit-Position: refs/heads/master@{#429905}

[whywhat, 2016-11-02 20:25:31]

avayvod@chromium.org changed reviewers:
+ hubbe@chromium.org, mlamouri@chromium.org

[whywhat, 2016-11-02 20:25:32]

PTaL

hubbe: media/blink/
mlamouri: media/remote/

[hubbe, 2016-11-02 21:17:45]

media/blink LGTM

https://codereview.chromium.org/2468303002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp
(right):

https://codereview.chromium.org/2468303002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp:270: //
TODO(avayvod): stop remote playback too. https://crbug.com/657566.
remove todo?

[whywhat, 2016-11-02 21:50:53]

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

[commit-bot: I haz the power, 2016-11-02 21:51:24]

Dry run: 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-11-02 23:16:56]

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

[commit-bot: I haz the power, 2016-11-02 23:16:56]

Dry run: This issue passed the CQ dry run.

[mlamouri (slow - plz ping), 2016-11-03 16:12:40]

lgtm but we need tests for these.

https://codereview.chromium.org/2468303002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp
(right):

https://codereview.chromium.org/2468303002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp:272:
m_mediaElement->requestRemotePlaybackStop();
What would happen if the state was `connecting` at this point?

[whywhat, 2016-11-03 20:09:22]

avayvod@chromium.org changed reviewers:
+ dcheng@chromium.org, jam@chromium.org

[whywhat, 2016-11-03 20:09:22]

+jam for content/
+dcheng for IPC messages

[whywhat, 2016-11-03 20:12:56]

On 2016/11/03 at 16:12:40, mlamouri wrote:
> lgtm but we need tests for these.

As I said in the description: suggestions are welcome :)

Unit test for RemotePlayback would be too trivial.
We could have something like the Instrumentation Cast tests but they are
historically flaky and don't catch breakages in practice so I'm hesitant adding
more.
E2E tests wouldn't be part of this cl.

> 
>
https://codereview.chromium.org/2468303002/diff/1/third_party/WebKit/Source/m...
> File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp
(right):
> 
>
https://codereview.chromium.org/2468303002/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp:272:
m_mediaElement->requestRemotePlaybackStop();
> What would happen if the state was `connecting` at this point?

We will disconnect still.

[jam, 2016-11-04 00:43:36]

On 2016/11/03 20:09:22, whywhat wrote:
> +jam for content/
please use media/owners
> +dcheng for IPC messages

[dcheng, 2016-11-04 01:51:45]

ipc lgtm

https://codereview.chromium.org/2468303002/diff/1/chrome/browser/media/androi...
File chrome/browser/media/android/remote/remote_media_player_bridge.cc (right):

https://codereview.chromium.org/2468303002/diff/1/chrome/browser/media/androi...
chrome/browser/media/android/remote/remote_media_player_bridge.cc:282:
CHECK(env);
Please don't use CHECK for this:
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

[whywhat, 2016-11-04 15:00:27]

Rebase, addressed comments

[whywhat, 2016-11-04 15:01:06]

https://codereview.chromium.org/2468303002/diff/1/chrome/browser/media/androi...
File chrome/browser/media/android/remote/remote_media_player_bridge.cc (right):

https://codereview.chromium.org/2468303002/diff/1/chrome/browser/media/androi...
chrome/browser/media/android/remote/remote_media_player_bridge.cc:282:
CHECK(env);
On 2016/11/04 at 01:51:44, dcheng wrote:
> Please don't use CHECK for this:
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

Done. Seems like everyone else is using DCHECK(env) these days.

https://codereview.chromium.org/2468303002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp
(right):

https://codereview.chromium.org/2468303002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp:270: //
TODO(avayvod): stop remote playback too. https://crbug.com/657566.
On 2016/11/02 at 21:17:45, hubbe wrote:
> remove todo?

Done.

https://codereview.chromium.org/2468303002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp:272:
m_mediaElement->requestRemotePlaybackStop();
On 2016/11/03 at 16:12:40, mlamouri wrote:
> What would happen if the state was `connecting` at this point?

Replied.

[whywhat, 2016-11-04 15:01:17]

avayvod@chromium.org changed reviewers:
- jam@chromium.org

[whywhat, 2016-11-04 15:01:19]

The CQ bit was checked by avayvod@chromium.org

[whywhat, 2016-11-04 15:01:20]

The patchset sent to the CQ was uploaded after l-g-t-m from hubbe@chromium.org,
dcheng@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2468303002/#ps20001
(title: "Rebase, addressed comments")

[commit-bot: I haz the power, 2016-11-04 15:01:43]

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-11-04 16:10:58]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-04 16:14:46]

Description was changed from

==========
[RemotePlayback] Stop remote playback when disableRemotePlayback is set

When disableRemotePlayback attribute is added,
per spec we need to stop remote playback.

BUG=659669
TEST=manual (suggestions for reasonable tests are welcome)
==========

to

==========
[RemotePlayback] Stop remote playback when disableRemotePlayback is set

When disableRemotePlayback attribute is added,
per spec we need to stop remote playback.

BUG=659669
TEST=manual (suggestions for reasonable tests are welcome)

Committed: https://crrev.com/8d8c53b95cb9bd0d927c2b31137b54e73a9dc089
Cr-Commit-Position: refs/heads/master@{#429905}
==========

[commit-bot: I haz the power, 2016-11-04 16:14:48]

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