[webkit-reviews] review denied: [Bug 28095] [WINCE] W3C Geolocation backend : [Attachment 34366] W3C Geolocation for WINCE

[bugzilla-daemon at webkit.org]

George Staikos <staikos at kde.org> has denied  review:
Bug 28095: [WINCE] W3C Geolocation backend
https://bugs.webkit.org/show_bug.cgi?id=28095

Attachment 34366: W3C Geolocation for WINCE
https://bugs.webkit.org/attachment.cgi?id=34366&action=review

------- Additional Comments from George Staikos <staikos at kde.org>

> +    while (true) {
> +	   DWORD retVal = WaitForMultipleObjects(3, events, false, INFINITE);
> +	   if (retVal == WAIT_OBJECT_0) {
> +	       success = sendPosition();
> +	       if (!success)
> +		   break;
> +	   } else if (retVal == WAIT_OBJECT_0 + 1) {
> +	       success = sendDeviceState();
> +	       if (!success)
> +		   break;
> +	   } else if (retVal == WAIT_OBJECT_0 + 2)
> +	       break; // Terminate
> +	   else {
> +	       ASSERT_NOT_REACHED();
> +	       goto DO_CLEANUP;
> +	   }
> +    }
> +
> +    closeGPSDevice();
> +
> +DO_CLEANUP:
> +
> +    // Detach the thread so its resources are no longer of any concern to
anyone else
> +    detachThread(m_threadID);
> +
> +    // Clear the self refptr, possibly resulting in deletion
> +    m_selfRef = 0;
> +
> +    return 0;
> +}


   No need for a goto here.  Also there's no point to ASSERT and goto right
after.	If it can ever possibly goto, then you're leaking.

> +    bool providesAltitude = false;
> +    bool providesAltitudeAccuracy = false;
> +    bool providesHeading = false;
> +    bool providesSpeed = false;
> +
> +    DWORD retVal = GPSGetPosition(m_gpsHandle, &position, maxAge, 0);
> +    if (retVal != ERROR_SUCCESS) {
> +	   sendError();
> +	   return false;
> +    }
> +
> +    if (!(position.dwValidFields & GPS_VALID_LATITUDE)
> +	       || !(position.dwValidFields & GPS_VALID_LONGITUDE)
> +	       || !(position.dwValidFields &
GPS_VALID_HORIZONTAL_DILUTION_OF_PRECISION)) {
> +
> +	   // The current position is not yet available.
> +	   return true;
> +    }
> +
> +    if (position.dwValidFields & GPS_VALID_ALTITUDE_WRT_SEA_LEVEL)
> +	   providesAltitude = true;
> +
> +    if (position.dwValidFields & GPS_VALID_VERTICAL_DILUTION_OF_PRECISION)
> +	   providesAltitudeAccuracy = true;
> +
> +    if (position.dwValidFields & GPS_VALID_HEADING)
> +	   providesHeading = true;
> +
> +    if (position.dwValidFields & GPS_VALID_SPEED)
> +	   providesSpeed = true;


   Those providesXXX variables should get initialized directly and without an
if() down where they're needed.