[NetInfo] Browser changes to support connection.downlinkMax

[NetInfo] Browser changes to support connection.downlinkMax

Patch 1) https://crrev.com/1308943005
Patch 2) https://crrev.com/1306423004 (this)
Patch 3) https://crrev.com/1309903006

Also depends on https://codereview.chromium.org/1306653003/

BUG=412741
Committed: https://crrev.com/300a1d28b57bc0d49603c97828f1833f577bd46a
Cr-Commit-Position: refs/heads/master@{#349739}

[jkarlin, 2015-09-04 18:53:38]

Patchset #2 (id:20001) has been deleted

[jkarlin, 2015-09-04 18:53:44]

Patchset #2 (id:40001) has been deleted

[jkarlin, 2015-09-04 18:53:49]

Patchset #2 (id:20002) has been deleted

[jkarlin, 2015-09-04 18:53:55]

Patchset #2 (id:70001) has been deleted

[jkarlin, 2015-09-04 18:54:00]

Patchset #2 (id:90001) has been deleted

[jkarlin, 2015-09-04 18:54:04]

Patchset #2 (id:110001) has been deleted

[jkarlin, 2015-09-04 18:54:09]

Patchset #2 (id:80024) has been deleted

[jkarlin, 2015-09-04 18:54:17]

Patchset #1 (id:1) has been deleted

[jkarlin, 2015-09-08 15:11:50]

Paul: PTAL as this CL is largely about using the NetworkChangeNotifier.

[jkarlin, 2015-09-08 15:12:33]

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

[jkarlin, 2015-09-08 15:12:34]

(It helps to add the reviewer before sending the message).

Paul, PTAL as this is largely NetworkChangeNotifier related.

[jkarlin, 2015-09-09 16:16:19]

pauljensen@: Gentle ping

[pauljensen, 2015-09-09 16:21:23]

On 2015/09/09 16:16:19, jkarlin wrote:
> pauljensen@: Gentle ping

I was on bug triage duty last Thurs, Fri. Vacation Mon.  Build sheriff Tues,
Wed.  So my review backlog is huge and this CL is in the end of the queue :(

[jkarlin, 2015-09-09 17:06:32]

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

[jkarlin, 2015-09-09 17:06:32]

pauljensen@: Understood, thanks. Just trying to get things in by feature freeze.

adamk@: PTAL, thanks!

[pauljensen, 2015-09-11 14:25:37]

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

https://codereview.chromium.org/1306423004/diff/170001/content/browser/net_in...
content/browser/net_info_browsertest.cc:61: }
Now that you unprotected NCN::GetMaxBandwidthForConnectionSubType(), this
function seems a little useless.

https://codereview.chromium.org/1306423004/diff/170001/content/browser/render...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/1306423004/diff/170001/content/browser/render...
content/browser/renderer_host/render_view_host_impl.cc:500:
net::NetworkChangeNotifier::GetMaxBandwidth();
This looks racy.  Connection type could have changed before the GetMaxBandwidth
call.

https://codereview.chromium.org/1306423004/diff/170001/content/public/common/...
File content/public/common/web_preferences.cc (right):

https://codereview.chromium.org/1306423004/diff/170001/content/public/common/...
content/public/common/web_preferences.cc:101: net_info_max_bandwidth_mbps(0.0),
might want to replace 0.0 with
GetMaxBandWidthForConnectionType(net_info_connection_type)

https://codereview.chromium.org/1306423004/diff/170001/net/base/network_chang...
File net/base/network_change_notifier.h (right):

https://codereview.chromium.org/1306423004/diff/170001/net/base/network_chang...
net/base/network_change_notifier.h:247: // the NetInfo spec:
http://w3c.github.io/netinfo/.
Comment doesn't mention units, neither does function name.

[jkarlin, 2015-09-11 20:15:21]

Thanks.

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

https://codereview.chromium.org/1306423004/diff/170001/content/browser/net_in...
content/browser/net_info_browsertest.cc:61: }
On 2015/09/11 14:25:37, pauljensen wrote:
> Now that you unprotected NCN::GetMaxBandwidthForConnectionSubType(), this
> function seems a little useless.

Done.

