Bug 110252 – Implement img element's srcset attribute

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Theresa O'Connor]

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>.)

[Radar WebKit Bug Importer]

<rdar://problem/13246364>

[Vineet Chaudhary (vineetc)]

Created attachment 189615 [details]
wip_patch

[Vineet Chaudhary (vineetc)]

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.

[Vineet Chaudhary (vineetc)]

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().

[Vineet Chaudhary (vineetc)]

Created attachment 190345 [details]
updated_patch

[Maciej Stachowiak]

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?

[Maciej Stachowiak]

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.

[Tim Horton]

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).

[Ryosuke Niwa]

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?

[Beth Dakin]

Vineet, are you planning to continue working on this?

[Beth Dakin]

Un-assigning from Vineet due to inactivity.

[Theresa O'Connor]

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.

[Dean Jackson]

(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

[Romain Perier]

Created attachment 206753 [details]
Patch

[Romain Perier]

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 :)

[Early Warning System Bot]

Comment on attachment 206753 [details]
Patch

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

[Early Warning System Bot]

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

[EFL EWS Bot]

Comment on attachment 206753 [details]
Patch

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

[EFL EWS Bot]

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

[Chris Dumez]

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.

[Build Bot]

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

[Build Bot]

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

[Romain Perier]

Created attachment 206799 [details]
Patch

[Romain Perier]

(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

[EFL EWS Bot]

Comment on attachment 206799 [details]
Patch

Attachment 206799 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1082259

[Early Warning System Bot]

Comment on attachment 206799 [details]
Patch

Attachment 206799 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1084276

[Early Warning System Bot]

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

[EFL EWS Bot]

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

[Romain Perier]

Created attachment 206861 [details]
Patch

[Early Warning System Bot]

Comment on attachment 206861 [details]
Patch

Attachment 206861 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1099104

[Early Warning System Bot]

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

[Benjamin Poulain]

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.

[Romain Perier]

Created attachment 206979 [details]
Patch

[Romain Perier]

(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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Romain Perier]

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.

[Romain Perier]

We could either test the existance when parsing or when choosing the good candidate (I prefer the first case). What do you think ?

[Theresa O'Connor]

<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.

[Romain Perier]

(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

[Romain Perier]

Created attachment 207516 [details]
Patch

[Romain Perier]

See in attachment a new patch with several unit tests. Please review it again as I did some improvements.

[Build Bot]

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

[Build Bot]

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

[Yoav Weiss]

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?

[Romain Perier]

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

[Theresa O'Connor]

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.

[Romain Perier]

Ok,  it will be implemented in 2 weeks, I am on vacations now.

[Build Bot]

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

[Build Bot]

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

[Dean Jackson]

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.

[Dean Jackson]

(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.

[Romain Perier]

Nice to find a solution , feel free to contact me if you want some help for the future
Improvements.

[Dean Jackson]

Committed r153624: <http://trac.webkit.org/changeset/153624>

[Dean Jackson]

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.

[Romain Perier]

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).

[Csaba Osztrogonác]

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).

[Romain Perier]

This patch is already committed since a while now, I don't understand your request...