Changeset 153627 in webkit


Ignore:
Timestamp:
Aug 1, 2013 5:02:25 PM (11 years ago)
Author:
dino@apple.com
Message:

srcset algorithm breaks base64 src attributes
​https://bugs.webkit.org/show_bug.cgi?id=119413

Reviewed by Darin Adler.

Source/WebCore:

Base64 encoded src attributes typically have a COMMA
character which was breaking in the candidate matching
algorithm. Make sure to handle that case, and to unescape
any incoming URLs.

Slight cleanup of the srcset matching algorithm. The
candidates are now gathered from a single update
method. I've renamed the methods in the process.
This means we now reparse the srcset attribute if
only the src changes, but I think the code is
cleaner this way.

Tests: fast/hidpi/image-srcset-data-src.html

fast/hidpi/image-srcset-data-srcset.html
fast/hidpi/image-srcset-nomodifier.html
fast/hidpi/image-srcset-viewport-modifiers.html

  • html/HTMLImageElement.cpp:

(WebCore::HTMLImageElement::HTMLImageElement): No need to initialise m_bestFitImageURL.
(WebCore::HTMLImageElement::imageSourceURL): Use isEmpty() rather than checking for nullAtom.
(WebCore::HTMLImageElement::determineBestImageForScaleFactor): New renamed method that selects the best
candidate for the image source.
(WebCore::HTMLImageElement::collectImageCandidatesFromSrcSet): Gather the srcset images. Changes include
simplifying the whitespace and skipping candidates that we don't yet support.
(WebCore::HTMLImageElement::collectImageCandidateFromSrc): Add the src attribute to the list of candidates.
(WebCore::HTMLImageElement::parseAttribute): Now both attributes call determineBestImageForScaleFactor.

  • html/HTMLImageElement.h: No need for m_srcImageIndex any more.

LayoutTests:

Four new tests that exercise candidate matching. In particular:

  • base64 encoded src attributes
  • base64 encoded srcset attributes that are escaped
  • attributes without scale modifiers
  • attributes that have modifiers other than scale
  • fast/hidpi/image-srcset-change-dynamically-from-js.html: Minor change to add scale modifier.
  • fast/hidpi/image-srcset-data-src.html: Added.
  • fast/hidpi/image-srcset-data-srcset.html: Added.
  • fast/hidpi/image-srcset-nomodifier.html: Added.
  • fast/hidpi/image-srcset-viewport-modifiers.html: Added.
  • platform/mac/fast/hidpi/image-srcset-data-src-expected.png: Added.
  • platform/mac/fast/hidpi/image-srcset-data-src-expected.txt: Added.
  • platform/mac/fast/hidpi/image-srcset-data-srcset-expected.png: Added.
  • platform/mac/fast/hidpi/image-srcset-data-srcset-expected.txt: Added.
  • platform/mac/fast/hidpi/image-srcset-nomodifier-expected.png: Added.
  • platform/mac/fast/hidpi/image-srcset-nomodifier-expected.txt: Added.
  • platform/mac/fast/hidpi/image-srcset-viewport-modifiers-expected.png: Added.
  • platform/mac/fast/hidpi/image-srcset-viewport-modifiers-expected.txt: Added.
