Re: Hewlett-Packard Patch #1 - Getting CMake WinCE build working

[Patrick Gansterer]

Hi,

very cool to see this work!

I commented now here in the mail, but I think it might be better to move this discussion into (a) WebKit bugzilla or (b) GitHub to have a better context of the code.

On Thursday, July 12, 2012 11:18:42 PM UTC+2, Mark Salisbury wrote:

About Me:

I work for Hewlett-Packard.  I've been working on WebKit for close to 3 years now; we've been using WebKit at HP on high-end laserjet MFPs to help power our control panel user interface.  I've been involved in creating a new port of webkit using a closed/legacy graphics toolkit running on LynxOS as well as running webkit on products using Windows XP and Windows CE.  I've worked with my company's review board to get the OK on contributing work back to the community.  I believe I have some changes that will be valuable for the community.

I created a clone of webkit on github (if I had read these instructions - http://trac.webkit.org/wiki/WinCE - I probably would have used gitorious.org instead.  Does it matter to people which is used?).  Here's my clone:  https://github.com/masali-hp/webkit 

I have a series of patches that I'd like to see landed on webkit.org.  A lot of my work is focused on running webkit on Windows CE.  I think a good first step is getting feedback from other WinCE developers (you guys) on these changes.  Then I'll submit bugs/patches to webkit.org.

Change #1 - Get the CMake WinCE build (http://trac.webkit.org/wiki/WinCE) working again

You can view the patch here: https://github.com/masali-hp/webkit/commit/20aa3e3556b83e7b21e9e8cb2a611195799ad291

If you add a stub pthreads.h, the port builds, as the WinCE buildbot shows.

So some changes are not correct. The current "reference system" is the freely downloadable STANDARD_SDK500. Everything else should be solved a  device specific way: Is the timeBeginPeriod problem really WinCE specify, or is it only a problem with the implementation on your device? I've never seen the mentioned deadlock.

I try the get rid of ce_time.h, because it's no official header. Most of the remaining occurrences can be replaces with corresponding Windows API functions. E.g. https://bugs.webkit.org/show_bug.cgi?id=83436 (had no time to commit it and watch for regressions).

 

Points I'd want to bring up about this patch:

1)  We went with a surgical approach to solving the compile issue in MachineStackMarker.cpp.  Patrick Gansterer submitted a more general solution here to get this fixed:  https://bugs.webkit.org/show_bug.cgi?id=68429 

It seems the reason this patch wasn't accepted is that there are concerns about the destructor deleting a thread key when there could still be references to data that that key points to.  Maybe a patch that is specific to MachineStackMarker.cpp is more acceptable?

If you look at the obsolete patches you'll find similar patches. The only acceptable solution it to create a more generic one.

And: Your change is wrong, since you never call removeThread!

 

2)  Changes to OptionsWindows.cmake

- I've found that some code that checks to see if UNICODE is defined looks for _UNICODE, not UNICODE, so I've defined both.

Where is it required? My plan is to get rid of the UNICODE defines as a whole, and use the Unicode functions (ending with W) directly.

 

- I put in compiler build switches to disable RTTI and to generate PDBs (even for release builds).

Disabling RTTI might be ok, but for PDBs use the ReleaseWithDebugInfo configuration.

 

- Instead of redefining mainACRTStartup to WinMainCRTStartup, I set the linker subsystem build flag to WINDOWSCE.  I set it to /SUBSYSTEM:WINDOWSCE,6.0 which I expect would not cause a problem for CE 5.0.  I don't have 5.0 around.  If this does cause any issues for 5.0 leaving it at just /SUBSYSTEM:WINDOWSCE will probably work too.

I don't "like" this change: I created a simple working port for CMake for the WinCE port in the early days. Maybe this should be fixed in CMake code and not in WebKit. IMHO there must be a possibility to set the entry point with a CMake flag. Maybe there need to be added some code to CMake for this feature, but already my "STRING(REPLACE)" is very ugly and uses http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/os-win32/WinMain.cpp, which is ugly too.

 

- WebKit ends up creating a fair number of linker "importing an exported symbol" warnings.  We disable linker error 4049 to quiet these.

Do you know what causes this warnings? I still think there might be some miss configuration, so I didn't disabled them yet.

-- Patrick

[Mark Salisbury]

Thanks for reviewing this patch.  Per your suggestion, I'll post my response on github to have better context of the code:  https://github.com/masali-hp/webkit/commit/20aa3e3556b83e7b21e9e8cb2a611195799ad291 

[Mark Salisbury]

I've added a new branch to git hub, incorporating feedback from Patrick on the first branch:

https://github.com/masali-hp/webkit/commits/marks_branch_2

Feedback from anyone is welcome.  You should be able to post comments directly on github.

I will be away from work next week (we're expecting a baby boy...), so I may not get back until the following week...