Bug 27842 - Implement GraphicsContext::fillRoundRect() for WINCE port
Summary: Implement GraphicsContext::fillRoundRect() for WINCE port
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-07-30 11:12 PDT by Crystal Zhang
Modified: 2009-08-11 07:19 PDT (History)
2 users (show)

See Also:


Attachments
patch to implement fillRoundRect (5.53 KB, patch)
2009-07-30 11:14 PDT, Crystal Zhang
staikos: review-
Details | Formatted Diff | Diff
Updated patch to implement fillRoundRect (5.50 KB, patch)
2009-07-30 11:51 PDT, Crystal Zhang
staikos: review-
Details | Formatted Diff | Diff
Updated patch again (5.48 KB, patch)
2009-07-30 12:54 PDT, Crystal Zhang
eric: review-
Details | Formatted Diff | Diff
Refactor fillRoundRect() (4.69 KB, patch)
2009-08-07 12:50 PDT, Crystal Zhang
eric: review-
Details | Formatted Diff | Diff
Made some improvements to the code (4.60 KB, patch)
2009-08-10 09:13 PDT, Crystal Zhang
no flags Details | Formatted Diff | Diff
Add update to .h file (5.28 KB, patch)
2009-08-10 11:10 PDT, Crystal Zhang
staikos: review-
Details | Formatted Diff | Diff
Move drawRoundCorner's declaration to PLATFORM(WINCE) place (5.52 KB, patch)
2009-08-10 11:43 PDT, Crystal Zhang
staikos: review+
Details | Formatted Diff | Diff
Coding style change, use format-patch to generate patch (5.50 KB, patch)
2009-08-10 12:20 PDT, Crystal Zhang
staikos: review+
staikos: commit-queue+
Details | Formatted Diff | Diff
Fix the errors when applying the patch. (5.97 KB, patch)
2009-08-10 13:05 PDT, Crystal Zhang
no flags Details | Formatted Diff | Diff
Fix the errors when applying the patch. (6.03 KB, patch)
2009-08-10 14:30 PDT, Crystal Zhang
staikos: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Crystal Zhang 2009-07-30 11:12:44 PDT
Implement GraphicsContext::fillRoundRect() to draw box shadow.
Comment 1 Crystal Zhang 2009-07-30 11:14:03 PDT
Created attachment 33793 [details]
patch to implement fillRoundRect

patch to implement fillRoundRect().
Comment 2 George Staikos 2009-07-30 11:35:51 PDT
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.
Comment 3 Crystal Zhang 2009-07-30 11:51:55 PDT
Created attachment 33798 [details]
Updated patch to implement fillRoundRect
Comment 4 George Staikos 2009-07-30 11:55:34 PDT
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 5 George Staikos 2009-07-30 11:58:18 PDT
Comment on attachment 33798 [details]
Updated patch to implement fillRoundRect

Round() was renamed as pointed out by Yong.  Need to revisit this patch.
Comment 6 Crystal Zhang 2009-07-30 12:54:04 PDT
Created attachment 33809 [details]
Updated patch again

Update patch according to review.
Comment 7 Eric Seidel (no email) 2009-08-06 18:34:28 PDT
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.
Comment 8 Crystal Zhang 2009-08-07 12:50:54 PDT
Created attachment 34314 [details]
Refactor fillRoundRect()
Comment 9 Eric Seidel (no email) 2009-08-07 13:59:32 PDT
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.
Comment 10 Eric Seidel (no email) 2009-08-07 14:01:13 PDT
I think George should review all WinCE patches before they're posted here.  We're just wasting reviewer time catching basic errors like these.
Comment 11 Crystal Zhang 2009-08-10 09:13:24 PDT
Created attachment 34476 [details]
Made some improvements to the code
Comment 12 George Staikos 2009-08-10 10:47:00 PDT
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.
Comment 13 Crystal Zhang 2009-08-10 11:10:46 PDT
Created attachment 34484 [details]
Add update to .h file
Comment 14 George Staikos 2009-08-10 11:31:40 PDT
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.
Comment 15 Crystal Zhang 2009-08-10 11:43:41 PDT
Created attachment 34492 [details]
Move drawRoundCorner's declaration to PLATFORM(WINCE) place
Comment 16 George Staikos 2009-08-10 11:49:59 PDT
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.
Comment 17 Crystal Zhang 2009-08-10 12:20:44 PDT
Created attachment 34497 [details]
Coding style change, use format-patch to generate patch
Comment 18 Crystal Zhang 2009-08-10 13:05:25 PDT
Created attachment 34501 [details]
Fix the errors when applying the patch.

Fix the errors when applying the patch.
Comment 19 Crystal Zhang 2009-08-10 14:30:40 PDT
Created attachment 34516 [details]
Fix the errors when applying the patch.
Comment 20 George Staikos 2009-08-10 15:04:05 PDT
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.
Comment 21 Eric Seidel (no email) 2009-08-10 19:00:01 PDT
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.
Comment 22 Crystal Zhang 2009-08-11 07:19:45 PDT
Has been fixed and committed.