Explicitly represent "no surface ID yet" in WMPI.

Explicitly represent "no surface ID yet" in WMPI.

Previously, WMPI used kNoSurfaceID to indicate both that a surface
shouldn't be used for overlays on Android, and that no surface has
been provided yet by the SurfaceManager.  This could lead to a
deadlock if the SurfaceManager actually does return kNoSurfaceID,
and we're on pre-M Android.

This CL represents the surfac ID as a base::Optional, with the
unset state indicating that a call to SurfaceManager is pending.

BUG=

Review-Url: https://codereview.chromium.org/2856643002
Cr-Commit-Position: refs/heads/master@{#468455}
Committed: https://chromium.googlesource.com/chromium/src/+/e8e3f43d16a81d9b13795f885d011e29bd8d4164

[liberato (no reviews please), 2017-05-01 18:56:43]

liberato@chromium.org changed reviewers:
+ watk@chromium.org

[liberato (no reviews please), 2017-05-01 18:56:43]

decided to fix it as a separate CL since it's so small.

if i use the same approach on the routing tokens, then i'll end up with
base::Optional<base::Optional<base::UnguessableToken>>, since
base::UnguessableToken explicitly doesn't represent "no value" (kNoSurfaceID)
for security reasons.

thanks
-fl

[watk, 2017-05-01 19:36:38]

On 2017/05/01 18:56:43, liberato wrote:
> decided to fix it as a separate CL since it's so small.
> 
> if i use the same approach on the routing tokens, then i'll end up with
> base::Optional<base::Optional<base::UnguessableToken>>, since
> base::UnguessableToken explicitly doesn't represent "no value" (kNoSurfaceID)
> for security reasons.
> 
> thanks
> -fl

IMO it's confusing that overlay_surface_id_ has a value when overlay_enabled_ ==
false. Seems clearer if overlay_surface_id_ only has a value if overlay_enabled_
is true, and only after the surface_manager has given us one.

[liberato (no reviews please), 2017-05-01 20:28:47]

On 2017/05/01 19:36:38, watk wrote:
> On 2017/05/01 18:56:43, liberato wrote:
> > decided to fix it as a separate CL since it's so small.
> > 
> > if i use the same approach on the routing tokens, then i'll end up with
> > base::Optional<base::Optional<base::UnguessableToken>>, since
> > base::UnguessableToken explicitly doesn't represent "no value"
(kNoSurfaceID)
> > for security reasons.
> > 
> > thanks
> > -fl
> 
> IMO it's confusing that overlay_surface_id_ has a value when overlay_enabled_
==
> false. Seems clearer if overlay_surface_id_ only has a value if
overlay_enabled_
> is true, and only after the surface_manager has given us one.

i've uploaded PS2 that does that.  in retrospect, i like PS1 better.

i think that it's a little confusing that we need |overlay_enabled_| to
interpret |overlay_surface_id_| in PS2.  in PS1, |overlay_surface_id_|
always correctly answers the question "what surface should i be using?" by
itself, including "i don't know yet".

if |!overlay_enabled_|, the answer is just kNoSurfaceID, which doesn't
seem like a special case.  in PS2, it's a special case.

WDYT?

thanks
-fl

[liberato (no reviews please), 2017-05-01 20:31:39]

On 2017/05/01 20:28:47, liberato wrote:
> On 2017/05/01 19:36:38, watk wrote:
> > On 2017/05/01 18:56:43, liberato wrote:
> > > decided to fix it as a separate CL since it's so small.
> > > 
> > > if i use the same approach on the routing tokens, then i'll end up with
> > > base::Optional<base::Optional<base::UnguessableToken>>, since
> > > base::UnguessableToken explicitly doesn't represent "no value"
> (kNoSurfaceID)
> > > for security reasons.
> > > 
> > > thanks
> > > -fl
> > 
> > IMO it's confusing that overlay_surface_id_ has a value when
overlay_enabled_
> ==
> > false. Seems clearer if overlay_surface_id_ only has a value if
> overlay_enabled_
> > is true, and only after the surface_manager has given us one.
> 
> i've uploaded PS2 that does that.  in retrospect, i like PS1 better.
> 
> i think that it's a little confusing that we need |overlay_enabled_| to
> interpret |overlay_surface_id_| in PS2.  in PS1, |overlay_surface_id_|
> always correctly answers the question "what surface should i be using?" by
> itself, including "i don't know yet".
> 
> if |!overlay_enabled_|, the answer is just kNoSurfaceID, which doesn't
> seem like a special case.  in PS2, it's a special case.
> 
> WDYT?
> 
> thanks
> -fl

oh, i just noticed that, in PS1, 1712 doesn't need to check for
|overlay_enabled_|; |!overlay_surface_id_| is enough.

thanks
-fl

[watk, 2017-05-01 20:34:58]

PS1 is fine with me if you prefer it. LGTM!

https://codereview.chromium.org/2856643002/diff/1/media/blink/webmediaplayer_...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2856643002/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.cc:1712: if (overlay_enabled_ &&
!overlay_surface_id_)
overlay_enabled_ is redundant I believe

[liberato (no reviews please), 2017-05-01 20:42:42]

thanks for the feedback.  PS1 fits better into my refactor, so i'm going to go
with it.

thanks
-fl

https://codereview.chromium.org/2856643002/diff/1/media/blink/webmediaplayer_...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2856643002/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.cc:1712: if (overlay_enabled_ &&
!overlay_surface_id_)
On 2017/05/01 20:34:58, watk wrote:
> overlay_enabled_ is redundant I believe

Done.

[liberato (no reviews please), 2017-05-01 20:42:47]

The CQ bit was checked by liberato@chromium.org

[liberato (no reviews please), 2017-05-01 20:42:47]

The patchset sent to the CQ was uploaded after l-g-t-m from watk@chromium.org
Link to the patchset: https://codereview.chromium.org/2856643002/#ps40001
(title: "removed redundant check")

[commit-bot: I haz the power, 2017-05-01 20:43:39]

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, 2017-05-01 22:23:17]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493671367522190,
"parent_rev": "174d25a6b6a60a68090facf740178025c6eeebfb", "commit_rev":
"e8e3f43d16a81d9b13795f885d011e29bd8d4164"}

[commit-bot: I haz the power, 2017-05-01 22:23:28]

Description was changed from

==========
Explicitly represent "no surface ID yet" in WMPI.

Previously, WMPI used kNoSurfaceID to indicate both that a surface
shouldn't be used for overlays on Android, and that no surface has
been provided yet by the SurfaceManager.  This could lead to a
deadlock if the SurfaceManager actually does return kNoSurfaceID,
and we're on pre-M Android.

This CL represents the surfac ID as a base::Optional, with the
unset state indicating that a call to SurfaceManager is pending.

BUG=
==========

to

==========
Explicitly represent "no surface ID yet" in WMPI.

Previously, WMPI used kNoSurfaceID to indicate both that a surface
shouldn't be used for overlays on Android, and that no surface has
been provided yet by the SurfaceManager.  This could lead to a
deadlock if the SurfaceManager actually does return kNoSurfaceID,
and we're on pre-M Android.

This CL represents the surfac ID as a base::Optional, with the
unset state indicating that a call to SurfaceManager is pending.

BUG=

Review-Url: https://codereview.chromium.org/2856643002
Cr-Commit-Position: refs/heads/master@{#468455}
Committed:
https://chromium.googlesource.com/chromium/src/+/e8e3f43d16a81d9b13795f885d01...
==========

[commit-bot: I haz the power, 2017-05-01 22:23:29]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/e8e3f43d16a81d9b13795f885d01...