Moving slideshow to the filebrowser.

Moving slideshow to extension.

TEST=none
BUG=chromium-os:11430
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=82509
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=82851

[zel, 2011-04-20 16:51:21]

http://codereview.chromium.org/6873100/diff/3002/chrome/browser/resources/com...
File chrome/browser/resources/component_extension_resources.grd (right):

http://codereview.chromium.org/6873100/diff/3002/chrome/browser/resources/com...
chrome/browser/resources/component_extension_resources.grd:26: <include
name="IDR_FILE_MANAGER_BACKGROUND" file="file_manager/background.html"
flattenhtml="true" type="BINDATA" />
As I said, we should get rid of this one by any cost. I will try to pass tab
identifier with the event, so we can see which page it comes from.

http://codereview.chromium.org/6873100/diff/3002/chrome/browser/resources/fil...
File chrome/browser/resources/file_manager/manifest.json (right):

http://codereview.chromium.org/6873100/diff/3002/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/manifest.json:25: "filesystem:*.png"
please add capital cases of filters as well since they are case sensitive now

http://codereview.chromium.org/6873100/diff/3002/chrome/browser/resources/fil...
File chrome/browser/resources/file_manager/slideshow.html (right):

http://codereview.chromium.org/6873100/diff/3002/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/slideshow.html:178:
transitionTo(filelist[currentFileOffset], true);
you should test filelist.lenght here

