Adds NetInfo v3 Web API to Blink.

Adds Netinfo v3 to Blink.

The layout changes outside of netinfo/ are regolding.

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

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

[jkarlin, 2014-05-22 17:53:50]

jochen: Please review everything. Thanks!

[jochen (gone - plz use gerrit), 2014-05-23 08:03:57]

is it possible to split this change up?

will there be another person that is familar with this code? Please have him/her
also review these changes

[jkarlin, 2014-05-23 13:11:59]

On 2014/05/23 08:03:57, jochen wrote:
> is it possible to split this change up?
> 
> will there be another person that is familar with this code? Please have
him/her
> also review these changes

Added adamk as a reviewer as he's most familiar with the code.  I moved the
worker stuff to a new CL so it's 5 files smaller now.  Not easy to make it much
smaller.  At least most of the layout files are regolding.

[jkarlin, 2014-05-27 18:00:19]

On 2014/05/23 13:11:59, jkarlin wrote:
> On 2014/05/23 08:03:57, jochen wrote:
> > is it possible to split this change up?
> > 
> > will there be another person that is familar with this code? Please have
> him/her
> > also review these changes
> 
> Added adamk as a reviewer as he's most familiar with the code.  I moved the
> worker stuff to a new CL so it's 5 files smaller now.  Not easy to make it
much
> smaller.  At least most of the layout files are regolding.

Ping adamk.  Thanks!

[adamk, 2014-05-27 21:44:27]

Sorry for the delay, long flight followed by long weekend.

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
File Source/modules/netinfo/NavigatorNetworkInfoConnection.cpp (right):

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
Source/modules/netinfo/NavigatorNetworkInfoConnection.cpp:60:
visitor->trace(m_connection);
I think you also want to trace your WillBeHeapSupplement<Navigator> base class
here (based on looking at, e.g., modules/quota/NavigatorStorageQuota.cpp)

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
File Source/modules/netinfo/NavigatorNetworkInfoConnection.h (right):

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
Source/modules/netinfo/NavigatorNetworkInfoConnection.h:8: #include
"core/frame/Navigator.h"
I'll bet you don't need this include, since you have a forward-declaration
below.

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
Source/modules/netinfo/NavigatorNetworkInfoConnection.h:31: void
trace(Visitor*);
Please preface this with "virtual" and add an OVERRIDE annotation;
HeapSupplement has a trace() method you're overriding.

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
File Source/modules/netinfo/NetworkInfoConnection.cpp (right):

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInfoConnection.cpp:11: #include
"core/workers/WorkerGlobalScope.h"
Hmm, do you need this include for something?

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInfoConnection.cpp:13: #include
"public/platform/WebString.h"
I don't think you want this include

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInfoConnection.cpp:14: #include "v8.h"
Or this one

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInfoConnection.cpp:70: if (m_type == type)
Doesn't NetworkChangeNotifier guarantee that it'll only notify you if type
actually changes? That is, can this be an ASSERT?

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInfoConnection.cpp:89: bool hadEvents =
hasEventListeners();
This call to hasEventListeners() seems unnecessary, since you already have
m_observing guards in startObserving().

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInfoConnection.cpp:90: bool ret =
EventTargetWithInlineData::addEventListener(eventType, listener, useCapture);
The return value should be telling you whether the event listener got added, so
this could be restructured as:

if (!EventTargetWithInlineData::addEventListener(eventType, listener,
useCapture))
    return false;
startObserving();
return true;

Hmm, that may or may not be worth it. It just looks funny to me to treat the
bool return value as opaque even though you actually care about that signal.

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInfoConnection.cpp:98: bool hadEvents =
hasEventListeners();
Same here, no need to store the previous value.

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInfoConnection.cpp:99: bool ret =
EventTargetWithInlineData::removeEventListener(eventType, listener, useCapture);
Same here, this could be:

if (!EventTargetWithInlineData::removeEventListener(eventType, listener,
useCapture))
    return false;
if (!hasEventListeners())
    stopObserving();
return true;

