Refactor the Photo Editor to enable new feature work

Refactor the Photo Editor to enable new feature work

BUG=142542
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=152319

[Vladislav Kaznacheev, 2012-08-16 09:46:51]

Please review.

[dgozman, 2012-08-17 07:11:02]

slide_mode.js is not reviewed yet.

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
File chrome/browser/resources/file_manager/js/image_editor/gallery_scripts.js
(left):

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/image_editor/gallery_scripts.js:20:
//<include src="gallery.js"/>
This file should be removed, right? I don't see it in CL.

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
File chrome/browser/resources/file_manager/js/image_editor/image_editor.js
(left):

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/image_editor/image_editor.js:241:
this.getViewport().sizeByFrameAndFit(this.container_);
Is sizeByFrameAndFit used anymore?

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
File chrome/browser/resources/file_manager/js/media/util.js (right):

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/media/util.js:10: * @param
{function():boolean} opt_toolsActive Function that returns |true|
{function():boolean=}

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/media/util.js:68: if
(this.mouseOverTool_ || this.toolsActive_())
this.toolsActive_ may be null/undefined here.

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/media/util.js:112: if
(!this.toolsActive_())
Ditto.

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
File chrome/browser/resources/file_manager/js/photo/gallery.js (right):

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/photo/gallery.js:72: var parentPath =
pathParts.join('/');
Is there a method in PathUtil?

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/photo/gallery.js:83: } else if
(FileType.isImageOrVideo(entry.name)) {
You can pass entry instead of name here.

This is wrong since |entry| may be a directory named like an image: folder.jpg.

I think, we should ensure that FileType.isXXX works fine for directory entries.

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/photo/gallery.js:115:
displayStringFunction: strf
strf is not defined.

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/photo/gallery.js:250: // TODO: handle
write errors gracefully (suggest retry or saving elsewhere).
I think, this TODO should go to the save code now.

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/photo/gallery.js:545:
ShareMode.prototype.updateMenu = function(urls) {
You can use new FileTasks(urls).getExternals(callback)

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
File chrome/browser/resources/file_manager/js/photo/gallery_item.js (right):

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/photo/gallery_item.js:30: return
ImageUtil.getFullNameFromUrl(this.url_);
I think, we should move this and similar methods to util.js

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/photo/gallery_item.js:129: var name =
this.getFileName();
We do usually put the code after defining all helper functions.

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/photo/gallery_item.js:176:
console.log('3', newFile);
Debug?

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
File chrome/browser/resources/file_manager/js/photo/ribbon.js (right):

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/photo/ribbon.js:142:
this.removeChild(box);
2 extra spaces

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/photo/ribbon.js:174:
createChild(thumbnail, 'image-wrapper');
util.createChild ?

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/photo/ribbon.js:203:
Ribbon.prototype.updateThumbnail = function(index, url, metadata) {
I think, this method should update/invalidate thumbnails in cache too.
Otherwise, you can get old thumbnail after scrolling.

[Vladislav Kaznacheev, 2012-08-17 09:20:40]

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
File chrome/browser/resources/file_manager/js/image_editor/gallery_scripts.js
(left):

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/image_editor/gallery_scripts.js:20:
//<include src="gallery.js"/>
On 2012/08/17 07:11:03, dgozman wrote:
> This file should be removed, right? I don't see it in CL.

Done.

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
File chrome/browser/resources/file_manager/js/image_editor/image_editor.js
(left):

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/image_editor/image_editor.js:241:
this.getViewport().sizeByFrameAndFit(this.container_);
This logic moved to SlideMode
On 2012/08/17 07:11:03, dgozman wrote:
> Is sizeByFrameAndFit used anymore?

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
File chrome/browser/resources/file_manager/js/media/util.js (right):

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/media/util.js:10: * @param
{function():boolean} opt_toolsActive Function that returns |true|
On 2012/08/17 07:11:03, dgozman wrote:
> {function():boolean=}

Done.

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/media/util.js:68: if
(this.mouseOverTool_ || this.toolsActive_())
Initialized it with function() { return false } if the parameter is skipped
On 2012/08/17 07:11:03, dgozman wrote:
> this.toolsActive_ may be null/undefined here.

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/media/util.js:112: if
(!this.toolsActive_())
On 2012/08/17 07:11:03, dgozman wrote:
> Ditto.

Done.

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
File chrome/browser/resources/file_manager/js/photo/gallery.js (right):

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/photo/gallery.js:72: var parentPath =
pathParts.join('/');
No.
On 2012/08/17 07:11:03, dgozman wrote:
> Is there a method in PathUtil?

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/photo/gallery.js:83: } else if
(FileType.isImageOrVideo(entry.name)) {
On 2012/08/17 07:11:03, dgozman wrote:
> You can pass entry instead of name here.
> 
> This is wrong since |entry| may be a directory named like an image:
folder.jpg.
> 
> I think, we should ensure that FileType.isXXX works fine for directory
entries.

Done.

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/photo/gallery.js:115:
displayStringFunction: strf
It is now. It recently moved from file_manager.js to util.js.

On 2012/08/17 07:11:03, dgozman wrote:
> strf is not defined.

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/photo/gallery.js:250: // TODO: handle
write errors gracefully (suggest retry or saving elsewhere).
We already have it there. Removing here.
On 2012/08/17 07:11:03, dgozman wrote:
> I think, this TODO should go to the save code now.

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/photo/gallery.js:545:
ShareMode.prototype.updateMenu = function(urls) {
My idea is to get rid of FileTasks.getExternals completely. This logic is only
required for the Gallery and should live here. Also the Gallery does not really
need to go through "executeWhenAvailable".

On 2012/08/17 07:11:03, dgozman wrote:
> You can use new FileTasks(urls).getExternals(callback)

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
File chrome/browser/resources/file_manager/js/photo/gallery_item.js (right):

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/photo/gallery_item.js:30: return
ImageUtil.getFullNameFromUrl(this.url_);
Our numerous *util.js files need a refactoring. Lets do that soon but not now.

On 2012/08/17 07:11:03, dgozman wrote:
> I think, we should move this and similar methods to util.js

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/photo/gallery_item.js:129: var name =
this.getFileName();
On 2012/08/17 07:11:03, dgozman wrote:
> We do usually put the code after defining all helper functions.

Done.

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/photo/gallery_item.js:176:
console.log('3', newFile);
On 2012/08/17 07:11:03, dgozman wrote:
> Debug?

Done.

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
File chrome/browser/resources/file_manager/js/photo/ribbon.js (right):

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/photo/ribbon.js:142:
this.removeChild(box);
On 2012/08/17 07:11:03, dgozman wrote:
> 2 extra spaces

Done.

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/photo/ribbon.js:174:
createChild(thumbnail, 'image-wrapper');
On 2012/08/17 07:11:03, dgozman wrote:
> util.createChild ?

Done.

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/photo/ribbon.js:203:
Ribbon.prototype.updateThumbnail = function(index, url, metadata) {
Actually we only ever update the current image's thumbnail and it is guaranteed
to be in the ribbon. Added the cache update anyway.
On 2012/08/17 07:11:03, dgozman wrote:
> I think, this method should update/invalidate thumbnails in cache too.
> Otherwise, you can get old thumbnail after scrolling.

Done.

[dgozman, 2012-08-17 09:48:37]

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
File chrome/browser/resources/file_manager/js/photo/slide_mode.js (right):

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/photo/slide_mode.js:95:
overwriteOriginalBox, undefined, 'label');
better pass '' instead of undefined

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/photo/slide_mode.js:205:
setTimeout(this.requestPrefetch.bind(this, 1), 1000);
Need a comment what '1' means.

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/photo/slide_mode.js:282: *   Derived
from the new and the old selected indices if omitted.
Can we always derive direction and pass it around?

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/photo/slide_mode.js:316:
this.ribbon_.update(this.items_, this.selectedIndex_);
Why passing items here? Could they change?

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/photo/slide_mode.js:403: var self =
this;
While we are here, change |self| to |this|?

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/photo/slide_mode.js:471: if
(this.items_.length < 2) return;
<= 1 is cleaner

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/photo/slide_mode.js:646: var
selLabelHighlighted =
typo: sel -> set

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/photo/slide_mode.js:706: // Is this
required at all?
Debug?

[dgozman, 2012-08-17 09:55:00]

A little more comments.

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
File chrome/browser/resources/file_manager/js/photo/gallery.js (right):

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/photo/gallery.js:115:
displayStringFunction: strf
On 2012/08/17 09:20:40, Vladislav Kaznacheev wrote:
> It is now. It recently moved from file_manager.js to util.js.
> 
> On 2012/08/17 07:11:03, dgozman wrote:
> > strf is not defined.
> 

Well, do we need displayStringFunction anymore? Just use str/strf everywhere.

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
File chrome/browser/resources/file_manager/js/photo/gallery_item.js (right):

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/photo/gallery_item.js:30: return
ImageUtil.getFullNameFromUrl(this.url_);
On 2012/08/17 09:20:40, Vladislav Kaznacheev wrote:
> Our numerous *util.js files need a refactoring. Lets do that soon but not now.

100% agree.

> 
> On 2012/08/17 07:11:03, dgozman wrote:
> > I think, we should move this and similar methods to util.js
>

https://chromiumcodereview.appspot.com/10834354/diff/4002/chrome/browser/reso...
File chrome/browser/resources/file_manager/js/photo/gallery.js (right):

https://chromiumcodereview.appspot.com/10834354/diff/4002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/photo/gallery.js:83: } else if
(FileType.isImageOrVideo(entry)) {
Did you change file_type.js?

[Vladislav Kaznacheev, 2012-08-17 10:30:49]

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
File chrome/browser/resources/file_manager/js/photo/gallery.js (right):

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/photo/gallery.js:115:
displayStringFunction: strf
We can avoid passing it to Gallery, but the Gallery constructor wraps strf into
a function that prepends 'GALLERY_' to the string id. I can create a static
Gallery.strf that does the same.

On 2012/08/17 09:55:00, dgozman wrote:
> On 2012/08/17 09:20:40, Vladislav Kaznacheev wrote:
> > It is now. It recently moved from file_manager.js to util.js.
> > 
> > On 2012/08/17 07:11:03, dgozman wrote:
> > > strf is not defined.
> > 
> 
> Well, do we need displayStringFunction anymore? Just use str/strf everywhere.

https://chromiumcodereview.appspot.com/10834354/diff/4002/chrome/browser/reso...
File chrome/browser/resources/file_manager/js/photo/gallery.js (right):

https://chromiumcodereview.appspot.com/10834354/diff/4002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/photo/gallery.js:83: } else if
(FileType.isImageOrVideo(entry)) {
Actually it already works. I updated JSDoc to make it clear.
On 2012/08/17 09:55:00, dgozman wrote:
> Did you change file_type.js?

[dgozman, 2012-08-18 06:21:33]

I think, you missed my comments to this file:
https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...

[Vladislav Kaznacheev, 2012-08-20 07:25:33]

PTAL

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
File chrome/browser/resources/file_manager/js/photo/slide_mode.js (right):

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/photo/slide_mode.js:95:
overwriteOriginalBox, undefined, 'label');
On 2012/08/17 09:48:37, dgozman wrote:
> better pass '' instead of undefined

Done.

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/photo/slide_mode.js:205:
setTimeout(this.requestPrefetch.bind(this, 1), 1000);
On 2012/08/17 09:48:37, dgozman wrote:
> Need a comment what '1' means.

Done.

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/photo/slide_mode.js:282: *   Derived
from the new and the old selected indices if omitted.
There is one case when deriving the direction is not desirable: when we wrap
around (going left from the first item or right from the last).
On 2012/08/17 09:48:37, dgozman wrote:
> Can we always derive direction and pass it around?

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/photo/slide_mode.js:316:
this.ribbon_.update(this.items_, this.selectedIndex_);
Not really, but they will be able to in the next patch.
On 2012/08/17 09:48:37, dgozman wrote:
> Why passing items here? Could they change?

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/photo/slide_mode.js:403: var self =
this;
On 2012/08/17 09:48:37, dgozman wrote:
> While we are here, change |self| to |this|?

Done.

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/photo/slide_mode.js:471: if
(this.items_.length < 2) return;
On 2012/08/17 09:48:37, dgozman wrote:
> <= 1 is cleaner

Done.

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/photo/slide_mode.js:646: var
selLabelHighlighted =
On 2012/08/17 09:48:37, dgozman wrote:
> typo: sel -> set

Done.

https://chromiumcodereview.appspot.com/10834354/diff/2002/chrome/browser/reso...
chrome/browser/resources/file_manager/js/photo/slide_mode.js:706: // Is this
required at all?
On 2012/08/17 09:48:37, dgozman wrote:
> Debug?

Removed the comment. I still don't understand how this branch is supposed to
work, I will ask you in person.

[dgozman, 2012-08-20 12:46:06]

LGTM