Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2691933007:
Rotate an image bitmap in ImageLoader extension when loading detached. (Closed)
|
Created:
3 years, 10 months ago by yamaguchi Modified:
3 years, 10 months ago Reviewers:
mtomasz CC:
chromium-reviews, oka+watch_chromium.org, rginda+watch_chromium.org, mtomasz+watch_chromium.org, yamaguchi+watch_chromium.org, fukino+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRotate an image bitmap in ImageLoader extension when loading detached.
This change will fix the image orientation of the thumnail in a tooltip
when dragging a JPEG file [whose EXIF orientation != 1] in the Files app.
BUG=680420
TEST=browser_tests --gtest_filter=FileManagerJsTest.ImageOrientation
TEST=browser_tests --gtest_filter=FileManagerJsTest.ThumbnailLoader
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2691933007
Cr-Commit-Position: refs/heads/master@{#451231}
Committed: https://chromium.googlesource.com/chromium/src/+/a36f4e882b8288ff1f3e07d868ba88e19b0ef654
Patch Set 1 #Patch Set 2 : update comment. #
Total comments: 8
Patch Set 3 : Reuse 'orientation' field to make different transformation formats exclusive. #
Total comments: 6
Patch Set 4 : Fix the rotation direction and matrix representation. #Patch Set 5 : Update comment. #Patch Set 6 : Fix test failure in FileManagerJsTest.ThumbnailLoader. #
Messages
Total messages: 31 (21 generated)
Description was changed from ========== Rotate the image bitmap in ImageLoader extension when loading detached image. BUG=680420 TEST=browser_tests --gtest_filter=FileManagerJsTest.ImageOrientation ========== to ========== Rotate the image bitmap in ImageLoader extension when loading detached image. BUG=680420 TEST=browser_tests --gtest_filter=FileManagerJsTest.ImageOrientation CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Rotate the image bitmap in ImageLoader extension when loading detached image. BUG=680420 TEST=browser_tests --gtest_filter=FileManagerJsTest.ImageOrientation CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Rotate an image bitmap in ImageLoader extension when loading detached. This change will fix the image orientation of the thumnail in a tooltip when dragging a JPEG file [whose EXIF orientation != 1] in the Files app. BUG=680420 TEST=browser_tests --gtest_filter=FileManagerJsTest.ImageOrientation CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
yamaguchi@chromium.org changed reviewers: + mtomasz@chromium.org
PTAL Will you take a look because you worked on this component before?
Sorry for late. https://codereview.chromium.org/2691933007/diff/20001/ui/file_manager/file_ma... File ui/file_manager/file_manager/foreground/js/metadata/image_orientation.js (right): https://codereview.chromium.org/2691933007/diff/20001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/metadata/image_orientation.js:104: * @param {{scaleX:number, scaleY:number, rotate90: number}} transform nit: Please choose either ": " or ":" consistently in #104. https://codereview.chromium.org/2691933007/diff/20001/ui/file_manager/file_ma... File ui/file_manager/file_manager/foreground/js/metadata/image_orientation_unittest.js (right): https://codereview.chromium.org/2691933007/diff/20001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/metadata/image_orientation_unittest.js:53: var rotate90 = { scaleX: 1, scaleY: 1, rotate90: 1}; nit: Remove \s after {. https://codereview.chromium.org/2691933007/diff/20001/ui/file_manager/file_ma... File ui/file_manager/file_manager/foreground/js/thumbnail_loader.js (right): https://codereview.chromium.org/2691933007/diff/20001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/thumbnail_loader.js:463: var loader_options = { nit: camelCase for variable names in JS. https://codereview.chromium.org/2691933007/diff/20001/ui/file_manager/image_l... File ui/file_manager/image_loader/image_loader.js (right): https://codereview.chromium.org/2691933007/diff/20001/ui/file_manager/image_l... ui/file_manager/image_loader/image_loader.js:71: if (request.image_transform) { Instead of overriding, can we simply pass the "orientation" parameter in thumbnail_loader.js? I think it would result in a simpler code.
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL I found and fixed a logical bug. In the older revision, EXIF Orientation=5 and 7 (which does both mirror and rotation) was not rendered correctly. https://codereview.chromium.org/2691933007/diff/20001/ui/file_manager/file_ma... File ui/file_manager/file_manager/foreground/js/metadata/image_orientation.js (right): https://codereview.chromium.org/2691933007/diff/20001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/metadata/image_orientation.js:104: * @param {{scaleX:number, scaleY:number, rotate90: number}} transform On 2017/02/16 08:53:41, mtomasz wrote: > nit: Please choose either ": " or ":" consistently in #104. Done. https://codereview.chromium.org/2691933007/diff/20001/ui/file_manager/file_ma... File ui/file_manager/file_manager/foreground/js/metadata/image_orientation_unittest.js (right): https://codereview.chromium.org/2691933007/diff/20001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/metadata/image_orientation_unittest.js:53: var rotate90 = { scaleX: 1, scaleY: 1, rotate90: 1}; On 2017/02/16 08:53:41, mtomasz wrote: > nit: Remove \s after {. Done. https://codereview.chromium.org/2691933007/diff/20001/ui/file_manager/file_ma... File ui/file_manager/file_manager/foreground/js/thumbnail_loader.js (right): https://codereview.chromium.org/2691933007/diff/20001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/thumbnail_loader.js:463: var loader_options = { On 2017/02/16 08:53:41, mtomasz wrote: > nit: camelCase for variable names in JS. Done. https://codereview.chromium.org/2691933007/diff/20001/ui/file_manager/image_l... File ui/file_manager/image_loader/image_loader.js (right): https://codereview.chromium.org/2691933007/diff/20001/ui/file_manager/image_l... ui/file_manager/image_loader/image_loader.js:71: if (request.image_transform) { On 2017/02/16 08:53:41, mtomasz wrote: > Instead of overriding, can we simply pass the "orientation" parameter in > thumbnail_loader.js? I think it would result in a simpler code. Done. https://codereview.chromium.org/2691933007/diff/40001/ui/file_manager/file_ma... File ui/file_manager/file_manager/foreground/js/metadata/image_orientation.js (right): https://codereview.chromium.org/2691933007/diff/40001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/metadata/image_orientation.js:105: * rotate90: counterclockwise degrees / 90. rotate() in CSS transform property is clockwise. https://drafts.csswg.org/css-transforms/#two-d-transform-functions (We are making this parameter compatible to the one used here. https://cs.chromium.org/chromium/src/ui/file_manager/file_manager/common/js/u... ) https://codereview.chromium.org/2691933007/diff/40001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/metadata/image_orientation.js:118: orientation.b * scaleY, This was wrong. https://www.w3.org/TR/2dcontext/#transformations see the comment in the latest revision. https://codereview.chromium.org/2691933007/diff/40001/ui/file_manager/file_ma... File ui/file_manager/file_manager/foreground/js/metadata/image_orientation_unittest.js (right): https://codereview.chromium.org/2691933007/diff/40001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/metadata/image_orientation_unittest.js:58: var rotateAndFlipX = {scaleX: -1, scaleY: 1, rotate90: 1}; In CSS transformations, this means "rotate 90[deg] CW and then scale along the X axis (which is made vertical now) by -1". However, I think it's simpler to say "flip X and then rotate it 90[deg] CW".
https://codereview.chromium.org/2691933007/diff/40001/ui/file_manager/file_ma... File ui/file_manager/file_manager/foreground/js/metadata/image_orientation.js (right): https://codereview.chromium.org/2691933007/diff/40001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/metadata/image_orientation.js:118: orientation.b * scaleY, On 2017/02/16 14:01:18, yamaguchi wrote: > see the comment in the latest revision. See my source code comment in the latest Patchset.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
linux_chromium_chromeos_rel_ng is failing because of FileManagerJsTest.ThumbnailLoader. Thumbnail loader is no longer expected to rotate an image received in case of loadDetached. I will revise the test. https://cs.chromium.org/chromium/src/ui/file_manager/file_manager/foreground/...
https://codereview.chromium.org/2691933007/diff/40001/ui/file_manager/image_l... File ui/file_manager/image_loader/image_loader.js (right): https://codereview.chromium.org/2691933007/diff/40001/ui/file_manager/image_l... ui/file_manager/image_loader/image_loader.js:76: ImageOrientation.fromRotationAndScale(request.orientation); I know the logic here already did it, but it shouldn't be responsibility of image loader to perform conversions to different orientation formats. Image loader shouldn't have dependency on file manager code, as it's a separate extension. Can we move both ImageOrientation.fromDriveOrientation and ImageOrientation.fromRotationAndScale calls to the calling files? Then this logic will become simpler, and we'll get rid of the circular dependency. If it's significant refactoring, then doing it in a separate CL is fine.
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
Description was changed from ========== Rotate an image bitmap in ImageLoader extension when loading detached. This change will fix the image orientation of the thumnail in a tooltip when dragging a JPEG file [whose EXIF orientation != 1] in the Files app. BUG=680420 TEST=browser_tests --gtest_filter=FileManagerJsTest.ImageOrientation CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Rotate an image bitmap in ImageLoader extension when loading detached. This change will fix the image orientation of the thumnail in a tooltip when dragging a JPEG file [whose EXIF orientation != 1] in the Files app. BUG=680420 TEST=browser_tests --gtest_filter=FileManagerJsTest.ImageOrientation TEST=browser_tests --gtest_filter=FileManagerJsTest.ThumbnailLoader CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2691933007/diff/40001/ui/file_manager/image_l... File ui/file_manager/image_loader/image_loader.js (right): https://codereview.chromium.org/2691933007/diff/40001/ui/file_manager/image_l... ui/file_manager/image_loader/image_loader.js:76: ImageOrientation.fromRotationAndScale(request.orientation); On 2017/02/17 02:49:18, mtomasz wrote: > I know the logic here already did it, but it shouldn't be responsibility of > image loader to perform conversions to different orientation formats. Image > loader shouldn't have dependency on file manager code, as it's a separate > extension. > > > Can we move both ImageOrientation.fromDriveOrientation and > ImageOrientation.fromRotationAndScale calls to the calling files? Then this > logic will become simpler, and we'll get rid of the circular dependency. > > > If it's significant refactoring, then doing it in a separate CL is fine. As discussed offline, we'll need to handle the circular dependency problem separately, because it is also happening with other files. Orientation is a class (which has prototype chain, unlike a plain hash map). We'd need to make sure if we can do that across extensions. I will be removing the handling of "Drive Orientation" with this parameter in a separate CL, because I haven't seen a code passing Drive Orientation so far.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by yamaguchi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487308475179470,
"parent_rev": "6b2bb377f6425e4504ee8b2265ffe9bda97e191d", "commit_rev":
"a36f4e882b8288ff1f3e07d868ba88e19b0ef654"}
Message was sent while issue was closed.
Description was changed from ========== Rotate an image bitmap in ImageLoader extension when loading detached. This change will fix the image orientation of the thumnail in a tooltip when dragging a JPEG file [whose EXIF orientation != 1] in the Files app. BUG=680420 TEST=browser_tests --gtest_filter=FileManagerJsTest.ImageOrientation TEST=browser_tests --gtest_filter=FileManagerJsTest.ThumbnailLoader CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Rotate an image bitmap in ImageLoader extension when loading detached. This change will fix the image orientation of the thumnail in a tooltip when dragging a JPEG file [whose EXIF orientation != 1] in the Files app. BUG=680420 TEST=browser_tests --gtest_filter=FileManagerJsTest.ImageOrientation TEST=browser_tests --gtest_filter=FileManagerJsTest.ThumbnailLoader CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2691933007 Cr-Commit-Position: refs/heads/master@{#451231} Committed: https://chromium.googlesource.com/chromium/src/+/a36f4e882b8288ff1f3e07d868ba... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/a36f4e882b8288ff1f3e07d868ba... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
