Issue 10399047: [Photo Editor] Save edited images immediately.

Issue 10399047: [Photo Editor] Save edited images immediately. (Closed)

Created:
8 years, 7 months ago by Vladislav Kaznacheev

Modified:
8 years, 7 months ago

Reviewers:
dgozman

CC:
chromium-reviews, rginda+watch_chromium.org, arv (Not doing code reviews)

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

[Photo Editor] Save edited images immediately. BUG=126898 TEST= Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=137643

Patch Set 2 : Addressed comments, rebased #

Patch Set 3 : Added a spinner cursor #

Created: 8 years, 7 months ago

Download [raw] [tar.bz2]

Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+210 lines, -362 lines)			Patch
chrome/browser/resources/file_manager/js/image_editor/commands.js	View	1	7 chunks	+78 lines, -42 lines	0 comments	Download
chrome/browser/resources/file_manager/js/image_editor/gallery.js	View	1 2	29 chunks	+96 lines, -192 lines	0 comments	Download
chrome/browser/resources/file_manager/js/image_editor/gallery_demo.js	View	1	1 chunk	+0 lines, -101 lines	0 comments	Download
chrome/browser/resources/file_manager/js/image_editor/image_editor.js	View		4 chunks	+29 lines, -25 lines	0 comments	Download
chrome/browser/resources/file_manager/js/image_editor/image_view.js	View		4 chunks	+7 lines, -2 lines	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

dgozman

Do we need some kind of spinner? LGTM https://chromiumcodereview.appspot.com/10399047/diff/1/chrome/browser/resources/file_manager/js/image_editor/commands.js File chrome/browser/resources/file_manager/js/image_editor/commands.js (right): https://chromiumcodereview.appspot.com/10399047/diff/1/chrome/browser/resources/file_manager/js/image_editor/commands.js#newcode81 chrome/browser/resources/file_manager/js/image_editor/commands.js:81: CommandQueue.prototype.save_ ...

8 years, 7 months ago (2012-05-16 16:05:40 UTC) #2

Vladislav Kaznacheev

8 years, 7 months ago (2012-05-17 09:04:40 UTC) #3

Thanks for the review!

Added a spinner (the standard one that kicks in after 1 second of busy state).

https://chromiumcodereview.appspot.com/10399047/diff/1/chrome/browser/resourc...
File chrome/browser/resources/file_manager/js/image_editor/commands.js (right):

https://chromiumcodereview.appspot.com/10399047/diff/1/chrome/browser/resourc...
chrome/browser/resources/file_manager/js/image_editor/commands.js:81:
CommandQueue.prototype.save_ = function() {
renamed to commit_ and added a JSDoc

On 2012/05/16 16:05:40, dgozman wrote:
> It's hard to understand the difference between |this.save_| and
> |this.saveFunction_|. Maybe rename to something like externalSaveFunction and
> saveAndBecomeAvailable?

https://chromiumcodereview.appspot.com/10399047/diff/1/chrome/browser/resourc...
chrome/browser/resources/file_manager/js/image_editor/commands.js:85:
this.setBusy_.bind(this, false));
Rewrote for clarity.
And yes, this assumes isBusy==true, there is a check in setBusy
On 2012/05/16 16:05:40, dgozman wrote:
> Is this setBusy a parameter to saveFunction? Add a comment please.
> Also, does this method assume that |isBusy==true| at the call time?

https://chromiumcodereview.appspot.com/10399047/diff/1/chrome/browser/resourc...
File chrome/browser/resources/file_manager/js/image_editor/gallery.js (left):

https://chromiumcodereview.appspot.com/10399047/diff/1/chrome/browser/resourc...
chrome/browser/resources/file_manager/js/image_editor/gallery.js:1216: if
(!dirEntry) {  // Happens only in gallery_demo.js
On 2012/05/16 16:05:40, dgozman wrote:
> I've just found the gallery_demo.js. We should remove it.

Done. Also removed the code needed to support it.

https://chromiumcodereview.appspot.com/10399047/diff/1/chrome/browser/resourc...
File chrome/browser/resources/file_manager/js/image_editor/gallery.js (right):

https://chromiumcodereview.appspot.com/10399047/diff/1/chrome/browser/resourc...
chrome/browser/resources/file_manager/js/image_editor/gallery.js:444:
this.selectedImageMetadata_, canvas, 1);
On 2012/05/16 16:05:40, dgozman wrote:
> Comment what 1 means.

Done.

Expand Messages | Collapse Messages