Srcset resource selection use a geometric mean to determine resource.

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

[Yoav Weiss, 2014-10-22 12:34:46]

yoav@yoav.ws changed reviewers:
+ mkwst@chromium.org

[Mike West, 2014-10-22 13:13:47]

Is this specced anywhere? It looks pretty reasonable.

[Yoav Weiss, 2014-10-22 13:26:33]

On 2014/10/22 13:13:47, Mike West wrote:
> Is this specced anywhere? It looks pretty reasonable.

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:...

That is in order to allow browsers to play around with such heuristics and
figure out the best trade-off between quality & download size and in the future
combine user preferences and bandwidth considerations into that resource
selection logic.

Geometric mean was proposed by Tab Atkins as a "decent first draft":
http://ircbot.responsiveimages.org/bot/log/respimg/2014-04-22#T66515

[Mike West, 2014-10-22 13:41:40]

Ok. LGTM % nits.

Perhaps you could link to the conversation in the CL description so the
backstory is clear.

https://codereview.chromium.org/667763004/diff/40001/Source/core/html/parser/...
File Source/core/html/parser/HTMLSrcsetParser.cpp (right):

https://codereview.chromium.org/667763004/diff/40001/Source/core/html/parser/...
Source/core/html/parser/HTMLSrcsetParser.cpp:329: float currentDensity;
Why do you define `next`, `nextDensity`, and `currentDensity` outside the loop?

https://codereview.chromium.org/667763004/diff/40001/Source/core/html/parser/...
Source/core/html/parser/HTMLSrcsetParser.cpp:338: ++next;
Should this be a loop? Or do you really only want to skip one?

https://codereview.chromium.org/667763004/diff/40001/Source/core/html/parser/...
Source/core/html/parser/HTMLSrcsetParser.cpp:378: float winningDensity =
imageCandidates[i].density();
Perhaps ASSERT that `i` ended up in a valid range?

[Yoav Weiss, 2014-10-22 13:54:31]

On 2014/10/22 13:41:40, Mike West wrote:
> Ok. LGTM % nits.
> 
> Perhaps you could link to the conversation in the CL description so the
> backstory is clear.

Done.

> 
>
https://codereview.chromium.org/667763004/diff/40001/Source/core/html/parser/...
> File Source/core/html/parser/HTMLSrcsetParser.cpp (right):
> 
>
https://codereview.chromium.org/667763004/diff/40001/Source/core/html/parser/...
> Source/core/html/parser/HTMLSrcsetParser.cpp:329: float currentDensity;
> Why do you define `next`, `nextDensity`, and `currentDensity` outside the
loop?

No good reason. Moved them into the loop.

> 
>
https://codereview.chromium.org/667763004/diff/40001/Source/core/html/parser/...
> Source/core/html/parser/HTMLSrcsetParser.cpp:338: ++next;
> Should this be a loop? Or do you really only want to skip one?

I really want to skip one, since src can only add a single candidate. Added an
assert that'd make it clear that this is my intention.

> 
>
https://codereview.chromium.org/667763004/diff/40001/Source/core/html/parser/...
> Source/core/html/parser/HTMLSrcsetParser.cpp:378: float winningDensity =
> imageCandidates[i].density();
> Perhaps ASSERT that `i` ended up in a valid range?

Good call. Added one.

[Yoav Weiss, 2014-10-22 14:03:06]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2014-10-22 14:04:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/667763004/60001

[commit-bot: I haz the power, 2014-10-22 14:56:17]

Committed patchset #4 (id:60001) as 184187