Gallery.app : Truncates the file after a blob of new image is created.

Gallery.app : Truncates the file after a blob of new image is created.

Previously Gallery.app truncates the file before a blob of new image is created.
This means that when some error happens during the creation of new image,
the file is overwritten with 0 bytes.
To avoid it, this CL creates the blob before truncates the file.

BUG=463837
TEST=out/Release/browser_tests --gtest_filter=GalleryJsTest.GalleryItemTest

Committed: https://crrev.com/0a7bfb6288eecd3fca6191f5f374c680fb788e32
Cr-Commit-Position: refs/heads/master@{#319817}

[yawano, 2015-03-09 05:35:52]

yawano@chromium.org changed reviewers:
+ fukino@chromium.org

[yawano, 2015-03-09 05:35:52]

PTAL. Thank you!

[fukino, 2015-03-09 07:12:35]

Thanks!

As this CL changes error handling and verification for the fix is not obvious,
so it's better to have test case or "TEST=" description about how to test the
code path mentioned in the issue.

https://codereview.chromium.org/988273002/diff/1/ui/file_manager/gallery/js/g...
File ui/file_manager/gallery/js/gallery_item.js (right):

https://codereview.chromium.org/988273002/diff/1/ui/file_manager/gallery/js/g...
ui/file_manager/gallery/js/gallery_item.js:267: var metadataItem =
metadataItems[0];
It's preferred to defer declaring variables until it's not used, so this code
block can be put after createWriter() as before.

Additionally, this block might be able to be a part of (the first part of) the
promise chain.

https://codereview.chromium.org/988273002/diff/1/ui/file_manager/gallery/js/g...
ui/file_manager/gallery/js/gallery_item.js:280: throw error;
Who handles this exception?

https://codereview.chromium.org/988273002/diff/1/ui/file_manager/gallery/js/g...
ui/file_manager/gallery/js/gallery_item.js:285: if (!newFile) {
optional nit: In if-else statement, inverted condition looks redundant to me
(Especially in simple cases).

[yawano, 2015-03-09 08:40:32]

Thank you for the reivew! Fixed issues, and added unit test. PTAL.

https://codereview.chromium.org/988273002/diff/1/ui/file_manager/gallery/js/g...
File ui/file_manager/gallery/js/gallery_item.js (right):

https://codereview.chromium.org/988273002/diff/1/ui/file_manager/gallery/js/g...
ui/file_manager/gallery/js/gallery_item.js:267: var metadataItem =
metadataItems[0];
On 2015/03/09 07:12:35, fukino wrote:
> It's preferred to defer declaring variables until it's not used, so this code
> block can be put after createWriter() as before.
> 
> Additionally, this block might be able to be a part of (the first part of) the
> promise chain. 

Changed to put this block in promise chain. In the promise chain, I put this
code block just after a promise which obtains metadata.

https://codereview.chromium.org/988273002/diff/1/ui/file_manager/gallery/js/g...
ui/file_manager/gallery/js/gallery_item.js:280: throw error;
On 2015/03/09 07:12:35, fukino wrote:
> Who handles this exception?

Done. After checking behavior, this exception is not handled properly. I changed
to use reject() call of Promise.

https://codereview.chromium.org/988273002/diff/1/ui/file_manager/gallery/js/g...
ui/file_manager/gallery/js/gallery_item.js:285: if (!newFile) {
On 2015/03/09 07:12:34, fukino wrote:
> optional nit: In if-else statement, inverted condition looks redundant to me
> (Especially in simple cases).

This promise is a promise to truncate the file to 0 byte if it overwrites. IMHO,
putting the codes of truncating the file (the main part of this promise) first
seems to be more readable for me. WDYT?

[fukino, 2015-03-10 01:45:13]

lgtm. Thanks!

[yawano, 2015-03-10 01:49:42]

On 2015/03/10 01:45:13, fukino wrote:
> lgtm. Thanks!

Thank you!

[yawano, 2015-03-10 01:49:47]

The CQ bit was checked by yawano@chromium.org

[commit-bot: I haz the power, 2015-03-10 01:50:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/988273002/40001

[commit-bot: I haz the power, 2015-03-10 01:54:06]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-10 01:55:11]

Patchset 3 (id:??) landed as
https://crrev.com/0a7bfb6288eecd3fca6191f5f374c680fb788e32
Cr-Commit-Position: refs/heads/master@{#319817}