Summary: | Implement GraphicsContext::fillRoundRect() for WINCE port | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Crystal Zhang <haizhang> | ||||||||||||||||||||||
Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | eric, staikos | ||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||
Hardware: | Other | ||||||||||||||||||||||||
OS: | Other | ||||||||||||||||||||||||
Attachments: |
|
Description
Crystal Zhang
2009-07-30 11:12:44 PDT
Created attachment 33793 [details]
patch to implement fillRoundRect
patch to implement fillRoundRect().
Comment on attachment 33793 [details]
patch to implement fillRoundRect
No need to include "Written by" in this patch and please use the proper/consistent email address
You should use 0 instead of NULL.
You have braces around IntersectClipRect twice where the coding guidelines say there should not be (not that I agree...)
The rest seems fine.
Created attachment 33798 [details]
Updated patch to implement fillRoundRect
Comment on attachment 33798 [details] Updated patch to implement fillRoundRect > + Written by Crystal Zhang <crystal.zhang@torchmobile.com> That should be removed on checkin. Comment on attachment 33798 [details]
Updated patch to implement fillRoundRect
Round() was renamed as pointed out by Yong. Need to revisit this patch.
Created attachment 33809 [details]
Updated patch again
Update patch according to review.
Comment on attachment 33809 [details]
Updated patch again
No no no. This is a bunch of copy/paste code. Please use functions (static inline perhaps?) instead. George should be able to help you.
Created attachment 34314 [details]
Refactor fillRoundRect()
Comment on attachment 34314 [details]
Refactor fillRoundRect()
What is "trRect" supposed to mean?
1244 IntRect trRect = fillRect;
See the webkit style guidelines about naming variables.
We really need a IntRet::centerPoint() function for this sort of thing:
// Draw top left half
1270 RECT clipRect(rectWin);
1271 clipRect.right = rectWin.left + (rectWin.right - rectWin.left) / 2;
1272 clipRect.bottom = rectWin.top + (rectWin.bottom - rectWin.top) / 2;
5 bool newClip;
1276 if (GetClipRgn(dc, clipRgn.get()) <= 0)
1277 newClip = true;
1278 else
1279 newClip = false;
simpler as:
bool needsNewClip = (GetClipRgn(dc, clipRegion.get()) <= 0);
Needs better variable names too.
Again, we need a "center point" function. compute it once, and then use it to make the various different rects you need.
1283 // Draw top right
1284 clipRect = rectWin;
1285 clipRect.left = rectWin.left + (rectWin.right - rectWin.left) / 2;
1286 clipRect.bottom = rectWin.top + (rectWin.bottom - rectWin.top) / 2;
IntPoint centerPoint(const IntRect&) can just be a static inline for now.
Please run check-webkit-style:
317 } else {
1318 IntersectClipRect(dc, clipRect.left, clipRect.top, clipRect.right, clipRect.bottom);
1319 }
What does "newClip" mean? Please give it a more descriptive name.
22 if (newClip)
1323 SelectClipRgn(dc, NULL);
1324 else
1325 SelectClipRgn(dc, clipRgn.get());
can be written as:
SelectClipRgn(dc, needsNewClip ? 0 : clipRgn.get())
WE don't use NULL in c++ code.
I think George should review all WinCE patches before they're posted here. We're just wasting reviewer time catching basic errors like these. Created attachment 34476 [details]
Made some improvements to the code
Comment on attachment 34476 [details]
Made some improvements to the code
Did you miss a change to the .h file? It seems like a new method is added.
Created attachment 34484 [details]
Add update to .h file
Comment on attachment 34484 [details] Add update to .h file > --- a/WebCore/platform/graphics/GraphicsContext.h > +++ b/WebCore/platform/graphics/GraphicsContext.h > @@ -221,6 +221,8 @@ namespace WebCore { > void fillRect(const FloatRect&, Generator&); > void fillRoundedRect(const IntRect&, const IntSize& topLeft, const IntSize& topRight, const IntSize& bottomLeft, const IntSize& bottomRight, const Color&); > > + void drawRoundCorner(bool newClip, RECT clipRect, RECT rectWin, HDC dc, int width, int height); > + > void clearRect(const FloatRect&); > > void strokeRect(const FloatRect&); That should be in a PLATFORM(WINCE) place or it will break the build for others. Created attachment 34492 [details]
Move drawRoundCorner's declaration to PLATFORM(WINCE) place
Comment on attachment 34492 [details] Move drawRoundCorner's declaration to PLATFORM(WINCE) place > + if(!dc) > + return; A space should be added after "if" when checking in. Created attachment 34497 [details]
Coding style change, use format-patch to generate patch
Created attachment 34501 [details]
Fix the errors when applying the patch.
Fix the errors when applying the patch.
Created attachment 34516 [details]
Fix the errors when applying the patch.
Comment on attachment 34516 [details]
Fix the errors when applying the patch.
When committing, if it's not applying cleanly please just manually merge if possible. This could be a never ending game that the files are changing underneath the patch between r+ and commit.
The commit queue isn't capable of any manual merging. If the commit fails, it will just be marked as commit-queue- and a committer (like yourself) will have to land it manually. "bugzilla-tool apply-patches" can help with this. If you're worry about it getting stale or needing manual intervention, probably best to just do it yourself. Has been fixed and committed. |