[Webkit-unassigned] [Bug 39724] [Qt] navigator.geolocation support for qt port

[bugzilla-daemon at webkit.org]

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


Kenneth Rohde Christiansen <kenneth at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #57751|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #7 from Kenneth Rohde Christiansen <kenneth at webkit.org>  2010-06-03 11:21:28 PST ---
(From update of attachment 57751)
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 &

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