Location:
trunk
Files:
12 added
5 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r153625 r153627  
     12013-08-01  Dean Jackson  <dino@apple.com>
     2
     3        srcset algorithm breaks base64 src attributes
     4        https://bugs.webkit.org/show_bug.cgi?id=119413
     5
     6        Reviewed by Darin Adler.
     7
     8        Four new tests that exercise candidate matching. In particular:
     9        - base64 encoded src attributes
     10        - base64 encoded srcset attributes that are escaped
     11        - attributes without scale modifiers
     12        - attributes that have modifiers other than scale
     13
     14        * fast/hidpi/image-srcset-change-dynamically-from-js.html: Minor change to add scale modifier.
     15        * fast/hidpi/image-srcset-data-src.html: Added.
     16        * fast/hidpi/image-srcset-data-srcset.html: Added.
     17        * fast/hidpi/image-srcset-nomodifier.html: Added.
     18        * fast/hidpi/image-srcset-viewport-modifiers.html: Added.
     19        * platform/mac/fast/hidpi/image-srcset-data-src-expected.png: Added.
     20        * platform/mac/fast/hidpi/image-srcset-data-src-expected.txt: Added.
     21        * platform/mac/fast/hidpi/image-srcset-data-srcset-expected.png: Added.
     22        * platform/mac/fast/hidpi/image-srcset-data-srcset-expected.txt: Added.
     23        * platform/mac/fast/hidpi/image-srcset-nomodifier-expected.png: Added.
     24        * platform/mac/fast/hidpi/image-srcset-nomodifier-expected.txt: Added.
     25        * platform/mac/fast/hidpi/image-srcset-viewport-modifiers-expected.png: Added.
     26        * platform/mac/fast/hidpi/image-srcset-viewport-modifiers-expected.txt: Added.
     27
    1282013-08-01  Dean Jackson  <dino@apple.com>
    229

  • trunk/LayoutTests/fast/hidpi/image-srcset-change-dynamically-from-js.html

    r153625 r153627  
    55        var img = document.getElementById("foo");
    66        img.src = "resources/blue-100-px-square.png"
    7         img.srcset = "resources/green-200-px-square.png";
     7        img.srcset = "resources/green-200-px-square.png 1x";
    88    }
    99

  • trunk/Source/WebCore/ChangeLog

    r153624 r153627  
     12013-08-01  Dean Jackson  <dino@apple.com>
     2
     3        srcset algorithm breaks base64 src attributes
     4        https://bugs.webkit.org/show_bug.cgi?id=119413
     5
     6        Reviewed by Darin Adler.
     7
     8        Base64 encoded src attributes typically have a COMMA
     9        character which was breaking in the candidate matching
     10        algorithm. Make sure to handle that case, and to unescape
     11        any incoming URLs.
     12
     13        Slight cleanup of the srcset matching algorithm. The
     14        candidates are now gathered from a single update
     15        method. I've renamed the methods in the process.
     16        This means we now reparse the srcset attribute if
     17        only the src changes, but I think the code is
     18        cleaner this way.
     19
     20        Tests: fast/hidpi/image-srcset-data-src.html
     21               fast/hidpi/image-srcset-data-srcset.html
     22               fast/hidpi/image-srcset-nomodifier.html
     23               fast/hidpi/image-srcset-viewport-modifiers.html
     24
     25        * html/HTMLImageElement.cpp:
     26        (WebCore::HTMLImageElement::HTMLImageElement): No need to initialise m_bestFitImageURL.
     27        (WebCore::HTMLImageElement::imageSourceURL): Use isEmpty() rather than checking for nullAtom.
     28        (WebCore::HTMLImageElement::determineBestImageForScaleFactor): New renamed method that selects the best
     29        candidate for the image source.
     30        (WebCore::HTMLImageElement::collectImageCandidatesFromSrcSet): Gather the srcset images. Changes include
     31        simplifying the whitespace and skipping candidates that we don't yet support.
     32        (WebCore::HTMLImageElement::collectImageCandidateFromSrc): Add the src attribute to the list of candidates.
     33        (WebCore::HTMLImageElement::parseAttribute): Now both attributes call determineBestImageForScaleFactor.
     34        * html/HTMLImageElement.h: No need for m_srcImageIndex any more.
     35
    1362013-08-01  Romain Perier  <romain.perier@gmail.com>
    237

  • trunk/Source/WebCore/html/HTMLImageElement.cpp

    r153624 r153627  
    5050    , m_form(form)
    5151    , m_compositeOperator(CompositeSourceOver)
    52     , m_bestFitImageURL(nullAtom)
    53     , m_srcImageIndex(notFound)
    5452{
    5553    ASSERT(hasTagName(imgTag));
    … …  
    115113const AtomicString& HTMLImageElement::imageSourceURL() const
    116114{
    117     return m_bestFitImageURL == nullAtom ? getAttribute(srcAttr) : m_bestFitImageURL;
    118 }
    119 
    120 void HTMLImageElement::updateBestImageForScaleFactor()
    121 {
     115    return m_bestFitImageURL.isEmpty() ? fastGetAttribute(srcAttr) : m_bestFitImageURL;
     116}
     117
     118void HTMLImageElement::determineBestImageForScaleFactor()
     119{
     120    m_imagesWithScale.clear();
     121    m_bestFitImageURL = nullAtom;
     122
     123    collectImageCandidatesFromSrcSet();
     124    collectImageCandidateFromSrc();
     125    stable_sort(m_imagesWithScale.begin(), m_imagesWithScale.end(), compareByScaleFactor);
     126
     127    for (size_t i = 1; i < m_imagesWithScale.size(); ++i) {
     128        if (m_imagesWithScale[i-1].scaleFactor == m_imagesWithScale[i].scaleFactor) {
     129            m_imagesWithScale.remove(i);
     130            i--;
     131        }
     132    }
     133
    122134    float pageScaleFactor = 1.0;
    123 
    124     m_bestFitImageURL = nullAtom;
    125135    if (Page* page = document()->page())
    126136        pageScaleFactor = page->deviceScaleFactor();
     137
    127138    for (size_t i = 0; i < m_imagesWithScale.size(); ++i) {
    128139        if (m_imagesWithScale[i].scaleFactor >= pageScaleFactor) {
    … …  
    133144}
    134145
    135 void HTMLImageElement::updateImagesFromSrcSet(const AtomicString& srcset)
    136 {
    137     const String& string = static_cast<const String&>(srcset);
    138     Vector<String> srcSet;
    139 
    140     string.split(',', srcSet);
    141     for (size_t i = 0; i < srcSet.size(); ++i) {
     146void HTMLImageElement::collectImageCandidatesFromSrcSet()
     147{
     148    const String& srcSetAttributeValue = fastGetAttribute(srcsetAttr).string().simplifyWhiteSpace(isHTMLSpace);
     149    Vector<String> srcSetTokens;
     150
     151    srcSetAttributeValue.split(',', srcSetTokens);
     152    for (size_t i = 0; i < srcSetTokens.size(); ++i) {
    142153        Vector<String> data;
    143154        float imgScaleFactor = 1.0;
    144155        bool validScaleFactor = false;
    145156
    146         srcSet[i].stripWhiteSpace().split(' ', data);
    147         if (data.size() > 0 && data.last().endsWith('x')) {
    148             imgScaleFactor = data.last().substring(0, data.last().length() - 1).toFloat(&validScaleFactor);
    149             if (!validScaleFactor)
    150                 imgScaleFactor = 1.0;
    151         }
    152         if (!data.size() || (data.size() == 1 && validScaleFactor))
     157        srcSetTokens[i].stripWhiteSpace().split(' ', data);
     158        // There must be at least one candidate descriptor, and the last one must
     159        // be a scale factor. Since we don't support descriptors other than scale,
     160        // it's better to discard any rule with such descriptors rather than accept
     161        // only the scale data.
     162        if (data.size() != 2)
    153163            continue;
     164        if (!data.last().endsWith('x'))
     165            continue;
     166
     167        imgScaleFactor = data.last().substring(0, data.last().length() - 1).toFloat(&validScaleFactor);
     168        if (!validScaleFactor)
     169            continue;
     170
    154171        ImageWithScale image;
    155         image.imageURL = data[0];
     172        image.imageURL = decodeURLEscapeSequences(data[0]);
    156173        image.scaleFactor = imgScaleFactor;
     174
    157175        m_imagesWithScale.append(image);
    158176    }
    159     const AtomicString& src = getAttribute(srcAttr);
     177}
     178
     179void HTMLImageElement::collectImageCandidateFromSrc()
     180{
     181    const AtomicString& src = fastGetAttribute(srcAttr);
    160182    ImageWithScale image;
    161183    if (!src.isEmpty()) {
    162         image.imageURL = getAttribute(srcAttr);
     184        image.imageURL = decodeURLEscapeSequences(src);
    163185        image.scaleFactor = 1.0;
    164186        m_imagesWithScale.append(image);
    165187    }
    166     stable_sort(m_imagesWithScale.begin(), m_imagesWithScale.end(), compareByScaleFactor);
    167 
    168     for (size_t i = 1; i < m_imagesWithScale.size(); ++i) {
    169         if (m_imagesWithScale[i-1].scaleFactor == m_imagesWithScale[i].scaleFactor) {
    170             m_imagesWithScale.remove(i);
    171             i--;
    172         }
    173     }
    174     if (!src.isEmpty())
    175         m_srcImageIndex = m_imagesWithScale.find(image);
    176188}
    177189
    … …  
    181193        if (renderer() && renderer()->isImage())
    182194            toRenderImage(renderer())->updateAltText();
    183     } else if (name == srcAttr) {
    184         if (m_srcImageIndex != notFound)
    185             m_imagesWithScale.remove(m_srcImageIndex);
    186         updateImagesFromSrcSet(value);
    187         updateBestImageForScaleFactor();
    188         m_imageLoader.updateFromElementIgnoringPreviousError();
    189     } else if (name == srcsetAttr) {
    190         m_imagesWithScale.clear();
    191         updateImagesFromSrcSet(value);
    192         updateBestImageForScaleFactor();
     195    } else if (name == srcAttr || name == srcsetAttr) {
     196        determineBestImageForScaleFactor();
    193197        m_imageLoader.updateFromElementIgnoringPreviousError();
    194198    }

  • trunk/Source/WebCore/html/HTMLImageElement.h

    r153624 r153627  
    110110#endif
    111111
    112     void updateImagesFromSrcSet(const AtomicString& srcset);
    113 
    114     void updateBestImageForScaleFactor();
     112    void collectImageCandidateFromSrc();
     113    void collectImageCandidatesFromSrcSet();
     114    void determineBestImageForScaleFactor();
    115115
    116116    struct ImageWithScale {
    … …  
    132132    CompositeOperator m_compositeOperator;
    133133    AtomicString m_bestFitImageURL;
    134     size_t m_srcImageIndex;
    135134    Vector<ImageWithScale> m_imagesWithScale;
    136135};
Note: See TracChangeset for help on using the changeset viewer.