Bug 69416

Created attachment 109784 [details] Patch v1 Adding draft patch, unlikely to build on non-Mac platforms yet, as it touches a lot of call sites in platform/. Uploading early to receive EWS results...

Attachment 109784 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/loader/cache/CachedImage.h:64: One space before end of line comments [whitespace/comments] [5] Source/WebCore/loader/cache/CachedImage.h:65: One space before end of line comments [whitespace/comments] [5] Total errors found: 2 in 77 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 109784 [details] Patch v1 Attachment 109784 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9959182

Comment on attachment 109784 [details] Patch v1 Attachment 109784 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/9954403

Comment on attachment 109784 [details] Patch v1 Attachment 109784 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9940730

Created attachment 109793 [details] Patch v2 First stab at fixing non-mac platforms.

Comment on attachment 109793 [details] Patch v2 Attachment 109793 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9944762

Comment on attachment 109793 [details] Patch v2 Attachment 109793 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9958278

Comment on attachment 109793 [details] Patch v2 Attachment 109793 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/9940745

Created attachment 109805 [details] Patch v3 Next round of fixes...

Comment on attachment 109805 [details] Patch v3 Attachment 109805 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9959212

Comment on attachment 109805 [details] Patch v3 Attachment 109805 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9954471

Created attachment 109844 [details] Patch v4 Next round of fixes...

Comment on attachment 109844 [details] Patch v4 I don't think it makes any sense to impose this upon the platform notion of Image. Image::size() is already virtual so can't you just implement the behavior you want in SVGImage's override?

