Wrap MediaMetadata in base::Optional in content and chrome

content/browser/android/web_contents_observer_proxy.h

 // Copyright 2015 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 CONTENT_BROWSER_ANDROID_WEB_CONTENTS_OBSERVER_PROXY_H_
 #define CONTENT_BROWSER_ANDROID_WEB_CONTENTS_OBSERVER_PROXY_H_
 
 #include <jni.h>
 
 #include "base/android/jni_weak_ref.h"
@@ skipping 54 matching lines @@
   void DocumentLoadedInFrame(RenderFrameHost* render_frame_host) override;
   void NavigationEntryCommitted(
       const LoadCommittedDetails& load_details) override;
   void WebContentsDestroyed() override;
   void DidAttachInterstitialPage() override;
   void DidDetachInterstitialPage() override;
   void DidChangeThemeColor(SkColor color) override;
   void DidStartNavigationToPendingEntry(
       const GURL& url,
       NavigationController::ReloadType reload_type) override;
-  void MediaSessionStateChanged(bool is_controllable,
-                                bool is_suspended,
-                                const MediaMetadata& metadata) override;
+  void MediaSessionStateChanged(
+      bool is_controllable,
+      bool is_suspended,
+      const base::Optional<MediaMetadata>& metadata) override;

[dcheng, 2016/09/06 21:11:16]

The documentation for base::Optional specifically recommends passing parameters
as T* rather than const base::Optional<T>&. Let's do that here and elsewhere,
except where we can't (e.g. the IPC message handler)

[mlamouri (slow - plz ping), 2016/09/07 08:55:53]

FWIW, the gist of this recommendation is to avoid methods taking an Optional<T>
instead of T* but I think it's fine to take an Optional<T> if it's expected that
the method is already  handling an Optional<T>. In other words, we should avoid
unwrapping the Optional<T> to pass it to a method taking T* then wrap T* back in
an Optional<T>.

[Zhiqiang Zhang (Slow), 2016/09/07 10:45:06]

I think passing `const base::Optional<T>&` is OK. We are not constructing new
Optionals, so should not have the problems described in the documentation.

   void SetToBaseURLForDataURLIfNeeded(std::string* url);
 
   void DidFailLoadInternal(bool is_provisional_load,
                            bool is_main_frame,
                            int error_code,
                            const base::string16& description,
                            const GURL& url,
                            bool was_ignored_by_handler);
 
   base::android::ScopedJavaGlobalRef<jobject> java_observer_;
   GURL base_url_of_last_started_data_url_;
 
   DISALLOW_COPY_AND_ASSIGN(WebContentsObserverProxy);
 };
 
 bool RegisterWebContentsObserverProxy(JNIEnv* env);
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_ANDROID_WEB_CONTENTS_OBSERVER_PROXY_H_