[RemotePlayback API] Resolve/reject prompt() properly when the element is 'connected'.

[RemotePlayback API] Resolve/reject prompt() properly when the element is 'connected'.

BUG=659803
TEST=new tests + manual

Committed: https://crrev.com/c1772b9fd4a490dd06348c94e3c35576f0274d73
Cr-Commit-Position: refs/heads/master@{#429683}

[whywhat, 2016-11-01 19:01:58]

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

[whywhat, 2016-11-01 19:02:00]

PTaL

[whywhat, 2016-11-01 19:02:06]

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

[commit-bot: I haz the power, 2016-11-01 19:02:32]

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-01 22:21:24]

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

[commit-bot: I haz the power, 2016-11-01 22:21:25]

Dry run: This issue passed the CQ dry run.

[mlamouri (slow - plz ping), 2016-11-02 14:59:10]

https://codereview.chromium.org/2469863002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/MediaRouteControllerDialogFactory.java
(right):

https://codereview.chromium.org/2469863002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/MediaRouteControllerDialogFactory.java:104:
Log.d(TAG, "onCancel " + mPlayer);
Any reason to keep this Log but not below in onDismiss?

https://codereview.chromium.org/2469863002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/MediaRouteControllerDialogFactory.java:113:
if (mPlayer != null) mPlayer.onRouteDialogCancelled();
style nit: not super important but the style is inconsistent with the method
just above ;)

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

https://codereview.chromium.org/2469863002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp:219:
m_promptPromiseResolver->resolve();
So if we go from not connected to connected or from connected to disconnected,
we resolve? Sounds like a comment could help understand what's happening here :)
Ideally, the code could be made more self-explanatory :)

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

https://codereview.chromium.org/2469863002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/remoteplayback/RemotePlaybackTest.cpp:113:
cancelPrompt(remotePlayback);
This is confusing. I initially thought of a bug: why does cancelling the prompt
rejects if already connected? What do we call cancelling the prompt in this
case? Said otherwise, what would we call not cancelling the prompt?

[whywhat, 2016-11-02 20:59:18]

Fixed comments

[whywhat, 2016-11-02 21:05:00]

PTAL

https://codereview.chromium.org/2469863002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/MediaRouteControllerDialogFactory.java
(right):

https://codereview.chromium.org/2469863002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/MediaRouteControllerDialogFactory.java:104:
Log.d(TAG, "onCancel " + mPlayer);
On 2016/11/02 at 14:59:10, mlamouri wrote:
> Any reason to keep this Log but not below in onDismiss?

Nope.

https://codereview.chromium.org/2469863002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/MediaRouteControllerDialogFactory.java:113:
if (mPlayer != null) mPlayer.onRouteDialogCancelled();
On 2016/11/02 at 14:59:10, mlamouri wrote:
> style nit: not super important but the style is inconsistent with the method
just above ;)

Not sure how, the blank line is missing?
I don't need onCancel, actually - onDismissed will be called in any case. If the
dialog is dismissed via Stop button we get stop event before this one.

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

https://codereview.chromium.org/2469863002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp:219:
m_promptPromiseResolver->resolve();
On 2016/11/02 at 14:59:10, mlamouri wrote:
> So if we go from not connected to connected or from connected to disconnected,
we resolve? Sounds like a comment could help understand what's happening here :)
Ideally, the code could be made more self-explanatory :)

Yes, if we go from not-connected to disconnected - it means prompt tried to
establish a connection and failed - we reject with error. Otherwise it's an
expected transition (connected to disconnected or anything to
connecting/connected) - so we resolve.

Expanded the comment above the if's

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

https://codereview.chromium.org/2469863002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/remoteplayback/RemotePlaybackTest.cpp:113:
cancelPrompt(remotePlayback);
On 2016/11/02 at 14:59:10, mlamouri wrote:
> This is confusing. I initially thought of a bug: why does cancelling the
prompt rejects if already connected? What do we call cancelling the prompt in
this case? Said otherwise, what would we call not cancelling the prompt?

Cancelling the prompt is dismissing the dialog. In both cases (connected or not)
user chose not to change the state so we throw an exception. Not cancelling the
prompt means either selecting a device or choosing to stop remote playback.

[whywhat, 2016-11-02 21:51:12]

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:56]

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

[whywhat, 2016-11-03 00:10:34]

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

https://codereview.chromium.org/2469863002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp:219:
m_promptPromiseResolver->resolve();
On 2016/11/02 at 21:05:00, whywhat wrote:
> On 2016/11/02 at 14:59:10, mlamouri wrote:
> > So if we go from not connected to connected or from connected to
disconnected, we resolve? Sounds like a comment could help understand what's
happening here :) Ideally, the code could be made more self-explanatory :)
> 
> Yes, if we go from not-connected to disconnected - it means prompt tried to
establish a connection and failed - we reject with error. Otherwise it's an
expected transition (connected to disconnected or anything to
connecting/connected) - so we resolve.
> 
> Expanded the comment above the if's

It becomes a bit easier to follow with the 'connecting' state I believe:
https://codereview.chromium.org/2464163004

[commit-bot: I haz the power, 2016-11-03 00:11:39]

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

[commit-bot: I haz the power, 2016-11-03 00:11:40]

Dry run: This issue passed the CQ dry run.

[mlamouri (slow - plz ping), 2016-11-03 16:06:24]

lgtm but I've opened https://github.com/w3c/remote-playback/issues/63 about the
issue I pointed.

[whywhat, 2016-11-03 20:08:03]

The CQ bit was checked by avayvod@chromium.org

[commit-bot: I haz the power, 2016-11-03 20:08:35]

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-03 20:14:57]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-03 20:16:42]

Description was changed from

==========
[RemotePlayback API] Resolve/reject prompt() properly when the element is
'connected'.

BUG=659803
TEST=new tests + manual
==========

to

==========
[RemotePlayback API] Resolve/reject prompt() properly when the element is
'connected'.

BUG=659803
TEST=new tests + manual

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

[commit-bot: I haz the power, 2016-11-03 20:16:43]

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