Added basic client hints support.

Added basic client hints support.

This CL adds basic client hints support for the DPR and RW hints.
It also includes an "Accept-CH" based opt-in mechanism.
It does not include:
* A decent Accept-CH parser
* Actual Resource width being sent as the RW header

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

[Yoav Weiss, 2015-02-24 13:04:05]

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

[Mike West, 2015-02-24 13:05:24]

On 2015/02/24 at 13:04:06, yoav wrote:
> 

I'm not going to have time to look at this until tomorrow. :(  If you need a
review more quickly, please consider one of the other friendly folks floating
around Europe. :)

[Yoav Weiss, 2015-02-24 13:48:43]

On 2015/02/24 13:05:24, Mike West wrote:
> On 2015/02/24 at 13:04:06, yoav wrote:
> > 
> 
> I'm not going to have time to look at this until tomorrow. :(  If you need a
> review more quickly, please consider one of the other friendly folks floating
> around Europe. :)

It can wait till tomorrow :)

[Mike West, 2015-02-25 09:03:14]

Looking good! A few questions.

https://codereview.chromium.org/951303003/diff/1/Source/core/fetch/Resource.cpp
File Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/951303003/diff/1/Source/core/fetch/Resource.c...
Source/core/fetch/Resource.cpp:449: // FIXME: Write an actual parser for this
comma delimited header.
Nit: Can you add a bug ID here?

When you add an actual parser, it would be good to add unit tests as well to
make sure we can easily check the various edge-cases (`dprw`, for instance :) ).

https://codereview.chromium.org/951303003/diff/1/Source/core/fetch/ResourceFe...
File Source/core/fetch/ResourceFetcherTest.cpp (right):

https://codereview.chromium.org/951303003/diff/1/Source/core/fetch/ResourceFe...
Source/core/fetch/ResourceFetcherTest.cpp:283:
expectHeader("http://www.example.com/1.gif", "RW", false, "");
Can you add another test that verifies that you can send RW without sending DPR?

I think it might make sense to test each in isolation just for clarity. It's
good to have a test that verifies that both are sent, but it's not clear (for
example) that you can 'setLayoutSize()' without first calling
'setDeviceScaleFactor()'. For these tests in particular it's kind of a stupid
question, but theoretical purity!

https://codereview.chromium.org/951303003/diff/1/Source/core/frame/LocalFrame.h
File Source/core/frame/LocalFrame.h (right):

https://codereview.chromium.org/951303003/diff/1/Source/core/frame/LocalFrame...
Source/core/frame/LocalFrame.h:237: bool m_shouldSendDPRHint;
How many hints will we end up with? Might it make sense to have a bitmasked flag
instead of lots of booleans?

[Mike West, 2015-02-25 09:06:08]

https://codereview.chromium.org/951303003/diff/1/LayoutTests/http/tests/misc/...
File LayoutTests/http/tests/misc/client-hints-accept.php (right):

https://codereview.chromium.org/951303003/diff/1/LayoutTests/http/tests/misc/...
LayoutTests/http/tests/misc/client-hints-accept.php:9: <img
src="resources/image-checks-for-dpr.php" onload="console.log('DPR sent.')"
onerror="console.log('DPR not sent.')">
Ditto.

https://codereview.chromium.org/951303003/diff/1/LayoutTests/http/tests/misc/...
File LayoutTests/http/tests/misc/client-hints-no-accept.html (right):

https://codereview.chromium.org/951303003/diff/1/LayoutTests/http/tests/misc/...
LayoutTests/http/tests/misc/client-hints-no-accept.html:6: <img
src="resources/image-checks-for-dpr.php" onload="console.log('DPR sent.')"
onerror="console.log('DPR not sent.')">
This is going to flake (and, in fact, did!). Please:

1. Write this as a `testharness.js` test with assertions so that we know what
passing looks like.
2. Chain the requests so that the ordering is always consistent.

[Yoav Weiss, 2015-02-25 09:14:49]

On 2015/02/25 09:03:14, Mike West wrote:
> Looking good! A few questions.
> 
>
https://codereview.chromium.org/951303003/diff/1/Source/core/fetch/Resource.cpp
> File Source/core/fetch/Resource.cpp (right):
> 
>
https://codereview.chromium.org/951303003/diff/1/Source/core/fetch/Resource.c...
> Source/core/fetch/Resource.cpp:449: // FIXME: Write an actual parser for this
> comma delimited header.
> Nit: Can you add a bug ID here?

Sure

