Adds type information to the NetworkStateNotifier. Also allows for registration of observers.

Adds type members and accessors to the NetworkStateNotifier.  Also allows for registration of thread-safe observers for said type information so that both workers and windows can receive notification.

This is part of adding NetInfo v3 to Chrome, and this code is used by https://codereview.chromium.org/291203002.

Design doc: https://docs.google.com/a/chromium.org/document/d/1LTk9uVMGi4kurzcF5ellsAJReTF31fFJMHrQwSVtBjc/ 

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

[jkarlin, 2014-05-20 14:32:58]

Adamk: Please review everything, thanks!

[jkarlin, 2014-05-20 14:43:53]

Darin: Please review the public/ files.

[adamk, 2014-05-20 15:00:36]

There are a variety of comments below, but I think the main issue with this
patch is making sure the lifetimes are clear and safe. Most especially, how are
the ExecutionContexts guaranteed to stay alive?

https://codereview.chromium.org/289333003/diff/60001/Source/core/page/Network...
File Source/core/page/NetworkStateNotifier.cpp (right):

https://codereview.chromium.org/289333003/diff/60001/Source/core/page/Network...
Source/core/page/NetworkStateNotifier.cpp:69: for (ObserverListMap::iterator it
= m_observers.begin(); it != m_observers.end(); ++it) {
What guarantees that the ExecutionContexts in this map are still alive when this
code runs?

https://codereview.chromium.org/289333003/diff/60001/Source/core/page/Network...
Source/core/page/NetworkStateNotifier.cpp:127: for (size_t i = 0; i <
observers->size(); ++i) {
Vector has a find() method (returning kNotFound if the item is not found) you
can use instead of this loop.

https://codereview.chromium.org/289333003/diff/60001/Source/core/page/Network...
File Source/core/page/NetworkStateNotifier.h (left):

https://codereview.chromium.org/289333003/diff/60001/Source/core/page/Network...
Source/core/page/NetworkStateNotifier.h:55: 
Nit: I'd leave this blank line.

https://codereview.chromium.org/289333003/diff/60001/Source/core/page/Network...
File Source/core/page/NetworkStateNotifier.h (right):

https://codereview.chromium.org/289333003/diff/60001/Source/core/page/Network...
Source/core/page/NetworkStateNotifier.h:47:
blink::WebNetworkConnection::ConnectionType) = 0;
Nit: Don't break lines like this in Blink; there's no line length limit, so
there's no need to break unless a line gets crazy long.

https://codereview.chromium.org/289333003/diff/60001/Source/core/page/Network...
Source/core/page/NetworkStateNotifier.h:50: typedef NetworkStateObserver*
ObserverType;
I found this typedef a bit confusing while reading the code, perhaps remove it?

https://codereview.chromium.org/289333003/diff/60001/Source/core/page/Network...
Source/core/page/NetworkStateNotifier.h:101: }
and maybe add a "// namespace WebCore" comment

https://codereview.chromium.org/289333003/diff/60001/Source/web/WebNetworkSta...
File Source/web/WebNetworkStateNotifier.cpp (right):

https://codereview.chromium.org/289333003/diff/60001/Source/web/WebNetworkSta...
Source/web/WebNetworkStateNotifier.cpp:42:
networkStateNotifier().setOnLine(onLine);
Do you plan to unify the setOnLine bit with the WebConnectionType bit? It
strikes me that "online" is probably something you can determine if you know the
connection type.

https://codereview.chromium.org/289333003/diff/60001/public/platform/WebNetwo...
File public/platform/WebNetworkConnection.h (right):

https://codereview.chromium.org/289333003/diff/60001/public/platform/WebNetwo...
public/platform/WebNetworkConnection.h:2: * Copyright (C) 2014 Google Inc. All
rights reserved.
New files should use the short version of the copyright header (see, e.g.,
WebBatteryStatus.h)

