Implement device-ram client hints header

Implement device-ram client hints header

BUG=718622
Review-Url: https://codereview.chromium.org/2860093003
Cr-Commit-Position: refs/heads/master@{#475694}
Committed: https://chromium.googlesource.com/chromium/src/+/3a2b4be52d3f11b216be38f9e5f2ca10962a4591

[panicker, 2017-05-05 17:19:29]

panicker@chromium.org changed reviewers:
+ panicker@chromium.org

[panicker, 2017-05-05 17:19:30]

Thanks for the CL! Looking good!
Left a couple comments.

https://codereview.chromium.org/2860093003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2860093003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:612: bool
should_send_device_ram =
also check if device-ram RunTimeFlagEnabled  is enabled?

https://codereview.chromium.org/2860093003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:625: if
(should_send_device_ram) {
Nit: extract this math into a helper
Also makes unittesting a bit easier

https://codereview.chromium.org/2860093003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp (right):

https://codereview.chromium.org/2860093003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp:524:
TEST_F(FrameFetchContextHintsTest, MonitorDeviceRamHints) {
Also add a unittest with different values of physical ram (include edge cases)
to test the computed value

[fmeawad, 2017-05-23 20:50:54]

Shubhie, can you take another look before I send it to a wider audience. Also
this version still floors.

https://codereview.chromium.org/2860093003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2860093003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:612: bool
should_send_device_ram =
On 2017/05/05 17:19:30, panicker wrote:
> also check if device-ram RunTimeFlagEnabled  is enabled?

I don't think I need/should since ShouldSendDeviceRam already have that check.

https://codereview.chromium.org/2860093003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:625: if
(should_send_device_ram) {
On 2017/05/05 17:19:30, panicker wrote:
> Nit: extract this math into a helper
> Also makes unittesting a bit easier

Done.

https://codereview.chromium.org/2860093003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp (right):

https://codereview.chromium.org/2860093003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp:524:
TEST_F(FrameFetchContextHintsTest, MonitorDeviceRamHints) {
On 2017/05/05 17:19:30, panicker wrote:
> Also add a unittest with different values of physical ram (include edge cases)
> to test the computed value

Done.

[fmeawad, 2017-05-23 20:51:22]

Description was changed from

==========
Implement device-ram client hints header

WIP: Missing tests

BUG=718622
==========

to

==========
Implement device-ram client hints header

BUG=718622
==========

[fmeawad, 2017-05-23 20:54:07]

The CQ bit was checked by fmeawad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-23 20:55:10]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fmeawad, 2017-05-23 20:59:10]

The CQ bit was checked by fmeawad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-23 20:59:48]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 05:32:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-24 05:32:03]

Dry run: This issue passed the CQ dry run.

[Yoav Weiss, 2017-05-24 07:48:01]

yoav@yoav.ws changed reviewers:
+ yoav@yoav.ws

[Yoav Weiss, 2017-05-24 07:48:03]

Thanks for working on this! A few nits.

https://codereview.chromium.org/2860093003/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/http/tests/misc/client-hints-accept.php
(right):

https://codereview.chromium.org/2860093003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/misc/client-hints-accept.php:2:
header("ACCEPT-CH: DPR, Width, Viewport-Width, Device-Ram");
Nit: Maybe "Device-RAM"? (I don't think it makes any actual difference, but
"Ram" makes me think
https://qph.ec.quoracdn.net/main-qimg-fe73b568eee0e7ebcaca8f13a9957062-c :D)

Similar nit to other places with "Ram" in them.

https://codereview.chromium.org/2860093003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/2860093003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/UseCounter.h:1606: kClientHintsDeviceRam =
1995,
s/Ram/RAM/
See https://www.chromium.org/blink/coding-style#TOC-Names

Same comment everywhere else with "Ram"

https://codereview.chromium.org/2860093003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2860093003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:613: int power = -1;
I find the MSB counting here a bit confusing. Maybe start `power` at 0 and count
until `physical_memory_mb` is 1?

https://codereview.chromium.org/2860093003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:616:
physical_memory_mb = physical_memory_mb >> 1;
Nit: "physical_memory_mb >>= 1;" might be a bit clearer.

https://codereview.chromium.org/2860093003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:620: return ((1 <<
power) / 1024.0);
Implicit conversion from int to float... Can you make that conversion explicit?

