NetInfo: Slight update to the default network quality value

NetInfo: Slight update to the default network quality value

This CL changes the default values of |downlink| to be a multiple of
25 kbps.

BUG=727786
Review-Url: https://codereview.chromium.org/2912013002
Cr-Commit-Position: refs/heads/master@{#477227}
Committed: https://chromium.googlesource.com/chromium/src/+/420ae3445c9b1530ba81ac651966d01548c04d09

[tbansal1, 2017-05-30 05:22:56]

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

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

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-30 07:14:42]

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

[commit-bot: I haz the power, 2017-05-30 07:14:43]

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

[tbansal1, 2017-05-30 07:35:08]

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

[commit-bot: I haz the power, 2017-05-30 07:35:22]

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-30 09:32:16]

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

[commit-bot: I haz the power, 2017-05-30 09:32:17]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-05-30 16:55:59]

Description was changed from

==========
NQE default values 2

BUG=
==========

to

==========
NetInfo network quality extension: Use default network quality values

The spec mentions that if the network quality is unavailable, the user 
agent can use the default network quality values. This CL changes the default
values of RTT and throughput.

BUG=
==========

[tbansal1, 2017-05-30 17:03:53]

Description was changed from

==========
NetInfo network quality extension: Use default network quality values

The spec mentions that if the network quality is unavailable, the user 
agent can use the default network quality values. This CL changes the default
values of RTT and throughput.

BUG=
==========

to

==========
NetInfo: Slight update to the default network quality value

This CL changes the default values of throughput to be a multiple of 25 kbps.

BUG=
==========

[tbansal1, 2017-05-30 17:46:18]

Patchset #3 (id:40001) has been deleted

[tbansal1, 2017-05-30 17:49:50]

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

[tbansal1, 2017-05-30 17:49:53]

Patchset #3 (id:60001) has been deleted

[tbansal1, 2017-05-30 17:50:02]

Patchset #1 (id:1) has been deleted

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

Dry run: CQ is trying da patch.

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

[tbansal1, 2017-05-30 17:51:02]

Patchset #1 (id:20001) has been deleted

[tbansal1, 2017-05-30 18:15:35]

Description was changed from

==========
NetInfo: Slight update to the default network quality value

This CL changes the default values of throughput to be a multiple of 25 kbps.

BUG=
==========

to

==========
NetInfo: Slight update to the default network quality value

This CL changes the default values of throughput to be a multiple of
25 kbps.

BUG=727786
==========

[tbansal1, 2017-05-30 18:16:07]

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

[tbansal1, 2017-05-30 18:16:07]

haraken: ptal. Thanks.

[tbansal1, 2017-05-30 18:16:40]

Description was changed from

==========
NetInfo: Slight update to the default network quality value

This CL changes the default values of throughput to be a multiple of
25 kbps.

BUG=727786
==========

to

==========
NetInfo: Slight update to the default network quality value

This CL changes the default values of |downlink| to be a multiple of
25 kbps.

BUG=727786
==========

[commit-bot: I haz the power, 2017-05-30 22:52:53]

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

[commit-bot: I haz the power, 2017-05-30 22:52:54]

Dry run: This issue passed the CQ dry run.

[haraken, 2017-05-31 00:14:46]

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

[haraken, 2017-05-31 00:14:47]

LGTM but I guess jkarlin would be a better reviewer for modules/netinfo/.

[tbansal1, 2017-05-31 00:15:08]

jkarlin: ptal. Thanks.

[jkarlin, 2017-05-31 11:39:28]

The spec is still unclear on what the value should be if the value is unknown.
Can you please get that clarified?

[tbansal1, 2017-05-31 16:14:22]

On 2017/05/31 11:39:28, jkarlin wrote:
> The spec is still unclear on what the value should be if the value is unknown.
> Can you please get that clarified?

