Bug 28095 – [WINCE] W3C Geolocation backend

• Component: WebCore Misc.
• Status: UNCONFIRMED

[Kwang Yul Seo]

Implement W3C Geolocation backend with WINCE's GPS Intermediate Driver functions.

[Kwang Yul Seo]

Created attachment 34364 [details]
W3C Geolocation for WINCE

[Kwang Yul Seo]

Before I submit this patch, I checked the Torch Mobile WINCE port at git://code.staikos.net/WebKit-CE and  couldn't find GeolocationServiceWince.* there. 

Staikos: If you are going to commit your W3C Geolocation code, reject this patch please.

[Eric Seidel (no email)]

Comment on attachment 34364 [details]
W3C Geolocation for WINCE

This looks sane, non-harmful. :)  I've never worked with our geolocation code before, so I am not able to help in that way.

I've not seen m_hFoo used to designate that foo is a handle.  Normally we would write out Handle in the name if necessary.  Or we would have a HandlePtr that new how to close in the destructor.  Sadly HandlePtr doesn't exist yet. :)

[George Staikos]

Comment on attachment 34364 [details]
W3C Geolocation for WINCE

um, it's even full of spelling mistakes in the class names?

Some patches get rejected for a space out of place, and this gets r+ for a much more prominent issue?

[George Staikos]

Also we don't have a geolocation service to contribute, but Kwang Yul I think your company should be releasing the source code to opensource portions of your browser that you have been distributing in binary form.

[Kwang Yul Seo]

Created attachment 34366 [details]
W3C Geolocation for WINCE

Fix the spelling errors in the class name and change m_hXXX to m_xxxHandle.

[Eric Seidel (no email)]

Comment on attachment 34366 [details]
W3C Geolocation for WINCE

Humans are definitely error-prone devices...   And sadly, I've never been able to spell. :)

Still looks good to me.

[George Staikos]

Comment on attachment 34366 [details]
W3C Geolocation for WINCE


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

[George Staikos]

(In reply to comment #7)
> (From update of attachment 34366 [details])
> Humans are definitely error-prone devices...   And sadly, I've never been able
> to spell. :)
> 
> Still looks good to me.

I think you need to read the patch you're reviewing.

[Kwang Yul Seo]

Created attachment 34369 [details]
W3C Geolocation for WINCE

- Remove goto after ASSERT
- Initialize ProvidesXXX variables directly without if().

[Kwang Yul Seo]

Staikos: We are preparing to release the open source portion of our source code
to the public soon. We use the source code of your WINCE port, but have diverged in some ways. We are willing to contribute our code in the area where 
it does not conflict with yours.

This is my first patch. Thank you for reviewing the patch.

[George Staikos]

Comment on attachment 34369 [details]
W3C Geolocation for WINCE

You still have the goto there?  I'm not fundamentally opposed to it but it seems to be against webkit policy according to previous reviewers.

Also it should all be guarded with ENABLE(GEOLOCATION) I think.

Rest looks acceptable to me.

[George Staikos]

(In reply to comment #11)
> Staikos: We are preparing to release the open source portion of our source code
> to the public soon. We use the source code of your WINCE port, but have
> diverged in some ways. We are willing to contribute our code in the area where 
> it does not conflict with yours.
> 
> This is my first patch. Thank you for reviewing the patch.

I think you need to read the license and fix your copyright acknowledgments as well.  The source should already be available this far in and you need to be properly acknowledging the copyrights which it seems is not the case.  I think this is a fundamental first step before you start contributing to the community.

[Kwang Yul Seo]

Created attachment 34370 [details]
W3C Geolocation for WINCE

- Remove goto.
- Put ENABLE(GEOLOCATION) guards.

[Kwang Yul Seo]

CC'ing Sam and Greg, the Geolocation reviewers.

[George Staikos]

In case you still don't understand, the reason your patch hasn't got r+ so far is because your company is openly and willfully violating the WebKit license and some copyrights.  This is not welcome, and I don't think WebKit wants contributions of any sort from those who are violating the license.

[Kwang Yul Seo]

We released the open source portion of our source code. The source tarballs are available from our web site:

http://www.dorothybrowser.com/sources/


Also, we created the third party copyright and acknowledgements page:

http://www.dorothybrowser.com/?pn=legal



We hope that we can keep a good relationship with the WebKit community. If you find any problem, please let me know so that I can fix it immediately.

[Adam Barth]

Comment on attachment 34370 [details]
W3C Geolocation for WINCE

+ WAIT_OBJECT_0 + 1

This is super ugly.  Can we use enums to remove these magical constants?

+ m_selfRef = 0;

RefPtr is not designed to be use across threads like this.  I'm also not sure if GeolocationServiceThread::~GeolocationServiceThread can close those handles on a random thread.

+ domTimeStamp -= 0x19db1ded53e8000
+ domTimeStamp /= 10000000

Magic constants are the devil.

+ WNDCLASS wcex       = {0};

This and the following lines are not proper style.

+ GeolocationServiceWince.h

One class per file, please.

[Kwang Yul Seo]

I didn't know that RefPtr is not designed be use across threads. I followed the same technique used the DatabaseThread. It also uses RefPtr<DatabaseThread> m_selfRef. Is this a bug too?

I will fix other things as you suggested.

[Adam Barth]

(In reply to comment #19)
> I didn't know that RefPtr is not designed be use across threads. I followed the
> same technique used the DatabaseThread. It also uses RefPtr<DatabaseThread>
> m_selfRef. Is this a bug too?

The problem is that the reference count is not protected by a lock.  If two folks try to increment the count at the same time, the count might only increment by one, leading to disaster.