Wrap MediaMetadata in base::Optional in content and chrome

Wrap MediaMetadata in base::Optional in content and chrome

Since base::Optional<> has already been in use. It's helpful to wrap
MediaMetadata in base::Optional<>, so that the browser can know whether
the page has set the metadata or not.

BUG=643195
Committed: https://crrev.com/24ae1ae1f9d4e7f6668cfc554cfb8bface3650b9
Cr-Commit-Position: refs/heads/master@{#417255}

[Zhiqiang Zhang (Slow), 2016-09-01 16:10:33]

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

[Zhiqiang Zhang (Slow), 2016-09-01 16:10:34]

Moved this CL out to reduce the "per-frame metadata" CL.

I'm adding you to review first and will later add other reviewers for
rubber-stamping.

[Zhiqiang Zhang (Slow), 2016-09-01 16:13:37]

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

[commit-bot: I haz the power, 2016-09-01 16:13:51]

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-09-01 16:26:25]

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

[commit-bot: I haz the power, 2016-09-01 16:26:25]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Zhiqiang Zhang (Slow), 2016-09-01 16:59:25]

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

[commit-bot: I haz the power, 2016-09-01 16:59:39]

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

[mlamouri (slow - plz ping), 2016-09-01 17:47:50]

lgtm

https://codereview.chromium.org/2300083002/diff/1/content/browser/media/andro...
File content/browser/media/android/browser_media_session_manager.cc (right):