This was resolved in https://github.com/WICG/netinfo/issues/55.
Exact diff:
https://s3.amazonaws.com/pr-preview/WICG/netinfo/01edb11...888c220.html#-dfn-...


When the estimate is unavailable, the user agent should synthesize the
estimate from the connection type.
NQE does that on the browser side:
https://cs.chromium.org/chromium/src/net/nqe/network_quality_estimator_params...
And, then it sends the default estimates via IPC/mojo.

However, we still need a default value on the Webkit side to use before the
estimate is sent to the Blink by the browser, or in case the browser side
NQE is unavailable. In that case, we fallback on the
using a synthesized estimate which happens to be the fastest value.

[jkarlin, 2017-05-31 16:23:01]

On 2017/05/31 16:14:22, tbansal1 wrote:
> On 2017/05/31 11:39:28, jkarlin wrote:
> > The spec is still unclear on what the value should be if the value is
unknown.
> > Can you please get that clarified?
> 
> This was resolved in https://github.com/WICG/netinfo/issues/55.
> Exact diff:
>
https://s3.amazonaws.com/pr-preview/WICG/netinfo/01edb11...888c220.html#-dfn-...
> 
> 
> When the estimate is unavailable, the user agent should synthesize the
> estimate from the connection type.
> NQE does that on the browser side:
>
https://cs.chromium.org/chromium/src/net/nqe/network_quality_estimator_params...
> And, then it sends the default estimates via IPC/mojo.

Sounds good.

> 
> However, we still need a default value on the Webkit side to use before the
> estimate is sent to the Blink by the browser, 

Hmm, why isn't a default value available on renderer creation? Connection type
and max bandwidth are. We should add this data as well.

> or in case the browser side
> NQE is unavailable. In that case, we fallback on the
> using a synthesized estimate which happens to be the fastest value.

When is that possible? Can't we derive defaults based on connection type in that
situation as the spec says?

[tbansal1, 2017-05-31 16:53:27]