> 
> When you add an actual parser, it would be good to add unit tests as well to
> make sure we can easily check the various edge-cases (`dprw`, for instance :)
).

Definitely :)

> 
>
https://codereview.chromium.org/951303003/diff/1/Source/core/fetch/ResourceFe...
> File Source/core/fetch/ResourceFetcherTest.cpp (right):
> 
>
https://codereview.chromium.org/951303003/diff/1/Source/core/fetch/ResourceFe...
> Source/core/fetch/ResourceFetcherTest.cpp:283:
> expectHeader("http://www.example.com/1.gif", "RW", false, "");
> Can you add another test that verifies that you can send RW without sending
DPR?
> 
> I think it might make sense to test each in isolation just for clarity. It's
> good to have a test that verifies that both are sent, but it's not clear (for
> example) that you can 'setLayoutSize()' without first calling
> 'setDeviceScaleFactor()'. For these tests in particular it's kind of a stupid
> question, but theoretical purity!

Sure

> 
>
https://codereview.chromium.org/951303003/diff/1/Source/core/frame/LocalFrame.h
> File Source/core/frame/LocalFrame.h (right):
> 
>
https://codereview.chromium.org/951303003/diff/1/Source/core/frame/LocalFrame...
> Source/core/frame/LocalFrame.h:237: bool m_shouldSendDPRHint;
> How many hints will we end up with? Might it make sense to have a bitmasked
flag
> instead of lots of booleans?

Currently it's just these two, maybe later we'd add a third one (MD, or Maximum
Downlink bandwidth hint), so I don't think a bitmask is necessary just yet.

[Yoav Weiss, 2015-02-25 09:15:23]

On 2015/02/25 09:06:08, Mike West wrote:
>
https://codereview.chromium.org/951303003/diff/1/LayoutTests/http/tests/misc/...
> File LayoutTests/http/tests/misc/client-hints-accept.php (right):
> 
>
https://codereview.chromium.org/951303003/diff/1/LayoutTests/http/tests/misc/...
> LayoutTests/http/tests/misc/client-hints-accept.php:9: <img
> src="resources/image-checks-for-dpr.php" onload="console.log('DPR sent.')"
> onerror="console.log('DPR not sent.')">
> Ditto.
> 
>
https://codereview.chromium.org/951303003/diff/1/LayoutTests/http/tests/misc/...
> File LayoutTests/http/tests/misc/client-hints-no-accept.html (right):
> 
>
https://codereview.chromium.org/951303003/diff/1/LayoutTests/http/tests/misc/...
> LayoutTests/http/tests/misc/client-hints-no-accept.html:6: <img
> src="resources/image-checks-for-dpr.php" onload="console.log('DPR sent.')"
> onerror="console.log('DPR not sent.')">
> This is going to flake (and, in fact, did!). Please:
> 
> 1. Write this as a `testharness.js` test with assertions so that we know what
> passing looks like.
> 2. Chain the requests so that the ordering is always consistent.

Doh! Will do

[Yoav Weiss, 2015-02-25 09:24:04]

On 2015/02/25 09:03:14, Mike West wrote:
> 
>
https://codereview.chromium.org/951303003/diff/1/Source/core/fetch/Resource.cpp
> File Source/core/fetch/Resource.cpp (right):
> 
>
https://codereview.chromium.org/951303003/diff/1/Source/core/fetch/Resource.c...
> Source/core/fetch/Resource.cpp:449: // FIXME: Write an actual parser for this
> comma delimited header.
> Nit: Can you add a bug ID here?
> 
> When you add an actual parser, it would be good to add unit tests as well to
> make sure we can easily check the various edge-cases (`dprw`, for instance :)
).
> 

Also, do we have a generic parser that takes a comma delimited string and sticks
the values between the commas in a map (while ignoring whitespace)? Cause that'd
pretty much do the trick here.

[Mike West, 2015-02-25 09:27:56]

On 2015/02/25 at 09:24:04, yoav wrote:
> On 2015/02/25 09:03:14, Mike West wrote:
> > 
> >
https://codereview.chromium.org/951303003/diff/1/Source/core/fetch/Resource.cpp
> > File Source/core/fetch/Resource.cpp (right):
> > 
> >
https://codereview.chromium.org/951303003/diff/1/Source/core/fetch/Resource.c...
> > Source/core/fetch/Resource.cpp:449: // FIXME: Write an actual parser for
this
> > comma delimited header.
> > Nit: Can you add a bug ID here?
> > 
> > When you add an actual parser, it would be good to add unit tests as well to
> > make sure we can easily check the various edge-cases (`dprw`, for instance
:) ).
> > 
> 
> Also, do we have a generic parser that takes a comma delimited string and
sticks the values between the commas in a map (while ignoring whitespace)? Cause
that'd pretty much do the trick here.

