Begin implementation of the context menu for Video and Audio tags.

Begin implementation of the context menu for Video and Audio tags.

This code should enable the creation of a basic context menu for the Video and Audio tags.  The actions for fullscreen, save screenshot, loop, and set playback rate are not yet implemented.

BUG=15686
TEST=None

[awong, 2009-07-14 07:26:00]

Here's the chromium side of the context menu code.

I still haven't figured out how to find the correct media player instance given
a hit.  Will need to bug you guys about that tomorrow. But at least the basic
functionality works!

[scherkus (not reviewing), 2009-07-14 19:10:36]

First pass, I can't comment as to whether the style for the IPC/context menu
stuff is good, perhaps get Darin (or similar) to give it a good once-over?

Don't forget to add a BUG= and TEST=

http://codereview.chromium.org/149604/diff/1/4
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/149604/diff/1/4#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
bump copyright year

http://codereview.chromium.org/149604/diff/1/4#newcode316
Line 316: // TODO(ajwong): Enable after we actually implement this.
for the sake of grep:
Enable -> Enable fullscreen

http://codereview.chromium.org/149604/diff/1/4#newcode331
Line 331: // TODO(ajwong): Enable after we actually implement this.
for the sake of grep:
Enable -> Enable save screenshot as

http://codereview.chromium.org/149604/diff/1/5
File chrome/browser/tab_contents/render_view_context_menu.h (right):

http://codereview.chromium.org/149604/diff/1/5#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
bump copyright year

http://codereview.chromium.org/149604/diff/1/5#newcode83
Line 83: void AppendMediaItems();
nit: these should be in the order that they are declared in the .cc file (or
vice-versa)

from a quick glance it looks like that rule was being followed here

http://codereview.chromium.org/149604/diff/1/5#newcode107
Line 107: // menu. If another rate is reportd, it will be considered unknown and
reportd -> reported

http://codereview.chromium.org/149604/diff/1/5#newcode113
Line 113: static const double kDoubleTimePlaybackRate;
would it make sense for these to be inside the .cc file?

http://codereview.chromium.org/149604/diff/1/6
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/149604/diff/1/6#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
bump copyright year

http://codereview.chromium.org/149604/diff/1/12
File webkit/glue/context_menu.h (right):

http://codereview.chromium.org/149604/diff/1/12#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
bump copyright year

http://codereview.chromium.org/149604/diff/1/12#newcode107
Line 107: // invoked on.
I'd give some sample elements where this is used (i.e., img, audio, video)

http://codereview.chromium.org/149604/diff/1/13
File webkit/glue/context_menu_client_impl.cc (right):

http://codereview.chromium.org/149604/diff/1/13#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
bump copyright year

http://codereview.chromium.org/149604/diff/1/14
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/149604/diff/1/14#newcode240
Line 240: return true;
so it looks like we have the context menu portion disabled, but say we support
fullscreen here?

what if we left the context menu in, then returned false here?

http://codereview.chromium.org/149604/diff/1/16
File webkit/glue/webview_delegate.h (right):

http://codereview.chromium.org/149604/diff/1/16#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
bump copyright year

http://codereview.chromium.org/149604/diff/1/16#newcode60
Line 60: struct WebPreferences;
I'm confused as to what the ordering should be here :P

[scherkus (not reviewing), 2009-07-14 19:11:33]

fixed alpha's email address

[darin (slow to review), 2009-07-14 21:38:55]

http://codereview.chromium.org/149604/diff/1/10
File webkit/api/src/WebMediaPlayerClientImpl.cpp (right):

