Bug 110252 - Implement img element's srcset attribute
: Implement img element's srcset attribute
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Images
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Romain Perier
: InRadar, WebExposed
Depends on:
Blocks: 119360 119407 119413 119423
  Show dependency treegraph
 
Reported: 2013-02-19 12:35 PST by Theresa O'Connor
Modified: 2013-11-05 09:06 PST (History)
29 users (show)

See Also:


Attachments
wip_patch (6.43 KB, patch)
2013-02-21 15:19 PST, Vineet Chaudhary (vineetc)
no flags Details | Formatted Diff | Diff
wip_patch_002 (13.10 KB, patch)
2013-02-22 13:47 PST, Vineet Chaudhary (vineetc)
no flags Details | Formatted Diff | Diff
updated_patch (17.08 KB, patch)
2013-02-26 13:03 PST, Vineet Chaudhary (vineetc)
rniwa: review-
Details | Formatted Diff | Diff
Patch (59.62 KB, patch)
2013-07-16 04:03 PDT, Romain Perier
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion (543.71 KB, application/zip)
2013-07-16 06:43 PDT, Build Bot
no flags Details
Patch (59.74 KB, patch)
2013-07-16 11:08 PDT, Romain Perier
no flags Details | Formatted Diff | Diff
Patch (59.78 KB, patch)
2013-07-17 00:00 PDT, Romain Perier
no flags Details | Formatted Diff | Diff
Patch (59.95 KB, patch)
2013-07-18 03:57 PDT, Romain Perier
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 (487.63 KB, application/zip)
2013-07-18 11:51 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (479.40 KB, application/zip)
2013-07-18 15:10 PDT, Build Bot
no flags Details
Patch (202.61 KB, patch)
2013-07-26 01:44 PDT, Romain Perier
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (341.56 KB, application/zip)
2013-07-26 09:10 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 (311.46 KB, application/zip)
2013-07-28 12:17 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Theresa O'Connor 2013-02-19 12:35:25 PST
The feature has been present in the WHATWG HTML spec for several months now.

http://www.whatwg.org/specs/web-apps/current-work/multipage/embedded-content-1.html#the-img-element

It is also being pursued in a smaller document at the W3C:

http://www.w3.org/html/wg/drafts/srcset/w3c-srcset/Overview.html

(All normative requirements in this latter document are also present in the WHATWG spec, but you might find it easier to reference as it only deals with <img srcset>.)
Comment 1 Radar WebKit Bug Importer 2013-02-19 12:35:40 PST
<rdar://problem/13246364>
Comment 2 Vineet Chaudhary (vineetc) 2013-02-21 15:19:52 PST
Created attachment 189615 [details]
wip_patch
Comment 3 Vineet Chaudhary (vineetc) 2013-02-21 15:20:14 PST
Uploading patch just expose the srcset attribute to javascripts.
there is whatwg discussion thread on "srcset"
http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2012-May/035746.html

ToDo>> 
1)It needs to parse the given string for srcset attr.
  like we need to keep the parse values of urls and its descriptors in some datastructure(maybe map)

2)user-agents/ports needs to read and compare these values to compare with current viewport settings
  to select appropriate image source in "ImageLoader"

Please let me know if any concerns/suggestions to proceed further.
Comment 4 Vineet Chaudhary (vineetc) 2013-02-22 13:47:23 PST
Created attachment 189817 [details]
wip_patch_002

Adding some "srcset" string parsing mechanism and storing values in map.
Idea is to parse the url and descriptors values and compare those with 
current viewport values to select appropriate image "src" before 
ImageLoader requestImage().
Comment 5 Vineet Chaudhary (vineetc) 2013-02-26 13:03:30 PST
Created attachment 190345 [details]
updated_patch
Comment 6 Maciej Stachowiak 2013-02-27 01:06:00 PST
Comment on attachment 190345 [details]
updated_patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190345&action=review

