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
Kwang Yul Seo
2009-08-08 00:18:23 PDT
Created attachment 34364 [details]
W3C Geolocation for WINCE
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 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 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?
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. Created attachment 34366 [details]
W3C Geolocation for WINCE
Fix the spelling errors in the class name and change m_hXXX to m_xxxHandle.
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 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. (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. Created attachment 34369 [details]
W3C Geolocation for WINCE
- Remove goto after ASSERT
- Initialize ProvidesXXX variables directly without if().
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 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.
(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. Created attachment 34370 [details]
W3C Geolocation for WINCE
- Remove goto.
- Put ENABLE(GEOLOCATION) guards.
CC'ing Sam and Greg, the Geolocation reviewers. 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. 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 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.
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. (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. |