Bug 28095

Summary: [WINCE] W3C Geolocation backend
Product: WebKit Reporter: Kwang Yul Seo <skyul>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: UNCONFIRMED ---    
Severity: Normal CC: abarth, bolsinga, sam, staikos, syoichi, xhiloh
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Other   
Attachments:
Description Flags
W3C Geolocation for WINCE
staikos: review-
W3C Geolocation for WINCE
staikos: review-
W3C Geolocation for WINCE
staikos: review-
W3C Geolocation for WINCE abarth: review-

Description Kwang Yul Seo 2009-08-08 00:18:23 PDT
Implement W3C Geolocation backend with WINCE's GPS Intermediate Driver functions.
Comment 1 Kwang Yul Seo 2009-08-08 00:31:08 PDT
Created attachment 34364 [details]
W3C Geolocation for WINCE
Comment 2 Kwang Yul Seo 2009-08-08 00:39:26 PDT
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.
Comment 3 Eric Seidel (no email) 2009-08-08 07:41:11 PDT
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. :)
Comment 4 George Staikos 2009-08-08 07:45:57 PDT
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?
Comment 5 George Staikos 2009-08-08 07:47:53 PDT
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.
Comment 6 Kwang Yul Seo 2009-08-08 08:46:50 PDT
Created attachment 34366 [details]
W3C Geolocation for WINCE

Fix the spelling errors in the class name and change m_hXXX to m_xxxHandle.
Comment 7 Eric Seidel (no email) 2009-08-08 08:51:21 PDT
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.
Comment 8 George Staikos 2009-08-08 08:54:32 PDT
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.
Comment 9 George Staikos 2009-08-08 08:55:24 PDT
(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.
Comment 10 Kwang Yul Seo 2009-08-08 09:25:49 PDT
Created attachment 34369 [details]
W3C Geolocation for WINCE

- Remove goto after ASSERT
- Initialize ProvidesXXX variables directly without if().
Comment 11 Kwang Yul Seo 2009-08-08 09:28:37 PDT
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.
Comment 12 George Staikos 2009-08-08 09:33:28 PDT
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.
Comment 13 George Staikos 2009-08-08 09:36:16 PDT
(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.
Comment 14 Kwang Yul Seo 2009-08-08 10:18:22 PDT
Created attachment 34370 [details]
W3C Geolocation for WINCE

- Remove goto.
- Put ENABLE(GEOLOCATION) guards.
Comment 15 Kwang Yul Seo 2009-08-26 01:02:18 PDT
CC'ing Sam and Greg, the Geolocation reviewers.
Comment 16 George Staikos 2009-08-26 09:17:28 PDT
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.
Comment 17 Kwang Yul Seo 2009-08-27 09:19:48 PDT
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.
Comment 18 Adam Barth 2009-09-01 18:05:56 PDT
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.
Comment 19 Kwang Yul Seo 2009-09-03 19:58:59 PDT
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.
Comment 20 Adam Barth 2009-09-04 08:34:54 PDT
(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.