Support the DPR header for client hints.

Support the DPR header for client hints.

As specified in
https://github.com/igrigorik/http-client-hints/blob/master/draft-grigorik-http-client-hints-01.txt#L255

Similar to a DPR specification in srcset, the DPR header should cause us
to divide the intrinsic height by the dpr.

BUG=303863
R=pdr@chromium.org
CC=yoav@yoav.ws

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=160843

[pdr., 2013-10-26 01:26:30]

This is great work.

https://codereview.chromium.org/46313002/diff/1/LayoutTests/http/tests/images...
File LayoutTests/http/tests/images/image-with-dpr-header-expected.html (right):

https://codereview.chromium.org/46313002/diff/1/LayoutTests/http/tests/images...
LayoutTests/http/tests/images/image-with-dpr-header-expected.html:3: <img
src="http://127.0.0.1:8000/resources/square100.png" height="25" width="25">
Do you mind adding an additional test or two for caches and revalidation (see:
LayoutTests/http/tests/cache)?

For example:
1) Load cached.png with ch-dpr=2, cache it with dpr=2
2) Navigate
3) Re-load cached.png which will send a revalidation request to the server
4) If the server responds with a 304, is the dpr header preserved?

Another example:
1) Load cached.png with ch-dpr=2, cache it with dpr=2
2) Navigate
3) Re-load cached.png which will send a revalidation request to the server
4) If the server responds with a 304 but a different dpr, what happens?

https://codereview.chromium.org/46313002/diff/1/LayoutTests/http/tests/resour...
File LayoutTests/http/tests/resources/dpr.php (right):

https://codereview.chromium.org/46313002/diff/1/LayoutTests/http/tests/resour...
LayoutTests/http/tests/resources/dpr.php:12: header("DPR: ". $dpr);
Should the header name be "DPR" or "CH-DPR"?

I think the spec means to actually use "CH-DPR" ("3.1.  The Client Hints Request
Header Field".)

https://codereview.chromium.org/46313002/diff/1/Source/core/fetch/ImageResour...
File Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/46313002/diff/1/Source/core/fetch/ImageResour...
Source/core/fetch/ImageResource.cpp:26: #include "RuntimeEnabledFeatures.h"
Nit: Can you put this include one line lower?
...
#include "core/fetch/ImageResource.h"

#include "RuntimeEnabledFeatures.h"
#include "core/fetch/ImageResourceClient.h"
...

https://codereview.chromium.org/46313002/diff/1/Source/core/fetch/ImageResour...
File Source/core/fetch/ImageResource.h (right):

https://codereview.chromium.org/46313002/diff/1/Source/core/fetch/ImageResour...
Source/core/fetch/ImageResource.h:67: // The device pixel resolution we got from
the server for this image, or 1.0
Nit: change this to:
// The device pixel ratio we got from the server for this image, or 1.0.

https://codereview.chromium.org/46313002/diff/1/Source/core/fetch/ImageResour...
Source/core/fetch/ImageResource.h:68: float dprHeaderValue() const { return
m_dprHeaderValue; }
Bikeshed: Do you mind spelling dpr out? For example,
"devicePixelRatioHeaderValue()", "m_devicePixelRatioHeaderValue", etc.

I think spelling it out will make this a little easier to understand for
dpr-noobs, and it's consistent with how we named things for srcset.

https://codereview.chromium.org/46313002/diff/1/Source/core/page/RuntimeEnabl...
File Source/core/page/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/46313002/diff/1/Source/core/page/RuntimeEnabl...
Source/core/page/RuntimeEnabledFeatures.in:21: ClientHintsDpr
status=experimental
Do you mind sending out an intent to implement email to blink-dev?

[Yoav Weiss, 2013-10-26 12:41:24]

Awesome!

Regarding the choice to use the server's DPR header to override the client's
previous imageDevicePixelRatio(), I'd be more comfortable it it was made more
explicit. (e.g. by adding a comment to the relevant code and test).

Currently the priority of which DPR value takes precedence is not specified
anywhere (I believe it should be, but I'll open an issue on the CH spec for
that), so the decision is left to the UA. I think we should make that decision
explicit.

[Yoav Weiss, 2013-10-27 20:18:50]

On 2013/10/26 12:41:24, Yoav Weiss wrote:
> Awesome!
> 
> Regarding the choice to use the server's DPR header to override the client's
> previous imageDevicePixelRatio(), I'd be more comfortable it it was made more
> explicit. (e.g. by adding a comment to the relevant code and test).
> 
> Currently the priority of which DPR value takes precedence is not specified
> anywhere (I believe it should be, but I'll open an issue on the CH spec for
> that), so the decision is left to the UA. I think we should make that decision
> explicit.

The spec was modified to make the server's DPR value precedence mandatory (
https://github.com/igrigorik/http-client-hints/blob/master/draft-grigorik-htt...
), so my comment is no longer relevant.

[pdr, 2013-10-27 21:04:38]

https://codereview.chromium.org/46313002/diff/1/LayoutTests/http/tests/resour...
File LayoutTests/http/tests/resources/dpr.php (right):

https://codereview.chromium.org/46313002/diff/1/LayoutTests/http/tests/resour...
LayoutTests/http/tests/resources/dpr.php:12: header("DPR: ". $dpr);
On 2013/10/26 01:26:31, pdr wrote:
> Should the header name be "DPR" or "CH-DPR"?
> 
> I think the spec means to actually use "CH-DPR" ("3.1.  The Client Hints
Request
> Header Field".)

For posterity, I was just mixed up here. DPR is the correct value after all.

https://codereview.chromium.org/46313002/diff/1/Source/core/page/RuntimeEnabl...
File Source/core/page/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/46313002/diff/1/Source/core/page/RuntimeEnabl...
Source/core/page/RuntimeEnabledFeatures.in:21: ClientHintsDpr
status=experimental
On 2013/10/26 01:26:31, pdr wrote:
> Do you mind sending out an intent to implement email to blink-dev?

We're good here too:
https://groups.google.com/a/chromium.org/d/topic/blink-dev/c38s7y6dH-Q/discus...

[cbiesinger, 2013-10-29 00:15:57]

All comments addressed, including a comment that the server-sent DPR header
overrides srcset/srcN.

Please take another look!

[pdr., 2013-10-29 02:58:18]

LGTM, but I think the 304 case with different DPR headers still isn't tested.
Can you add that?

[cbiesinger, 2013-10-29 15:10:47]

On 2013/10/29 02:58:18, pdr wrote:
> LGTM, but I think the 304 case with different DPR headers still isn't tested.
> Can you add that?

I couldn't get PHP to send a DPR header (or indeed any custom header) when it
sends a 304 :(

[cbiesinger, 2013-10-29 15:12:29]

On 2013/10/29 15:10:47, cbiesinger wrote:
> On 2013/10/29 02:58:18, pdr wrote:
> > LGTM, but I think the 304 case with different DPR headers still isn't
tested.
> > Can you add that?
> 
> I couldn't get PHP to send a DPR header (or indeed any custom header) when it
> sends a 304 :(

Ah, see the first answer on
http://stackoverflow.com/questions/6491419/apache-ignoring-php-headers-when-s...

[commit-bot: I haz the power, 2013-10-29 15:44:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cbiesinger@chromium.org/46313002/160001

[commit-bot: I haz the power, 2013-10-29 16:44:36]

Change committed as 160843