http://codereview.chromium.org/149604/diff/1/10#newcode204
Line 204: bool WebMediaPlayerClientImpl::supportsFullscreen() const {
nit: "{" should be on the next line (webkit style)

http://codereview.chromium.org/149604/diff/1/10#newcode210
Line 210: bool WebMediaPlayerClientImpl::supportsSave() const {
ditto

[awong, 2009-07-15 02:53:13]

This addresses the specific comments from Andrew and Darin.

The other comments from the UI review thread with beng@ and others will probably
come in a separate CL.

http://codereview.chromium.org/149604/diff/1/4
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/149604/diff/1/4#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
On 2009/07/14 19:10:36, scherkus wrote:
> bump copyright year

Done.

http://codereview.chromium.org/149604/diff/1/4#newcode316
Line 316: // TODO(ajwong): Enable after we actually implement this.
On 2009/07/14 19:10:36, scherkus wrote:
> for the sake of grep:
> Enable -> Enable fullscreen

Done.

http://codereview.chromium.org/149604/diff/1/4#newcode331
Line 331: // TODO(ajwong): Enable after we actually implement this.
On 2009/07/14 19:10:36, scherkus wrote:
> for the sake of grep:
> Enable -> Enable save screenshot as

Done.

http://codereview.chromium.org/149604/diff/1/5
File chrome/browser/tab_contents/render_view_context_menu.h (right):

http://codereview.chromium.org/149604/diff/1/5#newcode83
Line 83: void AppendMediaItems();
On 2009/07/14 19:10:36, scherkus wrote:
> nit: these should be in the order that they are declared in the .cc file (or
> vice-versa)
> 
> from a quick glance it looks like that rule was being followed here

Done.

http://codereview.chromium.org/149604/diff/1/5#newcode107
Line 107: // menu. If another rate is reportd, it will be considered unknown and
On 2009/07/14 19:10:36, scherkus wrote:
> reportd -> reported

Done.

http://codereview.chromium.org/149604/diff/1/5#newcode113
Line 113: static const double kDoubleTimePlaybackRate;
On 2009/07/14 19:10:36, scherkus wrote:
> would it make sense for these to be inside the .cc file?

Moved.

http://codereview.chromium.org/149604/diff/1/6
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/149604/diff/1/6#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
On 2009/07/14 19:10:36, scherkus wrote:
> bump copyright year

Done.

http://codereview.chromium.org/149604/diff/1/10
File webkit/api/src/WebMediaPlayerClientImpl.cpp (right):

http://codereview.chromium.org/149604/diff/1/10#newcode204
Line 204: bool WebMediaPlayerClientImpl::supportsFullscreen() const {
On 2009/07/14 21:38:55, darin wrote:
> nit: "{" should be on the next line (webkit style)

Done.

http://codereview.chromium.org/149604/diff/1/10#newcode210
Line 210: bool WebMediaPlayerClientImpl::supportsSave() const {
On 2009/07/14 21:38:55, darin wrote:
> ditto

Done.

http://codereview.chromium.org/149604/diff/1/12
File webkit/glue/context_menu.h (right):

http://codereview.chromium.org/149604/diff/1/12#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
On 2009/07/14 19:10:36, scherkus wrote:
> bump copyright year

Done.

http://codereview.chromium.org/149604/diff/1/12#newcode107
Line 107: // invoked on.
On 2009/07/14 19:10:36, scherkus wrote:
> I'd give some sample elements where this is used (i.e., img, audio, video)

Done.

http://codereview.chromium.org/149604/diff/1/13
File webkit/glue/context_menu_client_impl.cc (right):

http://codereview.chromium.org/149604/diff/1/13#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
On 2009/07/14 19:10:36, scherkus wrote:
> bump copyright year

Done.

http://codereview.chromium.org/149604/diff/1/14
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/149604/diff/1/14#newcode240
Line 240: return true;
On 2009/07/14 19:10:36, scherkus wrote:
> so it looks like we have the context menu portion disabled, but say we support
> fullscreen here?
> 
> what if we left the context menu in, then returned false here?

The context menu completely ignores this right now, and probably will keep doing
that.  I made it return true incase WebCore eventually decides to do anything
based on this attribute, but it's currently just ignored here.

Would you like me to hook it into the context menu?

http://codereview.chromium.org/149604/diff/1/16
File webkit/glue/webview_delegate.h (right):

http://codereview.chromium.org/149604/diff/1/16#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
On 2009/07/14 19:10:36, scherkus wrote:
> bump copyright year

Done.

http://codereview.chromium.org/149604/diff/1/16#newcode60
Line 60: struct WebPreferences;
On 2009/07/14 19:10:36, scherkus wrote:
> I'm confused as to what the ordering should be here :P

Yeah, same here.  They put structs first in this block, and structs last in the
previous block.  I personally wouldn'te ven distinguish.  *sigh*.

I put structs last now to match the previous block.

[scherkus (not reviewing), 2009-07-15 20:51:21]

again, a little clueless but it LGTM

one nit on a comment, with a pun for your enjoyment

http://codereview.chromium.org/149604/diff/1007/48
File webkit/glue/context_menu.h (right):

http://codereview.chromium.org/149604/diff/1007/48#newcode72
Line 72: // True if the playback for this element should loop.
nit: wording is throwing me for a loop (PUN INTENTIONAL)

perhaps "True if playback is looping for this media element."

[awong, 2009-07-15 20:57:11]

one sec...adding play/pause mute/unmute!  Will upload new Cl in a few mins.

On Wed, Jul 15, 2009 at 1:51 PM, <scherkus@chromium.org> wrote:

> again, a little clueless but it LGTM
>
> one nit on a comment, with a pun for your enjoyment
>
>
>
> http://codereview.chromium.org/149604/diff/1007/48
> File webkit/glue/context_menu.h (right):
>
> http://codereview.chromium.org/149604/diff/1007/48#newcode72
> Line 72: // True if the playback for this element should loop.
> nit: wording is throwing me for a loop (PUN INTENTIONAL)
>
> perhaps "True if playback is looping for this media element."
>
>
> http://codereview.chromium.org/149604
>

[awong, 2009-07-15 21:43:34]

Okay, ready for final review.

[scherkus (not reviewing), 2009-07-16 00:08:51]

bunch of style nits, but LGTM

http://codereview.chromium.org/149604/diff/1029/60
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/149604/diff/1029/60#newcode123
Line 123: void RenderViewContextMenu::AppendAudioItems(ContextMenuMediaParams
media_params) {
over 80 chars

http://codereview.chromium.org/149604/diff/1029/60#newcode131
Line 131: void RenderViewContextMenu::AppendVideoItems(ContextMenuMediaParams
media_params) {
over 80 chars

http://codereview.chromium.org/149604/diff/1029/60#newcode141
Line 141: void RenderViewContextMenu::AppendMediaItems(ContextMenuMediaParams
media_params) {
over 80 chars

http://codereview.chromium.org/149604/diff/1029/61
File chrome/browser/tab_contents/render_view_context_menu.h (right):

http://codereview.chromium.org/149604/diff/1029/61#newcode79
Line 79: void AppendAudioItems(ContextMenuMediaParams  media_params);
two spaces -> one space

http://codereview.chromium.org/149604/diff/1029/68
File webkit/glue/context_menu.h (right):

http://codereview.chromium.org/149604/diff/1029/68#newcode72
Line 72: enum PlayerState {
throwing a crazy idea out here:

does it make sense to have some inline functions that check for these state
bits?

bool IsLooping() { return player_state & PLAYER_LOOP; }

that kind of stuff.  I'm on the fence and don't really care myself.

http://codereview.chromium.org/149604/diff/1029/68#newcode121
Line 121: // This is the URL of the top level page that the context menu was
invoked
extra space

http://codereview.chromium.org/149604/diff/1029/69
File webkit/glue/context_menu_client_impl.cc (right):

http://codereview.chromium.org/149604/diff/1029/69#newcode196
Line 196: media_params.player_state |= ContextMenuMediaParams::PLAYER_PAUSED;
over indented

http://codereview.chromium.org/149604/diff/1029/69#newcode199
Line 199: media_params.player_state |= ContextMenuMediaParams::PLAYER_MUTED;
over indented

http://codereview.chromium.org/149604/diff/1029/69#newcode202
Line 202: media_params.player_state |= ContextMenuMediaParams::PLAYER_LOOP;
over indented

http://codereview.chromium.org/149604/diff/1029/69#newcode205
Line 205: media_params.player_state |= ContextMenuMediaParams::PLAYER_CAN_SAVE;
over indented

[awong, 2009-07-16 00:17:50]

fixed style issues. committing.

http://codereview.chromium.org/149604/diff/1029/60
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/149604/diff/1029/60#newcode123
Line 123: void RenderViewContextMenu::AppendAudioItems(ContextMenuMediaParams
media_params) {
On 2009/07/16 00:08:51, scherkus wrote:
> over 80 chars

Done.

http://codereview.chromium.org/149604/diff/1029/60#newcode131
Line 131: void RenderViewContextMenu::AppendVideoItems(ContextMenuMediaParams
media_params) {
On 2009/07/16 00:08:51, scherkus wrote:
> over 80 chars

Done.

http://codereview.chromium.org/149604/diff/1029/60#newcode141
Line 141: void RenderViewContextMenu::AppendMediaItems(ContextMenuMediaParams
media_params) {
On 2009/07/16 00:08:51, scherkus wrote:
> over 80 chars

Done.

http://codereview.chromium.org/149604/diff/1029/68
File webkit/glue/context_menu.h (right):

http://codereview.chromium.org/149604/diff/1029/68#newcode72
Line 72: enum PlayerState {
On 2009/07/16 00:08:51, scherkus wrote:
> throwing a crazy idea out here:
> 
> does it make sense to have some inline functions that check for these state
> bits?
> 
> bool IsLooping() { return player_state & PLAYER_LOOP; }
> 
> that kind of stuff.  I'm on the fence and don't really care myself.

Since it's a struct, I would like to avoid behavior.  If it starts getting more
complicated, we can think extracting it later, but a single bitmask isn't sooo
bad I don't think.

http://codereview.chromium.org/149604/diff/1029/68#newcode121
Line 121: // This is the URL of the top level page that the context menu was
invoked
On 2009/07/16 00:08:51, scherkus wrote:
> extra space

Done.

http://codereview.chromium.org/149604/diff/1029/69
File webkit/glue/context_menu_client_impl.cc (right):

http://codereview.chromium.org/149604/diff/1029/69#newcode196
Line 196: media_params.player_state |= ContextMenuMediaParams::PLAYER_PAUSED;
On 2009/07/16 00:08:51, scherkus wrote:
> over indented

Done.

http://codereview.chromium.org/149604/diff/1029/69#newcode199
Line 199: media_params.player_state |= ContextMenuMediaParams::PLAYER_MUTED;
On 2009/07/16 00:08:51, scherkus wrote:
> over indented

Done.

http://codereview.chromium.org/149604/diff/1029/69#newcode202
Line 202: media_params.player_state |= ContextMenuMediaParams::PLAYER_LOOP;
On 2009/07/16 00:08:51, scherkus wrote:
> over indented

Done.

http://codereview.chromium.org/149604/diff/1029/69#newcode205
Line 205: media_params.player_state |= ContextMenuMediaParams::PLAYER_CAN_SAVE;
On 2009/07/16 00:08:51, scherkus wrote:
> over indented

Done.