https://codereview.chromium.org/289333003/diff/60001/public/platform/WebNetwo...
public/platform/WebNetworkConnection.h:35: #include "WebString.h"
Not clear why you need WebString here

https://codereview.chromium.org/289333003/diff/60001/public/platform/WebNetwo...
public/platform/WebNetworkConnection.h:39: struct WebNetworkConnection {
Can you add a comment here pointing at the spec defining these types? I can
imagine someone not realizing that these correspond 1:1 with the spec.

https://codereview.chromium.org/289333003/diff/60001/public/platform/WebNetwo...
public/platform/WebNetworkConnection.h:40: 
Nit: remove blank line

[adamk, 2014-05-20 15:04:29]

Ah, looking at https://codereview.chromium.org/291203002 I have a better
understanding...it looks like the observers are all ActiveDOMObjects, and the
ActiveDOMObject's hasPendingActivity() is guaranteed to keep themselves (and
thus their contexts) alive until stop() is called...it seems like it would be
nice to have more guarantees about that, but that seems like it might work ok.

[adamk, 2014-05-20 15:12:50]

https://codereview.chromium.org/289333003/diff/60001/Source/core/page/Network...
File Source/core/page/NetworkStateNotifier.cpp (right):

https://codereview.chromium.org/289333003/diff/60001/Source/core/page/Network...
Source/core/page/NetworkStateNotifier.cpp:83: MutexLocker locker(m_mutex);
I think you need to lock for this entire method, else your observerList may
become invalid if m_observers moves it around in memory while you're iterating
(this could happen if another thread calls addObserver or removeObserver).

But locking this whole methods doesn't seem ideal. If you instead make the
values in m_observers OwnPtr<Vector<NetworkStateObserver*>>, then you should be
guaranteed that the pointer won't change while you're on the context thread.

[jkarlin, 2014-05-20 16:20:22]

https://codereview.chromium.org/289333003/diff/60001/Source/core/page/Network...
File Source/core/page/NetworkStateNotifier.cpp (right):

https://codereview.chromium.org/289333003/diff/60001/Source/core/page/Network...
Source/core/page/NetworkStateNotifier.cpp:69: for (ObserverListMap::iterator it
= m_observers.begin(); it != m_observers.end(); ++it) {
On 2014/05/20 15:00:36, adamk wrote:
> What guarantees that the ExecutionContexts in this map are still alive when
this
> code runs?

The observer must call removeObserver before the ExecutionContext dissappears. 
Since removeObserver is called on the ExecutionContext's thread this is safe. 
Added a comment to this effect.

https://codereview.chromium.org/289333003/diff/60001/Source/core/page/Network...
Source/core/page/NetworkStateNotifier.cpp:83: MutexLocker locker(m_mutex);
On 2014/05/20 15:12:50, adamk wrote:
> I think you need to lock for this entire method, else your observerList may
> become invalid if m_observers moves it around in memory while you're iterating
> (this could happen if another thread calls addObserver or removeObserver).
> 
> But locking this whole methods doesn't seem ideal. If you instead make the
> values in m_observers OwnPtr<Vector<NetworkStateObserver*>>, then you should
be
> guaranteed that the pointer won't change while you're on the context thread.

Nice catch, thanks. Made an alternate fix where the value of the hash table is
now a pointer.

https://codereview.chromium.org/289333003/diff/60001/Source/core/page/Network...
Source/core/page/NetworkStateNotifier.cpp:127: for (size_t i = 0; i <
observers->size(); ++i) {
On 2014/05/20 15:00:36, adamk wrote:
> Vector has a find() method (returning kNotFound if the item is not found) you
> can use instead of this loop.

Done.

https://codereview.chromium.org/289333003/diff/60001/Source/core/page/Network...
File Source/core/page/NetworkStateNotifier.h (right):

https://codereview.chromium.org/289333003/diff/60001/Source/core/page/Network...
Source/core/page/NetworkStateNotifier.h:47:
blink::WebNetworkConnection::ConnectionType) = 0;
On 2014/05/20 15:00:36, adamk wrote:
> Nit: Don't break lines like this in Blink; there's no line length limit, so
> there's no need to break unless a line gets crazy long.

Done.

https://codereview.chromium.org/289333003/diff/60001/Source/core/page/Network...
Source/core/page/NetworkStateNotifier.h:50: typedef NetworkStateObserver*
ObserverType;
On 2014/05/20 15:00:36, adamk wrote:
> I found this typedef a bit confusing while reading the code, perhaps remove
it?

Done.

https://codereview.chromium.org/289333003/diff/60001/Source/core/page/Network...
Source/core/page/NetworkStateNotifier.h:101: }
On 2014/05/20 15:00:36, adamk wrote:
> and maybe add a "// namespace WebCore" comment