> Source/WebCore/loader/ImageLoader.cpp:205
> +    while (i < length) {
> +        while (buffer[i] == ' ') {
> +            if (i >= length)
> +                break;
> +            i++;
> +        }
> +        keyBegin = i;
> +        while (buffer[i] != ' ') {
> +            if (i >= length)
> +                break;
> +            i++;
> +        }
> +        keyEnd = i;
> +        String url = buffer.substring(keyBegin, keyEnd - keyBegin);
> +        while (buffer[i] == ',') {
> +            if (i >= length)
> +                break;
> +            i++;
> +        }
> +        keyBegin = i;
> +        while (buffer[i] != ',') {
> +            if (i >= length)
> +                break;
> +            i++;
> +        }
> +        keyEnd = i;
> +        String descriptor = buffer.substring(keyBegin, keyEnd - keyBegin);
> +        if (!url.isEmpty())
> +            processImageSourceAttributes(url, descriptor);
> +    }

This parsing code is messy and way too complicated for a simple format like the srcset descriptor list. For starters, why did you write all those 5-line while loops instead of using String::find?
Comment 7 Maciej Stachowiak 2013-02-27 01:10:40 PST
Comment on attachment 190345 [details]
updated_patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190345&action=review

> LayoutTests/ChangeLog:12
> +        * fast/dom/HTMLImageElement/image-srcset-attribute-expected.txt: Added.
> +        * fast/dom/HTMLImageElement/image-srcset-attribute.html: Added.

This test checks the DOM behavior but it doesn't test that srcset processing chooses the right candidate image.
Comment 8 Tim Horton 2013-02-27 01:20:51 PST
Comment on attachment 190345 [details]
updated_patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190345&action=review

> Source/WebCore/loader/ImageLoader.cpp:216
> +    int keyBegin, keyEnd, i = 0;

I don't think we usually do this.

> Source/WebCore/loader/ImageLoader.h:44
> +        Valuedensity = 1

Not sure what's going on here but I think it's a capitalization problem.

> Source/WebCore/loader/ImageLoader.h:48
> +    int density;

Density? Looking at the spec, I guess this is "pixel density". Still a bit odd.

> Source/WebCore/page/Chrome.h:169
> +    AtomicString getImageSource(const HashMap<String, ImageSourceDescriptors>&) const;

If you have to use this long type in lots of places you could typedef it (or not, I'm never clear exactly what our stance on that is).
Comment 9 Ryosuke Niwa 2013-04-08 14:42:21 PDT
Comment on attachment 190345 [details]
updated_patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190345&action=review

Where are we selecting the correct image?

> Source/WebCore/ChangeLog:9
> +        Spec : http://www.whatwg.org/specs/web-apps/current-work/multipage/embedded-content-1.html#the-img-element

Could you refer to the specific version of the spec you used?

> Source/WebCore/dom/Element.cpp:1132
> +const QualifiedName& Element::imageSourceSetAttributeName() const
> +{
> +    return srcsetAttr;
> +}

Why do we need this function? Since there is no alternative name to srcsetAttr we should just hardcode it.

> Source/WebCore/html/HTMLImageElement.cpp:308
> +    return document()->completeURL(getAttribute(srcsetAttr));

How does this work? completeURL doesn't support srcset syntax. r- because of this.

>> Source/WebCore/loader/ImageLoader.cpp:205
>> +    }
> 
> This parsing code is messy and way too complicated for a simple format like the srcset descriptor list. For starters, why did you write all those 5-line while loops instead of using String::find?

I completely agree with Maciej here. r- because of this as well.
In addition, this code should probably live in HTMLImageElement instead.
ImageLoader should only be concerned about loading images, not a particular syntax used in an element that loads an image.

> Source/WebCore/loader/ImageLoader.cpp:214
> +void ImageLoader::processImageSourceAttributes(String& urlString, String& descString)

I would also put this on HTMLImageElement.

> Source/WebCore/loader/ImageLoader.h:47
> +    int width;
> +    int height;