https://codereview.chromium.org/1306423004/diff/170001/content/browser/render...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/1306423004/diff/170001/content/browser/render...
content/browser/renderer_host/render_view_host_impl.cc:500:
net::NetworkChangeNotifier::GetMaxBandwidth();
On 2015/09/11 14:25:37, pauljensen wrote:
> This looks racy.  Connection type could have changed before the
GetMaxBandwidth
> call.

Ugh, I forgot NCN was thread safe. Okay, changed GetMaxBandwidth to supply both
values in the upstream CL and used it here.

https://codereview.chromium.org/1306423004/diff/170001/content/public/common/...
File content/public/common/web_preferences.cc (right):

https://codereview.chromium.org/1306423004/diff/170001/content/public/common/...
content/public/common/web_preferences.cc:101: net_info_max_bandwidth_mbps(0.0),
On 2015/09/11 14:25:37, pauljensen wrote:
> might want to replace 0.0 with
> GetMaxBandWidthForConnectionType(net_info_connection_type)

Done.

https://codereview.chromium.org/1306423004/diff/170001/net/base/network_chang...
File net/base/network_change_notifier.h (right):

https://codereview.chromium.org/1306423004/diff/170001/net/base/network_chang...
net/base/network_change_notifier.h:247: // the NetInfo spec:
http://w3c.github.io/netinfo/.
On 2015/09/11 14:25:37, pauljensen wrote:
> Comment doesn't mention units, neither does function name.

Done.

[pauljensen, 2015-09-15 17:39:24]

lgtm

https://codereview.chromium.org/1306423004/diff/250001/net/base/network_chang...
File net/base/network_change_notifier.h (right):

https://codereview.chromium.org/1306423004/diff/250001/net/base/network_chang...
net/base/network_change_notifier.h:246: // Returns a theoretical upper limit (in
mbps) on download bandwidth given a
mbps->Mbps

[jkarlin, 2015-09-15 17:51:49]

Thanks. adamk PTAL!

https://codereview.chromium.org/1306423004/diff/250001/net/base/network_chang...
File net/base/network_change_notifier.h (right):

https://codereview.chromium.org/1306423004/diff/250001/net/base/network_chang...
net/base/network_change_notifier.h:246: // Returns a theoretical upper limit (in
mbps) on download bandwidth given a
On 2015/09/15 17:39:24, pauljensen wrote:
> mbps->Mbps

Hmm, I believe they're the same. grep wars show 38 files with mbps and 69 with
Mbps.

[pauljensen, 2015-09-15 17:58:32]

https://codereview.chromium.org/1306423004/diff/250001/net/base/network_chang...
File net/base/network_change_notifier.h (right):

https://codereview.chromium.org/1306423004/diff/250001/net/base/network_chang...
net/base/network_change_notifier.h:246: // Returns a theoretical upper limit (in
mbps) on download bandwidth given a
On 2015/09/15 17:51:49, jkarlin wrote:
> On 2015/09/15 17:39:24, pauljensen wrote:
> > mbps->Mbps
> 
> Hmm, I believe they're the same. grep wars show 38 files with mbps and 69 with
> Mbps. 

Should be M.  m=milli M=mega
https://en.wikipedia.org/wiki/Metric_prefix
You might want to use Mibit/s to be more accurate.
https://en.wikipedia.org/wiki/Data_rate_units#Mebibit_per_second

[jkarlin, 2015-09-15 18:07:19]

https://codereview.chromium.org/1306423004/diff/250001/net/base/network_chang...
File net/base/network_change_notifier.h (right):

https://codereview.chromium.org/1306423004/diff/250001/net/base/network_chang...
net/base/network_change_notifier.h:246: // Returns a theoretical upper limit (in
mbps) on download bandwidth given a
On 2015/09/15 17:58:32, pauljensen wrote:
> On 2015/09/15 17:51:49, jkarlin wrote:
> > On 2015/09/15 17:39:24, pauljensen wrote:
> > > mbps->Mbps
> > 
> > Hmm, I believe they're the same. grep wars show 38 files with mbps and 69
with
> > Mbps. 
> 
> Should be M.  m=milli M=mega
> https://en.wikipedia.org/wiki/Metric_prefix
> You might want to use Mibit/s to be more accurate.
> https://en.wikipedia.org/wiki/Data_rate_units#Mebibit_per_second

