Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
136823005:
Fix distorted image issue when open some images in a new browser window. (Closed)
|
Created:
6 years, 10 months ago by yi Modified:
6 years, 10 months ago CC:
blink-reviews, dglazkov+blink, Nate Chapin, gavinp+loader_chromium.org, adamk+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink@master Visibility:
Public. |
DescriptionFix distorted image issue when open some images in a new browser window
This patch tries to fix a distorted image issue when opening certain
images in a new browser window. The root cause is because the image
size is needed before the image element's renderer (which contains the
information that determines whether the image's orientation will affect
the width and height) gets created.
Fix the issue by calling Document::updateStyleIfNeeded() to create the
image element's renderer before retrieving the image size.
BUG=320244
R=abarth@chromium.org
TEST=fast/images/exif-orientation-height-image-document.html
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=167708
Patch Set 1 #Patch Set 2 : add layout test #Patch Set 3 : not plumb knowledge to ImageResource #
Total comments: 2
Patch Set 4 : create renderer to get correct size #
Total comments: 5
Patch Set 5 : update test #Patch Set 6 : update layout test #Patch Set 7 : use Noel's test #
Total comments: 2
Patch Set 8 : updated based Noel's comments #
Total comments: 1
Patch Set 9 : expand the comment #Messages
Total messages: 48 (0 generated)
For an image document, its image element's render object may not be created before doing the layout/recalculate css style. So, the ImageResource::imageSizeForRenderer always ignore the image orientation if there is no render object. This patch allows the image resource to check the image orientation for image document which has no render object.
On 2014/01/31 19:32:19, yi wrote: > For an image document, its image element's render object may not be created > before doing the layout/recalculate css style. So, the > ImageResource::imageSizeForRenderer always ignore the image orientation if there > is no render object. This patch allows the image resource to check the image > orientation for image document which has no render object. ^^^ This description belongs in the CL itself, not just a comment. Can this not be covered by a LayoutTest?
On 2014/02/07 06:04:28, Levi wrote: > On 2014/01/31 19:32:19, yi wrote: > > For an image document, its image element's render object may not be created > > before doing the layout/recalculate css style. So, the > > ImageResource::imageSizeForRenderer always ignore the image orientation if > there > > is no render object. This patch allows the image resource to check the image > > orientation for image document which has no render object. > > ^^^ This description belongs in the CL itself, not just a comment. > > Can this not be covered by a LayoutTest? Thanks for the comments. I will check to see whether I can add a LayoutTest.
On 2014/02/07 17:44:18, yi wrote: > > > ... is no render object. This patch allows the image resource to check the image > > > orientation for image document which has no render object. "image document"? I think you meant to say "image element" ? I not the image has been added to the document. So why does its HTMLImageElement not have a renderer()?
On 2014/02/10 09:56:38, Noel Gordon (Google) wrote:
> On 2014/02/07 17:44:18, yi wrote:
> > > > ... is no render object. This patch allows the image resource to check
the
> image
> > > > orientation for image document which has no render object.
>
> "image document"? I think you meant to say "image element" ?
>
> I not the image has been added to the document. So why does its
> HTMLImageElement not have a renderer()?
Actually, this issue only happens when open a window with an image, e.g. call
window.open("xxx.jpg"). Then, the top level document is an image document which
owns an image element. In this case, when page layouts itself, image element may
try to get its image resource's size before its renderer gets created.
Why can't you just force creation of the renderer instead? Adding this doesn't feel right.
abarth@ might know more about what's going in in ImageDocuments too, but plumbing knowledge of it down into the ImageLoader doesn't look right. We should be forcing the document to recalcStyle and produce renderers before asking questions of the ImageLoader that require the renderer.
I agree with esprehn that this doesn't seem like the right approach. ImageResource shouldn't have any knowledge that ImageDocuments exist.
On 2014/02/13 03:22:36, abarth wrote: > I agree with esprehn that this doesn't seem like the right approach. > ImageResource shouldn't have any knowledge that ImageDocuments exist. thank you both for the comments. I will update the patch.
After digging into the code again, I think it is unlikely for us to force to produce renderers before asking image size that require the renderer. That's because image size may get queried at the very begin of page loading (in ImageDocumentParser::appendBytes), at when there is no renderer style information that is needed for creating a renderer. I also tried to update the image size after the renderer gets created ( in document::finishedParsing), and it results in assertion failures when browsers redo the layout. So I modified my previous patch to avoid to add any unwanted knowledge to the ImageResource - a new function is added for ImageResource, which returns bitmap image's size with respecting the orientation. Then, it is up to ImageDocument to decide which function it should use to get the correct image size at different phases.
I'm sorry, but I can't really understand the problem you're trying to solve or why you've chosen the approach you have. Can you please add a more descriptive description that explains these things? That will both help me understand this CL and will help future people digging through the code understand why you made this change. Specifically, please include a description of (1) what goes wrong before your CL, (2) why it goes wrong, and (3) why you're making this particular change to make it go right.
On 2014/02/14 03:11:43, abarth wrote: > I'm sorry, but I can't really understand the problem you're trying to solve or > why you've chosen the approach you have. Can you please add a more descriptive > description that explains these things? That will both help me understand this > CL and will help future people digging through the code understand why you made > this change. > > Specifically, please include a description of (1) what goes wrong before your > CL, (2) why it goes wrong, and (3) why you're making this particular change to > make it go right. Thanks for looking at it, abarth! I have updated the description for this issue, please review it for me and let me know if you have further question.
https://codereview.chromium.org/136823005/diff/200001/Source/core/fetch/Image... File Source/core/fetch/ImageResource.cpp (right): https://codereview.chromium.org/136823005/diff/200001/Source/core/fetch/Image... Source/core/fetch/ImageResource.cpp:263: return IntSize(); So, this won't work for SVG images? Should this be an ASSERT so indicate that it's an error to call this function for non-bitmap images? https://codereview.chromium.org/136823005/diff/200001/Source/core/html/ImageD... File Source/core/html/ImageDocument.cpp (right): https://codereview.chromium.org/136823005/diff/200001/Source/core/html/ImageD... Source/core/html/ImageDocument.cpp:381: return imageResource->imageSizeForRenderer(m_imageElement->renderer(), zoomFactor); Won't this still be broken for non-bitmap images? /me is confused.
It seems like this approach is broken for non-bitmap images. Instead of trying to do this computation without the RenderObject, why not just update style and cause the render object to be created?
On 2014/02/14 00:56:19, yi wrote: > After digging into the code again, I think it is unlikely for us to force to > produce renderers before asking image size that require the renderer. That's > because image size may get queried at the very begin of page loading (in > ImageDocumentParser::appendBytes), at when there is no renderer style > information that is needed for creating a renderer. I also tried to update the > image size after the renderer gets created ( in document::finishedParsing), and > it results in assertion failures when browsers redo the layout. That doesn't make sense to me. We should be able to build the DOM, then compute style, then make decisions based on the style. It's possible that means we'll need to refactor this code a bit to have a sensible pipeline, but that's worth doing. > So I modified my previous patch to avoid to add any unwanted knowledge to the > ImageResource - a new function is added for ImageResource, which returns bitmap > image's size with respecting the orientation. Then, it is up to ImageDocument to > decide which function it should use to get the correct image size at different > phases. Unfortunately, that approach doesn't appear to work for non-bitmap images.
Thanks for the comments, abarth. I have updated the patch and please take a look when you get time.
This is looking pretty good, some nits about the test which would be nice to fix. The major thing is to not branch on renderer(), I'd just call it unconditionally. Please add a comment above the call with an explanation about why you need to do it too. https://codereview.chromium.org/136823005/diff/300001/LayoutTests/fast/images... File LayoutTests/fast/images/image-distorted-aspect-ratios.html (right): https://codereview.chromium.org/136823005/diff/300001/LayoutTests/fast/images... LayoutTests/fast/images/image-distorted-aspect-ratios.html:2: <html> We often leave off the <html>, <body> and <head> elements since they're just clutter. https://codereview.chromium.org/136823005/diff/300001/LayoutTests/fast/images... LayoutTests/fast/images/image-distorted-aspect-ratios.html:21: function loadImageInNewWindow() onload = function() { https://codereview.chromium.org/136823005/diff/300001/LayoutTests/fast/images... LayoutTests/fast/images/image-distorted-aspect-ratios.html:53: ". The image width should be less than height."); This test could probably be better written with js-test.js and shouldBe() and friends? https://codereview.chromium.org/136823005/diff/300001/Source/core/html/ImageD... File Source/core/html/ImageDocument.cpp (right): https://codereview.chromium.org/136823005/diff/300001/Source/core/html/ImageD... Source/core/html/ImageDocument.cpp:134: document()->updateStyleIfNeeded(); Remove the if statement, just call updateStyleIfNeeded() unconditionally.
https://codereview.chromium.org/136823005/diff/300001/Source/core/html/ImageD... File Source/core/html/ImageDocument.cpp (right): https://codereview.chromium.org/136823005/diff/300001/Source/core/html/ImageD... Source/core/html/ImageDocument.cpp:147: IntSize size = flooredIntSize(cachedImage->imageSizeForRenderer(document()->imageElement()->renderer(), 1.0f)); The renderer() is non-null here with this CL. That will change what is drawn in the document.title, right?
On 2014/02/19 02:13:02, esprehn wrote: > This is looking pretty good, some nits about the test which would be nice to > fix. The major thing is to not branch on renderer(), I'd just call it > unconditionally. Please add a comment above the call with an explanation about > why you need to do it too. > > https://codereview.chromium.org/136823005/diff/300001/LayoutTests/fast/images... > File LayoutTests/fast/images/image-distorted-aspect-ratios.html (right): > > https://codereview.chromium.org/136823005/diff/300001/LayoutTests/fast/images... > LayoutTests/fast/images/image-distorted-aspect-ratios.html:2: <html> > We often leave off the <html>, <body> and <head> elements since they're just > clutter. > > https://codereview.chromium.org/136823005/diff/300001/LayoutTests/fast/images... > LayoutTests/fast/images/image-distorted-aspect-ratios.html:21: function > loadImageInNewWindow() > onload = function() { > > https://codereview.chromium.org/136823005/diff/300001/LayoutTests/fast/images... > LayoutTests/fast/images/image-distorted-aspect-ratios.html:53: ". The image > width should be less than height."); > This test could probably be better written with js-test.js and shouldBe() and > friends? > > https://codereview.chromium.org/136823005/diff/300001/Source/core/html/ImageD... > File Source/core/html/ImageDocument.cpp (right): > > https://codereview.chromium.org/136823005/diff/300001/Source/core/html/ImageD... > Source/core/html/ImageDocument.cpp:134: document()->updateStyleIfNeeded(); > Remove the if statement, just call updateStyleIfNeeded() unconditionally. Thanks, will fix them.
On 2014/02/19 04:15:56, Noel Gordon (Google) wrote: > https://codereview.chromium.org/136823005/diff/300001/Source/core/html/ImageD... > File Source/core/html/ImageDocument.cpp (right): > > https://codereview.chromium.org/136823005/diff/300001/Source/core/html/ImageD... > Source/core/html/ImageDocument.cpp:147: IntSize size = > flooredIntSize(cachedImage->imageSizeForRenderer(document()->imageElement()->renderer(), > 1.0f)); > The renderer() is non-null here with this CL. That will change what is drawn in > the document.title, right? Correct. I can update the test to reflect this.
updated the patch according to esprehn & noel's comments
On 2014/02/19 17:51:48, yi wrote: > > The renderer() is non-null here with this CL. That will change what is drawn > in > > the document.title, right? > > Correct. I can update the test to reflect this. I can't see your change ImageDocument.cpp in the review tool for some reason, but the recall the comment there says: // Report the natural image size in the page title, regardless of zoom level. Do you still do that? If not, my view is we should do what the comment says.
Also, I have another test on https://codereview.chromium.org/168583003 for this bug that I like to land. I was hoping you could use the test image I created for your test.
On 2014/02/20 02:05:58, Noel Gordon (Google) wrote: > On 2014/02/19 17:51:48, yi wrote: > > > > The renderer() is non-null here with this CL. That will change what is > drawn > > in > > > the document.title, right? > > > > Correct. I can update the test to reflect this. > > I can't see your change ImageDocument.cpp in the review tool for some reason, > but > the recall the comment there says: > > // Report the natural image size in the page title, regardless of zoom level. > > Do you still do that? If not, my view is we should do what the comment says. Take your test image for example, what should be displayed as title? If the expected tile is 82x1281, no more change needed in ImageDocument.cpp
On 2014/02/20 06:38:52, Noel Gordon (Google) wrote: > Also, I have another test on https://codereview.chromium.org/168583003 > for this bug that I like to land. I was hoping you could use the test > image I created for your test. If you don't mind, I will just use your test.
On 2014/02/20 22:22:26, yi wrote: > If you don't mind, I will just use your test. ok.
On 2014/02/20 22:21:41, yi wrote: > > Take your test image for example, what should be displayed as title? If the > expected title is 82x1281, no more change needed in ImageDocument.cpp The title should be 1281x82 (viz., the title code should ignore orientation).
On 2014/02/20 23:09:10, Noel Gordon (Google) wrote: > On 2014/02/20 22:21:41, yi wrote: > > > > Take your test image for example, what should be displayed as title? If the > > expected title is 82x1281, no more change needed in ImageDocument.cpp > > The title should be 1281x82 (viz., the title code should ignore orientation). Shall we open another bug for this title issue? My change is not the root cause because imageSizeForRenderer() always considers the image orientation unless you pass a null renderer. However, pass a null renderer here seems wrong since this will only affect non-bitmap images.
On 2014/02/20 23:09:10, Noel Gordon (Google) wrote: > On 2014/02/20 22:21:41, yi wrote: > > > > Take your test image for example, what should be displayed as title? If the > > expected title is 82x1281, no more change needed in ImageDocument.cpp > > The title should be 1281x82 (viz., the title code should ignore orientation). BTW, my latest firefox on Mac shows title 82 x 1281 for this test image.
On 2014/02/20 23:45:13, yi wrote: > On 2014/02/20 23:09:10, Noel Gordon (Google) wrote: > > > > The title should be 1281x82 (viz., the title code should ignore orientation). > > Shall we open another bug for this title issue? Perhaps open a bug to write a test for the image title. Best I can tell, there is no test for it. > My change is not the root cause > because imageSizeForRenderer() always considers the image orientation unless you > pass a null renderer. However, pass a null renderer here seems wrong since this > will only affect non-bitmap images. Using a null renderer would return the natural image size for all image types, including bitmap images I think.
On 2014/02/21 00:58:00, Noel Gordon (Google) wrote: > On 2014/02/20 23:45:13, yi wrote: > > On 2014/02/20 23:09:10, Noel Gordon (Google) wrote: > > > > > > The title should be 1281x82 (viz., the title code should ignore > orientation). > > > > Shall we open another bug for this title issue? > > Perhaps open a bug to write a test for the image title. Best I can tell, > there is no test for it. Thanks for the comments, will do it. > > > My change is not the root cause > > because imageSizeForRenderer() always considers the image orientation unless > you > > pass a null renderer. However, pass a null renderer here seems wrong since > this > > will only affect non-bitmap images. > > Using a null renderer would return the natural image size for all image > types, including bitmap images I think.
On 2014/02/20 23:47:24, yi wrote: > BTW, my latest firefox on Mac shows title 82 x 1281 for this test image. The Windows image viewer reports 1281 x 82. My Windows Firefox reports 1281 x 82 also :) All very confusing. Perhaps that's part of the reason why the comment said to report the natural image size (1281 x 82).
On 2014/02/21 01:05:14, Noel Gordon (Google) wrote: > On 2014/02/20 23:47:24, yi wrote: > > > BTW, my latest firefox on Mac shows title 82 x 1281 for this test image. > > The Windows image viewer reports 1281 x 82. My Windows Firefox reports > 1281 x 82 also :) All very confusing. Perhaps that's part of the reason > why the comment said to report the natural image size (1281 x 82). Is there a spec to define the 'natural image size'? On Mac, both image viewer, and file info dialog reports 82 x 1281 :( I will go ahead to add a test for the title, and meanwhile, look for the spec for it. In case I can't find the spec, I would like to address this issue on a separated patch. How do you think?
BTW, I'm not too worried about the title. I was just pointing out that your CL changed behavior and no longer matches the comment.
> I would like to address this issue on a separated patch. How do you think? Yes, that's fine with me.
On 2014/02/21 01:18:58, Noel Gordon (Google) wrote: > > I would like to address this issue on a separated patch. How do you think? > > Yes, that's fine with me. Could you please review this patch for me without considering the title issue? I have created a layout test for the title issue at https://codereview.chromium.org/174073006/, but I realized it will be connivence for me to upstream this patch first.
Thank you, let me look again (ignoring the title) ...
Looking pretty good, a few minor things. https://codereview.chromium.org/136823005/diff/600001/LayoutTests/TestExpecta... File LayoutTests/TestExpectations (left): https://codereview.chromium.org/136823005/diff/600001/LayoutTests/TestExpecta... LayoutTests/TestExpectations:662: Instead of removing these lines, mark them as needing a rebaseline and the system will automatically create new test results for each platform once your patch lands ... crbug.com/320244 fast/images/exif-orientation-height-image-document.html [ NeedsRebaseline ] crbug.com/320244 virtual/deferred/fast/images/exif-orientation-height-image-document.html [ NeedsRebaseline ] https://codereview.chromium.org/136823005/diff/600001/Source/core/html/ImageD... File Source/core/html/ImageDocument.cpp (right): https://codereview.chromium.org/136823005/diff/600001/Source/core/html/ImageD... Source/core/html/ImageDocument.cpp:133: document()->updateStyleIfNeeded(); I think Elliot said: >> Please add a comment above the call with an explanation about >> why you need to do it too. Could you re-check his review comments and address them?
On 2014/02/21 01:14:32, yi wrote: > Is there a spec to define the 'natural image size'? That's a very good question. <img> elements have a naturalWidth / naturalHeight property [1], and those must return the intrinsic width / height of the image. The intrinsic width / height (dimensions) were defined in CSS2.1 by http://www.w3.org/TR/CSS21/conform.html (refer to the section on Intrinsic Dimensions). CSS3.0 has a more elaborate definition intrinsic width / height http://www.w3.org/TR/css3-images/#intrinsic-dimensions and mentions "natural size". For raster images, the CSS2.0 and CSS3.0 definitions don't specify if "intrinsic size" includes or excludes image orientation in my reading of those specs. I note that CSS4.0 adds a new CSS image-orientation property, and that property (which is subject to change) says that the intrinsic height and width are derived from the rotated rather than the original image dimensions. So no, I don't see good spec definition of "natural image size". Maybe we should just report the width x height the user sees on their screen.
Thanks, Noel. I have updated my patch according to your comments.
And thank you, that's looking good. The other reviewers might still have comments, but your change seems fine to me.
lgtm, might be nice to expand the comment a bit in case that bug vanishes. https://codereview.chromium.org/136823005/diff/770001/Source/core/html/ImageD... File Source/core/html/ImageDocument.cpp (right): https://codereview.chromium.org/136823005/diff/770001/Source/core/html/ImageD... Source/core/html/ImageDocument.cpp:133: // Make sure image renderer gets created - crbug.com/320244 // Make sure the image renderer gets created because we need the renderer // to read the aspect ratio. See crbug.com/320244
The CQ bit was checked by yi.shen@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yi.shen@samsung.com/136823005/830001
On 2014/02/22 02:48:28, esprehn wrote: > lgtm, might be nice to expand the comment a bit in case that bug vanishes. > > https://codereview.chromium.org/136823005/diff/770001/Source/core/html/ImageD... > File Source/core/html/ImageDocument.cpp (right): > > https://codereview.chromium.org/136823005/diff/770001/Source/core/html/ImageD... > Source/core/html/ImageDocument.cpp:133: // Make sure image renderer gets created > - crbug.com/320244 > // Make sure the image renderer gets created because we need the renderer > // to read the aspect ratio. See crbug.com/320244 Thank you guys!
Message was sent while issue was closed.
Change committed as 167708 |
