Bug 98273 - [BlackBerry] Implementing the NetworkInfo API for BB port
Summary: [BlackBerry] Implementing the NetworkInfo API for BB port
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit BlackBerry (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-03 09:08 PDT by otcheung
Modified: 2012-10-03 14:32 PDT (History)
7 users (show)

See Also:


Attachments
Patch (14.03 KB, patch)
2012-10-03 13:35 PDT, otcheung
no flags Details | Formatted Diff | Diff
Patch (14.30 KB, patch)
2012-10-03 13:42 PDT, otcheung
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description otcheung 2012-10-03 09:08:57 PDT
Implement the NetworkInfoClient for the BlackBerry port
Comment 1 otcheung 2012-10-03 13:35:17 PDT
Created attachment 166951 [details]
Patch
Comment 2 otcheung 2012-10-03 13:42:50 PDT
Created attachment 166954 [details]
Patch
Comment 3 Rob Buis 2012-10-03 13:47:28 PDT
Comment on attachment 166954 [details]
Patch

Looks good.
Comment 4 WebKit Review Bot 2012-10-03 14:11:00 PDT
Comment on attachment 166954 [details]
Patch

Clearing flags on attachment: 166954

Committed r130320: <http://trac.webkit.org/changeset/130320>
Comment 5 WebKit Review Bot 2012-10-03 14:11:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Daniel Bates 2012-10-03 14:28:49 PDT
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.
Comment 7 Daniel Bates 2012-10-03 14:32:13 PDT
(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.