http://codereview.chromium.org/6873100/diff/3002/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/slideshow.html:211: function
nextButtonClick() {
The slideshow is now looping through the list of selected files, we could also
make it loop through all files int the current directory since this extension is
now part of file browser (no permission issues as result of that). Once you get
entries, you can get its parent (directory) and build the list of entries from
there (starting looping at the selected file).

[zelidrag1, 2011-04-20 16:53:16]

LGTM

with nit below and additional work items to be filed in the tracker:

On Wed, Apr 20, 2011 at 9:51 AM, <zelidrag@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/6873100/diff/3002/chrome/browser/resources/com...
> File chrome/browser/resources/component_extension_resources.grd (right):
>
>
>
http://codereview.chromium.org/6873100/diff/3002/chrome/browser/resources/com...
> chrome/browser/resources/component_extension_resources.grd:26: <include
> name="IDR_FILE_MANAGER_BACKGROUND" file="file_manager/background.html"
> flattenhtml="true" type="BINDATA" />
> As I said, we should get rid of this one by any cost. I will try to pass
> tab identifier with the event, so we can see which page it comes from.
>
>
>
http://codereview.chromium.org/6873100/diff/3002/chrome/browser/resources/fil...
> File chrome/browser/resources/file_manager/manifest.json (right):
>
>
>
http://codereview.chromium.org/6873100/diff/3002/chrome/browser/resources/fil...
> chrome/browser/resources/file_manager/manifest.json:25:
> "filesystem:*.png"
> please add capital cases of filters as well since they are case
> sensitive now
>
>
>
http://codereview.chromium.org/6873100/diff/3002/chrome/browser/resources/fil...
> File chrome/browser/resources/file_manager/slideshow.html (right):
>
>
>
http://codereview.chromium.org/6873100/diff/3002/chrome/browser/resources/fil...
> chrome/browser/resources/file_manager/slideshow.html:178:
> transitionTo(filelist[currentFileOffset], true);
> you should test filelist.lenght here
>
>
>
http://codereview.chromium.org/6873100/diff/3002/chrome/browser/resources/fil...
> chrome/browser/resources/file_manager/slideshow.html:211: function
> nextButtonClick() {
> The slideshow is now looping through the list of selected files, we
> could also make it loop through all files int the current directory
> since this extension is now part of file browser (no permission issues
> as result of that). Once you get entries, you can get its parent
> (directory) and build the list of entries from there (starting looping
> at the selected file).
>
>
> http://codereview.chromium.org/6873100/
>

[commit-bot: I haz the power, 2011-04-21 14:05:21]

Can't process patch for file
chrome/browser/resources/file_manager/slideshow.html.
A +

[serya%chromium.org, 2011-04-21 14:05:31]

http://codereview.chromium.org/6873100/diff/3002/chrome/browser/resources/fil...
File chrome/browser/resources/file_manager/manifest.json (right):

http://codereview.chromium.org/6873100/diff/3002/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/manifest.json:25: "filesystem:*.png"
On 2011/04/20 16:51:21, zel wrote:
> please add capital cases of filters as well since they are case sensitive now

Done.

http://codereview.chromium.org/6873100/diff/3002/chrome/browser/resources/fil...
File chrome/browser/resources/file_manager/slideshow.html (right):

http://codereview.chromium.org/6873100/diff/3002/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/slideshow.html:178:
transitionTo(filelist[currentFileOffset], true);
On 2011/04/20 16:51:21, zel wrote:
> you should test filelist.lenght here

Done.

http://codereview.chromium.org/6873100/diff/3002/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/slideshow.html:211: function
nextButtonClick() {
On 2011/04/20 16:51:21, zel wrote:
> The slideshow is now looping through the list of selected files, we could also
> make it loop through all files int the current directory since this extension
is
> now part of file browser (no permission issues as result of that). Once you
get
> entries, you can get its parent (directory) and build the list of entries from
> there (starting looping at the selected file).

Will do separately since it's a new feature.

[commit-bot: I haz the power, 2011-04-21 14:05:47]

Can't process patch for file
chrome/browser/resources/file_manager/slideshow.html.
A +

[commit-bot: I haz the power, 2011-04-21 17:28:29]

Can't process patch for file
chrome/browser/resources/file_manager/slideshow.html.
A  +

[serya%chromium.org, 2011-04-23 06:55:49]

Fixed browser test in linux_chromeos. A few tests doesn't expect any new
background pages (such as tests of the Task Manager what check specific number
of processes in the panel).

The background page removed (that's also good from performance perspective). The
slideshow task is handled around chrome.fileBrowserPrivate.executeTask.

[zel, 2011-04-23 07:42:46]

http://codereview.chromium.org/6873100/diff/12015/chrome/browser/resources/fi...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

http://codereview.chromium.org/6873100/diff/12015/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/js/file_manager.js:984: //             
Adding it crushes a few of browser tests (they wonder
typo: "crashes"

also, the tests are not angry anymore (i had to do the same fix for picasa ext),
but it is still good that we don't use background page here

http://codereview.chromium.org/6873100/diff/12015/chrome/browser/resources/fi...
File chrome/browser/resources/file_manager/slideshow.html (right):

http://codereview.chromium.org/6873100/diff/12015/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/slideshow.html:48: background:
url('chrome://resources/images/mediaplayer_prev.png');
this resource should be part of this extension, not shared

http://codereview.chromium.org/6873100/diff/12015/chrome/browser/resources/sh...
File chrome/browser/resources/shared_resources.grd (right):

http://codereview.chromium.org/6873100/diff/12015/chrome/browser/resources/sh...
chrome/browser/resources/shared_resources.grd:36:
file="shared/images/mediaplayer_prev.png" type="BINDATA" />
why is this here instead of the extension directory?

[serya%chromium.org, 2011-04-23 08:34:13]

http://codereview.chromium.org/6873100/diff/12015/chrome/browser/resources/fi...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

http://codereview.chromium.org/6873100/diff/12015/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/js/file_manager.js:984: //             
Adding it crushes a few of browser tests (they wonder
On 2011/04/23 07:42:46, zel wrote:
> typo: "crashes"
> 
> also, the tests are not angry anymore (i had to do the same fix for picasa
ext),
> but it is still good that we don't use background page here

Done.

http://codereview.chromium.org/6873100/diff/12015/chrome/browser/resources/sh...
File chrome/browser/resources/shared_resources.grd (right):

http://codereview.chromium.org/6873100/diff/12015/chrome/browser/resources/sh...
chrome/browser/resources/shared_resources.grd:36:
file="shared/images/mediaplayer_prev.png" type="BINDATA" />
On 2011/04/23 07:42:46, zel wrote:
> why is this here instead of the extension directory?

It was shared among slideshow.html and mediaplayer.html (also we have
mediaplayer_full_screen.png, mediaplayer_pause.png, mediaplayer_play.png,
mediaplayer_playlist.png, mediaplayer_vol_high.png, mediaplayer_vol_mute.png and
they are not included in any resource pack) and placed in the "shared"
directory. If we are going to resuscitate the mediaplayer we need to keep the
images in the shared resources (or clode them).

[zel, 2011-04-23 16:46:47]

LGTM

http://codereview.chromium.org/6873100/diff/12015/chrome/browser/resources/sh...
File chrome/browser/resources/shared_resources.grd (right):

http://codereview.chromium.org/6873100/diff/12015/chrome/browser/resources/sh...
chrome/browser/resources/shared_resources.grd:36:
file="shared/images/mediaplayer_prev.png" type="BINDATA" />
On 2011/04/23 08:34:13, serya%chromium.org wrote:
> On 2011/04/23 07:42:46, zel wrote:
> > why is this here instead of the extension directory?
> 
> It was shared among slideshow.html and mediaplayer.html (also we have
> mediaplayer_full_screen.png, mediaplayer_pause.png, mediaplayer_play.png,
> mediaplayer_playlist.png, mediaplayer_vol_high.png, mediaplayer_vol_mute.png
and
> they are not included in any resource pack) and placed in the "shared"
> directory. If we are going to resuscitate the mediaplayer we need to keep the
> images in the shared resources (or clode them).

I can live with that right now, but we should move media player into this
component in M13 and then this resource will be actually private to file browser
component.

[commit-bot: I haz the power, 2011-04-25 07:42:12]

Can't process patch for file
chrome/browser/resources/file_manager/slideshow.html.
A  +