Ah the elusive millibit. Fine fine. Done.

[pauljensen, 2015-09-15 18:08:30]

lgtm

[adamk, 2015-09-15 18:48:24]

I'm happy to take a look but I'm not an OWNER in any of this code. Given that
most of the stuff I'd look at is plumbing anyway, perhaps we could find a
content/OWNER to look at this now?

[jkarlin, 2015-09-15 18:55:07]

jkarlin@chromium.org changed reviewers:
+ jochen@chromium.org
- adamk@chromium.org

[jkarlin, 2015-09-15 18:55:08]

-r adamk by request 

jochen@chromium.org: Please review content/ changes for OWNERS which are
primarily plumbing changes.

[jochen (gone - plz use gerrit), 2015-09-16 11:26:18]

content lgtm

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

https://codereview.chromium.org/1306423004/diff/290001/content/browser/net_in...
content/browser/net_info_browsertest.cc:18: using net::NetworkChangeNotifier;
please no using statements

https://codereview.chromium.org/1306423004/diff/290001/net/base/network_chang...
File net/base/network_change_notifier.h (right):

https://codereview.chromium.org/1306423004/diff/290001/net/base/network_chang...
net/base/network_change_notifier.h:249: static double
GetMaxBandwidthForConnectionSubtype(ConnectionSubtype subtype);
What about adding the unit (Mbps) to the function name?

[jkarlin, 2015-09-16 12:09:36]

Thanks!

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

https://codereview.chromium.org/1306423004/diff/290001/content/browser/net_in...
content/browser/net_info_browsertest.cc:18: using net::NetworkChangeNotifier;
On 2015/09/16 11:26:18, jochen wrote:
> please no using statements

Done.

https://codereview.chromium.org/1306423004/diff/290001/net/base/network_chang...
File net/base/network_change_notifier.h (right):

https://codereview.chromium.org/1306423004/diff/290001/net/base/network_chang...
net/base/network_change_notifier.h:249: static double
GetMaxBandwidthForConnectionSubtype(ConnectionSubtype subtype);
On 2015/09/16 11:26:18, jochen wrote:
> What about adding the unit (Mbps) to the function name?

Good idea, but in a different CL. This is a pre-existing function that I made
public in this CL. TODO added.

[jkarlin, 2015-09-18 15:17:45]

The CQ bit was checked by jkarlin@chromium.org

[jkarlin, 2015-09-18 15:17:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
pauljensen@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1306423004/#ps350001
(title: "Address comments from PS9")

[commit-bot: I haz the power, 2015-09-18 15:18:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1306423004/350001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1306423004/350001

[commit-bot: I haz the power, 2015-09-18 15:27:47]

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

[commit-bot: I haz the power, 2015-09-18 15:27:48]

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

[jkarlin, 2015-09-18 15:34:53]

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

[jkarlin, 2015-09-18 15:34:54]

tsepez@chromium.org: Please review changes in view_messages.h and
common_param_traits_macros.h. Thank you!

[Tom Sepez, 2015-09-18 15:47:33]

Messages LGTM.

[jkarlin, 2015-09-18 15:49:00]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2015-09-18 15:49:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1306423004/350001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1306423004/350001

[commit-bot: I haz the power, 2015-09-18 17:17:49]

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

[commit-bot: I haz the power, 2015-09-18 17:17:50]

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

[jkarlin, 2015-09-18 18:16:57]

The CQ bit was checked by jkarlin@chromium.org

[jkarlin, 2015-09-18 18:16:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
pauljensen@chromium.org, tsepez@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1306423004/#ps370001
(title: "Minor browser test fix")

[commit-bot: I haz the power, 2015-09-18 18:18:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1306423004/370001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1306423004/370001

[commit-bot: I haz the power, 2015-09-18 19:32:59]

Committed patchset #13 (id:370001)

[commit-bot: I haz the power, 2015-09-18 19:34:01]

Patchset 13 (id:??) landed as
https://crrev.com/300a1d28b57bc0d49603c97828f1833f577bd46a
Cr-Commit-Position: refs/heads/master@{#349739}