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>
Severity: Normal CC: buildbot, commit-queue, esprehn+autocc, gtk-ews, rniwa, xan.lopez, yoav
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Description Flags
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2
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]
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]

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

New failing tests:
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]
Comment 6 Andreas Kling 2013-08-23 09:02:31 PDT
Comment on attachment 209439 [details]

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]
Comment 8 kov's GTK+ EWS bot 2013-08-23 09:49:24 PDT
Comment on attachment 209466 [details]

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]

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]

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]

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?