Moving mediaplayer to the chrome filebrowser. Observable behaviour should not change.

Moving mediaplayer to the chrome filebrowser. Observable behaviour should not change.


BUG=chromium-os:14880
TEST=Click "play" button on an audion file in the file browser. Check that the mediaplayer panel has popped up. Click on another file. Check that the mediaplayer playlist panel has popped up and contains 2 items.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=87002

[SeRya, 2011-05-24 13:47:25]

Zel, have a look.

We discussed that the mediaplayer API should be part of fileBrowserPrivate
namespace. I'd prefer to keep it in a separate namespace (may be in a nested
namespace of fileBrowserPrivate).

There is an issue I'm aware but have not fixed yet: PlayList may reset pending
playback request flag that would cause loosing such a request.

[zel, 2011-05-24 17:54:24]

adding asargent@ as the second reviewer

http://codereview.chromium.org/7067020/diff/1050/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_mediaplayer_private_api.cc (right):

http://codereview.chromium.org/7067020/diff/1050/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_mediaplayer_private_api.cc:20:
CHECK(args_->GetInteger(0, &position));
I would't crash here. Just return false if GetInteger() fails.

http://codereview.chromium.org/7067020/diff/1050/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_mediaplayer_private_api.h (right):

http://codereview.chromium.org/7067020/diff/1050/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_mediaplayer_private_api.h:26: class
GetPlaylistMediaplayerFunction : public AsyncExtensionFunction {
None of these methods needs to be async (derived from AsyncExtensionFunction).
Please convert them to sync methods instead.

http://codereview.chromium.org/7067020/diff/1050/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_mediaplayer_private_api.h:38:
DECLARE_EXTENSION_FUNCTION_NAME("mediaPlayerPrivate.playbackError");
rename this to mediaPlayerPrivate.setPlaybackError

http://codereview.chromium.org/7067020/diff/1050/chrome/browser/ui/webui/medi...
File chrome/browser/ui/webui/mediaplayer_ui.cc (right):

http://codereview.chromium.org/7067020/diff/1050/chrome/browser/ui/webui/medi...
chrome/browser/ui/webui/mediaplayer_ui.cc:76: MediaPlayer::~MediaPlayer() {
can we move this to its own file media_player.cc/.h and put it somewhere in
chrome/browser/chromeos/media directory.

http://codereview.chromium.org/7067020/diff/1050/chrome/common/extensions/api...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/7067020/diff/1050/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.json:5119: "name":
"setPlaylistPositionAndPlay",
please rename to 'playAt'

http://codereview.chromium.org/7067020/diff/1050/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.json:5152: "type": "object",
this object should be defined outside of the function definition - see "types"
section under fileBrowserPrivate API for an example

http://codereview.chromium.org/7067020/diff/1050/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.json:5191: "name":
"showPlaylistPanel",
Why do have both show and toggle Panel methods? Can we merge these two?

http://codereview.chromium.org/7067020/diff/1050/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.json:5201: "parameters": []
we we want to add details about what changed here?

http://codereview.chromium.org/7067020/diff/1050/webkit/fileapi/file_system_m...
File webkit/fileapi/file_system_mount_point_provider.h (right):

http://codereview.chromium.org/7067020/diff/1050/webkit/fileapi/file_system_m...
webkit/fileapi/file_system_mount_point_provider.h:74: // Gets virtual path by
known filesystem path.
Add: Returns false when filesystem path is not exposed by this provider.

[SeRya, 2011-05-25 14:23:49]

http://codereview.chromium.org/7067020/diff/1050/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_mediaplayer_private_api.cc (right):

http://codereview.chromium.org/7067020/diff/1050/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_mediaplayer_private_api.cc:20:
CHECK(args_->GetInteger(0, &position));
On 2011/05/24 17:54:25, zel wrote:
> I would't crash here. Just return false if GetInteger() fails.

Done.

http://codereview.chromium.org/7067020/diff/1050/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_mediaplayer_private_api.h (right):

http://codereview.chromium.org/7067020/diff/1050/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_mediaplayer_private_api.h:26: class
GetPlaylistMediaplayerFunction : public AsyncExtensionFunction {
On 2011/05/24 17:54:25, zel wrote:
> None of these methods needs to be async (derived from AsyncExtensionFunction).
> Please convert them to sync methods instead.

Done.

http://codereview.chromium.org/7067020/diff/1050/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_mediaplayer_private_api.h:38:
DECLARE_EXTENSION_FUNCTION_NAME("mediaPlayerPrivate.playbackError");
On 2011/05/24 17:54:25, zel wrote:
> rename this to mediaPlayerPrivate.setPlaybackError

Done.

http://codereview.chromium.org/7067020/diff/1050/chrome/browser/ui/webui/medi...
File chrome/browser/ui/webui/mediaplayer_ui.cc (right):

http://codereview.chromium.org/7067020/diff/1050/chrome/browser/ui/webui/medi...
chrome/browser/ui/webui/mediaplayer_ui.cc:76: MediaPlayer::~MediaPlayer() {
On 2011/05/24 17:54:25, zel wrote:
> can we move this to its own file media_player.cc/.h and put it somewhere in
> chrome/browser/chromeos/media directory.

Done.

http://codereview.chromium.org/7067020/diff/1050/chrome/common/extensions/api...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/7067020/diff/1050/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.json:5119: "name":
"setPlaylistPositionAndPlay",
On 2011/05/24 17:54:25, zel wrote:
> please rename to 'playAt'

Done.

http://codereview.chromium.org/7067020/diff/1050/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.json:5152: "type": "object",
On 2011/05/24 17:54:25, zel wrote:
> this object should be defined outside of the function definition - see "types"
> section under fileBrowserPrivate API for an example

Done.

http://codereview.chromium.org/7067020/diff/1050/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.json:5191: "name":
"showPlaylistPanel",
On 2011/05/24 17:54:25, zel wrote:
> Why do have both show and toggle Panel methods? Can we merge these two?

This method was not used. I removed it.

http://codereview.chromium.org/7067020/diff/1050/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.json:5201: "parameters": []
On 2011/05/24 17:54:25, zel wrote:
> we we want to add details about what changed here?

I think we may keep it as is for now. We could pass a flag to inform that
playlist items have not changed (when we just move current position) to avoid
rebuilding playlist. Since we are going to move he playlist from separate panel
I'd postpone that to keep this CL a bit simplier.

http://codereview.chromium.org/7067020/diff/1050/webkit/fileapi/file_system_m...
File webkit/fileapi/file_system_mount_point_provider.h (right):

http://codereview.chromium.org/7067020/diff/1050/webkit/fileapi/file_system_m...
webkit/fileapi/file_system_mount_point_provider.h:74: // Gets virtual path by
known filesystem path.
On 2011/05/24 17:54:25, zel wrote:
> Add: Returns false when filesystem path is not exposed by this provider.

Done.

[zel, 2011-05-25 14:36:43]

LGTM

http://codereview.chromium.org/7067020/diff/1050/chrome/common/extensions/api...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/7067020/diff/1050/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.json:5201: "parameters": []
On 2011/05/25 14:23:49, SeRya wrote:
> On 2011/05/24 17:54:25, zel wrote:
> > we we want to add details about what changed here?
> 
> I think we may keep it as is for now. We could pass a flag to inform that
> playlist items have not changed (when we just move current position) to avoid
> rebuilding playlist. Since we are going to move he playlist from separate
panel
> I'd postpone that to keep this CL a bit simplier.

All right. We will remove this event anyway.

[asargent_no_longer_on_chrome, 2011-05-25 21:00:20]

It looks like this change is adding a new, non-experimental top-level extensions
API. We've started a new process where we have teams implementing extension
API's go through a design review process with several members of our team before
signing off on them. The document to sign up for this is at go/extensionsreview.
If for some reason this change is really time critical and you need the review
scheduled more quickly than the available slots you see there, please reach out
to Erik and/or Rahul.

[zel, 2011-05-25 21:14:29]

Nope, this is adding *private* API only that will be accessible only from
our file browser component extension (see
http://codereview.chromium.org/7067020/diff/6004/chrome/common/extensions/ext...
for
details).


On Wed, May 25, 2011 at 2:00 PM, <asargent@chromium.org> wrote:

> It looks like this change is adding a new, non-experimental top-level
> extensions
> API. We've started a new process where we have teams implementing extension
> API's go through a design review process with several members of our team
> before
> signing off on them. The document to sign up for this is at
> go/extensionsreview.
> If for some reason this change is really time critical and you need the
> review
> scheduled more quickly than the available slots you see there, please reach
> out
> to Erik and/or Rahul.
>
>
>
> http://codereview.chromium.org/7067020/
>