Send enter / exit fullscreen signal to AVDA

media/blink/webmediaplayer_impl.h

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef MEDIA_BLINK_WEBMEDIAPLAYER_IMPL_H_
 #define MEDIA_BLINK_WEBMEDIAPLAYER_IMPL_H_
 
 #include <stdint.h>
 
 #include <memory>
@@ skipping 241 matching lines @@
   friend class WebMediaPlayerImplTest;
   friend class WebMediaPlayerImplBackgroundBehaviorTest;
 
   void EnableOverlay();
   void DisableOverlay();
 
   // Do we have overlay information?  For CVV, this is a surface id.  For
   // 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();
   void OnPipelineResumed();
   void OnDemuxerOpened();
 
   // Pipeline::Client overrides.
   void OnError(PipelineStatus status) override;
   void OnEnded() override;
   void OnMetadata(PipelineMetadata metadata) override;
@@ skipping 493 matching lines @@
 
     // Use AndroidOverlay for overlays.
     kUseAndroidOverlay,
   };
 
   OverlayMode overlay_mode_ = OverlayMode::kNoOverlays;
 
   // 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);
 };
 
 }  // namespace media
 
 #endif  // MEDIA_BLINK_WEBMEDIAPLAYER_IMPL_H_