Use the provided connection type to determine if online.

Use the provided connection type to determine if online.

Calling IsOffline() could race and return an answer for a different type.

BUG=381282
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276050

[jkarlin, 2014-06-05 19:29:06]

Thanks!

[erikwright (departed), 2014-06-05 19:37:50]

It looks like wtc@ might be more appropriate. I'm only really an owner for
cookie stuff.

[jkarlin, 2014-06-06 16:38:43]

On 2014/06/05 19:37:50, erikwright wrote:
> It looks like wtc@ might be more appropriate. I'm only really an owner for
> cookie stuff.

Adding pauljensen@ as he is familiar with this type of bug.

[pauljensen, 2014-06-06 17:32:59]

Why calculate is_online here?  why not just pass the type (which implies
online/offline-ness).

[jkarlin, 2014-06-06 18:02:48]

On 2014/06/06 17:32:59, pauljensen wrote:
> Why calculate is_online here?  why not just pass the type (which implies
> online/offline-ness).

Done.  I was going to wait for another CL for that but this one is fine.

[pauljensen, 2014-06-06 18:29:55]

lgtm

[jkarlin, 2014-06-06 18:52:08]

tsepez: Please review view_messages.h
jochen: Please review all for OWNERS

[Tom Sepez, 2014-06-06 19:09:19]

Messages LGTM.

https://codereview.chromium.org/317933002/diff/20001/content/renderer/render_...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/317933002/diff/20001/content/renderer/render_...
content/renderer/render_thread_impl.cc:1360: bool online = type !=
net::NetworkChangeNotifier::CONNECTION_NONE;
nit: I wish that the net:: header that defined these constants would include a
convenience function to tell whether the given value represents an offline state
(in case they get changed in the future).  Then you're code wouldn't break ...

[wtc, 2014-06-06 20:00:05]

Patch set 2 LGTM.

https://codereview.chromium.org/317933002/diff/20001/content/common/view_mess...
File content/common/view_messages.h (right):

https://codereview.chromium.org/317933002/diff/20001/content/common/view_mess...
content/common/view_messages.h:841:
IPC_MESSAGE_CONTROL1(ViewMsg_NetworkStateChanged,

Nit: should this message also be renamed ViewMsg_NetworkTypeChanged?

https://codereview.chromium.org/317933002/diff/20001/content/renderer/render_...
File content/renderer/render_thread_impl.h (right):

https://codereview.chromium.org/317933002/diff/20001/content/renderer/render_...
content/renderer/render_thread_impl.h:436: void
OnNetworkStateChanged(net::NetworkChangeNotifier::ConnectionType type);

Nit: should this method be renamed OnNetworkTypeChanged? Or do you want to keep
the current, more general name?

[jkarlin, 2014-06-09 11:25:44]

Note, new file to review: render_view_host_impl.cc

https://codereview.chromium.org/317933002/diff/20001/content/common/view_mess...
File content/common/view_messages.h (right):

https://codereview.chromium.org/317933002/diff/20001/content/common/view_mess...
content/common/view_messages.h:841:
IPC_MESSAGE_CONTROL1(ViewMsg_NetworkStateChanged,
On 2014/06/06 20:00:05, wtc wrote:
> 
> Nit: should this message also be renamed ViewMsg_NetworkTypeChanged?

Done.

https://codereview.chromium.org/317933002/diff/20001/content/renderer/render_...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/317933002/diff/20001/content/renderer/render_...
content/renderer/render_thread_impl.cc:1360: bool online = type !=
net::NetworkChangeNotifier::CONNECTION_NONE;
On 2014/06/06 19:09:19, Tom Sepez wrote:
> nit: I wish that the net:: header that defined these constants would include a
> convenience function to tell whether the given value represents an offline
state
> (in case they get changed in the future).  Then you're code wouldn't break ...

Agree.  For another CL though.

https://codereview.chromium.org/317933002/diff/20001/content/renderer/render_...
File content/renderer/render_thread_impl.h (right):

https://codereview.chromium.org/317933002/diff/20001/content/renderer/render_...
content/renderer/render_thread_impl.h:436: void
OnNetworkStateChanged(net::NetworkChangeNotifier::ConnectionType type);
On 2014/06/06 20:00:05, wtc wrote:
> 
> Nit: should this method be renamed OnNetworkTypeChanged? Or do you want to
keep
> the current, more general name?

Done.

[wtc, 2014-06-09 17:11:46]

Patch set 3 LGTM.

[jkarlin, 2014-06-09 17:30:48]

jam: PTAL for OWNERS of content/

[jam, 2014-06-09 22:54:04]

lgtm

[jkarlin, 2014-06-10 10:42:04]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2014-06-10 10:42:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jkarlin@chromium.org/317933002/40001

[commit-bot: I haz the power, 2014-06-10 16:12:25]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)

[commit-bot: I haz the power, 2014-06-10 16:27:30]

Change committed as 276050