[panicker, 2017-05-24 17:27:20]

LGTM, modulo Yoav's comments.

https://codereview.chromium.org/2860093003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2860093003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:609: float
FrameFetchContext::ClientHintsDeviceRam(int physical_memory_mb) {
let's add a comment that this math with change and what we plan to do - to avoid
any confusion.

https://codereview.chromium.org/2860093003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp (right):

https://codereview.chromium.org/2860093003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp:540:
ExpectHeader("http://www.example.com/1.gif", "Viewport-Width", false, "");
Maybe drop Viewport-Width, Width?

[panicker, 2017-05-24 17:28:33]

https://codereview.chromium.org/2860093003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5 (right):

https://codereview.chromium.org/2860093003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5:344: status:
"experimental",
this should be "test" to start with. When we update the math to match the
(latest) spec proposal then we can switch to "experimental"

[fmeawad, 2017-05-24 18:22:31]

The CQ bit was checked by fmeawad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 18:23:18]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fmeawad, 2017-05-24 18:24:57]

Comments addressed, PTAL

https://codereview.chromium.org/2860093003/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/http/tests/misc/client-hints-accept.php
(right):

https://codereview.chromium.org/2860093003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/misc/client-hints-accept.php:2:
header("ACCEPT-CH: DPR, Width, Viewport-Width, Device-Ram");
On 2017/05/24 07:48:02, Yoav Weiss wrote:
> Nit: Maybe "Device-RAM"? (I don't think it makes any actual difference, but
> "Ram" makes me think
> https://qph.ec.quoracdn.net/main-qimg-fe73b568eee0e7ebcaca8f13a9957062-c :D)
> 
> Similar nit to other places with "Ram" in them.

Done.

https://codereview.chromium.org/2860093003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/2860093003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/UseCounter.h:1606: kClientHintsDeviceRam =
1995,
On 2017/05/24 07:48:02, Yoav Weiss wrote:
> s/Ram/RAM/
> See https://www.chromium.org/blink/coding-style#TOC-Names
> 
> Same comment everywhere else with "Ram"

Done.

https://codereview.chromium.org/2860093003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2860093003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:609: float
FrameFetchContext::ClientHintsDeviceRam(int physical_memory_mb) {
On 2017/05/24 17:27:20, panicker wrote:
> let's add a comment that this math with change and what we plan to do - to
avoid
> any confusion.

Done.

https://codereview.chromium.org/2860093003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:613: int power = -1;
On 2017/05/24 07:48:02, Yoav Weiss wrote:
> I find the MSB counting here a bit confusing. Maybe start `power` at 0 and
count
> until `physical_memory_mb` is 1?

The loop does an extra iteration, to compensate, we init to -1. That saves a
subtraction operation. The logic might change to rounding instead of flooring.
Added a comment for now.

Also considering changing it to use:
https://graphics.stanford.edu/~seander/bithacks.html#RoundUpPowerOf2

https://codereview.chromium.org/2860093003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:616:
physical_memory_mb = physical_memory_mb >> 1;
On 2017/05/24 07:48:02, Yoav Weiss wrote:
> Nit: "physical_memory_mb >>= 1;" might be a bit clearer.

Done.

https://codereview.chromium.org/2860093003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:620: return ((1 <<
power) / 1024.0);
On 2017/05/24 07:48:02, Yoav Weiss wrote:
> Implicit conversion from int to float... Can you make that conversion
explicit?

Done.

https://codereview.chromium.org/2860093003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp (right):

https://codereview.chromium.org/2860093003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp:540:
ExpectHeader("http://www.example.com/1.gif", "Viewport-Width", false, "");
On 2017/05/24 17:27:20, panicker wrote:
> Maybe drop Viewport-Width, Width?

Actually we need to make sure we are not setting any other client header.

https://codereview.chromium.org/2860093003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5 (right):