I don't think so, but it shouldn't be too hard to implement. :)  If you want to
create such a component, I'd be happy to review it.

[Yoav Weiss, 2015-02-25 09:33:53]

On 2015/02/25 09:27:56, Mike West wrote:
> On 2015/02/25 at 09:24:04, yoav wrote:
> > On 2015/02/25 09:03:14, Mike West wrote:
> > > 
> > >
>
https://codereview.chromium.org/951303003/diff/1/Source/core/fetch/Resource.cpp
> > > File Source/core/fetch/Resource.cpp (right):
> > > 
> > >
>
https://codereview.chromium.org/951303003/diff/1/Source/core/fetch/Resource.c...
> > > Source/core/fetch/Resource.cpp:449: // FIXME: Write an actual parser for
> this
> > > comma delimited header.
> > > Nit: Can you add a bug ID here?
> > > 
> > > When you add an actual parser, it would be good to add unit tests as well
to
> > > make sure we can easily check the various edge-cases (`dprw`, for instance
> :) ).
> > > 
> > 
> > Also, do we have a generic parser that takes a comma delimited string and
> sticks the values between the commas in a map (while ignoring whitespace)?
Cause
> that'd pretty much do the trick here.
> 
> I don't think so, but it shouldn't be too hard to implement. :)  If you want
to
> create such a component, I'd be happy to review it.

OK, might as well make it generic :)

[Yoav Weiss, 2015-02-25 11:04:11]

On 2015/02/25 09:33:53, Yoav Weiss wrote:
> On 2015/02/25 09:27:56, Mike West wrote:
> > On 2015/02/25 at 09:24:04, yoav wrote:
> > > On 2015/02/25 09:03:14, Mike West wrote:
> > > > 
> > > >
> >
>
https://codereview.chromium.org/951303003/diff/1/Source/core/fetch/Resource.cpp
> > > > File Source/core/fetch/Resource.cpp (right):
> > > > 
> > > >
> >
>
https://codereview.chromium.org/951303003/diff/1/Source/core/fetch/Resource.c...
> > > > Source/core/fetch/Resource.cpp:449: // FIXME: Write an actual parser for
> > this
> > > > comma delimited header.
> > > > Nit: Can you add a bug ID here?
> > > > 
> > > > When you add an actual parser, it would be good to add unit tests as
well
> to
> > > > make sure we can easily check the various edge-cases (`dprw`, for
instance
> > :) ).
> > > > 
> > > 
> > > Also, do we have a generic parser that takes a comma delimited string and
> > sticks the values between the commas in a map (while ignoring whitespace)?
> Cause
> > that'd pretty much do the trick here.
> > 
> > I don't think so, but it shouldn't be too hard to implement. :)  If you want
> to
> > create such a component, I'd be happy to review it.
> 
> OK, might as well make it generic :)

Uploaded new tests. Let me know if it works.

[Mike West, 2015-02-25 11:37:12]

LGTM % test comments.

https://codereview.chromium.org/951303003/diff/20001/LayoutTests/http/tests/m...
File LayoutTests/http/tests/misc/client-hints-no-accept-expected.txt (right):

https://codereview.chromium.org/951303003/diff/20001/LayoutTests/http/tests/m...
LayoutTests/http/tests/misc/client-hints-no-accept-expected.txt:3: PASS
Client-Hints not sent when Aceept-CH header is not present
Drop the 'expected.txt' files for these kinds of tests.

https://codereview.chromium.org/951303003/diff/20001/LayoutTests/http/tests/m...
File LayoutTests/http/tests/misc/client-hints-no-accept.html (right):

https://codereview.chromium.org/951303003/diff/20001/LayoutTests/http/tests/m...
LayoutTests/http/tests/misc/client-hints-no-accept.html:6: var t =
async_test('Client-Hints not sent when Aceept-CH header is not present');
Nit: s/Aceept/Accept/

https://codereview.chromium.org/951303003/diff/20001/LayoutTests/http/tests/m...
LayoutTests/http/tests/misc/client-hints-no-accept.html:21: img.src =
'resources/image-checks-for-dpr.php';
Doesn't assigning to `.src` trigger a load right away? I've always put the
`onload`/`onerror` bits above the assignment for that reason. But I guess you
know better than I...

[Yoav Weiss, 2015-02-25 11:59:50]