On 2017/05/31 16:23:01, jkarlin wrote:
> On 2017/05/31 16:14:22, tbansal1 wrote:
> > On 2017/05/31 11:39:28, jkarlin wrote:
> > > The spec is still unclear on what the value should be if the value is
> unknown.
> > > Can you please get that clarified?
> > 
> > This was resolved in https://github.com/WICG/netinfo/issues/55.
> > Exact diff:
> >
>
https://s3.amazonaws.com/pr-preview/WICG/netinfo/01edb11...888c220.html#-dfn-...
> > 
> > 
> > When the estimate is unavailable, the user agent should synthesize the
> > estimate from the connection type.
> > NQE does that on the browser side:
> >
>
https://cs.chromium.org/chromium/src/net/nqe/network_quality_estimator_params...
> > And, then it sends the default estimates via IPC/mojo.
> 
> Sounds good.
> 
> > 
> > However, we still need a default value on the Webkit side to use before the
> > estimate is sent to the Blink by the browser, 
> 
> Hmm, why isn't a default value available on renderer creation? Connection type
> and max bandwidth are. We should add this data as well.
That's a good question. I tried adding DCHECK here. That DCHECK does not trigger
when running Chromium, but it triggers in some of the tests on win and
chromeos platforms.
(See the other CL which tried this:
https://codereview.chromium.org/2908233002/).
I was planning to file a bug against myself and debug it later since I need to
merge this back to M-60. Other option is to try to land that other CL,
but disable that DCHECK here for win
and chromeos, and then I debug those tests later. WYDT?
> 
> > or in case the browser side
> > NQE is unavailable. In that case, we fallback on the
> > using a synthesized estimate which happens to be the fastest value.
> 
> When is that possible?
> Can't we derive defaults based on connection type in that
> situation as the spec says?
It really should not happen but AFAIK it is happening in some tests
on win and chromeos.

[tbansal1, 2017-05-31 16:54:25]

On 2017/05/31 16:53:27, tbansal1 wrote:
> On 2017/05/31 16:23:01, jkarlin wrote:
> > On 2017/05/31 16:14:22, tbansal1 wrote:
> > > On 2017/05/31 11:39:28, jkarlin wrote:
> > > > The spec is still unclear on what the value should be if the value is
> > unknown.
> > > > Can you please get that clarified?
> > > 
> > > This was resolved in https://github.com/WICG/netinfo/issues/55.
> > > Exact diff:
> > >
> >
>
https://s3.amazonaws.com/pr-preview/WICG/netinfo/01edb11...888c220.html#-dfn-...
> > > 
> > > 
> > > When the estimate is unavailable, the user agent should synthesize the
> > > estimate from the connection type.
> > > NQE does that on the browser side:
> > >
> >
>
https://cs.chromium.org/chromium/src/net/nqe/network_quality_estimator_params...
> > > And, then it sends the default estimates via IPC/mojo.
> > 
> > Sounds good.
> > 
> > > 
> > > However, we still need a default value on the Webkit side to use before
the
> > > estimate is sent to the Blink by the browser, 
> > 
> > Hmm, why isn't a default value available on renderer creation? Connection
type
> > and max bandwidth are. We should add this data as well.
> That's a good question. I tried adding DCHECK here. That DCHECK does not
trigger
> when running Chromium, but it triggers in some of the tests on win and
> chromeos platforms.
> (See the other CL which tried this:
> https://codereview.chromium.org/2908233002/).
> I was planning to file a bug against myself and debug it later since I need to
> merge this back to M-60. Other option is to try to land that other CL,
> but disable that DCHECK here for win
> and chromeos, and then I debug those tests later. WYDT?
of interest is line 68 on right hand side here:
https://codereview.chromium.org/2908233002/diff/90001/third_party/WebKit/Sour...
> > 
> > > or in case the browser side
> > > NQE is unavailable. In that case, we fallback on the
> > > using a synthesized estimate which happens to be the fastest value.
> > 
> > When is that possible?
> > Can't we derive defaults based on connection type in that
> > situation as the spec says?
> It really should not happen but AFAIK it is happening in some tests
> on win and chromeos.

[jkarlin, 2017-05-31 17:27:50]

> > > Hmm, why isn't a default value available on renderer creation? Connection
> type
> > > and max bandwidth are. We should add this data as well.
> > That's a good question. I tried adding DCHECK here. That DCHECK does not
> trigger
> > when running Chromium, but it triggers in some of the tests on win and
> > chromeos platforms.
> > (See the other CL which tried this:
> > https://codereview.chromium.org/2908233002/).
> > I was planning to file a bug against myself and debug it later since I need
to
> > merge this back to M-60. Other option is to try to land that other CL,
> > but disable that DCHECK here for win
> > and chromeos, and then I debug those tests later. WYDT?

Clearly there is a race. Looking at the current code for connection.type and
maxBandwidth there also appears to be a race. The BrowserOnlineStateObserver
watches for render process creation messages and immediately IPCs the type and
maxBandwidth over. So far the DCHECKs have held however. Perhaps you could do
the same for NQE.

Please do file the DCHECK bug.

[jkarlin, 2017-05-31 17:44:22]

My general thought is that this is unspecced behavior. You're only running into
this problem due to a race in render process startup. We shouldn't hit this with
correct implementation.

Given that, I don't see why making this % 25 is preferable to what is already
there, +Infinity, until the underlying issue is fixed.

https://codereview.chromium.org/2912013002/diff/80001/content/browser/net_inf...
File content/browser/net_info_browsertest.cc (right):

https://codereview.chromium.org/2912013002/diff/80001/content/browser/net_inf...
content/browser/net_info_browsertest.cc:182: EXPECT_EQ(0,
RunScriptExtractDouble("getRtt()"));
Doesn't rtt return an unsigned long long?

https://codereview.chromium.org/2912013002/diff/80001/content/browser/net_inf...
content/browser/net_info_browsertest.cc:183: EXPECT_EQ(0,
static_cast<int>(RunScriptExtractDouble("getRtt()")) % 25);
ditto, and everywhere else in this file.

[jkarlin, 2017-05-31 17:57:14]

On 2017/05/31 17:44:22, jkarlin wrote:
> My general thought is that this is unspecced behavior. You're only running
into
> this problem due to a race in render process startup. We shouldn't hit this
with
> correct implementation.
> 
> Given that, I don't see why making this % 25 is preferable to what is already
> there, +Infinity, until the underlying issue is fixed.
> 
>
https://codereview.chromium.org/2912013002/diff/80001/content/browser/net_inf...
> File content/browser/net_info_browsertest.cc (right):
> 
>
https://codereview.chromium.org/2912013002/diff/80001/content/browser/net_inf...
> content/browser/net_info_browsertest.cc:182: EXPECT_EQ(0,
> RunScriptExtractDouble("getRtt()"));
> Doesn't rtt return an unsigned long long?
> 
>
https://codereview.chromium.org/2912013002/diff/80001/content/browser/net_inf...
> content/browser/net_info_browsertest.cc:183: EXPECT_EQ(0,
> static_cast<int>(RunScriptExtractDouble("getRtt()")) % 25);
> ditto, and everywhere else in this file.

I see, this CL is so that layout tests can verify that the bandwidth is a
multiple of 25. Alright, lgtm so long as you fix the underlying issue in a
follow up.

[tbansal1, 2017-06-01 20:36:21]

Patchset #2 (id:100001) has been deleted

[tbansal1, 2017-06-01 20:41:33]

> Please do file the DCHECK bug.

https://bugs.chromium.org/p/chromium/issues/detail?id=728771

[tbansal1, 2017-06-01 20:41:49]

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

[tbansal1, 2017-06-01 20:41:52]

https://codereview.chromium.org/2912013002/diff/80001/content/browser/net_inf...
File content/browser/net_info_browsertest.cc (right):

https://codereview.chromium.org/2912013002/diff/80001/content/browser/net_inf...
content/browser/net_info_browsertest.cc:182: EXPECT_EQ(0,
RunScriptExtractDouble("getRtt()"));
On 2017/05/31 17:44:22, jkarlin wrote:
> Doesn't rtt return an unsigned long long?

Done.

https://codereview.chromium.org/2912013002/diff/80001/content/browser/net_inf...
content/browser/net_info_browsertest.cc:183: EXPECT_EQ(0,
static_cast<int>(RunScriptExtractDouble("getRtt()")) % 25);
On 2017/05/31 17:44:22, jkarlin wrote:
> ditto, and everywhere else in this file.

Done.

[commit-bot: I haz the power, 2017-06-01 20:42:22]

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-06-01 20:42:24]

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

[commit-bot: I haz the power, 2017-06-01 20:42:25]

Dry run: CQ cannot get SignCLA result. Please try later.

[tbansal1, 2017-06-01 20:43:18]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2017-06-01 20:43:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, jkarlin@chromium.org
Link to the patchset: https://codereview.chromium.org/2912013002/#ps120001
(title: "jkarlin comments")

[commit-bot: I haz the power, 2017-06-01 20:44:04]

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-06-01 20:53:36]

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

[commit-bot: I haz the power, 2017-06-01 20:53:37]

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

[tbansal1, 2017-06-01 20:54:57]

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

[commit-bot: I haz the power, 2017-06-01 20:55:39]

Dry run: CQ is trying da patch.

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

[tbansal1, 2017-06-01 21:06:58]

Patchset #2 (id:120001) has been deleted

[tbansal1, 2017-06-01 21:08:02]

tbansal@chromium.org changed reviewers:
+ nasko@chromium.org

[tbansal1, 2017-06-01 21:08:03]

nasko: ptal content/browser/net_info_browsertest.cc. Thanks.

[tbansal1, 2017-06-01 21:08:08]

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

[commit-bot: I haz the power, 2017-06-01 21:08:42]

Dry run: CQ is trying da patch.

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

[nasko, 2017-06-01 22:16:13]

https://codereview.chromium.org/2912013002/diff/140001/content/browser/net_in...
File content/browser/net_info_browsertest.cc (right):

https://codereview.chromium.org/2912013002/diff/140001/content/browser/net_in...
content/browser/net_info_browsertest.cc:102:
EXPECT_TRUE(base::StringToInt(RunScriptExtractString(script), &data));
ExecuteScriptAndExtractInt?

https://codereview.chromium.org/2912013002/diff/140001/content/browser/net_in...
content/browser/net_info_browsertest.cc:188: EXPECT_EQ(0,
RunScriptExtractInt("getRtt()"));
Why can't ExecuteScriptAndExtractInt be used?

https://codereview.chromium.org/2912013002/diff/140001/content/browser/net_in...
content/browser/net_info_browsertest.cc:191: double downlink_mbps =
RunScriptExtractDouble("getDownlink()");
Similarly ExecuteScriptAndExtractDouble?

https://codereview.chromium.org/2912013002/diff/140001/content/browser/net_in...
content/browser/net_info_browsertest.cc:198:
EXPECT_TRUE(static_cast<int>(downlink_mbps * 1000) % 25 == 0 ||
Shouldn't we be dividing the mbps to get kbps instead of multiplying it to get
gbps?

https://codereview.chromium.org/2912013002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/netinfo/NetworkInformation.cpp (right):

https://codereview.chromium.org/2912013002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/netinfo/NetworkInformation.cpp:83:
downlink_kbps = (std::numeric_limits<double>::max());
nit: No need for () around std::numeric?

[tbansal1, 2017-06-01 23:06:20]

Patchset #3 (id:160001) has been deleted

[tbansal1, 2017-06-01 23:26:47]

nasko: ptal. Thanks.

https://codereview.chromium.org/2912013002/diff/140001/content/browser/net_in...
File content/browser/net_info_browsertest.cc (right):

https://codereview.chromium.org/2912013002/diff/140001/content/browser/net_in...
content/browser/net_info_browsertest.cc:102:
EXPECT_TRUE(base::StringToInt(RunScriptExtractString(script), &data));
On 2017/06/01 22:16:13, nasko (slow) wrote:
> ExecuteScriptAndExtractInt?

Done.

https://codereview.chromium.org/2912013002/diff/140001/content/browser/net_in...
content/browser/net_info_browsertest.cc:188: EXPECT_EQ(0,
RunScriptExtractInt("getRtt()"));
On 2017/06/01 22:16:12, nasko (slow) wrote:
> Why can't ExecuteScriptAndExtractInt be used?

Can be. RunScriptExtractInt() is slightly easier to use since it directly
returns the int value.

https://codereview.chromium.org/2912013002/diff/140001/content/browser/net_in...
content/browser/net_info_browsertest.cc:191: double downlink_mbps =
RunScriptExtractDouble("getDownlink()");
On 2017/06/01 22:16:13, nasko (slow) wrote:
> Similarly ExecuteScriptAndExtractDouble?

Same as above. Helper method RunScriptExtractDouble() makes the code slightly
more readable here.

https://codereview.chromium.org/2912013002/diff/140001/content/browser/net_in...
content/browser/net_info_browsertest.cc:198:
EXPECT_TRUE(static_cast<int>(downlink_mbps * 1000) % 25 == 0 ||
On 2017/06/01 22:16:12, nasko (slow) wrote:
> Shouldn't we be dividing the mbps to get kbps instead of multiplying it to get
> gbps?

nope, multiplying it by 1000 would give kbps.
We want to verify if getDownlink() is a multiple of 0.025 mbps (i.e., 25 kbps).
So, e.g., if getDownlink() returns 1.275, then we multiple by 1000 and then take
mod 25.

[tbansal1, 2017-06-01 23:26:51]

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

[commit-bot: I haz the power, 2017-06-01 23:27:40]

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-06-02 04:26:15]

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

[commit-bot: I haz the power, 2017-06-02 04:26:16]

Dry run: This issue passed the CQ dry run.

[nasko, 2017-06-03 00:09:47]

https://codereview.chromium.org/2912013002/diff/140001/content/browser/net_in...
File content/browser/net_info_browsertest.cc (right):

https://codereview.chromium.org/2912013002/diff/140001/content/browser/net_in...
content/browser/net_info_browsertest.cc:198:
EXPECT_TRUE(static_cast<int>(downlink_mbps * 1000) % 25 == 0 ||
On 2017/06/01 23:26:46, tbansal1 wrote:
> On 2017/06/01 22:16:12, nasko (slow) wrote:
> > Shouldn't we be dividing the mbps to get kbps instead of multiplying it to
get
> > gbps?
> 
> nope, multiplying it by 1000 would give kbps.

Well, when you have mbps and multiply by 1000, you get gbps, don't you? Maybe we
are using different terminology.

> We want to verify if getDownlink() is a multiple of 0.025 mbps (i.e., 25
kbps).
> So, e.g., if getDownlink() returns 1.275, then we multiple by 1000 and then
take
> mod 25.

That statement will always be true though. X * 1000 % 25 is guaranteed to be 0.
It is so, be cause it can be represented as (X * 40 * 25) % 25. Any multiple of
25 will be 0 mod 25.

[tbansal1, 2017-06-04 11:48:13]

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

[commit-bot: I haz the power, 2017-06-04 11:48:19]

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-06-04 13:19:02]

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

[commit-bot: I haz the power, 2017-06-04 13:19:03]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[tbansal1, 2017-06-04 20:40:05]

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

[commit-bot: I haz the power, 2017-06-04 20:40:13]

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-06-04 21:11:18]

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

[commit-bot: I haz the power, 2017-06-04 21:11:19]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-06-05 13:52:20]