https://codereview.chromium.org/2300083002/diff/1/content/browser/media/andro...
content/browser/media/android/browser_media_session_manager.cc:36: if
(insecure_metadata &&
Maybe `insecure_metadata.has_value()`

https://codereview.chromium.org/2300083002/diff/1/content/public/browser/web_...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/2300083002/diff/1/content/public/browser/web_...
content/public/browser/web_contents_observer.h:11: #include "base/optional.h"
can't you forward declare `const T&`?

https://codereview.chromium.org/2300083002/diff/1/content/renderer/media/andr...
File content/renderer/media/android/renderer_media_session_manager.cc (right):

https://codereview.chromium.org/2300083002/diff/1/content/renderer/media/andr...
content/renderer/media/android/renderer_media_session_manager.cc:73: (!metadata
|| MediaMetadataSanitizer::CheckSanity(metadata.value()))
Maybe `!metadata.has_value()` instead of `!metadata`?

https://codereview.chromium.org/2300083002/diff/1/content/renderer/media/andr...
File content/renderer/media/android/renderer_media_session_manager.h (right):

https://codereview.chromium.org/2300083002/diff/1/content/renderer/media/andr...
content/renderer/media/android/renderer_media_session_manager.h:13: #include
"base/optional.h"
ditto

[commit-bot: I haz the power, 2016-09-01 17:54:41]

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

[commit-bot: I haz the power, 2016-09-01 17:54:42]

Dry run: This issue passed the CQ dry run.

[Zhiqiang Zhang (Slow), 2016-09-01 18:19:37]

https://codereview.chromium.org/2300083002/diff/1/content/public/browser/web_...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/2300083002/diff/1/content/public/browser/web_...
content/public/browser/web_contents_observer.h:11: #include "base/optional.h"
On 2016/09/01 17:47:49, mlamouri (slow) wrote:
> can't you forward declare `const T&`?

Sorry, I didn't quite follow here.
Do you mean forward declare base::Optional<> or content::MediaMetadata?

[Zhiqiang Zhang (Slow), 2016-09-02 12:11:41]

zqzhang@chromium.org changed reviewers:
+ dalecurtis@google.com, dcheng@chromium.org, sievers@chromium.org
- avayvod@chromium.org

[Zhiqiang Zhang (Slow), 2016-09-02 12:11:42]

PTAL, addressed Mounir's comments.

+dalecurtis to approve media-related changes
+sievers to approve changes to web_contents_observer*
+dcheng to approve changes to media_session_messages_android.h

https://codereview.chromium.org/2300083002/diff/1/content/browser/media/andro...
File content/browser/media/android/browser_media_session_manager.cc (right):

https://codereview.chromium.org/2300083002/diff/1/content/browser/media/andro...
content/browser/media/android/browser_media_session_manager.cc:36: if
(insecure_metadata &&
On 2016/09/01 17:47:49, mlamouri (slow) wrote:
> Maybe `insecure_metadata.has_value()`

Done.

https://codereview.chromium.org/2300083002/diff/1/content/public/browser/web_...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/2300083002/diff/1/content/public/browser/web_...
content/public/browser/web_contents_observer.h:11: #include "base/optional.h"
On 2016/09/01 17:47:49, mlamouri (slow) wrote:
> can't you forward declare `const T&`?

Done.

https://codereview.chromium.org/2300083002/diff/1/content/renderer/media/andr...
File content/renderer/media/android/renderer_media_session_manager.cc (right):

https://codereview.chromium.org/2300083002/diff/1/content/renderer/media/andr...
content/renderer/media/android/renderer_media_session_manager.cc:73: (!metadata
|| MediaMetadataSanitizer::CheckSanity(metadata.value()))
On 2016/09/01 17:47:49, mlamouri (slow) wrote:
> Maybe `!metadata.has_value()` instead of `!metadata`?

Done.

https://codereview.chromium.org/2300083002/diff/1/content/renderer/media/andr...
File content/renderer/media/android/renderer_media_session_manager.h (right):

https://codereview.chromium.org/2300083002/diff/1/content/renderer/media/andr...
content/renderer/media/android/renderer_media_session_manager.h:13: #include
"base/optional.h"
On 2016/09/01 17:47:49, mlamouri (slow) wrote:
> ditto

Done.

[Zhiqiang Zhang (Slow), 2016-09-05 12:37:58]

zqzhang@chromium.org changed reviewers:
+ jochen@chromium.org
- sievers@chromium.org

[Zhiqiang Zhang (Slow), 2016-09-05 12:37:58]

+jochen to approve changes to web_contents_observer*

[jochen (gone - plz use gerrit), 2016-09-06 15:56:21]

lgtm

[DaleCurtis, 2016-09-06 19:26:06]

dalecurtis@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[DaleCurtis, 2016-09-06 19:26:07]

lgtm

https://codereview.chromium.org/2300083002/diff/40001/content/renderer/media/...
File content/renderer/media/android/renderer_media_session_manager.cc (right):

https://codereview.chromium.org/2300083002/diff/40001/content/renderer/media/...
content/renderer/media/android/renderer_media_session_manager.cc:74:
(!metadata.has_value() ||
Is this what git cl format did? It seems odd.

[DaleCurtis, 2016-09-06 19:26:15]

dalecurtis@chromium.org changed reviewers:
- dalecurtis@google.com

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

https://codereview.chromium.org/2300083002/diff/40001/content/browser/android...
File content/browser/android/web_contents_observer_proxy.h (right):

https://codereview.chromium.org/2300083002/diff/40001/content/browser/android...
content/browser/android/web_contents_observer_proxy.h:78: const
base::Optional<MediaMetadata>& metadata) override;
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)

https://codereview.chromium.org/2300083002/diff/40001/content/renderer/media/...
File content/renderer/media/android/renderer_media_session_manager.h (right):

https://codereview.chromium.org/2300083002/diff/40001/content/renderer/media/...
content/renderer/media/android/renderer_media_session_manager.h:18: template
<typename T> class Optional;
Let's just #include base/optional.h. One day this is going to be the C++
standard optional, and the forward declare will horribly explode. The same is
true for all other forward declarations.

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

https://codereview.chromium.org/2300083002/diff/40001/content/browser/android...
File content/browser/android/web_contents_observer_proxy.h (right):

https://codereview.chromium.org/2300083002/diff/40001/content/browser/android...
content/browser/android/web_contents_observer_proxy.h:78: const
base::Optional<MediaMetadata>& metadata) override;
On 2016/09/06 at 21:11:16, dcheng wrote:
> 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)

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]

PTAL for dcheng.

https://codereview.chromium.org/2300083002/diff/40001/content/browser/android...
File content/browser/android/web_contents_observer_proxy.h (right):