https://codereview.chromium.org/2860093003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5:344: status:
"experimental",
On 2017/05/24 17:28:33, panicker wrote:
> this should be "test" to start with. When we update the math to match the
> (latest) spec proposal then we can switch to "experimental"

Done.

[commit-bot: I haz the power, 2017-05-24 18:27:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-24 18:27:46]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[fmeawad, 2017-05-24 18:38:29]

The CQ bit was checked by fmeawad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 18:39:32]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 23:21:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-24 23:21:26]

Dry run: This issue passed the CQ dry run.

[haraken, 2017-05-25 04:17:46]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2017-05-25 04:17:48]

Implementation-wise LGTM

https://codereview.chromium.org/2860093003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/ChromeClient.h (right):

https://codereview.chromium.org/2860093003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/ChromeClient.h:402: float device_ram_mb_;

Is this unused?

[kinuko, 2017-05-25 05:05:50]

kinuko@chromium.org changed reviewers:
+ kinuko@chromium.org

[kinuko, 2017-05-25 05:05:51]

https://codereview.chromium.org/2860093003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp (right):

https://codereview.chromium.org/2860093003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp:497: class
FrameFetchContextHintsTest : public FrameFetchContextTest {
For these ones I suspect we could just use TestingPlatformSupport so that we
don't need to add SetDevicePhysicalMemoryMBForTesting on Platform (but only add
it on the test platform class)?

[kinuko, 2017-05-25 05:31:28]

https://codereview.chromium.org/2860093003/diff/80001/content/child/blink_pla...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/2860093003/diff/80001/content/child/blink_pla...
content/child/blink_platform_impl.cc:365:
static_cast<size_t>(base::SysInfo::AmountOfPhysicalMemoryMB())) {
Hum, this doesn't seem we need public platform plumbing actually?

[haraken, 2017-05-25 05:40:51]

On 2017/05/25 05:31:28, kinuko wrote:
>
https://codereview.chromium.org/2860093003/diff/80001/content/child/blink_pla...
> File content/child/blink_platform_impl.cc (right):
> 
>
https://codereview.chromium.org/2860093003/diff/80001/content/child/blink_pla...
> content/child/blink_platform_impl.cc:365:
> static_cast<size_t>(base::SysInfo::AmountOfPhysicalMemoryMB())) {
> Hum, this doesn't seem we need public platform plumbing actually?

This is needed because core/ cannot call base::SysInfo... Another way to do this
would be to create a thin wrapper in platform/.

However, I really want to fix this issue by just letting core/ use base/ types
(as we've been discussing in platform-architecture-dev@).

[kinuko, 2017-05-25 05:44:36]

On 2017/05/25 05:40:51, haraken wrote:
> On 2017/05/25 05:31:28, kinuko wrote:
> >
>
https://codereview.chromium.org/2860093003/diff/80001/content/child/blink_pla...
> > File content/child/blink_platform_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/2860093003/diff/80001/content/child/blink_pla...
> > content/child/blink_platform_impl.cc:365:
> > static_cast<size_t>(base::SysInfo::AmountOfPhysicalMemoryMB())) {
> > Hum, this doesn't seem we need public platform plumbing actually?
> 
> This is needed because core/ cannot call base::SysInfo... Another way to do
this
> would be to create a thin wrapper in platform/.

Yep, my comment meant to say platform/ solution would be less code if we need a
thin wrapper 'somewhere'.

> However, I really want to fix this issue by just letting core/ use base/ types
> (as we've been discussing in platform-architecture-dev@).

Makes sense.

[fmeawad, 2017-05-25 19:52:54]

> Yep, my comment meant to say platform/ solution would be less code if we need
a
> thin wrapper 'somewhere'.
Does it make sense as part of MemoryCoordinator next to is_low_end_device?

Other comments addressed. PTAL.

[fmeawad, 2017-05-25 19:53:35]

https://codereview.chromium.org/2860093003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp (right):

https://codereview.chromium.org/2860093003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp:497: class
FrameFetchContextHintsTest : public FrameFetchContextTest {
On 2017/05/25 05:05:51, kinuko wrote:
> For these ones I suspect we could just use TestingPlatformSupport so that we
> don't need to add SetDevicePhysicalMemoryMBForTesting on Platform (but only
add
> it on the test platform class)?

Done.

https://codereview.chromium.org/2860093003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/ChromeClient.h (right):

https://codereview.chromium.org/2860093003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/ChromeClient.h:402: float device_ram_mb_;
On 2017/05/25 04:17:47, haraken wrote:
> 
> Is this unused?

Yes, it was moved to blink_platform_impl, Sorry about that. Removed now.

[haraken, 2017-05-26 03:50:52]

On 2017/05/25 19:52:54, fmeawad wrote:
> > Yep, my comment meant to say platform/ solution would be less code if we
need
> a
> > thin wrapper 'somewhere'.
> Does it make sense as part of MemoryCoordinator next to is_low_end_device?

Sounds good.

[Yoav Weiss, 2017-05-26 05:44:23]

https://codereview.chromium.org/2860093003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2860093003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:620: }
```
int power = 0;
// Extract the MSB location.
while (physical_memory_mb > 1) {
  physical_memory_mb >>= 1;
  power++;
}
```

gives you the same end result, is less confusing and performs one less
iteration.

https://codereview.chromium.org/2860093003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:622: return
(float)(1 << power) / (float)1024.0;
static_cast<float>() would be better for the first. No need to cast the second,
as 1024.0 is already a float.

[fmeawad, 2017-05-26 17:18:50]

The CQ bit was checked by fmeawad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 17:19:15]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-26 19:24:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-26 19:24:23]

Dry run: This issue passed the CQ dry run.

[fmeawad, 2017-05-26 20:13:17]

All comments addressed, PTAL

https://codereview.chromium.org/2860093003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2860093003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:620: }
On 2017/05/26 05:44:22, Yoav Weiss wrote:
> ```
> int power = 0;
> // Extract the MSB location.
> while (physical_memory_mb > 1) {
>   physical_memory_mb >>= 1;
>   power++;
> }
> ```
> 
> gives you the same end result, is less confusing and performs one less
> iteration.

Done. and thank you for your patience.

https://codereview.chromium.org/2860093003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:622: return
(float)(1 << power) / (float)1024.0;
On 2017/05/26 05:44:22, Yoav Weiss wrote:
> static_cast<float>() would be better for the first. No need to cast the
second,
> as 1024.0 is already a float.

Done.

[Yoav Weiss, 2017-05-26 20:48:15]

LGTM! Thanks for working on this :)