nasko: ptal. Thanks.

https://codereview.chromium.org/2912013002/diff/140001/content/browser/net_in...
File content/browser/net_info_browsertest.cc (right):

https://codereview.chromium.org/2912013002/diff/140001/content/browser/net_in...
content/browser/net_info_browsertest.cc:198:
EXPECT_TRUE(static_cast<int>(downlink_mbps * 1000) % 25 == 0 ||
On 2017/06/03 00:09:47, nasko (slow) wrote:
> On 2017/06/01 23:26:46, tbansal1 wrote:
> > On 2017/06/01 22:16:12, nasko (slow) wrote:
> > > Shouldn't we be dividing the mbps to get kbps instead of multiplying it to
> get
> > > gbps?
> > 
> > nope, multiplying it by 1000 would give kbps.
We probably are.
|downlink_mbps| is a double. Let's say calling the netinfo API returns that
bandwidth is 1.23456 mbps. 
So, |downlink_mbps| would be set to 1.23456. If the caller wants to know the
speed in kbps, it needs to multiple |downlink_mbps| by 1000, which will give us
1234.56 kbps (same as 1.23456 mbps).
> 
> Well, when you have mbps and multiply by 1000, you get gbps, don't you? Maybe
we
> are using different terminology.
> 
> > We want to verify if getDownlink() is a multiple of 0.025 mbps (i.e., 25
> kbps).
> > So, e.g., if getDownlink() returns 1.275, then we multiple by 1000 and then
> take
> > mod 25.
> 
> That statement will always be true though. X * 1000 % 25 is guaranteed to be
0.
Right but only if X was a int which is not the case here. Here, X is a double.
e.g., if X is 1.23456, then 
static_cast<int>(X * 1000)  will be 1234 which is not a multiple of 25.
> It is so, be cause it can be represented as (X * 40 * 25) % 25. Any multiple
of
> 25 will be 0 mod 25.

Let me know if you have suggestions for improving the readability. Sorry for the
confusion.

[nasko, 2017-06-05 23:43:01]

LGTM

https://codereview.chromium.org/2912013002/diff/140001/content/browser/net_in...
File content/browser/net_info_browsertest.cc (right):

https://codereview.chromium.org/2912013002/diff/140001/content/browser/net_in...
content/browser/net_info_browsertest.cc:198:
EXPECT_TRUE(static_cast<int>(downlink_mbps * 1000) % 25 == 0 ||
On 2017/06/05 13:52:20, tbansal1 wrote:
> On 2017/06/03 00:09:47, nasko (slow) wrote:
> > On 2017/06/01 23:26:46, tbansal1 wrote:
> > > On 2017/06/01 22:16:12, nasko (slow) wrote:
> > > > Shouldn't we be dividing the mbps to get kbps instead of multiplying it
to
> > get
> > > > gbps?
> > > 
> > > nope, multiplying it by 1000 would give kbps.
> We probably are.
> |downlink_mbps| is a double. Let's say calling the netinfo API returns that
> bandwidth is 1.23456 mbps. 
> So, |downlink_mbps| would be set to 1.23456. If the caller wants to know the
> speed in kbps, it needs to multiple |downlink_mbps| by 1000, which will give
us
> 1234.56 kbps (same as 1.23456 mbps).
> > 
> > Well, when you have mbps and multiply by 1000, you get gbps, don't you?
Maybe
> we
> > are using different terminology.
> > 
> > > We want to verify if getDownlink() is a multiple of 0.025 mbps (i.e., 25
> > kbps).
> > > So, e.g., if getDownlink() returns 1.275, then we multiple by 1000 and
then
> > take
> > > mod 25.
> > 
> > That statement will always be true though. X * 1000 % 25 is guaranteed to be
> 0.
> Right but only if X was a int which is not the case here. Here, X is a double.
> e.g., if X is 1.23456, then 
> static_cast<int>(X * 1000)  will be 1234 which is not a multiple of 25.
> > It is so, be cause it can be represented as (X * 40 * 25) % 25. Any multiple
> of
> > 25 will be 0 mod 25.
> 
> Let me know if you have suggestions for improving the readability. Sorry for
the
> confusion.

Thanks for the explanation. If you were to only multiply the fractional part, it
probably would've been a bit more clear, as 1mb is always multiple of 25kb.
Also, including in a comment an example is always helpful to see.

[tbansal1, 2017-06-06 05:14:20]

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

[commit-bot: I haz the power, 2017-06-06 05:14: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-06-06 07:24:39]

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

[commit-bot: I haz the power, 2017-06-06 07:24:40]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-06-06 07:26:58]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2017-06-06 07:26:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, jkarlin@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2912013002/#ps200001
(title: "Rebased, nasko comments")

[commit-bot: I haz the power, 2017-06-06 07:27:14]

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-06-06 07:31:42]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1496734018208160,
"parent_rev": "429f40de51f53800ddbb1e9180a16878a0e098dc", "commit_rev":
"420ae3445c9b1530ba81ac651966d01548c04d09"}

[commit-bot: I haz the power, 2017-06-06 07:31:56]

Description was changed from

==========
NetInfo: Slight update to the default network quality value

This CL changes the default values of |downlink| to be a multiple of
25 kbps.

BUG=727786
==========

to

==========
NetInfo: Slight update to the default network quality value

This CL changes the default values of |downlink| to be a multiple of
25 kbps.

BUG=727786

Review-Url: https://codereview.chromium.org/2912013002
Cr-Commit-Position: refs/heads/master@{#477227}
Committed:
https://chromium.googlesource.com/chromium/src/+/420ae3445c9b1530ba81ac651966...
==========

[commit-bot: I haz the power, 2017-06-06 07:31:58]

Committed patchset #4 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/420ae3445c9b1530ba81ac651966...