Again, not totally clear to me that this is better, but it at least makes use of
the return value.

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInfoConnection.cpp:107: bool hadEvents =
hasEventListeners();
And here.

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInfoConnection.cpp:109: if (hadEvents &&
!hasEventListeners())
I'd rather that you change this to an ASSERT(!hasEventListeners())...

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInfoConnection.cpp:110: stopObserving();
...and then always call stopObserving() when removeAllEventListeners() is
called.

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInfoConnection.cpp:119: // Prevent collection of
this object when there are active listeners.
Can you add an ASSERT(hasEventListeners()) here?

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
File Source/modules/netinfo/NetworkInfoConnection.idl (right):

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInfoConnection.idl:5: enum
NetworkInfoConnectionType {
This name doesn't match the spec (which just says "ConnectionType"). But maybe
that's of no consequence?

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInfoConnection.idl:16: Exposed=Window&Worker,
I don't see any worker support or tests in this change. Should this just be
Window for now?

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInfoConnection.idl:20: interface
NetworkInfoConnection : EventTarget {
Nit: I think this line should be merged into the previous line, i.e.:

] interface NetworkInfoConnection ...

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInfoConnection.idl:20: interface
NetworkInfoConnection : EventTarget {
The spec calls this "NetworkInformation", why the difference here? That's a
web-exposed difference for sure.

[adamk, 2014-05-27 21:45:37]

Oh, one more thing: some tests that test the GC would be great. You can use
internals.observeGC() to make those easy to write. I'd ask for one test that
tests that adding and then removing a listeners allows things to be collected.
And then one that adds a listener in a subframe, then destroys that frame and
ensures that stuff in that frame is collected.

[jochen (gone - plz use gerrit), 2014-05-28 11:57:03]

https://codereview.chromium.org/291203002/diff/190001/LayoutTests/netinfo/bas...
File LayoutTests/netinfo/basic-operation.html (right):

https://codereview.chromium.org/291203002/diff/190001/LayoutTests/netinfo/bas...
LayoutTests/netinfo/basic-operation.html:1: <html>
<!DOCTYPE html>

https://codereview.chromium.org/291203002/diff/190001/LayoutTests/netinfo/bas...
LayoutTests/netinfo/basic-operation.html:22:
internals.setNetworkConnectionInfo(newConnectionType);
check for existence of window.internals before accessing it

[jkarlin, 2014-05-28 13:31:29]

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
File Source/modules/netinfo/NavigatorNetworkInfoConnection.cpp (right):

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
Source/modules/netinfo/NavigatorNetworkInfoConnection.cpp:60:
visitor->trace(m_connection);
On 2014/05/27 21:44:27, adamk wrote:
> I think you also want to trace your WillBeHeapSupplement<Navigator> base class
> here (based on looking at, e.g., modules/quota/NavigatorStorageQuota.cpp)

Done.

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
File Source/modules/netinfo/NavigatorNetworkInfoConnection.h (right):

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
Source/modules/netinfo/NavigatorNetworkInfoConnection.h:8: #include
"core/frame/Navigator.h"
On 2014/05/27 21:44:27, adamk wrote:
> I'll bet you don't need this include, since you have a forward-declaration
> below.

Done.

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
Source/modules/netinfo/NavigatorNetworkInfoConnection.h:31: void
trace(Visitor*);
On 2014/05/27 21:44:27, adamk wrote:
> Please preface this with "virtual" and add an OVERRIDE annotation;
> HeapSupplement has a trace() method you're overriding.

Done.

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
File Source/modules/netinfo/NetworkInfoConnection.cpp (right):

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInfoConnection.cpp:11: #include
"core/workers/WorkerGlobalScope.h"
On 2014/05/27 21:44:27, adamk wrote:
> Hmm, do you need this include for something?

Done.

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInfoConnection.cpp:13: #include
"public/platform/WebString.h"
On 2014/05/27 21:44:27, adamk wrote:
> I don't think you want this include

Done.

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInfoConnection.cpp:14: #include "v8.h"
On 2014/05/27 21:44:27, adamk wrote:
> Or this one

Done.

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInfoConnection.cpp:70: if (m_type == type)
On 2014/05/27 21:44:27, adamk wrote:
> Doesn't NetworkChangeNotifier guarantee that it'll only notify you if type
> actually changes? That is, can this be an ASSERT?

It's actually possible for the same observer to get called twice for a single
event if it removes and then adds itself again during notification. Added a
comment here and to NetworkStateNotifier.

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInfoConnection.cpp:89: bool hadEvents =
hasEventListeners();
On 2014/05/27 21:44:27, adamk wrote:
> This call to hasEventListeners() seems unnecessary, since you already have
> m_observing guards in startObserving().

Done.

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInfoConnection.cpp:90: bool ret =
EventTargetWithInlineData::addEventListener(eventType, listener, useCapture);
On 2014/05/27 21:44:27, adamk wrote:
> The return value should be telling you whether the event listener got added,
so
> this could be restructured as:
> 
> if (!EventTargetWithInlineData::addEventListener(eventType, listener,
> useCapture))
>     return false;
> startObserving();
> return true;
> 
> Hmm, that may or may not be worth it. It just looks funny to me to treat the
> bool return value as opaque even though you actually care about that signal.

Done.

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInfoConnection.cpp:98: bool hadEvents =
hasEventListeners();
On 2014/05/27 21:44:27, adamk wrote:
> Same here, no need to store the previous value.

Done.

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInfoConnection.cpp:99: bool ret =
EventTargetWithInlineData::removeEventListener(eventType, listener, useCapture);
On 2014/05/27 21:44:27, adamk wrote:
> Same here, this could be:
> 
> if (!EventTargetWithInlineData::removeEventListener(eventType, listener,
> useCapture))
>     return false;
> if (!hasEventListeners())
>     stopObserving();
> return true;
> 
> Again, not totally clear to me that this is better, but it at least makes use
of
> the return value.

Done.

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInfoConnection.cpp:107: bool hadEvents =
hasEventListeners();
On 2014/05/27 21:44:27, adamk wrote:
> And here.

Done.

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInfoConnection.cpp:109: if (hadEvents &&
!hasEventListeners())
On 2014/05/27 21:44:27, adamk wrote:
> I'd rather that you change this to an ASSERT(!hasEventListeners())...

Done.

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInfoConnection.cpp:110: stopObserving();
On 2014/05/27 21:44:27, adamk wrote:
> ...and then always call stopObserving() when removeAllEventListeners() is
> called.

Done.

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInfoConnection.cpp:119: // Prevent collection of
this object when there are active listeners.
On 2014/05/27 21:44:27, adamk wrote:
> Can you add an ASSERT(hasEventListeners()) here?

Done.

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
File Source/modules/netinfo/NetworkInfoConnection.idl (right):

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInfoConnection.idl:5: enum
NetworkInfoConnectionType {
On 2014/05/27 21:44:27, adamk wrote:
> This name doesn't match the spec (which just says "ConnectionType"). But maybe
> that's of no consequence?

Done.

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInfoConnection.idl:16: Exposed=Window&Worker,
On 2014/05/27 21:44:27, adamk wrote:
> I don't see any worker support or tests in this change. Should this just be
> Window for now?

Done.

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInfoConnection.idl:20: interface
NetworkInfoConnection : EventTarget {
On 2014/05/27 21:44:27, adamk wrote:
> Nit: I think this line should be merged into the previous line, i.e.:
> 
> ] interface NetworkInfoConnection ...

Done.

https://codereview.chromium.org/291203002/diff/190001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInfoConnection.idl:20: interface
NetworkInfoConnection : EventTarget {
On 2014/05/27 21:44:27, adamk wrote:
> The spec calls this "NetworkInformation", why the difference here? That's a
> web-exposed difference for sure.

Done.

[jkarlin, 2014-05-28 15:34:00]

https://codereview.chromium.org/291203002/diff/190001/LayoutTests/netinfo/bas...
File LayoutTests/netinfo/basic-operation.html (right):

https://codereview.chromium.org/291203002/diff/190001/LayoutTests/netinfo/bas...
LayoutTests/netinfo/basic-operation.html:1: <html>
On 2014/05/28 11:57:03, jochen wrote:
> <!DOCTYPE html>

Done.

https://codereview.chromium.org/291203002/diff/190001/LayoutTests/netinfo/bas...
LayoutTests/netinfo/basic-operation.html:22:
internals.setNetworkConnectionInfo(newConnectionType);
On 2014/05/28 11:57:03, jochen wrote:
> check for existence of window.internals before accessing it

Done.

[jkarlin, 2014-05-28 15:34:14]

On 2014/05/27 21:45:37, adamk wrote:
> Oh, one more thing: some tests that test the GC would be great. You can use
> internals.observeGC() to make those easy to write. I'd ask for one test that
> tests that adding and then removing a listeners allows things to be collected.
> And then one that adds a listener in a subframe, then destroys that frame and
> ensures that stuff in that frame is collected.

Done!

[jkarlin, 2014-05-28 17:38:09]

Okay, ready for review again.  Not sure why the new layout tests are failing on
the try bots, they're working here.

[adamk, 2014-05-28 18:31:28]

lgtm

But the failing tests clearly need to be figured out before landing. One thing
you might try (at least it worked in the past) is to call gc() multiple times,
to force multiple full GCs. I thought that the gc() call itself did that now,
but perhaps not?

[jkarlin, 2014-05-28 19:04:32]

PTAL jochen.  Thanks so much!

Failing tests should be fixed (forgot to upload netinfo_common.js).

[jkarlin, 2014-05-28 19:04:35]

PTAL jochen.  Thanks so much!

Failing tests should be fixed (forgot to upload netinfo_common.js).

[Inactive, 2014-05-28 20:18:20]

https://codereview.chromium.org/291203002/diff/330001/Source/modules/netinfo/...
File Source/modules/netinfo/NetworkInformation.cpp (right):

https://codereview.chromium.org/291203002/diff/330001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInformation.cpp:25: return kCellular;
So we keep constructing WTF Strings from const char* here? Shouldn't we use
DEFINE_STATIC_LOCAL() macro and define those as static Strings?

https://codereview.chromium.org/291203002/diff/330001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInformation.cpp:72:
dispatchEvent(Event::create(EventTypeNames::typechange));
The spec says to queue the event but it seems we are firing synchronously? I
don't know it if matters here, just pointing this out.

https://codereview.chromium.org/291203002/diff/330001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInformation.cpp:143: m_type =
networkStateNotifier().connectionType();
nit: Could be in the initializer list?

https://codereview.chromium.org/291203002/diff/330001/Source/modules/netinfo/...
File Source/modules/netinfo/NetworkInformation.h (right):

https://codereview.chromium.org/291203002/diff/330001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInformation.h:16: class Dictionary;
unused?

https://codereview.chromium.org/291203002/diff/330001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInformation.h:18: class ServiceWorker;
unused?

https://codereview.chromium.org/291203002/diff/330001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInformation.h:30: ~NetworkInformation();
Would be nice to mark it as virtual explicitly.

https://codereview.chromium.org/291203002/diff/330001/Source/modules/netinfo/...
File Source/modules/netinfo/NetworkInformation.idl (right):

https://codereview.chromium.org/291203002/diff/330001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInformation.idl:16: Exposed=Window,
nit: Not needed, this is the default. The spec uses [Exposed] because it exposes
this to workers as well.

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

https://codereview.chromium.org/291203002/diff/330001/Source/modules/netinfo/...
File Source/modules/netinfo/NetworkInformation.cpp (right):

https://codereview.chromium.org/291203002/diff/330001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInformation.cpp:72:
dispatchEvent(Event::create(EventTypeNames::typechange));
On 2014/05/28 20:18:20, Chris Dumez wrote:
> The spec says to queue the event but it seems we are firing synchronously? I
> don't know it if matters here, just pointing this out.

I noticed this too, but I think we're effectively doing what the spec says:
since the browser process notifies the renderer via IPC, connectionTypeChange
only ever runs as a consequence of having been queued as a task. And indeed, we
only set m_type immediately before dispatching the event.

[Inactive, 2014-05-28 22:47:28]

https://codereview.chromium.org/291203002/diff/330001/Source/modules/netinfo/...
File Source/modules/netinfo/NetworkInformation.cpp (right):

https://codereview.chromium.org/291203002/diff/330001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInformation.cpp:72:
dispatchEvent(Event::create(EventTypeNames::typechange));
On 2014/05/28 22:44:08, adamk wrote:
> On 2014/05/28 20:18:20, Chris Dumez wrote:
> > The spec says to queue the event but it seems we are firing synchronously? I
> > don't know it if matters here, just pointing this out.
> 
> I noticed this too, but I think we're effectively doing what the spec says:
> since the browser process notifies the renderer via IPC, connectionTypeChange
> only ever runs as a consequence of having been queued as a task. And indeed,
we
> only set m_type immediately before dispatching the event.

It makes sense, thanks for clarifying.

[jkarlin, 2014-05-29 11:37:55]

https://codereview.chromium.org/291203002/diff/330001/Source/modules/netinfo/...
File Source/modules/netinfo/NetworkInformation.cpp (right):

https://codereview.chromium.org/291203002/diff/330001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInformation.cpp:25: return kCellular;
On 2014/05/28 20:18:20, Chris Dumez wrote:
> So we keep constructing WTF Strings from const char* here? Shouldn't we use
> DEFINE_STATIC_LOCAL() macro and define those as static Strings?

Thanks for the suggestion!  Done.

https://codereview.chromium.org/291203002/diff/330001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInformation.cpp:72:
dispatchEvent(Event::create(EventTypeNames::typechange));
On 2014/05/28 22:47:28, Chris Dumez wrote:
> On 2014/05/28 22:44:08, adamk wrote:
> > On 2014/05/28 20:18:20, Chris Dumez wrote:
> > > The spec says to queue the event but it seems we are firing synchronously?
I
> > > don't know it if matters here, just pointing this out.
> > 
> > I noticed this too, but I think we're effectively doing what the spec says:
> > since the browser process notifies the renderer via IPC,
connectionTypeChange
> > only ever runs as a consequence of having been queued as a task. And indeed,
> we
> > only set m_type immediately before dispatching the event.
> 
> It makes sense, thanks for clarifying.

Done.

https://codereview.chromium.org/291203002/diff/330001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInformation.cpp:143: m_type =
networkStateNotifier().connectionType();
On 2014/05/28 20:18:20, Chris Dumez wrote:
> nit: Could be in the initializer list?

Done.

https://codereview.chromium.org/291203002/diff/330001/Source/modules/netinfo/...
File Source/modules/netinfo/NetworkInformation.h (right):

https://codereview.chromium.org/291203002/diff/330001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInformation.h:16: class Dictionary;
On 2014/05/28 20:18:20, Chris Dumez wrote:
> unused?

Done.

https://codereview.chromium.org/291203002/diff/330001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInformation.h:18: class ServiceWorker;
On 2014/05/28 20:18:20, Chris Dumez wrote:
> unused?

Done.

https://codereview.chromium.org/291203002/diff/330001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInformation.h:30: ~NetworkInformation();
On 2014/05/28 20:18:20, Chris Dumez wrote:
> Would be nice to mark it as virtual explicitly.

Done.

https://codereview.chromium.org/291203002/diff/330001/Source/modules/netinfo/...
File Source/modules/netinfo/NetworkInformation.idl (right):

https://codereview.chromium.org/291203002/diff/330001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInformation.idl:16: Exposed=Window,
On 2014/05/28 20:18:20, Chris Dumez wrote:
> nit: Not needed, this is the default. The spec uses [Exposed] because it
exposes
> this to workers as well.

Done.

[jkarlin, 2014-05-29 11:39:44]

https://codereview.chromium.org/291203002/diff/330001/Source/modules/netinfo/...
File Source/modules/netinfo/NetworkInformation.cpp (right):

https://codereview.chromium.org/291203002/diff/330001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInformation.cpp:72:
dispatchEvent(Event::create(EventTypeNames::typechange));
On 2014/05/29 11:37:56, jkarlin wrote:
> On 2014/05/28 22:47:28, Chris Dumez wrote:
> > On 2014/05/28 22:44:08, adamk wrote:
> > > On 2014/05/28 20:18:20, Chris Dumez wrote:
> > > > The spec says to queue the event but it seems we are firing
synchronously?
> I
> > > > don't know it if matters here, just pointing this out.
> > > 
> > > I noticed this too, but I think we're effectively doing what the spec
says:
> > > since the browser process notifies the renderer via IPC,
> connectionTypeChange
> > > only ever runs as a consequence of having been queued as a task. And
indeed,
> > we
> > > only set m_type immediately before dispatching the event.
> > 
> > It makes sense, thanks for clarifying.
> 
> Done.

I should clarify that by 'done' I'm just agreeing that we're already queuing the
task, no change was made to the code.

[jkarlin, 2014-05-29 11:40:01]

PTAL jochen, thanks!

[adamk, 2014-05-29 16:57:55]

https://codereview.chromium.org/291203002/diff/370001/Source/modules/netinfo/...
File Source/modules/netinfo/NetworkInformation.cpp (right):

https://codereview.chromium.org/291203002/diff/370001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInformation.cpp:17: DEFINE_STATIC_LOCAL(const
String, cellular, ("cellular"));
These aren't going to play nice with the multi-threading. See
https://code.google.com/p/chromium/issues/detail?id=367982

[jkarlin, 2014-05-29 17:12:56]

https://codereview.chromium.org/291203002/diff/370001/Source/modules/netinfo/...
File Source/modules/netinfo/NetworkInformation.cpp (right):

https://codereview.chromium.org/291203002/diff/370001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInformation.cpp:17: DEFINE_STATIC_LOCAL(const
String, cellular, ("cellular"));
On 2014/05/29 16:57:56, adamk wrote:
> These aren't going to play nice with the multi-threading. See
> https://code.google.com/p/chromium/issues/detail?id=367982

This is only called by NetworkInformation::type() which is on the context
thread.  Shouldn't have multiple threads accessing it.  I can either add an
assert that I'm on the context thread or go back to using a global char*.  Which
do you prefer?  I'm more in favor of the char* but I can go either way.

[adamk, 2014-05-29 18:07:57]

https://codereview.chromium.org/291203002/diff/370001/Source/modules/netinfo/...
File Source/modules/netinfo/NetworkInformation.cpp (right):

https://codereview.chromium.org/291203002/diff/370001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInformation.cpp:17: DEFINE_STATIC_LOCAL(const
String, cellular, ("cellular"));
On 2014/05/29 17:12:56, jkarlin wrote:
> On 2014/05/29 16:57:56, adamk wrote:
> > These aren't going to play nice with the multi-threading. See
> > https://code.google.com/p/chromium/issues/detail?id=367982
> 
> This is only called by NetworkInformation::type() which is on the context
> thread.  Shouldn't have multiple threads accessing it.

Each instance of NetworkInformation is on its own thread, but there's only a
single version of connectionTypeToString (and thus of each DEFINE_STATIC_LOCAL)
that all threads will call into. The unsafety is that the initialization of
these static locals is not thread-safe, so if two threads happen to call type()
there's a race.

> I can either add an
> assert that I'm on the context thread or go back to using a global char*. 
Which
> do you prefer?  I'm more in favor of the char* but I can go either way.

Please go back to the const char[], I think that's the only safe thing to do.
You could imagine caching the strings per instance, but I can't imagine that
this is performance sensitive code.

[jkarlin, 2014-05-29 18:12:58]

On 2014/05/29 18:07:57, adamk wrote:
>
https://codereview.chromium.org/291203002/diff/370001/Source/modules/netinfo/...
> File Source/modules/netinfo/NetworkInformation.cpp (right):
> 
>
https://codereview.chromium.org/291203002/diff/370001/Source/modules/netinfo/...
> Source/modules/netinfo/NetworkInformation.cpp:17: DEFINE_STATIC_LOCAL(const
> String, cellular, ("cellular"));
> On 2014/05/29 17:12:56, jkarlin wrote:
> > On 2014/05/29 16:57:56, adamk wrote:
> > > These aren't going to play nice with the multi-threading. See
> > > https://code.google.com/p/chromium/issues/detail?id=367982
> > 
> > This is only called by NetworkInformation::type() which is on the context
> > thread.  Shouldn't have multiple threads accessing it.
> 
> Each instance of NetworkInformation is on its own thread, but there's only a
> single version of connectionTypeToString (and thus of each
DEFINE_STATIC_LOCAL)
> that all threads will call into. The unsafety is that the initialization of
> these static locals is not thread-safe, so if two threads happen to call
type()
> there's a race.
> 
> > I can either add an
> > assert that I'm on the context thread or go back to using a global char*. 
> Which
> > do you prefer?  I'm more in favor of the char* but I can go either way.
> 
> Please go back to the const char[], I think that's the only safe thing to do.
> You could imagine caching the strings per instance, but I can't imagine that
> this is performance sensitive code.

Done.  Yes, of course.  Thanks for the catch!

[adamk, 2014-05-30 00:13:50]

https://codereview.chromium.org/291203002/diff/380001/Source/modules/netinfo/...
File Source/modules/netinfo/NetworkInformation.cpp (right):

https://codereview.chromium.org/291203002/diff/380001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInformation.cpp:20: const char none[] = "none";
fwiw I don't think there's much use for these constants, you could just put the
strings in the switch statement below.

[jkarlin, 2014-05-30 01:51:23]

Done.  PTAL.

https://codereview.chromium.org/291203002/diff/380001/Source/modules/netinfo/...
File Source/modules/netinfo/NetworkInformation.cpp (right):

https://codereview.chromium.org/291203002/diff/380001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInformation.cpp:20: const char none[] = "none";
On 2014/05/30 00:13:51, adamk wrote:
> fwiw I don't think there's much use for these constants, you could just put
the
> strings in the switch statement below.

Done.

[jkarlin, 2014-05-30 10:46:13]

haraken: PTAL at Source/modules/

[haraken, 2014-05-30 15:42:18]

https://codereview.chromium.org/291203002/diff/400001/Source/modules/netinfo/...
File Source/modules/netinfo/NetworkInformation.h (right):

https://codereview.chromium.org/291203002/diff/400001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInformation.h:19: : public
RefCountedWillBeGarbageCollected<NetworkInformation>

This should be RefCountedWillBeRefCountedGarbageCollected.

I'd be happy if you could just check a build with 'enable_oilpan=1'. Sorry about
the extra work...

https://codereview.chromium.org/291203002/diff/400001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInformation.h:24:
DEFINE_EVENT_TARGET_REFCOUNTING(RefCountedWillBeGarbageCollected<NetworkInformation>);

I think "DEFINE_EVENT_TARGET(NetworkInformation)" will work.

[jkarlin, 2014-05-30 16:19:38]

https://codereview.chromium.org/291203002/diff/400001/Source/modules/netinfo/...
File Source/modules/netinfo/NetworkInformation.h (right):

https://codereview.chromium.org/291203002/diff/400001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInformation.h:19: : public
RefCountedWillBeGarbageCollected<NetworkInformation>
On 2014/05/30 15:42:19, haraken wrote:
> 
> This should be RefCountedWillBeRefCountedGarbageCollected.
> 
> I'd be happy if you could just check a build with 'enable_oilpan=1'. Sorry
about
> the extra work...

Done.  Built (with clang) and ran netinfo/* tests with enable_oilpan=1.

https://codereview.chromium.org/291203002/diff/400001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInformation.h:24:
DEFINE_EVENT_TARGET_REFCOUNTING(RefCountedWillBeGarbageCollected<NetworkInformation>);
On 2014/05/30 15:42:19, haraken wrote:
> 
> I think "DEFINE_EVENT_TARGET(NetworkInformation)" will work.

There is no DEFINE_EVENT_TARGET, REFCOUNTED_EVENT_TARGET didn't work though. 
Kept with original (which is what BatteryManager.h uses in battery module).

[haraken, 2014-05-30 16:28:00]

>
https://codereview.chromium.org/291203002/diff/400001/Source/modules/netinfo/...
> Source/modules/netinfo/NetworkInformation.h:24:
>
DEFINE_EVENT_TARGET_REFCOUNTING(RefCountedWillBeGarbageCollected<NetworkInformation>);
> On 2014/05/30 15:42:19, haraken wrote:
> > 
> > I think "DEFINE_EVENT_TARGET(NetworkInformation)" will work.
> 
> There is no DEFINE_EVENT_TARGET, REFCOUNTED_EVENT_TARGET didn't work though. 
> Kept with original (which is what BatteryManager.h uses in battery module).

Did you rebase with ToT? Recently there was a change about EventTarget, and I
think REFCOUNTED_EVENT_TARGET should work. BatteryManager.h is also using
REFCOUNTED_EVENT_TARGET.

Mostly looks good, so let me take a close look tomorrow :)

[jkarlin, 2014-05-30 17:52:40]

On 2014/05/30 16:28:00, haraken wrote:
> >
>
https://codereview.chromium.org/291203002/diff/400001/Source/modules/netinfo/...
> > Source/modules/netinfo/NetworkInformation.h:24:
> >
>
DEFINE_EVENT_TARGET_REFCOUNTING(RefCountedWillBeGarbageCollected<NetworkInformation>);
> > On 2014/05/30 15:42:19, haraken wrote:
> > > 
> > > I think "DEFINE_EVENT_TARGET(NetworkInformation)" will work.
> > 
> > There is no DEFINE_EVENT_TARGET, REFCOUNTED_EVENT_TARGET didn't work though.

> > Kept with original (which is what BatteryManager.h uses in battery module).
> 
> Did you rebase with ToT? Recently there was a change about EventTarget, and I
> think REFCOUNTED_EVENT_TARGET should work. BatteryManager.h is also using
> REFCOUNTED_EVENT_TARGET.
> 
> Mostly looks good, so let me take a close look tomorrow :)

Rebased and updated the macros (added the gc mixin as well).  Works well with
oilpan.  Thanks!

[adamk, 2014-05-30 20:05:42]

still lgtm

[haraken, 2014-05-31 08:47:15]

LGTM

https://codereview.chromium.org/291203002/diff/460001/Source/modules/netinfo/...
File Source/modules/netinfo/NetworkInformation.h (right):

https://codereview.chromium.org/291203002/diff/460001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInformation.h:44: virtual void stop() OVERRIDE;

Just to confirm: You don't need to override suspend() and resume(), right?

https://codereview.chromium.org/291203002/diff/460001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInformation.h:59: // Whether ActiveDomObject::stop
has been called.

ActiveDOMObject

[jkarlin, 2014-05-31 11:12:10]

https://codereview.chromium.org/291203002/diff/460001/Source/modules/netinfo/...
File Source/modules/netinfo/NetworkInformation.h (right):

https://codereview.chromium.org/291203002/diff/460001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInformation.h:44: virtual void stop() OVERRIDE;
On 2014/05/31 08:47:16, haraken wrote:
> 
> Just to confirm: You don't need to override suspend() and resume(), right?

I don't think so since I'm not creating any async methods that would need to be
held back.  I'm just using ActiveDOMObject to stop my object from collecting
while listening to event handlers. Or are there other times that I'd need to
suspend/resume?

https://codereview.chromium.org/291203002/diff/460001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInformation.h:59: // Whether ActiveDomObject::stop
has been called.
On 2014/05/31 08:47:16, haraken wrote:
> 
> ActiveDOMObject

Done.

[haraken, 2014-05-31 13:09:09]

still LGTM

https://codereview.chromium.org/291203002/diff/460001/Source/modules/netinfo/...
File Source/modules/netinfo/NetworkInformation.h (right):

https://codereview.chromium.org/291203002/diff/460001/Source/modules/netinfo/...
Source/modules/netinfo/NetworkInformation.h:44: virtual void stop() OVERRIDE;
On 2014/05/31 11:12:11, jkarlin wrote:
> On 2014/05/31 08:47:16, haraken wrote:
> > 
> > Just to confirm: You don't need to override suspend() and resume(), right?
> 
> I don't think so since I'm not creating any async methods that would need to
be
> held back.  I'm just using ActiveDOMObject to stop my object from collecting
> while listening to event handlers. Or are there other times that I'd need to
> suspend/resume?

Makes sense, if it's just for keeping the object alive, you don't need to do
anything when the context is suspended/resumed.

[jkarlin, 2014-05-31 13:15:10]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2014-05-31 13:15:20]

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

[commit-bot: I haz the power, 2014-05-31 20:20:56]

Change committed as 175200

[aa979586, 2014-09-28 22:59:48]

aa979586@gmail.com changed reviewers:
+ aa979586@gmail.com

[aa979586, 2014-09-28 22:59:49]

lgtm