Send enter / exit fullscreen signal to AVDA

media/blink/webmediaplayer_impl.h

Index: media/blink/webmediaplayer_impl.h
diff --git a/media/blink/webmediaplayer_impl.h b/media/blink/webmediaplayer_impl.h
index 8a6c51b9c487bbc5eb0baaec8cdbe351055fe592..5b0dd2977bcbf295982d14d3296abaed83e33344 100644
--- a/media/blink/webmediaplayer_impl.h
+++ b/media/blink/webmediaplayer_impl.h
@@ -259,9 +259,15 @@ class MEDIA_BLINK_EXPORT WebMediaPlayerImpl
   // AndroidOverlay, this is the routing token.
   bool HaveOverlayInfo();
 
-  // Send the overlay surface ID / routing token to the decoder if we have it
-  // and if it has been requested.
-  void MaybeSendOverlayInfoToDecoder();
+  // Send OverlayInfo to the decoder.  If |send_factory_info| is true, then this
+  // will include the surface ID / routing token.  Note that this may cause the
+  // decoder to change factories, since it probably doesn't de-duplicate these.
+  // We will also include the current fullscreen state.
+  //
+  // If we've requested but not yet received the factory info, or if there's
+  // no decoder-provided callback to send the overlay info, then this call will
+  // do nothing.
+  void MaybeSendOverlayInfoToDecoder(bool send_factory_info);

[watk, 2017/05/25 18:09:36]

"factory info" is a pretty opaque term to me. It would be difficult to
understand what causing the decoder to "change factories" means if you weren't
familiar with your recent changes.

I suggest using the term "overlay id" to mean either a surface id or overlay
token.

Is it much work to make the decoder handle getting all the info every time?
Because it's pretty weird how we only set some of the fields in certain cases.

Seems like AVDA just has to do:

if (new_info.HasSameOverlayIdAs(current_info_)) {
  current_info = new_info;
  return;
}

// Do normal creation of factory.

[liberato (no reviews please), 2017/05/25 21:26:56]

Done.

 
   void OnPipelineSuspended();
   void OnBeforePipelineResume();
@@ -775,11 +781,14 @@ class MEDIA_BLINK_EXPORT WebMediaPlayerImpl
   // Optional callback to request the routing token for AndroidOverlay.
   RequestRoutingTokenCallback request_routing_token_cb_;
 
-  // Routing token, if we have one.  No value if we have a request pending to
-  // get it via |request_routing_token_cb_|.  A has_value() is_empty() token
-  // indicates that we requested and received an empty token.  Note that we
-  // can't send an empty token via IPC, so we handle that specially.
-  base::Optional<base::UnguessableToken> overlay_routing_token_;
+  // Routing token, if we have one.  If |!overlay_routing_token_|, then we have
+  // requested but not yet received one.  If |overlay_routing_token_|, but
+  // |!(*overlay_routing_token_)|, then there is no routing token (i.e., do not
+  // use overlays).

[watk, 2017/05/25 18:09:36]

This is really confusing. There's three levels of nullability. I think you need
some bools or a wrapper type. I don't have a specific suggestion, but I can tell
you that I spent five minutes trying to make sure I understood this.

I also don't think you need to explain what it means to have a null
RoutingToken. We should treat RoutingToken as an opaque type here. It shouldn't
matter to WMPI that a RoutingToken is actually an optional.

[liberato (no reviews please), 2017/05/25 21:26:56]

i cleaned up the description.

i don't think that having explicit bools is going to help; that's exactly the
same as base::Optional<>.  i don't see how it would be any more or less
complicated.

+  base::Optional<OverlayInfo::RoutingToken> overlay_routing_token_;
+
+  // Is the player fullscreen?

[watk, 2017/05/25 18:09:36]

Comment can be deleted  IMO

[liberato (no reviews please), 2017/05/25 21:26:56]

Done.

+  bool is_fullscreen_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(WebMediaPlayerImpl);
 };