(In reply to comment #14) > (From update of attachment 109844 [details]) > I don't think it makes any sense to impose this upon the platform notion of Image. Image::size() is already virtual so can't you just implement the behavior you want in SVGImage's override? You're right, now that I had some sleep I agree :-) It was a lazy approach with high cost on platform layer, I already have a different, cleaner solution in mind.

Created attachment 110115 [details] New approach - v1 A less intrusive approach, with minimal impact on the platform layer.

Attachment 110115 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/rendering/style/StyleImage.h:110: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/rendering/style/StyleImage.h:111: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 2 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 110115 [details] New approach - v1 View in context: https://bugs.webkit.org/attachment.cgi?id=110115&action=review > Source/WebCore/ChangeLog:41 > + ImageUser is just a simple typedef const void*, as we can't use RenderObject in platform/ as that would > + be a layering violation (and its also needed for non-RenderObject derived classes to use SVGImages). What is done with the ImageUser in SVGImage? We should create an abstract base class for that which RenderObject can inherit from and not use a const void*.

(In reply to comment #18) > What is done with the ImageUser in SVGImage? We should create an abstract base class for that which RenderObject can inherit from and not use a const void*. In a follow-up patch (see master bug 47156, whose patch makes us of this) SVGImage maintains a HashMap<ImageUser, IntSize> to return the correct override container sizes for a certain user of that Image. SVGImage::setImageContainerSize adds values to the HashMap, size() and paint() etc. query the HashMap. Your suggestion to use a new abstract base class instead of a typdef to const void* would work as well, though it's not only RenderObject who would need to inherit from it, but also eg. SVGFEImage (non RenderObject derived but uses SVGImages). Would you prefer if I'd introduce a "class ImageUser { }" and let RenderObject/SVGFEImage inherit from it?

(In reply to comment #19) > Would you prefer if I'd introduce a "class ImageUser { }" and let RenderObject/SVGFEImage inherit from it? I do think there is value even if the class has no members, but the value is not as great was I though if an “image user” truly is only a hash table key.

I wish you had tried my suggestion in https://bugs.webkit.org/show_bug.cgi?id=67087#c9 of going with multiple SVGImages per CachedImage, one per size the image is used in. I suspect (though have no proof, as this code is complex and I may be missing subtleties) that that would have led to a cleaner architecture without closing any avenues for future optimizations. Having to use out-of-band information transfer tricks like ImageUser here is usually a sign of architecture heading to a wrong direction.

(In reply to comment #21) > I wish you had tried my suggestion in https://bugs.webkit.org/show_bug.cgi?id=67087#c9 of going with multiple SVGImages per CachedImage, one per size the image is used in. I suspect (though have no proof, as this code is complex and I may be missing subtleties) that that would have led to a cleaner architecture without closing any avenues for future optimizations. Well, I told you on IRC how heavy constructing SVGImages are. The better approach is to cache multiple rendered ImageBuffers per size, but that's a future optimization. Your suggested approach would work, but you're creating seperated dom/render trees/documents for each of the SVGImages. I don't think that's a good idea. I plan to add an additional caching layering in CachedImage in future, that handles caching rendered ImageBuffers, but that's not going to avoid ImageUser. > Having to use out-of-band information transfer tricks like ImageUser here is usually a sign of architecture heading to a wrong direction. I'd agree if I'd use the ImageUser for anything else than putting it into a HashMap as key to map an arbitrary user of the CachedImage (eg. RenderObject or SVGFEImage) to a certain IntSize.

Created attachment 110828 [details] Next try Awaiting EWS results...

Attachment 110828 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1 Source/WebCore/loader/cache/ImageBySizeCache.h:26: Alphabetical sorting problem. [build/include_order] [4] Source/WebCore/ChangeLog:76: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5] Total errors found: 2 in 65 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 110828 [details] Next try Attachment 110828 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10067169

Created attachment 110834 [details] Next try - v2 Fix qt build & style issues - should be done now.

Comment on attachment 110834 [details] Next try - v2 Attachment 110834 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10062192

Comment on attachment 110834 [details] Next try - v2 View in context: https://bugs.webkit.org/attachment.cgi?id=110834&action=review Looks generally good, my comments are mostly about naming and style. > Source/WebCore/bindings/objc/DOM.mm:442 > - return cachedImage->image()->getNSImage(); > + return cachedImage->image(renderer)->getNSImage(); I think there should be a separate call for getting renderer specific version of the image, imageForRenderer() perhaps. CacheImage::image() (instead of image(0) as this patch does) should still return the generic image (or assert if that doesn't make sense). > Source/WebCore/html/HTMLImageElement.cpp:260 > - return m_imageLoader.image()->imageSize(1.0f).width(); > + return m_imageLoader.image()->imageSize(renderer(), 1.0f).width(); Similarly imageSizeForRenderer. > Source/WebCore/loader/cache/CachedImage.cpp:217 > -void CachedImage::setImageContainerSize(const IntSize& containerSize) > +void CachedImage::setImageContainerSize(const RenderObject* renderer, const IntSize& containerSize) setContainerSizeForRenderer perhaps? > Source/WebCore/loader/cache/CachedImage.cpp:239 > + // FIXME: Turn this on once, webkit.org/b/47156 lands. > +#if ENABLE(SVG) && 0 > + // SVGImages are independently cached per size in the ImageBySizeCache. > + RenderObjectSizeCountMap::const_iterator it = m_svgImageCache.clients().find(renderer); > + if (it != m_svgImageCache.clients().end()) { > + IntSize currentSize = it->second.first; > + ASSERT(!currentSize.isEmpty()); > + if (currentSize == containerSize) > + return; > + m_svgImageCache.removeClient(renderer); > + if (!containerSize.isEmpty()) > + m_svgImageCache.addClient(renderer, containerSize); > + return; > + } There is quite of bit of caching logic added to CachedImage here and elsewhere. I wonder if more of it could live in the cache class. > Source/WebCore/loader/cache/CachedImage.cpp:297 > -IntRect CachedImage::imageRect(float multiplier) const > +IntRect CachedImage::imageRect(const RenderObject* renderer, float multiplier) I can't find any clients for this call in this patch. Can the method be removed? > Source/WebCore/loader/cache/ImageBySizeCache.cpp:22 > -#include "CSSImageGeneratorValue.h" > +#include "ImageBySizeCache.h" This shouldn't be under loader/cache, that is really for classes closer to networking. rendering/ would probably be appropriate as this deals with mappings from RenderObjects. > Source/WebCore/loader/cache/ImageBySizeCache.cpp:66 > -Image* CSSImageGeneratorValue::getImage(RenderObject* renderer, const IntSize& size) > +Image* ImageBySizeCache::getImage(const RenderObject* renderer, const IntSize& size) Not a fan of the name "ImageBySizeCache". It doesn't really capture what this class does. From client perspective it really maps from RenderObject to Image. I couldn't came up with any great names though. > Source/WebCore/loader/cache/ImageBySizeCache.h:35 > +typedef pair<IntSize, int> SizeCountPair; > +typedef HashMap<const RenderObject*, SizeCountPair> RenderObjectSizeCountMap; Copy code I know, but I would rather use a struct instead of pair<IntSize, int>. The resulting code reads much better. > Source/WebCore/platform/win/ClipboardWin.cpp:242 > - ASSERT(image->image()->data()); > + ASSERT(image->image(0)->data()); These all go away if you just keep image() for non-renderer specific case.

Created attachment 110844 [details] Next try - v3 Fixed Anttis comments. I hope merging worked fine.

r=me

(In reply to comment #30) > r=me Thanks a lot, landed in r97448. Now continuing work on a new patch for bug 47156.

Comment on attachment 110844 [details] Next try - v3 Fixed mac builds in r97452. On recreating the XCode additions on my other machine, I forgot to mark ImageBySizeCache.h private.