Can we ever have negative width & height?
Comment 10 Beth Dakin 2013-04-11 16:00:00 PDT
Vineet, are you planning to continue working on this?
Comment 11 Beth Dakin 2013-04-25 10:44:10 PDT
Un-assigning from Vineet due to inactivity.
Comment 12 Theresa O'Connor 2013-04-25 10:59:37 PDT
FWIW, I think it would be better to tackle a subset of the srcset="" microsyntax and feature set, at least initially. The x specifier has a clear and immediate value (HiDPI), whereas it's less clear if the w and h specifiers are the right solution to the problems they set out to solve.
Comment 13 Dean Jackson 2013-04-25 14:18:41 PDT
(In reply to comment #12)
> FWIW, I think it would be better to tackle a subset of the srcset="" microsyntax and feature set, at least initially. The x specifier has a clear and immediate value (HiDPI), whereas it's less clear if the w and h specifiers are the right solution to the problems they set out to solve.

Agreed. It would still be extremely useful and on par feature-wise with CSS -image-set
Comment 14 Romain Perier 2013-07-16 04:03:04 PDT
Created attachment 206753 [details]
Patch
Comment 15 Romain Perier 2013-07-16 04:07:59 PDT
See a first version of my patch in attachement. For now it only supports the x specifier (as suggested by edward and dean). It includes tests and expected files.

Changes are documented in the corresponding ChangeLogs. Feel free to give me your feedbacks, all suggestions are welcome :)
Comment 16 Early Warning System Bot 2013-07-16 04:10:36 PDT
Comment on attachment 206753 [details]
Patch

Attachment 206753 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1088135
Comment 17 Early Warning System Bot 2013-07-16 04:15:05 PDT
Comment on attachment 206753 [details]
Patch

Attachment 206753 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1090170
Comment 18 EFL EWS Bot 2013-07-16 04:21:30 PDT
Comment on attachment 206753 [details]
Patch

Attachment 206753 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1089171
Comment 19 EFL EWS Bot 2013-07-16 04:33:33 PDT
Comment on attachment 206753 [details]
Patch

Attachment 206753 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1090177
Comment 20 Chris Dumez 2013-07-16 04:39:40 PDT
Comment on attachment 206753 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=206753&action=review

> Source/WebCore/html/HTMLImageElement.cpp:118
> +void HTMLImageElement::bestImageForScaleFactor()

Looks like a getter. Should probably be called updateBestImageForScaleFactor() or similar.

> Source/WebCore/html/HTMLImageElement.cpp:135
> +    const String& string = (String)value;

Please avoid C casting

> Source/WebCore/html/HTMLImageElement.cpp:158
> +    const AtomicString &src = getAttribute(srcAttr);

& on wrong side.

> Source/WebCore/html/HTMLImageElement.cpp:165
> +        [](ImageWithScale first, ImageWithScale second)

Shouldn't we pass ImageWithScale by const reference?