On 2015/02/25 11:37:12, Mike West wrote:
> LGTM % test comments.
> 
>
https://codereview.chromium.org/951303003/diff/20001/LayoutTests/http/tests/m...
> File LayoutTests/http/tests/misc/client-hints-no-accept-expected.txt (right):
> 
>
https://codereview.chromium.org/951303003/diff/20001/LayoutTests/http/tests/m...
> LayoutTests/http/tests/misc/client-hints-no-accept-expected.txt:3: PASS
> Client-Hints not sent when Aceept-CH header is not present
> Drop the 'expected.txt' files for these kinds of tests.

I'd love to, but the test runner insists. I had that happen before, and it
magically went away or was a result of JS dependencies.
Not sure how to convince testRunner that I don't need an expected.txt file...

> 
>
https://codereview.chromium.org/951303003/diff/20001/LayoutTests/http/tests/m...
> File LayoutTests/http/tests/misc/client-hints-no-accept.html (right):
> 
>
https://codereview.chromium.org/951303003/diff/20001/LayoutTests/http/tests/m...
> LayoutTests/http/tests/misc/client-hints-no-accept.html:6: var t =
> async_test('Client-Hints not sent when Aceept-CH header is not present');
> Nit: s/Aceept/Accept/
> 
Good catch.

>
https://codereview.chromium.org/951303003/diff/20001/LayoutTests/http/tests/m...
> LayoutTests/http/tests/misc/client-hints-no-accept.html:21: img.src =
> 'resources/image-checks-for-dpr.php';
> Doesn't assigning to `.src` trigger a load right away? I've always put the
> `onload`/`onerror` bits above the assignment for that reason. But I guess you
> know better than I...

It used to be that way, but we fixed it :D
Images are now loaded async (on the next microtask), which means that the order
in which we set our attributes doesn't matter, at least in Blink.

[Yoav Weiss, 2015-02-25 12:10:56]

On 2015/02/25 11:59:50, Yoav Weiss wrote:
> On 2015/02/25 11:37:12, Mike West wrote:
> > LGTM % test comments.
> > 
> >
>
https://codereview.chromium.org/951303003/diff/20001/LayoutTests/http/tests/m...
> > File LayoutTests/http/tests/misc/client-hints-no-accept-expected.txt
(right):
> > 
> >
>
https://codereview.chromium.org/951303003/diff/20001/LayoutTests/http/tests/m...
> > LayoutTests/http/tests/misc/client-hints-no-accept-expected.txt:3: PASS
> > Client-Hints not sent when Aceept-CH header is not present
> > Drop the 'expected.txt' files for these kinds of tests.
> 
> I'd love to, but the test runner insists. I had that happen before, and it
> magically went away or was a result of JS dependencies.
> Not sure how to convince testRunner that I don't need an expected.txt file...

Found it. Bad testharness.js...
I see who else uses it and weed it out in a separate CL.

> 
> > 
> >
>
https://codereview.chromium.org/951303003/diff/20001/LayoutTests/http/tests/m...
> > File LayoutTests/http/tests/misc/client-hints-no-accept.html (right):
> > 
> >
>
https://codereview.chromium.org/951303003/diff/20001/LayoutTests/http/tests/m...
> > LayoutTests/http/tests/misc/client-hints-no-accept.html:6: var t =
> > async_test('Client-Hints not sent when Aceept-CH header is not present');
> > Nit: s/Aceept/Accept/
> > 
> Good catch.
> 
> >
>
https://codereview.chromium.org/951303003/diff/20001/LayoutTests/http/tests/m...
> > LayoutTests/http/tests/misc/client-hints-no-accept.html:21: img.src =
> > 'resources/image-checks-for-dpr.php';
> > Doesn't assigning to `.src` trigger a load right away? I've always put the
> > `onload`/`onerror` bits above the assignment for that reason. But I guess
you
> > know better than I...
> 
> It used to be that way, but we fixed it :D
> Images are now loaded async (on the next microtask), which means that the
order
> in which we set our attributes doesn't matter, at least in Blink.

[Mike West, 2015-02-25 12:36:13]

Wunderbar. Land it. :)

[Yoav Weiss, 2015-02-25 13:01:58]

The CQ bit was checked by yoav@yoav.ws

[Yoav Weiss, 2015-02-25 13:01:58]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/951303003/#ps40001 (title:
"Removed expected files")

[commit-bot: I haz the power, 2015-02-25 13:02:58]

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

[commit-bot: I haz the power, 2015-02-25 14:29:10]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190831