Bug 11433

Summary: Fixes to get WebKit to run on windows
Product: WebKit Reporter: Bertrand Guiheneuf <guiheneuf>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Blocker CC: ap, guiheneuf, kkowalczyk
Priority: P1    
Version: 420+   
Hardware: PC   
OS: Windows XP   
Attachments:
Description Flags
Patch to get WebKit to compile and run on win32. Requires AffineTransformCairo.cpp too.
mjs: review-
Implementation of AffineTransform for Cairo
mjs: review-
Adaption of win32 patch to current TOT
none
Fix mapRect
mjs: review-
Main patch with correct change logs
mjs: review+
AffineTransformCairo.cpp mjs: review+

Description Bertrand Guiheneuf 2006-10-27 07:18:31 PDT
Bug fixes to get WebKit to compile again on Win32. 
Mostly adaptions to the new loader. 
Also implemented AffineTranform for cairo.
Comment 1 Bertrand Guiheneuf 2006-10-27 07:19:54 PDT
Created attachment 11241 [details]
Patch to get WebKit to compile and run on win32. Requires AffineTransformCairo.cpp too.
Comment 2 Bertrand Guiheneuf 2006-10-27 07:21:17 PDT
Created attachment 11242 [details]
Implementation of AffineTransform for Cairo
Comment 3 Maciej Stachowiak 2006-10-31 04:12:10 PST
Comment on attachment 11242 [details]
Implementation of AffineTransform for Cairo

Thanks for hte patch!

Mostly this looks good. However, the implementation of mapRect is not correct. Mapping a rect is not equivalent to mapping a point and a distance. Consider the case of a rotated rect. In that case, you need to get the final bounding box accounting for rotation. This is what the CG and Qt implementatios will do.

Also, the patch should include a ChangeLog entry.

Please address these issues and resubmit.
Comment 4 Bertrand Guiheneuf 2006-10-31 04:21:38 PST
Hmm, yes, you're right, my implementation of rect transformation made no sense. Sorry for that. I'll resubmit later today. 
Comment 5 Maciej Stachowiak 2006-11-01 19:17:29 PST
I think this patch, unfortunately, has been overtaken by the pace of other changes.

> BrowserExtensionWin.cpp

BrowserExtension is gone now, these changes will need to be made elsewhere.

The other changes look ok. Please update the BrowserExtension changes and make the fix I mentioned to AffineTransformCairo (I think the right way to map a rect is to map each corner and find the bounding box). 

This bug also included an AffineTransformCairo, so you may not need it if that one ends first: http://bugs.webkit.org/show_bug.cgi?id=11405
Comment 6 Maciej Stachowiak 2006-11-01 19:20:09 PST
Comment on attachment 11241 [details]
Patch to get WebKit to compile and run on win32. Requires AffineTransformCairo.cpp too.

r- for the minor issues above, please fix and resubmit. Also please make sure to include a ChangeLog.
Comment 7 Bertrand Guiheneuf 2006-11-02 11:10:08 PST
Created attachment 11349 [details]
Adaption of win32 patch to current TOT

* Adapt to recent changes
* Add changelog
Comment 8 Bertrand Guiheneuf 2006-11-02 11:11:36 PST
Created attachment 11350 [details]
Fix mapRect 

mapRect now returns the enclosing rect of transformed rect.
Comment 9 Bertrand Guiheneuf 2006-11-02 11:40:58 PST
Created attachment 11351 [details]
Main patch with correct change logs

Moved webkit related change descriptions into WebKit/ChangeLog 
Sorry for the spam.
Comment 10 Maciej Stachowiak 2006-11-03 16:13:59 PST
Comment on attachment 11350 [details]
Fix mapRect 

It's against the coding style guidelines to write single-line ifs with elses like this:

if (px < enclosingRectMinX) enclosingRectMinX = px;
else if (px > enclosingRectMaxX) enclosingRectMaxX = px;
    
It would probably be simpler to write this by computing all four transformed points and then using std::min and std::max to get the final coordinates, but this way is fine too if you make it follow the style guidelines.

r- for the style issue.
Comment 11 Bertrand Guiheneuf 2006-11-03 18:05:24 PST
I dont' see anything about this identation style in http://webkit.org/coding/coding-style.html
Please explain.
Comment 12 Bertrand Guiheneuf 2006-11-03 18:36:16 PST
Created attachment 11372 [details]
AffineTransformCairo.cpp

added the missing CR...
Comment 13 Maciej Stachowiak 2006-11-04 03:00:20 PST
Comment on attachment 11372 [details]
AffineTransformCairo.cpp

r=me
Comment 14 Maciej Stachowiak 2006-11-04 03:01:45 PST
Comment on attachment 11351 [details]
Main patch with correct change logs

r=me
Comment 15 Alexey Proskuryakov 2006-11-04 07:59:57 PST
Committed revision 17592.