Couple of nits and we're good to go

https://codereview.chromium.org/2860093003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2860093003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:619: int power = 0; 
// To compensate of the extra loop iteration.
The "compensate" comment seems out of place now

https://codereview.chromium.org/2860093003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/loader/fetch/ClientHintsPreferences.cpp
(right):

https://codereview.chromium.org/2860093003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/loader/fetch/ClientHintsPreferences.cpp:35:
if (accept_client_hints_header.Contains("device-ram")) {
Can you `&&` these two conditions?

[haraken, 2017-05-27 00:33:44]

LGTM

[kinuko, 2017-05-29 05:50:02]

lgtm

https://codereview.chromium.org/2860093003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp (right):

https://codereview.chromium.org/2860093003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp:49: #include
"platform/testing/TestingPlatformSupport.h"
No longer needed

[fmeawad, 2017-05-30 17:21:00]

https://codereview.chromium.org/2860093003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2860093003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:619: int power = 0; 
// To compensate of the extra loop iteration.
On 2017/05/26 20:48:15, Yoav Weiss wrote:
> The "compensate" comment seems out of place now

Done.

https://codereview.chromium.org/2860093003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp (right):

https://codereview.chromium.org/2860093003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp:49: #include
"platform/testing/TestingPlatformSupport.h"
On 2017/05/29 05:50:02, kinuko wrote:
> No longer needed

Done.

https://codereview.chromium.org/2860093003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/loader/fetch/ClientHintsPreferences.cpp
(right):

https://codereview.chromium.org/2860093003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/loader/fetch/ClientHintsPreferences.cpp:35:
if (accept_client_hints_header.Contains("device-ram")) {
On 2017/05/26 20:48:15, Yoav Weiss wrote:
> Can you `&&` these two conditions?

Done.

[fmeawad, 2017-05-30 17:21:07]

The CQ bit was checked by fmeawad@chromium.org

[fmeawad, 2017-05-30 17:21:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
panicker@chromium.org
Link to the patchset: https://codereview.chromium.org/2860093003/#ps140001
(title: "Minor fixes")

[commit-bot: I haz the power, 2017-05-30 17:22:25]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-30 17:25:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-30 17:25:16]

Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[fmeawad, 2017-05-30 17:28:09]

The CQ bit was checked by fmeawad@chromium.org

[fmeawad, 2017-05-30 17:28:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, kinuko@chromium.org, yoav@yoav.ws, panicker@chromium.org
Link to the patchset: https://codereview.chromium.org/2860093003/#ps160001
(title: "Address Nits + Rebase")

[commit-bot: I haz the power, 2017-05-30 17:28:50]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-30 19:37:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-30 19:37:17]

Failed to apply patch for third_party/WebKit/Source/core/frame/UseCounter.h:
While running git apply --index -3 -p1;
  error: patch failed: third_party/WebKit/Source/core/frame/UseCounter.h:1617
  Falling back to three-way merge...
  Applied patch to 'third_party/WebKit/Source/core/frame/UseCounter.h' with
conflicts.
  U third_party/WebKit/Source/core/frame/UseCounter.h

Patch:       third_party/WebKit/Source/core/frame/UseCounter.h
Index: third_party/WebKit/Source/core/frame/UseCounter.h
diff --git a/third_party/WebKit/Source/core/frame/UseCounter.h
b/third_party/WebKit/Source/core/frame/UseCounter.h
index
59c431c61c22bcb22d18826c80f0278d3c731bac..0ae83ba6e0a497dba5971bb23276c7678b5c8e01
100644
--- a/third_party/WebKit/Source/core/frame/UseCounter.h
+++ b/third_party/WebKit/Source/core/frame/UseCounter.h
@@ -1617,6 +1617,7 @@ class CORE_EXPORT UseCounter {
     kCredentialManagerPreventSilentAccess = 2012,
     kNetInfoEffectiveType = 2013,
     kV8SpeechRecognition_Start_Method = 2014,
+    kClientHintsDeviceRAM = 2015,
 
     // Add new features immediately above this line. Don't change assigned
     // numbers of any item, and don't reuse removed slots.

[fmeawad, 2017-05-30 19:41:57]

The CQ bit was checked by fmeawad@chromium.org

[fmeawad, 2017-05-30 19:41:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, kinuko@chromium.org, yoav@yoav.ws, panicker@chromium.org
Link to the patchset: https://codereview.chromium.org/2860093003/#ps180001
(title: "Rebase UseCounter.h")

[commit-bot: I haz the power, 2017-05-30 19:42:36]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-30 19:49:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-30 19:49:08]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[fmeawad, 2017-05-30 19:50:24]

The CQ bit was checked by fmeawad@chromium.org

[commit-bot: I haz the power, 2017-05-30 19:50:52]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-30 23:51:39]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1496173824531580,
"parent_rev": "5e8bef0e04a637592c3443eb58a32091449d987a", "commit_rev":
"3a2b4be52d3f11b216be38f9e5f2ca10962a4591"}

[commit-bot: I haz the power, 2017-05-30 23:51:49]

Description was changed from

==========
Implement device-ram client hints header

BUG=718622
==========

to

==========
Implement device-ram client hints header

BUG=718622

Review-Url: https://codereview.chromium.org/2860093003
Cr-Commit-Position: refs/heads/master@{#475694}
Committed:
https://chromium.googlesource.com/chromium/src/+/3a2b4be52d3f11b216be38f9e5f2...
==========

[commit-bot: I haz the power, 2017-05-30 23:51:51]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/3a2b4be52d3f11b216be38f9e5f2...