[Webkit-unassigned] [Bug 98273] [BlackBerry] Implementing the NetworkInfo API for BB port

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=98273





--- Comment #6 from Daniel Bates <dbates at webkit.org>  2012-10-03 14:28:49 PST ---
(From update of attachment 166954)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.