| Summary: | Implement image-set | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Beth Dakin <bdakin> | ||||||||||||||||||||
| Component: | CSS | Assignee: | Beth Dakin <bdakin> | ||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||
| Severity: | Normal | CC: | bdakin, ddkilzer, dglazkov, dino, eoconnor, gustavo, japhet, kennyluck, macpherson, menard, mike, mitz, ojan, paulirish, pnormand, webkit.review.bot, xan.lopez | ||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||
|
Description
Beth Dakin
2012-03-05 13:16:19 PST
You say on webkit-dev that this wouldn't be guarded by an ENABLE flag. Considering it exposes in-progress API to the Web, I wonder if it should be guarded. A port could ship with an unfinished implementation. Created attachment 131687 [details]
Patch: image-set for the content property
Here's an initial implementation. This version only hooks up image-set for the content property. I decided to add the ifdef after all. Platforms will have to opt in. If this gets reviewed and checked in, I will be sure to reply to webkit-dev to make sure other ports know that I added the ifdef after all. This implementation leaves the following important follow-up bugs:
-Hook up -webkit-image-set for all other CSS properties that take images (background, border, etc)
-image-set should update dynamically when the device scale factor changes
-image-set should kick in for all other forms of scaling instead of just device scale (page scale, CSS transforms, page zoom)
Attachment 131687 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/css/CSSStyleSelector.cpp:4182: Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html). [readability/pass_ptr] [5] Total errors found: 1 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style. Comment on attachment 131687 [details] Patch: image-set for the content property View in context: https://bugs.webkit.org/attachment.cgi?id=131687&action=review > Source/WebCore/css/CSSImageSetValue.cpp:13 > + * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY This version of the license is not current. Created attachment 132144 [details]
Patch with proper license (i think)
Created attachment 132728 [details]
Patch that works for all image properties
Here's my latest version of the patch. This hooks up image-set for background, borer-image, and masks as well as the original hook-up for content. There are still a couple of follow-up bugs:
-image-set should kick in dynamically when the device scale factor changes
-image-set should kick in for all other forms of scaling instead of just device scale (page scale, CSS transform, page zoom)
-We need real hi res and low res artwork that would actually be used in the border-image and mask image case to confirm that everything is actually happening properly.
Created attachment 132729 [details]
Marked a s a patch this time
Oops, forgot to mark that last one as a patch.
Attachment 132729 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/css/CSSStyleSelector.cpp:4169: Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html). [readability/pass_ptr] [5] Total errors found: 1 in 42 files If any of these errors are false positives, please file a bug against check-webkit-style. Comment on attachment 132729 [details] Marked a s a patch this time View in context: https://bugs.webkit.org/attachment.cgi?id=132729&action=review > Source/WebCore/css/CSSParser.cpp:6612 > +// FIXME: At this time, this is only hooked up for the content property, but in the future Oops! This comment is no longer accurate. Removed. Comment on attachment 132729 [details] Marked a s a patch this time Attachment 132729 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/11999025 Comment on attachment 132729 [details] Marked a s a patch this time Attachment 132729 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/11995025 Comment on attachment 132729 [details] Marked a s a patch this time Attachment 132729 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11994055 Comment on attachment 132729 [details] Marked a s a patch this time Attachment 132729 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12005051 Comment on attachment 132729 [details] Marked a s a patch this time Attachment 132729 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11994087 Comment on attachment 132729 [details] Marked a s a patch this time Attachment 132729 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12001056 Comment on attachment 132729 [details] Marked a s a patch this time Attachment 132729 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12001130 Created attachment 132851 [details]
Patch that will hopefully build
Created attachment 132880 [details]
Patch that will hopefully apply and build
Attachment 132880 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/css/CSSStyleSelector.cpp:4171: Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html). [readability/pass_ptr] [5] Total errors found: 1 in 42 files If any of these errors are false positives, please file a bug against check-webkit-style. Comment on attachment 132880 [details] Patch that will hopefully apply and build Attachment 132880 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12044009 Created attachment 132889 [details]
Another go for the bots
Comment on attachment 132851 [details] Patch that will hopefully build View in context: https://bugs.webkit.org/attachment.cgi?id=132851&action=review I wonder if you could split this up into two patches: one that does the parsing of the new syntax (and collects the info) and another that does the rendering side. Or three, if you want to separate the changes to Cached, Style and GeneratedImage. I also suggest you have some parsing tests, especially for broken input (a set without urls, or multiple scales, or missing commas, etc). e.g. LayoutTests/css3/filters/script-tests/filter-property-parsing-invalid.js and LayoutTests/css3/filters/script-tests/filter-property-parsing.js These were really helpful in finding errors in the input processing. > Source/JavaScriptCore/ChangeLog:8 > + For the time being, image-set it opt-in since the implementation is typo "it" Also, I never know when you should use Platform.h vs a flag in build-webkit. Platform is definitely easier. > Source/WebCore/ChangeLog:4 > + Implement image-set Maybe this should link to the proposal on www-style? And describe what it is. > Source/WebCore/ChangeLog:13 > + CSSImageSetValue inherits from CSSValueList and behaves a lot like > + CSSImageValue. > + * WebCore.xcodeproj/project.pbxproj: > + * css/CSSImageSetValue.h: Added. I assume CSSImageSetValue is never seen outside WebCore (i.e. can I get one in JavaScript)? > Source/WebCore/css/CSSImageSetValue.cpp:86 > +CSSImageSetValue::ImageWithScale CSSImageSetValue::bestImageForScaleFactor(float scaleFactor) > +{ > + ImageWithScale image; > + size_t numberOfImages = m_imagesInSet.size(); > + for (size_t i = 0; i < numberOfImages; ++i) { > + image = m_imagesInSet.at(i); > + if (image.scaleFactor >= scaleFactor) > + return image; > + } > + return image; > +} What happens if the requested scaleFactor is larger than all the available image scaleFactors? > Source/WebCore/css/CSSImageSetValue.cpp:100 > + // FIXME: In the future, we want to take much more than deviceScaleFactor into acount here. > + // All forms of scale should be included: Page::pageScaleFactor(), Frame::pageZoomFactor(), > + // and any CSS transforms. Link to bug? > Source/WebCore/css/CSSImageSetValue.cpp:125 > +StyleCachedImageSet* CSSImageSetValue::cachedImageSet(CachedResourceLoader* loader, ImageWithScale image) > +{ > + ASSERT(loader); > + > + if (!m_accessedBestFitImage) { > + ResourceRequest request(loader->document()->completeURL(image.imageURL)); > + if (CachedImage* cachedImage = loader->requestImage(request)) { > + m_imageSet = StyleCachedImageSet::create(cachedImage, image.scaleFactor, this); > + m_accessedBestFitImage = true; > + } > + } > + > + return (m_imageSet && m_imageSet->isCachedImageSet()) ? static_cast<StyleCachedImageSet*>(m_imageSet.get()) : 0; > +} > + > +StyleImage* CSSImageSetValue::cachedOrPendingImageSet() > +{ > + if (!m_imageSet) > + m_imageSet = StylePendingImage::create(this); > + > + return m_imageSet.get(); > +} This is a dumb question, but I don't understand when you need the OrPending version. I was originally wondering if you could simply test for m_imageSet being not null rather than using m_accessedBestFitImage, but then I saw that m_imageSet is also set in the latter function. Maybe this is used later, in which case I forgot to delete this note :) OK! I found it below. I don't have a strong opinion, but I wonder if you can just have the top function here. It would return null in the case of no m_imageSet and you could test for that in the setOrPendingFromValue function. But maybe I haven't got all the logic right! > Source/WebCore/css/CSSImageSetValue.h:50 > + StyleCachedImageSet* cachedImageSet(CachedResourceLoader*); > + // Returns a StyleCachedImageSet if the best fit image has been cached already, otherwise a StylePendingImage. > + StyleImage* cachedOrPendingImageSet(); The comment applies to the top function right? Maybe it should go before or on the same line? > Source/WebCore/css/CSSParser.cpp:2944 > +#if ENABLE(CSS_IMAGE_SET) > + else if (equalIgnoringCase(val->function->name, "-webkit-image-set(")) { > + parsedValue = parseImageSet(m_valueList.get()); > + if (!parsedValue) > + return false; > + } > +#endif > + else if (isGeneratedImageValue(val)) { Indentation on last line. > Source/WebCore/css/CSSParser.cpp:5577 > + else > + return false; > + } > +#endif > + else if (val->id == CSSValueNone) Indentation on last line. > Source/WebCore/css/CSSParser.cpp:6624 > + if (!functionArgs || !functionArgs->size()) > + return 0; > + > + if (!functionArgs->current()) > + return 0; Merge? > Source/WebCore/css/CSSParser.cpp:6644 > + double imageScaleFactor = 0; > + const String& string = arg->string; > + const UChar* current = string.characters(); > + const UChar* end = current + string.length(); > + parseDouble(current, end, 'x', imageScaleFactor); > + imageSet->append(cssValuePool()->createValue(imageScaleFactor, CSSPrimitiveValue::CSS_NUMBER)); Should we check for > 0? > Source/WebCore/css/CSSStyleSelector.cpp:3056 > +#if ENABLE(CSS_IMAGE_SET) > + else if (item->isImageSetValue()) { > + m_style->setContent(setOrPendingFromValue(CSSPropertyContent, static_cast<CSSImageSetValue*>(item)), didSet); > + didSet = true; > + } > +#endif Indentation on else if line. > Source/WebCore/platform/graphics/Image.cpp:173 > -void Image::computeIntrinsicDimensions(Length& intrinsicWidth, Length& intrinsicHeight, FloatSize& intrinsicRatio) > +void Image::computeIntrinsicDimensions(Length& intrinsicWidth, Length& intrinsicHeight, FloatSize& intrinsicRatio, float scaleFactor) > { > intrinsicRatio = size(); > + intrinsicRatio.scale(1 / scaleFactor); I think we can pass in a scaleFactor of 0 here, unless we've stopped it in the parsing step. > Source/WebCore/rendering/style/StyleCachedImageSet.cpp:70 > +IntSize StyleCachedImageSet::imageSize(const RenderObject* renderer, float multiplier) const > +{ > + IntSize scaledImageSize = m_bestFitImage->imageSizeForRenderer(renderer, multiplier); > + scaledImageSize.scale(1 / m_imageScaleFactor); > + return scaledImageSize; > +} Since we don't test for 0 scales in the parsing method, the image.scaleFactor that's used at creation could be 0, which means we could be dividing by 0 here. > LayoutTests/fast/hidpi/image-set-as-background.html:11 > + if (!window.layoutTestController) > + return; > + > + > + if (!window.sessionStorage) > + return; > + Merge? Attachment 132889 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/css/CSSStyleSelector.cpp:4171: Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html). [readability/pass_ptr] [5] Total errors found: 1 in 42 files If any of these errors are false positives, please file a bug against check-webkit-style. (In reply to comment #22) > (From update of attachment 132851 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=132851&action=review > > I wonder if you could split this up into two patches: one that does the parsing of the new syntax (and collects the info) and another that does the rendering side. Or three, if you want to separate the changes to Cached, Style and GeneratedImage. > Lalalalalalalalalala I can't hear yooooou!!!!!! (I might consider it if I must for the sake of review though.) > I also suggest you have some parsing tests, especially for broken input (a set without urls, or multiple scales, or missing commas, etc). e.g. LayoutTests/css3/filters/script-tests/filter-property-parsing-invalid.js and LayoutTests/css3/filters/script-tests/filter-property-parsing.js > > These were really helpful in finding errors in the input processing. > I will take a look and add one. > > Source/JavaScriptCore/ChangeLog:8 > > + For the time being, image-set it opt-in since the implementation is > > typo "it" > Fixed. > Also, I never know when you should use Platform.h vs a flag in build-webkit. Platform is definitely easier. > I don't know either, but I do generally favor easy things unless there is a compelling reason to do it the more complicated way. > > Source/WebCore/ChangeLog:4 > > + Implement image-set > > Maybe this should link to the proposal on www-style? And describe what it is. > Done. > > Source/WebCore/ChangeLog:13 > > + CSSImageSetValue inherits from CSSValueList and behaves a lot like > > + CSSImageValue. > > + * WebCore.xcodeproj/project.pbxproj: > > + * css/CSSImageSetValue.h: Added. > > I assume CSSImageSetValue is never seen outside WebCore (i.e. can I get one in JavaScript)? > You cannot. > > Source/WebCore/css/CSSImageSetValue.cpp:86 > > +CSSImageSetValue::ImageWithScale CSSImageSetValue::bestImageForScaleFactor(float scaleFactor) > > +{ > > + ImageWithScale image; > > + size_t numberOfImages = m_imagesInSet.size(); > > + for (size_t i = 0; i < numberOfImages; ++i) { > > + image = m_imagesInSet.at(i); > > + if (image.scaleFactor >= scaleFactor) > > + return image; > > + } > > + return image; > > +} > > What happens if the requested scaleFactor is larger than all the available image scaleFactors? > You will get the largest image we have. We may want to refine this later, but it strikes me as a good starting implementation. > > Source/WebCore/css/CSSImageSetValue.cpp:100 > > + // FIXME: In the future, we want to take much more than deviceScaleFactor into acount here. > > + // All forms of scale should be included: Page::pageScaleFactor(), Frame::pageZoomFactor(), > > + // and any CSS transforms. > > Link to bug? > Added. > > Source/WebCore/css/CSSImageSetValue.cpp:125 > > +StyleCachedImageSet* CSSImageSetValue::cachedImageSet(CachedResourceLoader* loader, ImageWithScale image) > > +{ > > + ASSERT(loader); > > + > > + if (!m_accessedBestFitImage) { > > + ResourceRequest request(loader->document()->completeURL(image.imageURL)); > > + if (CachedImage* cachedImage = loader->requestImage(request)) { > > + m_imageSet = StyleCachedImageSet::create(cachedImage, image.scaleFactor, this); > > + m_accessedBestFitImage = true; > > + } > > + } > > + > > + return (m_imageSet && m_imageSet->isCachedImageSet()) ? static_cast<StyleCachedImageSet*>(m_imageSet.get()) : 0; > > +} > > + > > +StyleImage* CSSImageSetValue::cachedOrPendingImageSet() > > +{ > > + if (!m_imageSet) > > + m_imageSet = StylePendingImage::create(this); > > + > > + return m_imageSet.get(); > > +} > > This is a dumb question, but I don't understand when you need the OrPending version. I was originally wondering if you could simply test for m_imageSet being not null rather than using m_accessedBestFitImage, but then I saw that m_imageSet is also set in the latter function. Maybe this is used later, in which case I forgot to delete this note :) OK! I found it below. I don't have a strong opinion, but I wonder if you can just have the top function here. It would return null in the case of no m_imageSet and you could test for that in the setOrPendingFromValue function. But maybe I haven't got all the logic right! > Definitely not dumb! I was combing through a lot of this when writing the patch, and I just mimicked what the other image classes do without fully understanding why they do what they do. I think it's good to have all of the image types match each other and return a pending image when they are not yet loaded, but at the same time, it would be good to understand why the existing classes do it that way. If there is not a compelling reason, we could consider changing them all at once. > > Source/WebCore/css/CSSImageSetValue.h:50 > > + StyleCachedImageSet* cachedImageSet(CachedResourceLoader*); > > + // Returns a StyleCachedImageSet if the best fit image has been cached already, otherwise a StylePendingImage. > > + StyleImage* cachedOrPendingImageSet(); > > The comment applies to the top function right? Maybe it should go before or on the same line? > Actually it applies to the bottom function. I added a space to make that more clear. > > Source/WebCore/css/CSSParser.cpp:2944 > > +#if ENABLE(CSS_IMAGE_SET) > > + else if (equalIgnoringCase(val->function->name, "-webkit-image-set(")) { > > + parsedValue = parseImageSet(m_valueList.get()); > > + if (!parsedValue) > > + return false; > > + } > > +#endif > > + else if (isGeneratedImageValue(val)) { > > Indentation on last line. > Fixed. > > Source/WebCore/css/CSSParser.cpp:5577 > > + else > > + return false; > > + } > > +#endif > > + else if (val->id == CSSValueNone) > > Indentation on last line. > Fixed. > > Source/WebCore/css/CSSParser.cpp:6624 > > + if (!functionArgs || !functionArgs->size()) > > + return 0; > > + > > + if (!functionArgs->current()) > > + return 0; > > Merge? > Done. > > Source/WebCore/css/CSSParser.cpp:6644 > > + double imageScaleFactor = 0; > > + const String& string = arg->string; > > + const UChar* current = string.characters(); > > + const UChar* end = current + string.length(); > > + parseDouble(current, end, 'x', imageScaleFactor); > > + imageSet->append(cssValuePool()->createValue(imageScaleFactor, CSSPrimitiveValue::CSS_NUMBER)); > > Should we check for > 0? > I think so. Added. > > Source/WebCore/css/CSSStyleSelector.cpp:3056 > > +#if ENABLE(CSS_IMAGE_SET) > > + else if (item->isImageSetValue()) { > > + m_style->setContent(setOrPendingFromValue(CSSPropertyContent, static_cast<CSSImageSetValue*>(item)), didSet); > > + didSet = true; > > + } > > +#endif > > Indentation on else if line. > Fixed. > > Source/WebCore/platform/graphics/Image.cpp:173 > > -void Image::computeIntrinsicDimensions(Length& intrinsicWidth, Length& intrinsicHeight, FloatSize& intrinsicRatio) > > +void Image::computeIntrinsicDimensions(Length& intrinsicWidth, Length& intrinsicHeight, FloatSize& intrinsicRatio, float scaleFactor) > > { > > intrinsicRatio = size(); > > + intrinsicRatio.scale(1 / scaleFactor); > > I think we can pass in a scaleFactor of 0 here, unless we've stopped it in the parsing step. > Fixed in parser. > > Source/WebCore/rendering/style/StyleCachedImageSet.cpp:70 > > +IntSize StyleCachedImageSet::imageSize(const RenderObject* renderer, float multiplier) const > > +{ > > + IntSize scaledImageSize = m_bestFitImage->imageSizeForRenderer(renderer, multiplier); > > + scaledImageSize.scale(1 / m_imageScaleFactor); > > + return scaledImageSize; > > +} > > Since we don't test for 0 scales in the parsing method, the image.scaleFactor that's used at creation could be 0, which means we could be dividing by 0 here. > Fixed in parser. > > LayoutTests/fast/hidpi/image-set-as-background.html:11 > > + if (!window.layoutTestController) > > + return; > > + > > + > > + if (!window.sessionStorage) > > + return; > > + > > Merge? Fixed. I will post a new patch once I make a parsing test. (In reply to comment #24) > (In reply to comment #22) > > (From update of attachment 132851 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=132851&action=review > > > > I wonder if you could split this up into two patches: one that does the parsing of the new syntax (and collects the info) and another that does the rendering side. Or three, if you want to separate the changes to Cached, Style and GeneratedImage. > > > > Lalalalalalalalalala I can't hear yooooou!!!!!! > > (I might consider it if I must for the sake of review though.) I actually wrote that part way through, but once I got to the end I wasn't too zapped. It's not a huge patch. > > > > Source/WebCore/css/CSSImageSetValue.cpp:86 > > > +CSSImageSetValue::ImageWithScale CSSImageSetValue::bestImageForScaleFactor(float scaleFactor) > > > +{ > > > + ImageWithScale image; > > > + size_t numberOfImages = m_imagesInSet.size(); > > > + for (size_t i = 0; i < numberOfImages; ++i) { > > > + image = m_imagesInSet.at(i); > > > + if (image.scaleFactor >= scaleFactor) > > > + return image; > > > + } > > > + return image; > > > +} > > > > What happens if the requested scaleFactor is larger than all the available image scaleFactors? > > > > You will get the largest image we have. We may want to refine this later, but it strikes me as a good starting implementation. So, won't that mean in this case you'd get an empty ImageWithScale here? Which would then be passed to CSSImageSetValue::cachedImageSet(CachedResourceLoader* loader, ImageWithScale image) and there would be no image to load. Sorry if I'm wrong. I didn't follow through everything carefully. > > > LayoutTests/fast/hidpi/image-set-as-background.html:11 > > > + if (!window.layoutTestController) > > > + return; > > > + > > > + > > > + if (!window.sessionStorage) > > > + return; > > > + > > > > Merge? > > Fixed. I forgot to say that this was in every test. (In reply to comment #25) > (In reply to comment #24) > > (In reply to comment #22) > > > (From update of attachment 132851 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=132851&action=review > > > > > > I wonder if you could split this up into two patches: one that does the parsing of the new syntax (and collects the info) and another that does the rendering side. Or three, if you want to separate the changes to Cached, Style and GeneratedImage. > > > > > > > Lalalalalalalalalala I can't hear yooooou!!!!!! > > > > (I might consider it if I must for the sake of review though.) > > I actually wrote that part way through, but once I got to the end I wasn't too zapped. It's not a huge patch. > Hehehe, thanks! I certainly don't want to be disagreeable, so I really would consider splitting it, but of course it's easier not to. > > > > > > > Source/WebCore/css/CSSImageSetValue.cpp:86 > > > > +CSSImageSetValue::ImageWithScale CSSImageSetValue::bestImageForScaleFactor(float scaleFactor) > > > > +{ > > > > + ImageWithScale image; > > > > + size_t numberOfImages = m_imagesInSet.size(); > > > > + for (size_t i = 0; i < numberOfImages; ++i) { > > > > + image = m_imagesInSet.at(i); > > > > + if (image.scaleFactor >= scaleFactor) > > > > + return image; > > > > + } > > > > + return image; > > > > +} > > > > > > What happens if the requested scaleFactor is larger than all the available image scaleFactors? > > > > > > > You will get the largest image we have. We may want to refine this later, but it strikes me as a good starting implementation. > > So, won't that mean in this case you'd get an empty ImageWithScale here? Which would then be passed to > CSSImageSetValue::cachedImageSet(CachedResourceLoader* loader, ImageWithScale image) > and there would be no image to load. > > Sorry if I'm wrong. I didn't follow through everything carefully. > I don't think you'll get an empty ImageWithScale. The function looks like this: CSSImageSetValue::ImageWithScale CSSImageSetValue::bestImageForScaleFactor(float scaleFactor) { ImageWithScale image; size_t numberOfImages = m_imagesInSet.size(); for (size_t i = 0; i < numberOfImages; ++i) { image = m_imagesInSet.at(i); if (image.scaleFactor >= scaleFactor) return image; } return image; } So imagine an input parameter of scaleFactor = 3. Also imagine m_imagesInSet has only one image in it with a scale of 1. We'll enter the loop for only one iteration, and our temporary variable will be set to the image with a scale factor of 1. We'll fail the conditional check, but then we will return that image anyway after the loop. > > > > > LayoutTests/fast/hidpi/image-set-as-background.html:11 > > > > + if (!window.layoutTestController) > > > > + return; > > > > + > > > > + > > > > + if (!window.sessionStorage) > > > > + return; > > > > + > > > > > > Merge? > > > > Fixed. > > I forgot to say that this was in every test. Caught that, thanks! Still working on that parsing test. Hopefully I'll have it soon. Created attachment 133140 [details]
Patch with parsing tests
(In reply to comment #26) > > > > > Source/WebCore/css/CSSImageSetValue.cpp:86 > > > > > +CSSImageSetValue::ImageWithScale CSSImageSetValue::bestImageForScaleFactor(float scaleFactor) > > > > > +{ > > > > > + ImageWithScale image; > > > > > + size_t numberOfImages = m_imagesInSet.size(); > > > > > + for (size_t i = 0; i < numberOfImages; ++i) { > > > > > + image = m_imagesInSet.at(i); > > > > > + if (image.scaleFactor >= scaleFactor) > > > > > + return image; > > > > > + } > > > > > + return image; > > > > > +} > > > > > > > > What happens if the requested scaleFactor is larger than all the available image scaleFactors? > > > > > > > > > > You will get the largest image we have. We may want to refine this later, but it strikes me as a good starting implementation. > > > > So, won't that mean in this case you'd get an empty ImageWithScale here? Which would then be passed to > > CSSImageSetValue::cachedImageSet(CachedResourceLoader* loader, ImageWithScale image) > > and there would be no image to load. > > > > Sorry if I'm wrong. I didn't follow through everything carefully. > > > > I don't think you'll get an empty ImageWithScale. The function looks like this: > > CSSImageSetValue::ImageWithScale CSSImageSetValue::bestImageForScaleFactor(float scaleFactor) > { > ImageWithScale image; > size_t numberOfImages = m_imagesInSet.size(); > for (size_t i = 0; i < numberOfImages; ++i) { > image = m_imagesInSet.at(i); > if (image.scaleFactor >= scaleFactor) > return image; > } > return image; > } > > So imagine an input parameter of scaleFactor = 3. Also imagine m_imagesInSet has only one image in it with a scale of 1. We'll enter the loop for only one iteration, and our temporary variable will be set to the image with a scale factor of 1. We'll fail the conditional check, but then we will return that image anyway after the loop. > Duh! I knew I shouldn't drink at work :) Created attachment 133150 [details]
Without Platform.h insanity this time
Comment on attachment 133150 [details] Without Platform.h insanity this time View in context: https://bugs.webkit.org/attachment.cgi?id=133150&action=review > Source/WTF/ChangeLog:10 > + For the time being, image-set is opt-in since the implementation is > + incomplete. > + * wtf/Platform.h: Missing blank link. Also, after Tim's recent email to webkit-dev, I think this should mention that it adds an ENABLE flag. > Source/WebCore/WebCore.xcodeproj/project.pbxproj:13653 > - FD315FA212B025B100C1A359 /* webaudio */, > + FD315FA212B025B100C1A359 /* Modules/webaudio */, Cruft. > Source/WebCore/WebCore.xcodeproj/project.pbxproj:21184 > - FD315FA212B025B100C1A359 /* webaudio */ = { > + FD315FA212B025B100C1A359 /* Modules/webaudio */ = { More cruft. > Source/WebCore/css/CSSParser.cpp:3137 > - } else if (isGeneratedImageValue(val)) { > + } > +#if ENABLE(CSS_IMAGE_SET) > + else if (equalIgnoringCase(val->function->name, "-webkit-image-set(")) { > + parsedValue = parseImageSet(m_valueList.get()); Indentation on line after #if. With this, couldn't you leave the existing "} else if" as is and start the bit inside the #if with "} else if" (but before the existing code)? > Source/WebCore/css/CSSParser.cpp:5767 > - } else if (val->id == CSSValueNone) > + } > +#if ENABLE(CSS_IMAGE_SET) > + else if (val->unit == CSSParserValue::Function && equalIgnoringCase(val->function->name, "-webkit-image-set(")) { Same comments on the order and changing here. > Source/WebCore/css/CSSStyleSelector.cpp:4536 > + if (current->isImageValue() || current->isImageGeneratorValue() > +#if ENABLE(CSS_IMAGE_SET) > + || current->isImageSetValue() > +#endif I *think* that the indentation is supposed to line up with the term in the conditional. > LayoutTests/ChangeLog:31 > + * platform/efl/Skipped: > + * platform/gtk/Skipped: What about the test_expectations files for Chrome? > LayoutTests/fast/css/script-tests/image-set-parsing.js:84 > +testImageSetRule("Single value for content", > + "content", > + "url('#a') 1x", 2, > + ["a", "1"]); > + some whacky indentation in this test > LayoutTests/fast/hidpi/image-set-out-of-order.html:35 > + width:100px; > + height:100px; nit: space > LayoutTests/fast/hidpi/image-set-simple.html:42 > + <div>This test passes if the div below is a blue 100px square when the deviceScaleFactor is 1, and if it is a 100px red square when the deviceScaleFactor is 2.</div> I wonder if we should use green instead of red. red == evil. Thanks Dean! I think I addressed all of your comments. http://trac.webkit.org/changeset/111637 The new pixel tests added in this patch would be great as reftests. That way when this is enabled for all ports, we wouldn't need a ton of platform specific results. Follow-up bugs: -webkit-image-set should update dynamically when the device scale factor changes https://bugs.webkit.org/show_bug.cgi?id=81939 -webkit-image-set should consider Page::pageScaleFactor(), Frame::pageZoomFactor(), and CSS transforms https://bugs.webkit.org/show_bug.cgi?id=81698 -webkit-image-set should support all the image functions WebKit supports, not just url() https://bugs.webkit.org/show_bug.cgi?id=81941 And I filed one port-specific bug to enable image-set for Chromium since it was required by the test expectations file: https://bugs.webkit.org/show_bug.cgi?id=81859 |