[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


maheshK <mahesh.kulkarni at nokia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #57751|0                           |1
        is obsolete|                            |
  Attachment #57846|                            |review?
               Flag|                            |




--- Comment #9 from maheshK <mahesh.kulkarni at nokia.com>  2010-06-03 23:27:11 PST ---
Created an attachment (id=57846)
 --> (https://bugs.webkit.org/attachment.cgi?id=57846)
proposed solution

(In reply to comment #7)

Thanks Kenneth and Laszlo for your review comments. 


> (From update of attachment 57751 [details])
> 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

Done

> 
> 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
> 

Done


> 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?
> 

this member is removed, read further for reason

> 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.
> 

Done.

> 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?

I just introduced a tr() to it for now. Actually geolocation common code takes care of all error messages given back to JS, m_lasterror is filled with this error message only for a special use case where updatePostion is called with a invalid GeoPostion value.

> 
> 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 &

private class is just to define a slot which connects to qtmobility signal. Because the signal is not defined with QtMobility (not fully qualified signal) I need to use "using namespace" in header file as work around until mobility fixes the issue. Patch has been sent for the same to mobility team as well. 

New patch has no GeoLocationQtServicePrivate.h file. geolocationserviceQt.h it self has "using namespace" instead. Which will be reverted as per webkit coding guidelines once the mobility fix is available.

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