Image element with src attribute and content: url() style can't be rendered correctly. For example: <image src="http://domain-which-does-not-exist.com/not-exist.jpg" style="content: url(url-for-image-exist.jpg)" /> webkit renders it as blank page. Even if src points to an exist image, a blank page renders also. If you use debug build, when reload the page, there is an ASSERT failure in CachedResource::removeClient().
Created attachment 62327 [details] Patch
Comment on attachment 62327 [details] Patch I tried <image src="http://google.com/images/logo.gif" style="content: url(http://www.google.com/intl/en_ALL/images/logos/images_logo_lg.gif)" /> This renders the src image in Firefox 3.6 and the generated image in Opera 10.6 and WebKit nightly. So, this patch changes WebKit to match the Firefox behavior, right? Is there a spec that covers this behavior that one of us is violating?
(In reply to comment #2) > (From update of attachment 62327 [details]) > I tried <image src="http://google.com/images/logo.gif" style="content: url(http://www.google.com/intl/en_ALL/images/logos/images_logo_lg.gif)" /> > > This renders the src image in Firefox 3.6 and the generated image in Opera 10.6 and WebKit nightly. So, this patch changes WebKit to match the Firefox behavior, right? Is there a spec that covers this behavior that one of us is violating? I am using latest code (on the top of commit 6255afb56f275ef6bb3ebe6d5484ad65a01e2529), webkit renders nothing. This patch doesn't change behavior to match Firfox, instead, just renders generated image.
> I am using latest code (on the top of commit 6255afb56f275ef6bb3ebe6d5484ad65a01e2529), webkit renders nothing. This patch doesn't change behavior to match Firfox, instead, just renders generated image. Oh, interesting, nothing renders in Safari 5, but the generated image renders in WebKit nightly on Mac at r63854. Looks like this is fixed already or did this regress since r63854?
Comment on attachment 62327 [details] Patch Why is this a manual test? Can't we see this with a render tree dump or a pixel test? WebCore/loader/ImageLoader.cpp:5 + * Copyright (C) Research In Motion Limited 2010. All rights reserved. Please don't add your copyright when changing < 10 lines. WebCore/rendering/RenderImage.h:7 + * Copyright (C) Research In Motion Limited 2010. All rights reserved. ditto
Created attachment 62762 [details] Revised patch
Comment on attachment 62762 [details] Revised patch What if the generated context is invalid?
(In reply to comment #7) > (From update of attachment 62762 [details]) > What if the generated context is invalid? I think it should behave as if this is an invalid image. Google Chrome 5.0 on Windows and Safari 5.0.1 on Windows behave as there is a broken image. Just as above discussion, this feature is broken in recently webkit trunk.
Comment on attachment 62762 [details] Revised patch This is a good catch. That code in ImageLoader is indeed wrong. But this patch will no longer apply, due to changes to ImageLoader.cpp that happened since it was posted. > + // If RenderImage is generated content we don't update the renderer either. The above is not a helpful comment. It says what the code does, but not why. As I understand the issue here, we want to change the image if this is a renderer we created, but not if it's a renderer that was created by other code. The fix should probably go in ImageLoader::renderImageResource, which can return 0 if the renderer is not for the loaded image, but rather some other renderer. Special casing the generated content version of RenderImage seems a little bit roundabout. Unfortunately, there is no longer a RenderImageGeneratedContent class to add a virtual function to, so we need a new approach to figuring out if this is an image renderer that the image loader should be overriding, or one it should not be. Somehow we have to distinguish the renderer created in RenderObject::createObject for generated content.
Created attachment 66680 [details] Rebased patch
Created attachment 66682 [details] Removed extra space from change log
Comment on attachment 66682 [details] Removed extra space from change log View in context: https://bugs.webkit.org/attachment.cgi?id=66682&action=review I wanted to R+ this patch, but I didn't understand it. > WebCore/rendering/RenderImageResource.h:71 > - RenderImageResource(); > + RenderImageResource(bool isGenerated); I'd use an enum instead of a bool so we can understand the call-sites better.
Created attachment 77401 [details] Change boolean to enum It makes sense to change boolean to enumeration for RenderImageResource.
Comment on attachment 77401 [details] Change boolean to enum This patch looks fine to me, but, as a rule, I don't review patches in "WebCore/rendering" because I don't understand rendering.
Comment on attachment 77401 [details] Change boolean to enum View in context: https://bugs.webkit.org/attachment.cgi?id=77401&action=review I don't think it’s right to make a special RenderImageResource. The real issue here is that RenderObject::createObject creates a RenderImage, but classes like HTMLImageElement assume that if the renderer is a RenderImage it’s the one they created. I think we should make a different renderer class, or add a flag to RenderImage, not RenderImageResource. I think the different class is probably the best way to handle it. I looked and found functions that make the incorrect assumption that an image is one created by the element and these are the ones I found: ImageLoader::renderImageResource HTMLImageElement::parseMappedAttribute HTMLImageElement::attach HitTestResult::absoluteImageURL absoluteURL functions generated by CodeGeneratorObjC.pm Those should probably use a function that returns false for the images that were created based on style, rather than the isImage function, which returns true for those. > WebCore/rendering/RenderImageResource.h:47 > + static PassOwnPtr<RenderImageResource> create(Type type = Normal) In the context of RenderImageResource, I don’t think “generated” is a clear enough term to be used. In this patch it means generated content, in other words, the CSS content attribute. But we also have a class named GeneratedImage in WebCore/platform/graphics, and this refers to images that are created by an algorithm, such as gradient, rather than with image data.
Thanks for tackling this. I’m sorry I didn’t spot this bigger issue earlier.
*** Bug 67721 has been marked as a duplicate of this bug. ***
As noted in bug 67721, sometimes things work just fine in Release builds.
<rdar://problem/10175703>
Created attachment 108642 [details] Patch v5
Comment on attachment 108642 [details] Patch v5 View in context: https://bugs.webkit.org/attachment.cgi?id=108642&action=review Patch is OK, but not great. I’d like to look at cleaner ways to do this in the future, but it seems fine to land this now. > Source/WebCore/loader/ImageLoader.cpp:257 > - if (renderer->isImage()) > + if (renderer->isImage() && !static_cast<RenderImage*>(renderer)->isGeneratedContent()) This is a non-obvious check, and it needs a comment to make clear why this is correct. We might even want to later rename renderImageResource longer term to make it clearer what the function returns. It seems that given we already use a different class for the generated content image, the bit could be on RenderImageResource instead of on RenderImage. We could even use a virtual function instead of a bit.
Created attachment 117555 [details] Patch for landing
Comment on attachment 117555 [details] Patch for landing Clearing flags on attachment: 117555 Committed r101753: <http://trac.webkit.org/changeset/101753>
All reviewed patches have been landed. Closing bug.