Bug 120168

Summary: Fix srcset's image candidate algorithm when DPR exceeds all candidates
Product: WebKit Reporter: Yoav Weiss <yoav>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, esprehn+autocc, gtk-ews, rniwa, xan.lopez, yoav
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2
none
Patch
none
Patch none

Description Yoav Weiss 2013-08-22 12:41:07 PDT
Fix srcset's image candidate algorithm when DPR exceeds all candidates
Comment 1 Yoav Weiss 2013-08-22 13:12:57 PDT
Created attachment 209387 [details]
Patch
Comment 2 Yoav Weiss 2013-08-22 13:17:26 PDT
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.
Comment 3 Build Bot 2013-08-22 16:12:26 PDT
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
Comment 4 Build Bot 2013-08-22 16:12:28 PDT
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
Comment 5 Yoav Weiss 2013-08-22 23:55:48 PDT
Created attachment 209439 [details]
Patch
Comment 6 Andreas Kling 2013-08-23 09:02:31 PDT
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.
Comment 7 Yoav Weiss 2013-08-23 09:11:31 PDT
Created attachment 209466 [details]
Patch
Comment 8 kov's GTK+ EWS bot 2013-08-23 09:49:24 PDT
Comment on attachment 209466 [details]
Patch

Attachment 209466 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1545483
Comment 9 Andreas Kling 2013-08-23 10:17:04 PDT
Comment on attachment 209466 [details]
Patch

gtk ews failure is not because of this patch.
Comment 10 WebKit Commit Bot 2013-08-23 10:40:22 PDT
Comment on attachment 209466 [details]
Patch

Clearing flags on attachment: 209466

Committed r154497: <http://trac.webkit.org/changeset/154497>
Comment 11 WebKit Commit Bot 2013-08-23 10:40:25 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Darin Adler 2013-08-23 12:00:56 PDT
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.
Comment 13 Yoav Weiss 2013-08-23 12:36:56 PDT
True. Should I upload that on this bug or open a new one?