Implement the NetworkInfoClient for the BlackBerry port
Created attachment 166951 [details] Patch
Created attachment 166954 [details] Patch
Comment on attachment 166954 [details] Patch Looks good.
Comment on attachment 166954 [details] Patch Clearing flags on attachment: 166954 Committed r130320: <http://trac.webkit.org/changeset/130320>
All reviewed patches have been landed. Closing bug.
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.
(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.