> Source/WebCore/html/HTMLImageElement.cpp:171
> +    for (size_t i = 1; i < m_imageSet.size(); i++) {

Looks like you are removing consecutive duplicates? If so, you may consider http://www.cplusplus.com/reference/algorithm/unique/

> Source/WebCore/html/HTMLImageElement.h:79
> +    virtual const AtomicString& imageSourceURL() const;

Missing OVERRIDE

> Source/WebCore/html/HTMLImageElement.h:124
> +    ssize_t m_bestFitImageIndex;

We seem to use size_t and notFound from NotFound.h. Not sure ssize_t is portable.
Comment 21 Build Bot 2013-07-16 06:43:41 PDT
Comment on attachment 206753 [details]
Patch

Attachment 206753 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1091175

New failing tests:
fullscreen/full-screen-iframe-with-max-width-height.html
Comment 22 Build Bot 2013-07-16 06:43:47 PDT
Created attachment 206775 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.3
Comment 23 Romain Perier 2013-07-16 11:08:56 PDT
Created attachment 206799 [details]
Patch
Comment 24 Romain Perier 2013-07-16 11:16:33 PDT
(In reply to comment #20)
> (From update of attachment 206753 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=206753&action=review
> 
> > Source/WebCore/html/HTMLImageElement.cpp:118
> > +void HTMLImageElement::bestImageForScaleFactor()
> 
> Looks like a getter. Should probably be called updateBestImageForScaleFactor() or similar.

DONE

> 
> > Source/WebCore/html/HTMLImageElement.cpp:135
> > +    const String& string = (String)value;
> 
> Please avoid C casting

DONE

> 
> > Source/WebCore/html/HTMLImageElement.cpp:158
> > +    const AtomicString &src = getAttribute(srcAttr);
> 
> & on wrong side.
>

DONE
 
> > Source/WebCore/html/HTMLImageElement.cpp:165
> > +        [](ImageWithScale first, ImageWithScale second)
> 
> Shouldn't we pass ImageWithScale by const reference?
>

DONE

> > Source/WebCore/html/HTMLImageElement.cpp:171
> > +    for (size_t i = 1; i < m_imageSet.size(); i++) {
> 
> Looks like you are removing consecutive duplicates? If so, you may consider http://www.cplusplus.com/reference/algorithm/unique/

It won't work, see the spec http://www.w3.org/html/wg/drafts/srcset/w3c-srcset/#processing-the-image-candidates  at item 16. You need to remove the last entry of a group of duplicates ones not the first one. I am not sure but I should probably move this loop before the call to std::sort  because according to the spec "candidates" is not sorted.

> 
> > Source/WebCore/html/HTMLImageElement.h:79
> > +    virtual const AtomicString& imageSourceURL() const;
> 
> Missing OVERRIDE
>

DONE

> > Source/WebCore/html/HTMLImageElement.h:124
> > +    ssize_t m_bestFitImageIndex;
> 
> We seem to use size_t and notFound from NotFound.h. Not sure ssize_t is portable.

DONE
Comment 25 EFL EWS Bot 2013-07-16 11:24:11 PDT
Comment on attachment 206799 [details]
Patch

Attachment 206799 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1082259
Comment 26 Early Warning System Bot 2013-07-16 11:28:04 PDT
Comment on attachment 206799 [details]
Patch

Attachment 206799 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1084276
Comment 27 Early Warning System Bot 2013-07-16 11:31:43 PDT
Comment on attachment 206799 [details]
Patch

Attachment 206799 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1080277
Comment 28 EFL EWS Bot 2013-07-16 11:32:41 PDT
Comment on attachment 206799 [details]
Patch

Attachment 206799 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1083267
Comment 29 Romain Perier 2013-07-17 00:00:45 PDT
Created attachment 206861 [details]
Patch
Comment 30 Early Warning System Bot 2013-07-17 00:12:57 PDT
Comment on attachment 206861 [details]
Patch

Attachment 206861 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1099104
Comment 31 Early Warning System Bot 2013-07-17 00:13:41 PDT
Comment on attachment 206861 [details]
Patch

Attachment 206861 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1110090
Comment 32 Benjamin Poulain 2013-07-17 00:54:00 PDT
Comment on attachment 206861 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=206861&action=review

Great work! That is quite something for a first patch :)
Sorry, I did not get the time to do a proper review today. Some quick comments bellow.

I think we should also add a Feature Flag (http://trac.webkit.org/wiki/AddingFeatures) if we want to mature this feature.

> Source/WebCore/html/HTMLImageElement.cpp:136
> +    const String& string = static_cast<String>(value);

static_cast<const String&>

> Source/WebCore/html/HTMLImageElement.cpp:138
> +    ImageWithScale image;

I would declare this where is it used instead.

> Source/WebCore/html/HTMLImageElement.cpp:141
> +    string.split(",", srcSet);

"," -> ','

I would move the declaration of srcSet just above this line to reduce the span declaration<->use.

> Source/WebCore/html/HTMLImageElement.cpp:142
> +    for (size_t i = 0; i < srcSet.size(); i++) {

i++ -> ++i

> Source/WebCore/html/HTMLImageElement.cpp:146
> +        srcSet[i].stripWhiteSpace().split(" ", data);

" " -> ' '

> Source/WebCore/html/HTMLImageElement.cpp:148
> +            if (data[1].endsWith("x")) {

"x" -> 'x'

> Source/WebCore/html/HTMLImageElement.cpp:165
> +    std::sort(m_imageSet.begin(), m_imageSet.end(), compareByScaleFactor);

Shouldn't the sort be stable?

What if the srcset gives several input for the same scale factor? The one we chose should not depends on the sort algorithm.

> Source/WebCore/html/HTMLImageElement.cpp:184
> +        updateImageSet(value);
> +        updateBestImageForScaleFactor();

Shouldn't this also be run when srcAttr changes?

> Source/WebCore/html/HTMLImageElement.h:117
> +        AtomicString imageURL;

Does this really need to be Atomic?

> Source/WebCore/html/HTMLImageElement.h:130
> +    Vector<ImageWithScale> m_imageSet;

The naming is not ideal, because m_imageSet is not a Set :)

> LayoutTests/ChangeLog:16
> +        * platform/mac/fast/hidpi/image-srcset-simple-expected.png:
> +        * platform/mac/fast/hidpi/image-srcset-simple-expected.txt:
> +        * platform/mac/fast/hidpi/image-srcset-src-selection-expected.png:
> +        * platform/mac/fast/hidpi/image-srcset-src-selection-expected.txt:

I would have more test coverage. We should try to cover as much as possible. e.g.:
-Various invalid input.
-Valid src, invalid input on the srcset.
-If you have a src (so scaleFactor = 1) and srcset also defines something for scaleFactor = 1, which image is selected.
-Changing the attributes dynamically from JavaScript.
-Removing the src attribute from JavaScript.

> LayoutTests/fast/hidpi/image-srcset-src-selection.html:34
> +    <div>This test passes if the div below is a blue 100px square when the deviceScaleFactor is 1. It simply ensures that the src attribute is taken into account by the selection algorithm when this one is processing the images candidates</div>

Missing period.
Comment 33 Romain Perier 2013-07-18 03:57:21 PDT
Created attachment 206979 [details]
Patch
Comment 34 Romain Perier 2013-07-18 04:00:25 PDT
(In reply to comment #32)
> (From update of attachment 206861 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=206861&action=review
> 
> Great work! That is quite something for a first patch :)
> Sorry, I did not get the time to do a proper review today. Some quick comments bellow.
> 
> I think we should also add a Feature Flag (http://trac.webkit.org/wiki/AddingFeatures) if we want to mature this feature.

I will look at it

> I would have more test coverage. We should try to cover as much as possible. e.g.:
> -Various invalid input.
> -Valid src, invalid input on the srcset.
> -If you have a src (so scaleFactor = 1) and srcset also defines something for scaleFactor = 1, which image is selected.
> -Changing the attributes dynamically from JavaScript.
> -Removing the src attribute from JavaScript.
> 
> > LayoutTests/fast/hidpi/image-srcset-src-selection.html:34
> > +    <div>This test passes if the div below is a blue 100px square when the deviceScaleFactor is 1. It simply ensures that the src attribute is taken into account by the selection algorithm when this one is processing the images candidates</div>
> 
> Missing period.

I agree, working on it



Except these two points above, all others issues are fixed.
Comment 35 Build Bot 2013-07-18 11:51:12 PDT
Comment on attachment 206979 [details]
Patch

Attachment 206979 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1100521

New failing tests:
http/tests/misc/acid2-pixel.html
http/tests/misc/acid2.html
http/tests/inspector/inspect-element.html
canvas/philip/tests/toDataURL.png.complexcolours.html
fast/css/acid2-pixel.html
canvas/philip/tests/toDataURL.jpeg.quality.basic.html
editing/pasteboard/copy-image-with-alt-text.html
fast/css/acid2.html
canvas/philip/tests/toDataURL.jpeg.primarycolours.html
fast/images/object-data-url-case-insensitivity.html
fast/canvas/webgl/oes-texture-half-float-not-supported.html
fast/inline/inline-position-top-align.html
compositing/overflow/image-load-overflow-scrollbars.html
fast/canvas/canvas-composite-image.html
canvas/philip/tests/security.dataURI.html
fast/forms/basic-buttons.html
fast/canvas/webgl/premultiplyalpha-test.html
http/tests/inspector/network/image-as-text-loading-data-url.html
fast/canvas/webgl/tex-image-and-sub-image-2d-with-image-rgb565.html
fast/canvas/canvas-scale-drawImage-shadow.html
canvas/philip/tests/toDataURL.jpeg.alpha.html
fast/canvas/canvas-drawImage-shadow.html
fast/canvas/canvas-image-shadow.html
fast/canvas/image-pattern-rotate.html
fast/canvas/canvas-composite-canvas.html
Comment 36 Build Bot 2013-07-18 11:51:16 PDT
Created attachment 207007 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3
Comment 37 Build Bot 2013-07-18 15:10:07 PDT
Comment on attachment 206979 [details]
Patch

Attachment 206979 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1091000

New failing tests:
http/tests/misc/acid2-pixel.html
http/tests/misc/acid2.html
fast/css/acid2-pixel.html
http/tests/inspector/inspect-element.html
canvas/philip/tests/toDataURL.png.complexcolours.html
fast/images/percent-height-image.html
fast/overflow/image-selection-highlight.html
canvas/philip/tests/toDataURL.jpeg.quality.basic.html
fast/images/object-data-url-case-insensitivity.html
fast/css/acid2.html
canvas/philip/tests/toDataURL.jpeg.primarycolours.html
fast/canvas/image-pattern-rotate.html
fast/canvas/webgl/oes-texture-half-float-not-supported.html
fast/inline/inline-position-top-align.html
compositing/overflow/image-load-overflow-scrollbars.html
fast/canvas/canvas-composite-image.html
canvas/philip/tests/security.dataURI.html
fast/forms/basic-buttons.html
fast/canvas/webgl/premultiplyalpha-test.html
http/tests/inspector/network/image-as-text-loading-data-url.html
fast/canvas/webgl/tex-image-and-sub-image-2d-with-image-rgb565.html
fast/canvas/canvas-scale-drawImage-shadow.html
canvas/philip/tests/toDataURL.jpeg.alpha.html
fast/loader/javascript-url-in-object.html
fast/canvas/canvas-drawImage-shadow.html
fast/canvas/canvas-image-shadow.html
fast/canvas/webgl/tex-image-and-sub-image-2d-with-image-rgba4444.html
fast/canvas/canvas-composite-canvas.html
Comment 38 Build Bot 2013-07-18 15:10:12 PDT
Created attachment 207027 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.3
Comment 39 Romain Perier 2013-07-24 06:41:13 PDT
Does it make sense for you to test that images exist ? I mean , we cannot apply the selection algorithm on a set of images if one of them does not exist. It does not make sense, imho. However nothing is specified in the spec about this part.
Comment 40 Romain Perier 2013-07-24 06:42:57 PDT
We could either test the existance when parsing or when choosing the good candidate (I prefer the first case). What do you think ?
Comment 41 Theresa O'Connor 2013-07-24 09:55:52 PDT
<img src> doesn't test for existence; you simply get a broken image if the image isn't there. <img srcset> is specced to behave in the same way. Please don't make up different behavior.
Comment 42 Romain Perier 2013-07-24 10:51:46 PDT
(In reply to comment #41)
> <img src> doesn't test for existence; you simply get a broken image if the image isn't there. <img srcset> is specced to behave in the same way. Please don't make up different behavior.

Ok, it's noted. thanks
Comment 43 Romain Perier 2013-07-26 01:44:43 PDT
Created attachment 207516 [details]
Patch
Comment 44 Romain Perier 2013-07-26 03:46:04 PDT
See in attachment a new patch with several unit tests. Please review it again as I did some improvements.
Comment 45 Build Bot 2013-07-26 09:10:32 PDT
Comment on attachment 207516 [details]
Patch

Attachment 207516 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1241267

New failing tests:
fast/canvas/webgl/premultiplyalpha-test.html
http/tests/inspector/network/image-as-text-loading-data-url.html
fast/canvas/image-pattern-rotate.html
fast/canvas/webgl/oes-texture-half-float-not-supported.html
fast/canvas/canvas-scale-drawImage-shadow.html
fast/css/acid2-pixel.html
canvas/philip/tests/toDataURL.jpeg.alpha.html
fast/canvas/canvas-drawImage-shadow.html
http/tests/inspector/inspect-element.html
compositing/overflow/image-load-overflow-scrollbars.html
fast/canvas/canvas-composite-image.html
canvas/philip/tests/security.dataURI.html
fast/canvas/canvas-composite-canvas.html
fast/canvas/canvas-image-shadow.html
fast/css/acid2.html
Comment 46 Build Bot 2013-07-26 09:10:36 PDT
Created attachment 207531 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.3
Comment 47 Yoav Weiss 2013-07-26 09:24:07 PDT
AFAICT, this patch doesn't add srcset support to the PreloadScanner, most probably leading to double download in cases where a blocking script is present at the page's head.
Would it be best to add PreloadScanner support as a patch on this bug, or on a separate bug?
Comment 48 Romain Perier 2013-07-26 13:15:28 PDT
AFAIK, the spec does not contain something about that, does not it? The path just 
Implements the standard with scale factors. others improvements might come later
Comment 49 Theresa O'Connor 2013-07-26 13:31:08 PDT
Romain,

The purpose of the feature is to avoid double asset loads. Yoav is right; without also teaching the preload scanner about srcset="", we won't avoid the double asset load in many common cases.
Comment 50 Romain Perier 2013-07-26 14:14:49 PDT
Ok,  it will be implemented in 2 weeks, I am on vacations now.
Comment 51 Build Bot 2013-07-28 12:17:33 PDT
Comment on attachment 207516 [details]
Patch

Attachment 207516 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1249779

New failing tests:
http/tests/inspector/network/image-as-text-loading-data-url.html
compositing/overflow/image-load-overflow-scrollbars.html
http/tests/misc/acid2-pixel.html
http/tests/misc/acid2.html
fast/css/acid2-pixel.html
canvas/philip/tests/toDataURL.jpeg.alpha.html
fast/canvas/canvas-drawImage-shadow.html
http/tests/inspector/inspect-element.html
editing/pasteboard/copy-image-with-alt-text.html
fast/canvas/canvas-composite-image.html
canvas/philip/tests/security.dataURI.html
fast/canvas/canvas-composite-canvas.html
fast/canvas/canvas-image-shadow.html
fast/css/acid2.html
Comment 52 Build Bot 2013-07-28 12:17:38 PDT
Created attachment 207611 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4
Comment 53 Dean Jackson 2013-07-31 17:26:47 PDT
I disagree. I think any updates to the preloader should be done in a separate patch. I've filed 
https://bugs.webkit.org/show_bug.cgi?id=119360
to cover the changes (and will upload a patch in a moment).

Meanwhile this patch breaks src="data:.." urls, as demonstrated by the test results above. I'm trying to fix this.
Comment 54 Dean Jackson 2013-07-31 19:27:53 PDT
(In reply to comment #53)

> Meanwhile this patch breaks src="data:.." urls, as demonstrated by the test results above. I'm trying to fix this.

This is due to a minor flaw in the algorithm, and will require a specification update. I now suggest we land this patch as is (great work Romain) and then some followup patches:

- the attribute splitting needs to take into account more than just spaces. The values might be separated by tabs or newlines.
- in order not to break data: urls that may (often!) have embedded COMMA characters, change the algorithm to require at least one space in the run of characters that make up an image candidate string. The spec requires at least one of the candidate selectors to be present, so there must be a space.
- update the HTMLPreloadScanner to handle srcset

Ben mentioned he also has some optimisations in mind.

Once I have patches ready for the above I'll formally review this patch and land it, then followup. I think it is mandatory that we don't land just this patch because all images with data: src attributes will break.
Comment 55 Romain Perier 2013-08-01 10:35:53 PDT
Nice to find a solution , feel free to contact me if you want some help for the future
Improvements.
Comment 56 Dean Jackson 2013-08-01 17:02:14 PDT
Committed r153624: <http://trac.webkit.org/changeset/153624>
Comment 57 Dean Jackson 2013-08-01 17:03:23 PDT
Congratulations Romain! The most obvious improvement would be to update the parser so that it doesn't create tokens but rather steps through the string building the candidate list.
Comment 58 Romain Perier 2013-08-02 10:11:08 PDT
Cool thanks for your review :)  . I will get back to you or with benjamin to get
more details on the feature next week (still on vacations).
Comment 59 Csaba Osztrogonác 2013-11-05 09:03:28 PST
Comment on attachment 207516 [details]
Patch

Cleared review? from attachment 207516 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).
Comment 60 Romain Perier 2013-11-05 09:06:50 PST
This patch is already committed since a while now, I don't understand your request...