[webkit-reviews] review denied: [Bug 39724] [Qt] navigator.geolocation support for qt port : [Attachment 57751] proposed solution

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has denied maheshK
<mahesh.kulkarni at nokia.com>'s request for review:
Bug 39724: [Qt] navigator.geolocation support for qt port
https://bugs.webkit.org/show_bug.cgi?id=39724

Attachment 57751: proposed solution
https://bugs.webkit.org/attachment.cgi?id=57751&action=review

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
WebCore/platform/qt/GeolocationServiceQt.cpp:43
 +	:m_parent(parent)
need a space after :

WebCore/platform/qt/GeolocationServiceQt.cpp:53
 +	if (gpsPos.isValid()) {
Would be better to say:

if (!gpsPos.isValid()) {
    ...
    return;
}

Then you avoid all that identation

WebCore/platform/qt/GeolocationServiceQt.cpp:61
 +			       
gpsPos.attribute(QGeoPositionInfo::HorizontalAccuracy):0.0;
wrong coding style, please check the coding style guide

WebCore/platform/qt/GeolocationServiceQt.cpp:64
 +	    double altitudeAccuracy =
providesAltitudeAccuracy?gpsPos.attribute(QGeoPositionInfo::VerticalAccuracy):0
.0;
please add spaces around the ? and the :

WebCore/platform/qt/GeolocationServiceQt.cpp:67
 +	    double heading =
providesHeading?gpsPos.attribute(QGeoPositionInfo::Direction):0.0;
here as well

WebCore/platform/qt/GeolocationServiceQt.cpp:70
 +	    double speed =
providesSpeed?gpsPos.attribute(QGeoPositionInfo::GroundSpeed):0.0;
and here

WebCore/platform/qt/GeolocationServiceQt.cpp:92
 +	m_location = QGeoPositionInfoSource::createDefaultSource(m_p);
m_p is a terrible variable name, in Qt we ALWAYS use d for the private, but
then again when implementing WebCore classes we never use privates as there is
no binary compatibility promise. Why do you need the private at all?

WebCore/platform/qt/GeolocationServiceQt.cpp:96
 +		    m_p, SLOT(positionUpdated(QGeoPositionInfo)));
The inner of the if spans two lines (one logically) so it needs braces. Again
please read teh coding style guide.

WebCore/platform/qt/GeolocationServiceQt.cpp:98
 +	    m_lastError =
PositionError::create(PositionError::POSITION_UNAVAILABLE, "GPS device not
found");
How do we deal with translation here? Simon?

WebCore/platform/qt/GeolocationServiceQtPrivate.h:43
 +  class GeolocationServiceQtPrivate : public QObject {
Given the comment, the name *Private is probably not the best. You need to come
up with a better name

WebCore/platform/qt/GeolocationServiceQtPrivate.h:48
 +  public slots:
please add a new line before 'public slots'. Also you cannot use 'slots' as the
user might use Boost, thus please use 'public Q_SLOTS' instead

WebCore/platform/qt/GeolocationServiceQtPrivate.h:50
 +	void positionUpdated(const QGeoPositionInfo &);
no space needed before &