Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1608143002:
support animated GIF (Closed)
Descriptionsupport animated GIF
- removes "device resolution" canvas.
- allows to use both img element and canvas element to show image. This allows to show animated GIF.
- If you need a ImageData of a image, convertes from img to canvas.
- This patch speeds up to load large jpg image (@ daisy spring)
- loading 14MB 12000x12000 JPG file:
8.62 -> 7.37sec
it crashed sometimes in the unpatched revision.
- loading 53MB, 8,180x11,950 PNG file:
6.89 -> 6.81sec
- it's time consuming to exec pixel operation.
- ex) making histgrams to check whether AutoColor filter is applicative.
- We should consider to use GLSL to process pixels.
BUG=446203
TEST=manually
Committed: https://crrev.com/1f78b825f0b429ecbadafd524f1a5cefc01aae4d
Cr-Commit-Position: refs/heads/master@{#371191}
Patch Set 1 #
Total comments: 28
Patch Set 2 : fix except anime issues #Patch Set 3 : refactor undo/redo to animate correctly #Patch Set 4 : fix nits #
Total comments: 13
Patch Set 5 : . #Patch Set 6 : ensure that fade attr. is false & do not animate #
Total comments: 4
Patch Set 7 : fix nits and a selector query in tests #Patch Set 8 : fix test #Patch Set 9 : fix test #
Total comments: 5
Patch Set 10 : rename width/height to imageWidth/imageHeight #Patch Set 11 : fix a regression bug that the original image is not removed then the filter applied. #
Total comments: 16
Patch Set 12 : fix in response to the reviews #
Total comments: 4
Patch Set 13 : set "fade" attribute. #Patch Set 14 : add assert to pass closure compiler #Messages
Total messages: 41 (13 generated)
Description was changed from
==========
support animated GIF
BUG=446203
TEST=manually
==========
to
==========
support animated GIF
- removes "device resolution" canvas.
- allows to use both img element and canvas element to show image. This allows
to show animated GIF.
- If you need a ImageData of a image, convertes from img to canvas.
- This patch speeds up to load large jpg image (@ daisy spring)
- loading 14MB 12000x12000 JPG file:
8.62 -> 7.37sec
it crashed sometimes in the unpatched revision.
- loading 53MB, 8,180x11,950 PNG file:
6.89 -> 6.81sec
BUG=446203
TEST=manually
==========
Description was changed from
==========
support animated GIF
- removes "device resolution" canvas.
- allows to use both img element and canvas element to show image. This allows
to show animated GIF.
- If you need a ImageData of a image, convertes from img to canvas.
- This patch speeds up to load large jpg image (@ daisy spring)
- loading 14MB 12000x12000 JPG file:
8.62 -> 7.37sec
it crashed sometimes in the unpatched revision.
- loading 53MB, 8,180x11,950 PNG file:
6.89 -> 6.81sec
BUG=446203
TEST=manually
==========
to
==========
support animated GIF
- removes "device resolution" canvas.
- allows to use both img element and canvas element to show image. This allows
to show animated GIF.
- If you need a ImageData of a image, convertes from img to canvas.
- This patch speeds up to load large jpg image (@ daisy spring)
- loading 14MB 12000x12000 JPG file:
8.62 -> 7.37sec
it crashed sometimes in the unpatched revision.
- loading 53MB, 8,180x11,950 PNG file:
6.89 -> 6.81sec
- it's time consuming to exec pixel operation.
- ex) making histgrams to check whether AutoColor filter is applicative.
- We should consider to use GLSL to process pixels.
BUG=446203
TEST=manually
==========
ryoh@chromium.org changed reviewers: + hirono@chromium.org, yawano@chromium.org
Hi, here is the gif-anime patch. Could you take a look?
https://codereview.chromium.org/1608143002/diff/1/ui/file_manager/gallery/js/... File ui/file_manager/gallery/js/image_editor/image_util.js (right): https://codereview.chromium.org/1608143002/diff/1/ui/file_manager/gallery/js/... ui/file_manager/gallery/js/image_editor/image_util.js:623: try { Why is this try closure needed? https://codereview.chromium.org/1608143002/diff/1/ui/file_manager/gallery/js/... File ui/file_manager/gallery/js/image_editor/image_view.js (left): https://codereview.chromium.org/1608143002/diff/1/ui/file_manager/gallery/js/... ui/file_manager/gallery/js/image_editor/image_view.js:544: if (opt_zoomToRect && this.screenImage_) { I think we cannot remove this zoom out effect. Could you double check? https://codereview.chromium.org/1608143002/diff/1/ui/file_manager/gallery/js/... ui/file_manager/gallery/js/image_editor/image_view.js:799: this.setTransform_(oldScreenImage, this.viewport_, effect); It looks we cannot remove this effect. https://codereview.chromium.org/1608143002/diff/1/ui/file_manager/gallery/js/... File ui/file_manager/gallery/js/image_editor/image_view.js (right): https://codereview.chromium.org/1608143002/diff/1/ui/file_manager/gallery/js/... ui/file_manager/gallery/js/image_editor/image_view.js:41: * The content canvas element. This is existing document error, but could you update the comment? https://codereview.chromium.org/1608143002/diff/1/ui/file_manager/gallery/js/... ui/file_manager/gallery/js/image_editor/image_view.js:260: this.contentImage_.getContext('2d'), canvas, deviceRect, imageRect); Can we ensure this.contentImage_ is a canvas here? https://codereview.chromium.org/1608143002/diff/1/ui/file_manager/gallery/js/... ui/file_manager/gallery/js/image_editor/image_view.js:305: ImageView.prototype.getScreenImageDataWith = function(width, height) { Could you update the caller of this method so that it does not repeat scaling large image twice? https://code.google.com/p/chromium/codesearch#chromium/src/ui/file_manager/ga... https://codereview.chromium.org/1608143002/diff/1/ui/file_manager/gallery/js/... ui/file_manager/gallery/js/image_editor/image_view.js:551: this.contentImage_, this.viewport_, null, 0); nit: It looks to fit in one line. https://codereview.chromium.org/1608143002/diff/1/ui/file_manager/gallery/js/... ui/file_manager/gallery/js/image_editor/image_view.js:579: * @param {!HTMLCanvasElement|!HTMLImageElement} canvas The source canvas. Please update 'canvas' with right name.
https://codereview.chromium.org/1608143002/diff/1/ui/file_manager/gallery/js/... File ui/file_manager/gallery/js/image_editor/commands.js (right): https://codereview.chromium.org/1608143002/diff/1/ui/file_manager/gallery/js/... ui/file_manager/gallery/js/image_editor/commands.js:12: * The canvas with the original image. small nit: please use the line until the end. i.e. @param {...} image The canvas ... [New line] ... image. Same goes for other comments. https://codereview.chromium.org/1608143002/diff/1/ui/file_manager/gallery/js/... File ui/file_manager/gallery/js/image_editor/filter.js (right): https://codereview.chromium.org/1608143002/diff/1/ui/file_manager/gallery/js/... ui/file_manager/gallery/js/image_editor/filter.js:33: * @return {!HTMLCanvasElement} canvas. nit: @private https://codereview.chromium.org/1608143002/diff/1/ui/file_manager/gallery/js/... File ui/file_manager/gallery/js/image_editor/image_view.js (right): https://codereview.chromium.org/1608143002/diff/1/ui/file_manager/gallery/js/... ui/file_manager/gallery/js/image_editor/image_view.js:213: * @return {HTMLCanvasElement|HTMLImageElement} The content image(or element). (or canvas) element? https://codereview.chromium.org/1608143002/diff/1/ui/file_manager/gallery/js/... ui/file_manager/gallery/js/image_editor/image_view.js:311: return this.contentImage_.getImageData(0,0,width,height); nit: put space after ",". https://codereview.chromium.org/1608143002/diff/1/ui/file_manager/gallery/js/... File ui/file_manager/gallery/js/slide_mode.js (right): https://codereview.chromium.org/1608143002/diff/1/ui/file_manager/gallery/js/... ui/file_manager/gallery/js/slide_mode.js:1302: this.imageView_.getImage(), Please add assertInstanceOf(, CanvasElement). GalleryDataModel.saveItem expect it as a canvas.
https://codereview.chromium.org/1608143002/diff/1/ui/file_manager/gallery/js/... File ui/file_manager/gallery/js/image_editor/image_util.js (right): https://codereview.chromium.org/1608143002/diff/1/ui/file_manager/gallery/js/... ui/file_manager/gallery/js/image_editor/image_util.js:494: image, opt_transform); nit: this line also fits in one line.
Hi, I updated the patch in response to your reviews and refactored undo/redo queue to animate correctly. Could you take another look? https://codereview.chromium.org/1608143002/diff/1/ui/file_manager/gallery/js/... File ui/file_manager/gallery/js/image_editor/commands.js (right): https://codereview.chromium.org/1608143002/diff/1/ui/file_manager/gallery/js/... ui/file_manager/gallery/js/image_editor/commands.js:12: * The canvas with the original image. On 2016/01/20 06:23:09, yawano wrote: > small nit: please use the line until the end. i.e. @param {...} image The canvas > ... [New line] ... image. > > Same goes for other comments. Done. https://codereview.chromium.org/1608143002/diff/1/ui/file_manager/gallery/js/... File ui/file_manager/gallery/js/image_editor/filter.js (right): https://codereview.chromium.org/1608143002/diff/1/ui/file_manager/gallery/js/... ui/file_manager/gallery/js/image_editor/filter.js:33: * @return {!HTMLCanvasElement} canvas. On 2016/01/20 06:23:09, yawano wrote: > nit: @private Done. https://codereview.chromium.org/1608143002/diff/1/ui/file_manager/gallery/js/... File ui/file_manager/gallery/js/image_editor/image_util.js (right): https://codereview.chromium.org/1608143002/diff/1/ui/file_manager/gallery/js/... ui/file_manager/gallery/js/image_editor/image_util.js:494: image, opt_transform); On 2016/01/20 06:41:33, yawano wrote: > nit: this line also fits in one line. Done. https://codereview.chromium.org/1608143002/diff/1/ui/file_manager/gallery/js/... ui/file_manager/gallery/js/image_editor/image_util.js:623: try { On 2016/01/20 06:07:09, hirono wrote: > Why is this try closure needed? It's copied from here: https://code.google.com/p/chromium/codesearch#chromium/src/ui/file_manager/ga... I think this try block is also not required. I will remove both of them. https://codereview.chromium.org/1608143002/diff/1/ui/file_manager/gallery/js/... File ui/file_manager/gallery/js/image_editor/image_view.js (left): https://codereview.chromium.org/1608143002/diff/1/ui/file_manager/gallery/js/... ui/file_manager/gallery/js/image_editor/image_view.js:544: if (opt_zoomToRect && this.screenImage_) { On 2016/01/20 06:07:09, hirono wrote: > I think we cannot remove this zoom out effect. Could you double check? Yeah, we can't remove. Thanks. https://codereview.chromium.org/1608143002/diff/1/ui/file_manager/gallery/js/... ui/file_manager/gallery/js/image_editor/image_view.js:799: this.setTransform_(oldScreenImage, this.viewport_, effect); On 2016/01/20 06:07:09, hirono wrote: > It looks we cannot remove this effect. Done. https://codereview.chromium.org/1608143002/diff/1/ui/file_manager/gallery/js/... File ui/file_manager/gallery/js/image_editor/image_view.js (right): https://codereview.chromium.org/1608143002/diff/1/ui/file_manager/gallery/js/... ui/file_manager/gallery/js/image_editor/image_view.js:41: * The content canvas element. On 2016/01/20 06:07:09, hirono wrote: > This is existing document error, but could you update the comment? Done. https://codereview.chromium.org/1608143002/diff/1/ui/file_manager/gallery/js/... ui/file_manager/gallery/js/image_editor/image_view.js:213: * @return {HTMLCanvasElement|HTMLImageElement} The content image(or element). On 2016/01/20 06:23:09, yawano wrote: > (or canvas) element? Done. https://codereview.chromium.org/1608143002/diff/1/ui/file_manager/gallery/js/... ui/file_manager/gallery/js/image_editor/image_view.js:260: this.contentImage_.getContext('2d'), canvas, deviceRect, imageRect); On 2016/01/20 06:07:10, hirono wrote: > Can we ensure this.contentImage_ is a canvas here? Done. https://codereview.chromium.org/1608143002/diff/1/ui/file_manager/gallery/js/... ui/file_manager/gallery/js/image_editor/image_view.js:305: ImageView.prototype.getScreenImageDataWith = function(width, height) { On 2016/01/20 06:07:09, hirono wrote: > Could you update the caller of this method so that it does not repeat scaling > large image twice? > https://code.google.com/p/chromium/codesearch#chromium/src/ui/file_manager/ga... Done. https://codereview.chromium.org/1608143002/diff/1/ui/file_manager/gallery/js/... ui/file_manager/gallery/js/image_editor/image_view.js:311: return this.contentImage_.getImageData(0,0,width,height); On 2016/01/20 06:23:09, yawano wrote: > nit: put space after ",". Done. https://codereview.chromium.org/1608143002/diff/1/ui/file_manager/gallery/js/... ui/file_manager/gallery/js/image_editor/image_view.js:551: this.contentImage_, this.viewport_, null, 0); On 2016/01/20 06:07:09, hirono wrote: > nit: It looks to fit in one line. Done. https://codereview.chromium.org/1608143002/diff/1/ui/file_manager/gallery/js/... ui/file_manager/gallery/js/image_editor/image_view.js:579: * @param {!HTMLCanvasElement|!HTMLImageElement} canvas The source canvas. On 2016/01/20 06:07:09, hirono wrote: > Please update 'canvas' with right name. Done. https://codereview.chromium.org/1608143002/diff/1/ui/file_manager/gallery/js/... File ui/file_manager/gallery/js/slide_mode.js (right): https://codereview.chromium.org/1608143002/diff/1/ui/file_manager/gallery/js/... ui/file_manager/gallery/js/slide_mode.js:1302: this.imageView_.getImage(), On 2016/01/20 06:23:09, yawano wrote: > Please add assertInstanceOf(, CanvasElement). GalleryDataModel.saveItem expect > it as a canvas. Done.
Thank you! https://codereview.chromium.org/1608143002/diff/60001/ui/file_manager/gallery... File ui/file_manager/gallery/js/image_editor/commands.js (right): https://codereview.chromium.org/1608143002/diff/60001/ui/file_manager/gallery... ui/file_manager/gallery/js/image_editor/commands.js:26: nit: remove blank line from line 26. And insert a blank line above line 22. https://codereview.chromium.org/1608143002/diff/60001/ui/file_manager/gallery... File ui/file_manager/gallery/js/image_editor/image_util.js (left): https://codereview.chromium.org/1608143002/diff/60001/ui/file_manager/gallery... ui/file_manager/gallery/js/image_editor/image_util.js:614: this.image_.onload = function() {}; Sorry, I didn't pointed this out in the last review. If we remove this.image_, the rest process of image loading (metadata fetch, transform) will continue even if it's cancelled. Also is it okay to remove the hack in line 617? https://codereview.chromium.org/1608143002/diff/60001/ui/file_manager/gallery... File ui/file_manager/gallery/js/image_editor/image_view.js (right): https://codereview.chromium.org/1608143002/diff/60001/ui/file_manager/gallery... ui/file_manager/gallery/js/image_editor/image_view.js:263: ImageRect.drawImage(this.contentImage_, canvas, deviceRect, imageRect); Is it okay to pass an image element as the first argument of ImageRect.drawImage? https://codereview.chromium.org/1608143002/diff/60001/ui/file_manager/gallery... ui/file_manager/gallery/js/image_editor/image_view.js:556: this.container_.appendChild(this.contentImage_); nit: unnecessary. contentImage_ should be children of container at this point. https://codereview.chromium.org/1608143002/diff/60001/ui/file_manager/gallery... ui/file_manager/gallery/js/image_editor/image_view.js:752: }.bind(this)); nit: add 0 as second argument. We don't emit it.
Hi, could you take another look? https://codereview.chromium.org/1608143002/diff/60001/ui/file_manager/gallery... File ui/file_manager/gallery/js/image_editor/commands.js (right): https://codereview.chromium.org/1608143002/diff/60001/ui/file_manager/gallery... ui/file_manager/gallery/js/image_editor/commands.js:26: On 2016/01/21 05:14:38, yawano wrote: > nit: remove blank line from line 26. And insert a blank line above line 22. Done. https://codereview.chromium.org/1608143002/diff/60001/ui/file_manager/gallery... File ui/file_manager/gallery/js/image_editor/image_util.js (left): https://codereview.chromium.org/1608143002/diff/60001/ui/file_manager/gallery... ui/file_manager/gallery/js/image_editor/image_util.js:614: this.image_.onload = function() {}; On 2016/01/21 05:14:38, yawano wrote: > Sorry, I didn't pointed this out in the last review. If we remove this.image_, > the rest process of image loading (metadata fetch, transform) will continue even > if it's cancelled. > > Also is it okay to remove the hack in line 617? I think it's controlled by "this.generation_" to cancel these process. Yeah, it's ok to remove this hack. We do not reuse img elements any more. https://codereview.chromium.org/1608143002/diff/60001/ui/file_manager/gallery... File ui/file_manager/gallery/js/image_editor/image_view.js (right): https://codereview.chromium.org/1608143002/diff/60001/ui/file_manager/gallery... ui/file_manager/gallery/js/image_editor/image_view.js:263: ImageRect.drawImage(this.contentImage_, canvas, deviceRect, imageRect); On 2016/01/21 05:14:38, yawano wrote: > Is it okay to pass an image element as the first argument of > ImageRect.drawImage? Done. https://codereview.chromium.org/1608143002/diff/60001/ui/file_manager/gallery... ui/file_manager/gallery/js/image_editor/image_view.js:556: this.container_.appendChild(this.contentImage_); On 2016/01/21 05:14:38, yawano wrote: > nit: unnecessary. contentImage_ should be children of container at this point. Done. https://codereview.chromium.org/1608143002/diff/60001/ui/file_manager/gallery... ui/file_manager/gallery/js/image_editor/image_view.js:752: }.bind(this)); On 2016/01/21 05:14:38, yawano wrote: > nit: add 0 as second argument. We don't emit it. Done.
Thank you! Please try to open an image which is larger than window size. You will see image is shown with zoom animation. Also please check that some animation seems to be applied to an image when window is resized. https://codereview.chromium.org/1608143002/diff/60001/ui/file_manager/gallery... File ui/file_manager/gallery/js/image_editor/image_util.js (left): https://codereview.chromium.org/1608143002/diff/60001/ui/file_manager/gallery... ui/file_manager/gallery/js/image_editor/image_util.js:614: this.image_.onload = function() {}; Yes, this.generation_ works. I've missed the last line of this method. This hack seems to be added for a long time ago, and if you don't see any problem, it will be okay to remove this hack for now. Thanks. https://codereview.chromium.org/1608143002/diff/60001/ui/file_manager/gallery... File ui/file_manager/gallery/js/image_editor/image_util.js (right): https://codereview.chromium.org/1608143002/diff/60001/ui/file_manager/gallery... ui/file_manager/gallery/js/image_editor/image_util.js:524: this.metadataModel_.get([entry], ['contentImageTransform']).then( nit: How about to add generation check here not to fetch unnecessary metadata?
Hi, thanks for the reviews. Update: - Fixed a probrem that you can't see any image when you select a image in thumbnail view mode. - Disabling CSS animation when the image inserted to canvas. You won't see any animation when you resize the window. Please take another look! https://codereview.chromium.org/1608143002/diff/60001/ui/file_manager/gallery... File ui/file_manager/gallery/js/image_editor/image_util.js (right): https://codereview.chromium.org/1608143002/diff/60001/ui/file_manager/gallery... ui/file_manager/gallery/js/image_editor/image_util.js:524: this.metadataModel_.get([entry], ['contentImageTransform']).then( On 2016/01/21 08:08:14, yawano wrote: > nit: How about to add generation check here not to fetch unnecessary metadata? It sounds nice. I will add checks.
lgtm. Please also wait l-g-t-m from hirono@. nit: Is the last item in the description what you are doing with this CL? It would be better to write what you do with this CL in description. https://codereview.chromium.org/1608143002/diff/100001/ui/file_manager/galler... File ui/file_manager/gallery/js/image_editor/image_util.js (right): https://codereview.chromium.org/1608143002/diff/100001/ui/file_manager/galler... ui/file_manager/gallery/js/image_editor/image_util.js:528: if (generation !== this.generation_) nit: I don't think we need this second check since onTransform do check in the beginning of its function.
It looks GalleryBrowserTest is broken. Could you take a look? https://codereview.chromium.org/1608143002/diff/100001/ui/file_manager/galler... File ui/file_manager/gallery/js/image_editor/image_view.js (right): https://codereview.chromium.org/1608143002/diff/100001/ui/file_manager/galler... ui/file_manager/gallery/js/image_editor/image_view.js:586: * canvas. nit: Please fix the indent
Hi, thanks for your reviews. I fixed the tests and all tests are green now. Could you take another look? https://codereview.chromium.org/1608143002/diff/100001/ui/file_manager/galler... File ui/file_manager/gallery/js/image_editor/image_util.js (right): https://codereview.chromium.org/1608143002/diff/100001/ui/file_manager/galler... ui/file_manager/gallery/js/image_editor/image_util.js:528: if (generation !== this.generation_) On 2016/01/21 09:11:32, yawano wrote: > nit: I don't think we need this second check since onTransform do check in the > beginning of its function. That's right! https://codereview.chromium.org/1608143002/diff/100001/ui/file_manager/galler... File ui/file_manager/gallery/js/image_editor/image_view.js (right): https://codereview.chromium.org/1608143002/diff/100001/ui/file_manager/galler... ui/file_manager/gallery/js/image_editor/image_view.js:586: * canvas. On 2016/01/21 10:12:16, hirono wrote: > nit: Please fix the indent Done.
https://codereview.chromium.org/1608143002/diff/160001/ui/file_manager/file_m... File ui/file_manager/file_manager/background/js/test_util_base.js (right): https://codereview.chromium.org/1608143002/diff/160001/ui/file_manager/file_m... ui/file_manager/file_manager/background/js/test_util_base.js:43: width: Number(element.width), Why don't you simply use renderedWidth? I don't think we need to have this width attribute here.
Thank you for your reviews! I will reply to you. Please take a look. https://codereview.chromium.org/1608143002/diff/160001/ui/file_manager/file_m... File ui/file_manager/file_manager/background/js/test_util_base.js (right): https://codereview.chromium.org/1608143002/diff/160001/ui/file_manager/file_m... ui/file_manager/file_manager/background/js/test_util_base.js:43: width: Number(element.width), On 2016/01/22 05:03:12, yawano wrote: > Why don't you simply use renderedWidth? I don't think we need to have this width > attribute here. It's needed to check both "size": - "width/height" for testing that images are loaded correctly. - "renderedWidth/renderedHeight" for testing that images are scaled correctly. There are two canvases before, full-res and device-res, so all we need to check is only width/height for these two canvases. We have only a full-res image now, so we have to check these two attributes instead.
Thank you! https://codereview.chromium.org/1608143002/diff/160001/ui/file_manager/file_m... File ui/file_manager/file_manager/background/js/test_util_base.js (right): https://codereview.chromium.org/1608143002/diff/160001/ui/file_manager/file_m... ui/file_manager/file_manager/background/js/test_util_base.js:43: width: Number(element.width), I get it. Thank you! nit: many of other elements don't have width/height fields. How about to rename this to imageWidth and set it only when element is image? Setting undefined will be okay if element is not image.
Thank you for the review. I renamed width/height to imageWidth/imageHeight, and fix a regression bug that the original image is not removed then you apply color filter: https://codereview.chromium.org/1608143002/diff2/180001:200001/ui/file_manage... https://codereview.chromium.org/1608143002/diff/160001/ui/file_manager/file_m... File ui/file_manager/file_manager/background/js/test_util_base.js (right): https://codereview.chromium.org/1608143002/diff/160001/ui/file_manager/file_m... ui/file_manager/file_manager/background/js/test_util_base.js:43: width: Number(element.width), On 2016/01/22 05:52:04, yawano wrote: > I get it. Thank you! > > nit: many of other elements don't have width/height fields. How about to rename > this to imageWidth and set it only when element is image? Setting undefined will > be okay if element is not image. Done.
lgtm with nits. Thank you! https://codereview.chromium.org/1608143002/diff/160001/ui/file_manager/file_m... File ui/file_manager/file_manager/background/js/test_util_base.js (right): https://codereview.chromium.org/1608143002/diff/160001/ui/file_manager/file_m... ui/file_manager/file_manager/background/js/test_util_base.js:43: width: Number(element.width), nit: 0 will be set even if the element doesn't have width field. https://codereview.chromium.org/1608143002/diff/200001/ui/file_manager/galler... File ui/file_manager/gallery/js/image_editor/image_view.js (right): https://codereview.chromium.org/1608143002/diff/200001/ui/file_manager/galler... ui/file_manager/gallery/js/image_editor/image_view.js:264: this.contentImage_ = canvas; nit: This line is unnecessary. replaceContent_() will do this.
Sorry for late! https://codereview.chromium.org/1608143002/diff/200001/ui/file_manager/galler... File ui/file_manager/gallery/js/image_editor/image_view.js (right): https://codereview.chromium.org/1608143002/diff/200001/ui/file_manager/galler... ui/file_manager/gallery/js/image_editor/image_view.js:263: this.replace(canvas); Maybe replaceContent is enough here since we don't pass effect? https://codereview.chromium.org/1608143002/diff/200001/ui/file_manager/galler... ui/file_manager/gallery/js/image_editor/image_view.js:629: ImageUtil.setAttribute(newContentImage, 'fade', true); To show 'fade' animation here, we should not remove oldContentImage in replaceContent_(). https://codereview.chromium.org/1608143002/diff/200001/ui/file_manager/galler... ui/file_manager/gallery/js/image_editor/image_view.js:753: setTimeout(function() { Actually setTimeout is not enough to ensure trigger transition, IIRC. Instead use requestAnimationFrame(function() { requestAnimationFrame(function() { .. }); }); https://codereview.chromium.org/1608143002/diff/200001/ui/file_manager/galler... ui/file_manager/gallery/js/image_editor/image_view.js:754: this.setTransform_(oldScreenImage, this.viewport_, effect); I think we can call setFade(false) here and remove setTimeout at #757. https://codereview.chromium.org/1608143002/diff/200001/ui/file_manager/galler... ui/file_manager/gallery/js/image_editor/image_view.js:760: if (oldScreenImage.parentNode) Please fix the indent. https://codereview.chromium.org/1608143002/diff/200001/ui/file_manager/galler... ui/file_manager/gallery/js/image_editor/image_view.js:761: oldScreenImage.parentNode.removeChild(oldScreenImage); Could you clear zIndex here (or we can ensure oldScreenImage is no longer used?) https://codereview.chromium.org/1608143002/diff/200001/ui/file_manager/integr... File ui/file_manager/integration_tests/gallery/open_image_files.js (right): https://codereview.chromium.org/1608143002/diff/200001/ui/file_manager/integr... ui/file_manager/integration_tests/gallery/open_image_files.js:51: chrome.test.assertEq(570, args[1].renderedHeight); Could you add assertions for imageWidth/imageHeight.
Hi, thank you for your reviews! I updated the patch in response to your review. Please take another look? https://codereview.chromium.org/1608143002/diff/200001/ui/file_manager/galler... File ui/file_manager/gallery/js/image_editor/image_view.js (right): https://codereview.chromium.org/1608143002/diff/200001/ui/file_manager/galler... ui/file_manager/gallery/js/image_editor/image_view.js:263: this.replace(canvas); On 2016/01/22 07:24:18, hirono wrote: > Maybe replaceContent is enough here since we don't pass effect? That's right, because we do not need any effect! https://codereview.chromium.org/1608143002/diff/200001/ui/file_manager/galler... ui/file_manager/gallery/js/image_editor/image_view.js:264: this.contentImage_ = canvas; On 2016/01/22 07:02:49, yawano wrote: > nit: This line is unnecessary. replaceContent_() will do this. Done. https://codereview.chromium.org/1608143002/diff/200001/ui/file_manager/galler... ui/file_manager/gallery/js/image_editor/image_view.js:629: ImageUtil.setAttribute(newContentImage, 'fade', true); On 2016/01/22 07:24:18, hirono wrote: > To show 'fade' animation here, we should not remove oldContentImage in > replaceContent_(). Done. https://codereview.chromium.org/1608143002/diff/200001/ui/file_manager/galler... ui/file_manager/gallery/js/image_editor/image_view.js:753: setTimeout(function() { On 2016/01/22 07:24:18, hirono wrote: > Actually setTimeout is not enough to ensure trigger transition, IIRC. > Instead use > > requestAnimationFrame(function() { > requestAnimationFrame(function() { > .. > }); > }); Done. https://codereview.chromium.org/1608143002/diff/200001/ui/file_manager/galler... ui/file_manager/gallery/js/image_editor/image_view.js:754: this.setTransform_(oldScreenImage, this.viewport_, effect); On 2016/01/22 07:24:18, hirono wrote: > I think we can call setFade(false) here and remove setTimeout at #757. merged to the closure above. https://codereview.chromium.org/1608143002/diff/200001/ui/file_manager/galler... ui/file_manager/gallery/js/image_editor/image_view.js:760: if (oldScreenImage.parentNode) On 2016/01/22 07:24:18, hirono wrote: > Please fix the indent. Done. https://codereview.chromium.org/1608143002/diff/200001/ui/file_manager/galler... ui/file_manager/gallery/js/image_editor/image_view.js:761: oldScreenImage.parentNode.removeChild(oldScreenImage); On 2016/01/22 07:24:18, hirono wrote: > Could you clear zIndex here (or we can ensure oldScreenImage is no longer used?) We may reuse the images. I will clear the z-index style, thanks. https://codereview.chromium.org/1608143002/diff/200001/ui/file_manager/integr... File ui/file_manager/integration_tests/gallery/open_image_files.js (right): https://codereview.chromium.org/1608143002/diff/200001/ui/file_manager/integr... ui/file_manager/integration_tests/gallery/open_image_files.js:51: chrome.test.assertEq(570, args[1].renderedHeight); On 2016/01/22 07:24:18, hirono wrote: > Could you add assertions for imageWidth/imageHeight. Done.
lgtm after resolving comments. Thanks! https://codereview.chromium.org/1608143002/diff/220001/ui/file_manager/galler... File ui/file_manager/gallery/js/image_editor/image_view.js (right): https://codereview.chromium.org/1608143002/diff/220001/ui/file_manager/galler... ui/file_manager/gallery/js/image_editor/image_view.js:628: this.container_.insertBefore(oldContentImage,this.container_.firstChild); nit: Please insert a space after ','. https://codereview.chromium.org/1608143002/diff/220001/ui/file_manager/galler... ui/file_manager/gallery/js/image_editor/image_view.js:628: this.container_.insertBefore(oldContentImage,this.container_.firstChild); Actually previous code applies 'fade' attribute to new image, so my previous comment was wrong (sorry...). Please set 'fade' to newContentImage here otherwise newContentImage is shown immediately. We still need to insert oldContentImage since we use it at 753, so you don't need to remove this line.
The CQ bit was checked by ryoh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yawano@chromium.org, hirono@chromium.org Link to the patchset: https://codereview.chromium.org/1608143002/#ps240001 (title: "set "fade" attribute.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1608143002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1608143002/240001
Hi, thanks for your review. I updated the CL and sent to the Commit Queue. https://codereview.chromium.org/1608143002/diff/220001/ui/file_manager/galler... File ui/file_manager/gallery/js/image_editor/image_view.js (right): https://codereview.chromium.org/1608143002/diff/220001/ui/file_manager/galler... ui/file_manager/gallery/js/image_editor/image_view.js:628: this.container_.insertBefore(oldContentImage,this.container_.firstChild); On 2016/01/25 04:21:31, hirono wrote: > nit: Please insert a space after ','. Done. https://codereview.chromium.org/1608143002/diff/220001/ui/file_manager/galler... ui/file_manager/gallery/js/image_editor/image_view.js:628: this.container_.insertBefore(oldContentImage,this.container_.firstChild); On 2016/01/25 04:21:31, hirono wrote: > Actually previous code applies 'fade' attribute to new image, so my previous > comment was wrong (sorry...). Please set 'fade' to newContentImage here > otherwise newContentImage is shown immediately. We still need to insert > oldContentImage since we use it at 753, so you don't need to remove this line. That's right! Thank you.
Message was sent while issue was closed.
Description was changed from
==========
support animated GIF
- removes "device resolution" canvas.
- allows to use both img element and canvas element to show image. This allows
to show animated GIF.
- If you need a ImageData of a image, convertes from img to canvas.
- This patch speeds up to load large jpg image (@ daisy spring)
- loading 14MB 12000x12000 JPG file:
8.62 -> 7.37sec
it crashed sometimes in the unpatched revision.
- loading 53MB, 8,180x11,950 PNG file:
6.89 -> 6.81sec
- it's time consuming to exec pixel operation.
- ex) making histgrams to check whether AutoColor filter is applicative.
- We should consider to use GLSL to process pixels.
BUG=446203
TEST=manually
==========
to
==========
support animated GIF
- removes "device resolution" canvas.
- allows to use both img element and canvas element to show image. This allows
to show animated GIF.
- If you need a ImageData of a image, convertes from img to canvas.
- This patch speeds up to load large jpg image (@ daisy spring)
- loading 14MB 12000x12000 JPG file:
8.62 -> 7.37sec
it crashed sometimes in the unpatched revision.
- loading 53MB, 8,180x11,950 PNG file:
6.89 -> 6.81sec
- it's time consuming to exec pixel operation.
- ex) making histgrams to check whether AutoColor filter is applicative.
- We should consider to use GLSL to process pixels.
BUG=446203
TEST=manually
==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from
==========
support animated GIF
- removes "device resolution" canvas.
- allows to use both img element and canvas element to show image. This allows
to show animated GIF.
- If you need a ImageData of a image, convertes from img to canvas.
- This patch speeds up to load large jpg image (@ daisy spring)
- loading 14MB 12000x12000 JPG file:
8.62 -> 7.37sec
it crashed sometimes in the unpatched revision.
- loading 53MB, 8,180x11,950 PNG file:
6.89 -> 6.81sec
- it's time consuming to exec pixel operation.
- ex) making histgrams to check whether AutoColor filter is applicative.
- We should consider to use GLSL to process pixels.
BUG=446203
TEST=manually
==========
to
==========
support animated GIF
- removes "device resolution" canvas.
- allows to use both img element and canvas element to show image. This allows
to show animated GIF.
- If you need a ImageData of a image, convertes from img to canvas.
- This patch speeds up to load large jpg image (@ daisy spring)
- loading 14MB 12000x12000 JPG file:
8.62 -> 7.37sec
it crashed sometimes in the unpatched revision.
- loading 53MB, 8,180x11,950 PNG file:
6.89 -> 6.81sec
- it's time consuming to exec pixel operation.
- ex) making histgrams to check whether AutoColor filter is applicative.
- We should consider to use GLSL to process pixels.
BUG=446203
TEST=manually
Committed: https://crrev.com/1f78b825f0b429ecbadafd524f1a5cefc01aae4d
Cr-Commit-Position: refs/heads/master@{#371191}
==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/1f78b825f0b429ecbadafd524f1a5cefc01aae4d Cr-Commit-Position: refs/heads/master@{#371191}
Message was sent while issue was closed.
Description was changed from
==========
support animated GIF
- removes "device resolution" canvas.
- allows to use both img element and canvas element to show image. This allows
to show animated GIF.
- If you need a ImageData of a image, convertes from img to canvas.
- This patch speeds up to load large jpg image (@ daisy spring)
- loading 14MB 12000x12000 JPG file:
8.62 -> 7.37sec
it crashed sometimes in the unpatched revision.
- loading 53MB, 8,180x11,950 PNG file:
6.89 -> 6.81sec
- it's time consuming to exec pixel operation.
- ex) making histgrams to check whether AutoColor filter is applicative.
- We should consider to use GLSL to process pixels.
BUG=446203
TEST=manually
Committed: https://crrev.com/1f78b825f0b429ecbadafd524f1a5cefc01aae4d
Cr-Commit-Position: refs/heads/master@{#371191}
==========
to
==========
support animated GIF
- removes "device resolution" canvas.
- allows to use both img element and canvas element to show image. This allows
to show animated GIF.
- If you need a ImageData of a image, convertes from img to canvas.
- This patch speeds up to load large jpg image (@ daisy spring)
- loading 14MB 12000x12000 JPG file:
8.62 -> 7.37sec
it crashed sometimes in the unpatched revision.
- loading 53MB, 8,180x11,950 PNG file:
6.89 -> 6.81sec
- it's time consuming to exec pixel operation.
- ex) making histgrams to check whether AutoColor filter is applicative.
- We should consider to use GLSL to process pixels.
BUG=446203
TEST=manually
Committed: https://crrev.com/1f78b825f0b429ecbadafd524f1a5cefc01aae4d
Cr-Commit-Position: refs/heads/master@{#371191}
==========
The CQ bit was checked by ryoh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hirono@chromium.org, yawano@chromium.org Link to the patchset: https://codereview.chromium.org/1608143002/#ps260001 (title: "add assert to pass closure compiler")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1608143002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1608143002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ryoh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1608143002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1608143002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) android_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) |