Done.

https://codereview.chromium.org/289333003/diff/60001/Source/web/WebNetworkSta...
File Source/web/WebNetworkStateNotifier.cpp (right):

https://codereview.chromium.org/289333003/diff/60001/Source/web/WebNetworkSta...
Source/web/WebNetworkStateNotifier.cpp:42:
networkStateNotifier().setOnLine(onLine);
On 2014/05/20 15:00:36, adamk wrote:
> Do you plan to unify the setOnLine bit with the WebConnectionType bit? It
> strikes me that "online" is probably something you can determine if you know
the
> connection type.

Today we determine online by connection type, but perhaps not in the future. 
I'd like to leave them separate.

https://codereview.chromium.org/289333003/diff/60001/public/platform/WebNetwo...
File public/platform/WebNetworkConnection.h (right):

https://codereview.chromium.org/289333003/diff/60001/public/platform/WebNetwo...
public/platform/WebNetworkConnection.h:35: #include "WebString.h"
On 2014/05/20 15:00:36, adamk wrote:
> Not clear why you need WebString here

Done.

https://codereview.chromium.org/289333003/diff/60001/public/platform/WebNetwo...
public/platform/WebNetworkConnection.h:39: struct WebNetworkConnection {
On 2014/05/20 15:00:36, adamk wrote:
> Can you add a comment here pointing at the spec defining these types? I can
> imagine someone not realizing that these correspond 1:1 with the spec.

Done.

https://codereview.chromium.org/289333003/diff/60001/public/platform/WebNetwo...
public/platform/WebNetworkConnection.h:40: 
On 2014/05/20 15:00:36, adamk wrote:
> Nit: remove blank line

Done.

[jkarlin, 2014-05-20 18:26:19]

Please hold off on review until I make a change. I need to change the notifier
behavior to allow adding/removing observers during notification.

[jkarlin, 2014-05-20 20:56:27]

On 2014/05/20 18:26:19, jkarlin wrote:
> Please hold off on review until I make a change. I need to change the notifier
> behavior to allow adding/removing observers during notification.

Okay, this is done.  Please resume review.

[adamk, 2014-05-21 09:21:31]

https://codereview.chromium.org/289333003/diff/90001/Source/core/page/Network...
File Source/core/page/NetworkStateNotifier.cpp (right):

https://codereview.chromium.org/289333003/diff/90001/Source/core/page/Network...
Source/core/page/NetworkStateNotifier.cpp:41: deleteAllValues(m_observers);
This won't be needed if you use OwnPtrs as values.

https://codereview.chromium.org/289333003/diff/90001/Source/core/page/Network...
Source/core/page/NetworkStateNotifier.cpp:89: it = m_observers.find(context);
The idiomatic way to handle this song and dance in a WTF HashMap is:

ObserverListMap::AddResult result = m_observers.add(context, nullptr);
if (result.isNewEntry)
  result.storedValue->value = adoptPtr(new ObserverList);
result.storedValue->value->observers.append(observer);

(which does only a single hash lookup instead of the 3 here; I realize
addObserver is probably not performance sensitive, but being idiomatic is easier
to read for me)

https://codereview.chromium.org/289333003/diff/90001/Source/core/page/Network...
Source/core/page/NetworkStateNotifier.cpp:92:
it->value->observers.append(observer);
Maybe add an ASSERT that the observer isn't already in the list.

https://codereview.chromium.org/289333003/diff/90001/Source/core/page/Network...
Source/core/page/NetworkStateNotifier.cpp:112: collectZeroedObservers(context);
All this zeroed observers logic is pretty tricky. Is there a way to write
unittests for it?

https://codereview.chromium.org/289333003/diff/90001/Source/core/page/Network...
Source/core/page/NetworkStateNotifier.cpp:121: // The context could have been
removed before the notification task got to run.
If this happens then the ASSERT above could crash. In general, I think you have
to first do a lookup in m_observers before accessing context (unless it's passed
in addObserver or removeObserver, naturally).

https://codereview.chromium.org/289333003/diff/90001/Source/core/page/Network...
Source/core/page/NetworkStateNotifier.cpp:146: void
NetworkStateNotifier::collectZeroedObservers(ExecutionContext* context)
Please add a context thread assertion here.

https://codereview.chromium.org/289333003/diff/90001/Source/core/page/Network...
Source/core/page/NetworkStateNotifier.cpp:150: ObserverList* list =
lockAndFindObserverList(context);
Maybe the callers should just pass in the list?

https://codereview.chromium.org/289333003/diff/90001/Source/core/page/Network...
Source/core/page/NetworkStateNotifier.cpp:151: ASSERT(list);
I think you also want to ASSERT(!list->iterating)

https://codereview.chromium.org/289333003/diff/90001/Source/core/page/Network...
Source/core/page/NetworkStateNotifier.cpp:162: delete list;
This delete can also go away if you use OwnPtrs as values.

https://codereview.chromium.org/289333003/diff/90001/Source/core/page/Network...
File Source/core/page/NetworkStateNotifier.h (right):

https://codereview.chromium.org/289333003/diff/90001/Source/core/page/Network...
Source/core/page/NetworkStateNotifier.h:89: typedef HashMap<ExecutionContext*,
ObserverList*> ObserverListMap;
This should store OwnPtrs instead of raw pointers: then HashMap will handle the
deletion for you.

[jkarlin, 2014-05-21 14:23:17]

https://codereview.chromium.org/289333003/diff/90001/Source/core/page/Network...
File Source/core/page/NetworkStateNotifier.cpp (right):

https://codereview.chromium.org/289333003/diff/90001/Source/core/page/Network...
Source/core/page/NetworkStateNotifier.cpp:41: deleteAllValues(m_observers);
On 2014/05/21 09:21:31, adamk wrote:
> This won't be needed if you use OwnPtrs as values.

Done.

https://codereview.chromium.org/289333003/diff/90001/Source/core/page/Network...
Source/core/page/NetworkStateNotifier.cpp:89: it = m_observers.find(context);
On 2014/05/21 09:21:31, adamk wrote:
> The idiomatic way to handle this song and dance in a WTF HashMap is:
> 
> ObserverListMap::AddResult result = m_observers.add(context, nullptr);
> if (result.isNewEntry)
>   result.storedValue->value = adoptPtr(new ObserverList);
> result.storedValue->value->observers.append(observer);
> 
> (which does only a single hash lookup instead of the 3 here; I realize
> addObserver is probably not performance sensitive, but being idiomatic is
easier
> to read for me)

Done.

https://codereview.chromium.org/289333003/diff/90001/Source/core/page/Network...
Source/core/page/NetworkStateNotifier.cpp:92:
it->value->observers.append(observer);
On 2014/05/21 09:21:31, adamk wrote:
> Maybe add an ASSERT that the observer isn't already in the list.

Done.

https://codereview.chromium.org/289333003/diff/90001/Source/core/page/Network...
Source/core/page/NetworkStateNotifier.cpp:112: collectZeroedObservers(context);
On 2014/05/21 09:21:31, adamk wrote:
> All this zeroed observers logic is pretty tricky. Is there a way to write
> unittests for it?

Done.

https://codereview.chromium.org/289333003/diff/90001/Source/core/page/Network...
Source/core/page/NetworkStateNotifier.cpp:121: // The context could have been
removed before the notification task got to run.
On 2014/05/21 09:21:31, adamk wrote:
> If this happens then the ASSERT above could crash. In general, I think you
have
> to first do a lookup in m_observers before accessing context (unless it's
passed
> in addObserver or removeObserver, naturally).

Done.

https://codereview.chromium.org/289333003/diff/90001/Source/core/page/Network...
Source/core/page/NetworkStateNotifier.cpp:146: void
NetworkStateNotifier::collectZeroedObservers(ExecutionContext* context)
On 2014/05/21 09:21:31, adamk wrote:
> Please add a context thread assertion here.

Done.

https://codereview.chromium.org/289333003/diff/90001/Source/core/page/Network...
Source/core/page/NetworkStateNotifier.cpp:150: ObserverList* list =
lockAndFindObserverList(context);
On 2014/05/21 09:21:31, adamk wrote:
> Maybe the callers should just pass in the list?

Done.

https://codereview.chromium.org/289333003/diff/90001/Source/core/page/Network...
Source/core/page/NetworkStateNotifier.cpp:151: ASSERT(list);
On 2014/05/21 09:21:31, adamk wrote:
> I think you also want to ASSERT(!list->iterating)

Done.

https://codereview.chromium.org/289333003/diff/90001/Source/core/page/Network...
Source/core/page/NetworkStateNotifier.cpp:162: delete list;
On 2014/05/21 09:21:31, adamk wrote:
> This delete can also go away if you use OwnPtrs as values.

Done.

https://codereview.chromium.org/289333003/diff/90001/Source/core/page/Network...
File Source/core/page/NetworkStateNotifier.h (right):

https://codereview.chromium.org/289333003/diff/90001/Source/core/page/Network...
Source/core/page/NetworkStateNotifier.h:89: typedef HashMap<ExecutionContext*,
ObserverList*> ObserverListMap;
On 2014/05/21 09:21:31, adamk wrote:
> This should store OwnPtrs instead of raw pointers: then HashMap will handle
the
> deletion for you.

Done.

[adamk, 2014-05-21 16:06:04]

Looking good, I'm super-excited to see the C++ test.

https://codereview.chromium.org/289333003/diff/110001/Source/core/page/Networ...
File Source/core/page/NetworkStateNotifierTest.cpp (right):

https://codereview.chromium.org/289333003/diff/110001/Source/core/page/Networ...
Source/core/page/NetworkStateNotifierTest.cpp:94:
blink::Platform::current()->currentThread()->exitRunLoop();
Rather than having to override this method, could you instead just post an
exitRunLoop task to your thread before you call enterRunLoop down in
NetworkStateNotifierTest::setType?

[jkarlin, 2014-05-21 16:34:01]

https://codereview.chromium.org/289333003/diff/110001/Source/core/page/Networ...
File Source/core/page/NetworkStateNotifierTest.cpp (right):

https://codereview.chromium.org/289333003/diff/110001/Source/core/page/Networ...
Source/core/page/NetworkStateNotifierTest.cpp:94:
blink::Platform::current()->currentThread()->exitRunLoop();
On 2014/05/21 16:06:05, adamk wrote:
> Rather than having to override this method, could you instead just post an
> exitRunLoop task to your thread before you call enterRunLoop down in
> NetworkStateNotifierTest::setType?

Good idea.  Done.

[adamk, 2014-05-22 07:02:44]

lgtm, and sorry for the slow reviews (I'll be back in SFO next week).

But you'll need darin's review for public/

[jkarlin, 2014-05-22 10:57:22]

On 2014/05/22 07:02:44, adamk wrote:
> lgtm, and sorry for the slow reviews (I'll be back in SFO next week).
> 
> But you'll need darin's review for public/

Thanks for the great review

[jkarlin, 2014-05-22 10:58:24]

jochen or darin: Please review public/ files, thanks!

[jochen (gone - plz use gerrit), 2014-05-22 11:05:58]

https://codereview.chromium.org/289333003/diff/140001/public/platform/WebNetw...
File public/platform/WebNetworkConnection.h (right):

https://codereview.chromium.org/289333003/diff/140001/public/platform/WebNetw...
public/platform/WebNetworkConnection.h:38: struct WebNetworkConnection {
any reason you put the enum inside a struct?

https://codereview.chromium.org/289333003/diff/140001/public/platform/WebNetw...
public/platform/WebNetworkConnection.h:40: enum ConnectionType {
I'd name this WebConnectionType, or the method below setConnectionType

https://codereview.chromium.org/289333003/diff/140001/public/platform/WebNetw...
public/platform/WebNetworkConnection.h:41: Cellular = 0,
enums should start with the name of the enum, e.g. ConnectionTypeCellular

https://codereview.chromium.org/289333003/diff/140001/public/platform/WebNetw...
public/platform/WebNetworkConnection.h:46: None
if you intend this send this over IPC, you'll want a ConnectionTypeLast =
ConnectinoTypeNone here

[jkarlin, 2014-05-22 11:27:37]

https://codereview.chromium.org/289333003/diff/140001/public/platform/WebNetw...
File public/platform/WebNetworkConnection.h (right):

https://codereview.chromium.org/289333003/diff/140001/public/platform/WebNetw...
public/platform/WebNetworkConnection.h:38: struct WebNetworkConnection {
On 2014/05/22 11:05:59, jochen wrote:
> any reason you put the enum inside a struct?

Done.

https://codereview.chromium.org/289333003/diff/140001/public/platform/WebNetw...
public/platform/WebNetworkConnection.h:40: enum ConnectionType {
On 2014/05/22 11:05:59, jochen wrote:
> I'd name this WebConnectionType, or the method below setConnectionType

Done.

https://codereview.chromium.org/289333003/diff/140001/public/platform/WebNetw...
public/platform/WebNetworkConnection.h:41: Cellular = 0,
On 2014/05/22 11:05:59, jochen wrote:
> enums should start with the name of the enum, e.g. ConnectionTypeCellular

Done.

https://codereview.chromium.org/289333003/diff/140001/public/platform/WebNetw...
public/platform/WebNetworkConnection.h:46: None
On 2014/05/22 11:05:59, jochen wrote:
> if you intend this send this over IPC, you'll want a ConnectionTypeLast =
> ConnectinoTypeNone here

Done.

[jochen (gone - plz use gerrit), 2014-05-22 12:13:46]

lgtm

https://codereview.chromium.org/289333003/diff/70011/public/platform/WebConne...
File public/platform/WebConnectionType.h (right):

https://codereview.chromium.org/289333003/diff/70011/public/platform/WebConne...
public/platform/WebConnectionType.h:34: #include "WebCommon.h"
not required?

[jkarlin, 2014-05-22 12:20:35]

https://codereview.chromium.org/289333003/diff/70011/public/platform/WebConne...
File public/platform/WebConnectionType.h (right):

https://codereview.chromium.org/289333003/diff/70011/public/platform/WebConne...
public/platform/WebConnectionType.h:34: #include "WebCommon.h"
On 2014/05/22 12:13:47, jochen wrote:
> not required?

Done.

[jkarlin, 2014-05-22 12:20:42]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2014-05-22 12:21:40]

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

[jkarlin, 2014-05-22 13:23:16]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2014-05-22 13:23:40]

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

[commit-bot: I haz the power, 2014-05-23 02:57:34]

Change committed as 174640