Bug 120168 – Fix srcset's image candidate algorithm when DPR exceeds all candidates

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yoav Weiss]

Fix srcset's image candidate algorithm when DPR exceeds all candidates

[Yoav Weiss]

Created attachment 209387 [details]
Patch

[Yoav Weiss]

When the DPR exceeded the 'x' qualifier of all image candidates, none was chosen.

According to the srcset spec: "If there are any entries in candidates that have an associated pixel density that is less than a user-agent-defined value giving the nominal pixel density of the display, then remove them, unless that would remove all the entries, in which case remove only the entries whose associated pixel density is less than the greatest such pixel density."

Fixed by returning the last candidate in the list of candidates sorted by their qualifier, in case none of them is equal or greater than the DPR.

[Build Bot]

Comment on attachment 209387 [details]
Patch

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

New failing tests:
fast/hidpi/image-srcset-fraction.html

[Build Bot]

Created attachment 209402 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4

[Yoav Weiss]

Created attachment 209439 [details]
Patch

[Andreas Kling]

Comment on attachment 209439 [details]
Patch

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

Looks good!
r=me with a minor style change:

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:370
> +    return imageCandidates[i-1].imageURL;

This would look slightly better as:
    return imageCandidates.last().imageURL;

Then we wouldn't have to keep the loop variable alive.

[Yoav Weiss]

Created attachment 209466 [details]
Patch

[kov's GTK+ EWS bot]

Comment on attachment 209466 [details]
Patch

Attachment 209466 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1545483

[Andreas Kling]

Comment on attachment 209466 [details]
Patch

gtk ews failure is not because of this patch.

[WebKit Commit Bot]

Comment on attachment 209466 [details]
Patch

Clearing flags on attachment: 209466

Committed r154497: <http://trac.webkit.org/changeset/154497>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 209466 [details]
Patch

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

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:369
>      for (size_t i = 0; i < imageCandidates.size(); ++i) {
>          if (imageCandidates[i].scaleFactor >= deviceScaleFactor)
>              return imageCandidates[i].imageURL;
>      }
> -    return String();
> +    return imageCandidates.last().imageURL;

Could also fix the loop to not waste any time looking at the scale factor for the last candidate by only going up to size - 1.

[Yoav Weiss]

True. Should I upload that on this bug or open a new one?