https://codereview.chromium.org/2300083002/diff/40001/content/browser/android...
content/browser/android/web_contents_observer_proxy.h:78: const
base::Optional<MediaMetadata>& metadata) override;
On 2016/09/07 08:55:53, mlamouri (slow) wrote:
> On 2016/09/06 at 21:11:16, dcheng wrote:
> > 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)
> 
> 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>.

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.

https://codereview.chromium.org/2300083002/diff/40001/content/renderer/media/...
File content/renderer/media/android/renderer_media_session_manager.cc (right):

https://codereview.chromium.org/2300083002/diff/40001/content/renderer/media/...
content/renderer/media/android/renderer_media_session_manager.cc:74:
(!metadata.has_value() ||
On 2016/09/06 19:26:07, DaleCurtis wrote:
> Is this what git cl format did? It seems odd.

Done.

https://codereview.chromium.org/2300083002/diff/40001/content/renderer/media/...
File content/renderer/media/android/renderer_media_session_manager.h (right):

https://codereview.chromium.org/2300083002/diff/40001/content/renderer/media/...
content/renderer/media/android/renderer_media_session_manager.h:18: template
<typename T> class Optional;
On 2016/09/06 21:11:16, dcheng wrote:
> Let's just #include base/optional.h. One day this is going to be the C++
> standard optional, and the forward declare will horribly explode. The same is
> true for all other forward declarations.

Done.

[dcheng, 2016-09-07 20:33:27]

Btw, I was just looking at the plumbing again: how does this get into the JNI?
The only place that handles this message is OnSetMetadata, and it doesn't appear
to call anything else after it does the sanity checking? What am I missing?

[Zhiqiang Zhang (Slow), 2016-09-07 20:43:42]

On 2016/09/07 20:33:27, dcheng wrote:
> Btw, I was just looking at the plumbing again: how does this get into the JNI?
> The only place that handles this message is OnSetMetadata, and it doesn't
appear
> to call anything else after it does the sanity checking? What am I missing?

Yeah, the plumbing stops at BrowserMediaSessionManager::OnSetMetadata() (see
`NOT_IMPLEMENTED()`).
We have a larger effort to eventually propagate the Metadata to Java. There's a
follow-up CL in progress:
https://codereview.chromium.org/2252783004/
we decide to split this CL out to make the follow-up CL smaller.

[dcheng, 2016-09-07 22:47:55]

LGTM

[Zhiqiang Zhang (Slow), 2016-09-07 23:05:51]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2016-09-07 23:05:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org, jochen@chromium.org, dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/2300083002/#ps60001
(title: "addressed comments")

[commit-bot: I haz the power, 2016-09-07 23:06:32]

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-09-07 23:39:43]

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

[commit-bot: I haz the power, 2016-09-07 23:39:44]

Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[Zhiqiang Zhang (Slow), 2016-09-08 10:25:50]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2016-09-08 10:25:51]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
dalecurtis@chromium.org, dcheng@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2300083002/#ps80001
(title: "fix Android build")

[commit-bot: I haz the power, 2016-09-08 10:26:10]

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-09-08 11:37:27]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-08 11:39:39]

Description was changed from

==========
Wrap MediaMetadata in base::Optional in content and chrome

Since base::Optional<> has already been in use. It's helpful to wrap
MediaMetadata in base::Optional<>, so that the browser can know whether
the page has set the metadata or not.

BUG=643195
==========

to

==========
Wrap MediaMetadata in base::Optional in content and chrome

Since base::Optional<> has already been in use. It's helpful to wrap
MediaMetadata in base::Optional<>, so that the browser can know whether
the page has set the metadata or not.

BUG=643195

Committed: https://crrev.com/24ae1ae1f9d4e7f6668cfc554cfb8bface3650b9
Cr-Commit-Position: refs/heads/master@{#417255}
==========

[commit-bot: I haz the power, 2016-09-08 11:39:40]

Patchset 5 (id:??) landed as
https://crrev.com/24ae1ae1f9d4e7f6668cfc554cfb8bface3650b9
Cr-Commit-Position: refs/heads/master@{#417255}