Bug 98273 – [BlackBerry] Implementing the NetworkInfo API for BB port

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit BlackBerry

Implement the NetworkInfoClient for the BlackBerry port

Created attachment 166951 [details]
Patch

Created attachment 166954 [details]
Patch

[Rob Buis]

Comment on attachment 166954 [details]
Patch

Looks good.

[WebKit Review Bot]

Comment on attachment 166954 [details]
Patch

Clearing flags on attachment: 166954

Committed r130320: <http://trac.webkit.org/changeset/130320>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Daniel Bates]

Comment on attachment 166954 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166954&action=review

I know that this patch was already committed. I briefly looked over this and noticed some minor nits. Feel free to ignore my remarks.

> Source/WebKit/blackberry/WebCoreSupport/NetworkInfoClientBlackBerry.cpp:36
> +static const double  networkSpeedNone = 0; // 0 Mb/s
> +static const double  networkSpeedEthernet = 20; // 20 Mb/s
> +static const double  networkSpeedWifi = 20; // 20 Mb/s
> +static const double  networkSpeed2G = 60 / 1024; // 60 kb/s
> +static const double  networkSpeed3G = 7; // 7 Mb/s
> +static const double  networkSpeed4G = 50; // 50 Mb/s
> +static const double  networkSpeedDefault = INFINITY; // w3c draft states it should be infinity

Nit: There is an extra space between typename and the identifier in these lines.

Nit: w3c => W3C

You may want to consider a single comment above these constants that describes that the network speed is in Mb/s instead of having an inline comment of the form "// X Mb/s" for each of these constants.

> Source/WebKit/blackberry/WebCoreSupport/NetworkInfoClientBlackBerry.cpp:49
> +    if (!m_isActive)
> +        // Add ourselves to listener so our values get updated whenever PPS calls.
> +        BlackBerry::Platform::NetworkInfo::instance()->addListener(this);

Nit: I suggest that we follow the WebKit Code Style Guidelines and uses braces for this if block.

> Source/WebKit/blackberry/WebCoreSupport/NetworkInfoClientBlackBerry.cpp:60
> +double NetworkInfoClientBlackBerry::bandwidth() const

Another way to implement this function is to extract the inner switch block into an inline static non-member function, say bandwidthForCellularType(). Then the body of case BlackBerry::Platform::NetworkTypeCellular can be written:

return bandwidthForCellularType(BlackBerry::Platform::NetworkInfo::instance()->getCurrentCellularType());

> Source/WebKit/blackberry/WebCoreSupport/NetworkInfoClientBlackBerry.cpp:94
> +    if (cellType == BlackBerry::Platform::NetworkTypeCellular || cellType == BlackBerry::Platform::NetworkTypeBB)
> +        return true;
> +    return false;

I would write this as:

return cellType == BlackBerry::Platform::NetworkTypeCellular || cellType == BlackBerry::Platform::NetworkTypeBB;

> Source/WebKit/blackberry/WebCoreSupport/NetworkInfoClientBlackBerry.cpp:102
> +    if (m_isActive) {
> +        RefPtr<NetworkInfo> newNetworkInfo = NetworkInfo::create(bandwidth(), metered());
> +        NetworkInfoController::from(m_webPagePrivate->m_page)->didChangeNetworkInformation(eventNames().webkitnetworkinfochangeEvent , newNetworkInfo.get());
> +    }

I suggest using an early return style here. Then we remove one level of indentation.

> Source/WebKit/blackberry/WebCoreSupport/NetworkInfoClientBlackBerry.cpp:108
> +    if (BlackBerry::Platform::NetworkInfo::instance()->getCurrentNetworkType() == BlackBerry::Platform::NetworkTypeCellular && m_isActive) {

Unless calling BlackBerry::Platform::NetworkInfo::instance()->getCurrentNetworkType() has side-effects I suggest reversing the order of the conjuncts in this expression such that we check m_isActive first. A benefit of reversing the order is that when m_isActive is false we short-circuit the if conditional and hence don't evaluate "BlackBerry::Platform::NetworkInfo::instance()->getCurrentNetworkType() == BlackBerry::Platform::NetworkTypeCellular"; => avoid the function call BlackBerry::Platform::NetworkInfo::instance()->getCurrentNetworkType().

You may also want to consider using an early return style here so as to avoid one level of indentation.

[Daniel Bates]

(In reply to comment #6)
> > Source/WebKit/blackberry/WebCoreSupport/NetworkInfoClientBlackBerry.cpp:108
> > +    if (BlackBerry::Platform::NetworkInfo::instance()->getCurrentNetworkType() == BlackBerry::Platform::NetworkTypeCellular && m_isActive) {
> 
> Unless calling BlackBerry::Platform::NetworkInfo::instance()->getCurrentNetworkType() has side-effects I suggest reversing the order of the conjuncts in this expression such that we check m_isActive first. A benefit of reversing the order is that when m_isActive is false we short-circuit the if conditional and hence don't evaluate "BlackBerry::Platform::NetworkInfo::instance()->getCurrentNetworkType() == BlackBerry::Platform::NetworkTypeCellular"; => avoid the function call BlackBerry::Platform::NetworkInfo::instance()->getCurrentNetworkType().
> 
> You may also want to consider using an early return style here so as to avoid one level of indentation.

Actually, it's probably more common that when we get here m_isActive is true; => the order of these conjuncts is fine.