[webkit-reviews] review denied: [Bug 27842] Implement GraphicsContext::fillRoundRect() for WINCE port : [Attachment 34314] Refactor fillRoundRect()

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Crystal Zhang
<crystal.zhang at torchmobile.com>'s request for review:
Bug 27842: Implement GraphicsContext::fillRoundRect() for WINCE port
https://bugs.webkit.org/show_bug.cgi?id=27842

Attachment 34314: Refactor fillRoundRect()
https://bugs.webkit.org/attachment.cgi?id=34314&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
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.