Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(2590)

Issue 667763004: Srcset resource selection use a geometric mean to determine resource. (Closed)

Created:
6 years, 2 months ago by Yoav Weiss
Modified:
6 years, 2 months ago
Reviewers:
Mike West
CC:
blink-reviews, dglazkov+blink, blink-reviews-html_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Project:
blink
Visibility:
Public.

Description

Srcset resource selection use a geometric mean to determine resource. Currently the srcset resource selection simply picks the first candidate with a density that's equal or larger than DPR. That results in cases where slight zooming causes a DPR of 1.1 and the download of a 2x resource, even though the 1x resource would have been enough. This CL fixes that, by making sure that a higher resolution resource is downloaded only if the DPR is larger than the geometric mean of that resource's density and the density of the resource that preceded it in the sorted candidate list. The srcset resource selection is intentionally defined as UA specific in the spec: https://html.spec.whatwg.org/multipage/embedded-content.html#the-img-element:image-source-2 That is in order to allow browsers to play around with resource selection logic heuristics. Geometric mean was proposed by Tab Atkins as a "decent first draft": http://ircbot.responsiveimages.org/bot/log/respimg/2014-04-22#T66515 BUG=425511 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184187

Patch Set 1 #

Patch Set 2 : Fixed a broken test #

Patch Set 3 : Added test cases #

Total comments: 3

Patch Set 4 : Review nits #

Unified diffs Side-by-side diffs Delta from patch set Stats (+46 lines, -13 lines) Patch
M LayoutTests/fast/dom/HTMLImageElement/image-sizes-1x.html View 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/fast/dom/HTMLImageElement/image-srcset-react-to-media-changes.html View 1 1 chunk +1 line, -1 line 0 comments Download
M Source/core/html/parser/HTMLSrcsetParser.cpp View 1 2 3 2 chunks +35 lines, -9 lines 0 comments Download
M Source/core/html/parser/HTMLSrcsetParserTest.cpp View 1 2 3 chunks +9 lines, -2 lines 0 comments Download

Messages

Total messages: 9 (2 generated)
Yoav Weiss
6 years, 2 months ago (2014-10-22 12:36:21 UTC) #2
Mike West
Is this specced anywhere? It looks pretty reasonable.
6 years, 2 months ago (2014-10-22 13:13:47 UTC) #3
Yoav Weiss
On 2014/10/22 13:13:47, Mike West wrote: > Is this specced anywhere? It looks pretty reasonable. ...
6 years, 2 months ago (2014-10-22 13:26:33 UTC) #4
Mike West
Ok. LGTM % nits. Perhaps you could link to the conversation in the CL description ...
6 years, 2 months ago (2014-10-22 13:41:40 UTC) #5
Yoav Weiss
On 2014/10/22 13:41:40, Mike West wrote: > Ok. LGTM % nits. > > Perhaps you ...
6 years, 2 months ago (2014-10-22 13:54:31 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/667763004/60001
6 years, 2 months ago (2014-10-22 14:04:17 UTC) #8
commit-bot: I haz the power
6 years, 2 months ago (2014-10-22 14:56:17 UTC) #9
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as 184187

Powered by Google App Engine
This is Rietveld 408576698