Bug 11433 - Fixes to get WebKit to run on windows
Summary: Fixes to get WebKit to run on windows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 420+
Hardware: PC Windows XP
: P1 Blocker
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-10-27 07:18 PDT by Bertrand Guiheneuf
Modified: 2006-11-04 07:59 PST (History)
3 users (show)

See Also:


Attachments
Patch to get WebKit to compile and run on win32. Requires AffineTransformCairo.cpp too. (44.83 KB, patch)
2006-10-27 07:19 PDT, Bertrand Guiheneuf
mjs: review-
Details | Formatted Diff | Diff
Implementation of AffineTransform for Cairo (5.49 KB, patch)
2006-10-27 07:21 PDT, Bertrand Guiheneuf
mjs: review-
Details | Formatted Diff | Diff
Adaption of win32 patch to current TOT (56.14 KB, patch)
2006-11-02 11:10 PST, Bertrand Guiheneuf
no flags Details | Formatted Diff | Diff
Fix mapRect (6.64 KB, patch)
2006-11-02 11:11 PST, Bertrand Guiheneuf
mjs: review-
Details | Formatted Diff | Diff
Main patch with correct change logs (56.45 KB, patch)
2006-11-02 11:40 PST, Bertrand Guiheneuf
mjs: review+
Details | Formatted Diff | Diff
AffineTransformCairo.cpp (6.57 KB, patch)
2006-11-03 18:36 PST, Bertrand Guiheneuf
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.