[webkit-reviews] review denied: [Bug 27349] [WINCE] Add GraphicsContextWince implementation for WinCE. : [Attachment 33091] coding style modifications and latest fixes

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has denied Yong Li
<yong.li at torchmobile.com>'s request for review:
Bug 27349: [WINCE] Add GraphicsContextWince implementation for WinCE.
https://bugs.webkit.org/show_bug.cgi?id=27349

Attachment 33091: coding style modifications and latest fixes
https://bugs.webkit.org/attachment.cgi?id=33091&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
> +#include "wtf/OwnPtr.h"

"wtf/OwnPtr.h" should be <wtf/OwnPtr.h> (with angle brackets instead of
quotes).

> +struct RotationTransform
> +{

The opening brace for type declarations (struct, class, enum) should be on the
same line as the type name. (Note that this differs from the rule for
functions.) (applies elsewhere)

> +    IntPoint origin()

This can be a const member function.

> +    IntRect mapRect(const IntRect& rect)
> +    {
> +	   return m_transform.mapRect(rect);
> +    }
> +
> +    FloatRect mapRect(const FloatRect& rect)
> +    {
> +	   return m_transform.mapRect(rect);
> +    }
> +
> +    IntPoint mapPoint(const IntPoint& point)
> +    {
> +	   return m_transform.mapPoint(point);
> +    }
> +
> +    FloatPoint mapPoint(const FloatPoint& point)
> +    {
> +	   return m_transform.mapPoint(point);
> +    }
> +
> +    FloatSize mapSize(const FloatSize& size)
> +    {
> +	   double w, h;
> +	   m_transform.map(size.width(), size.height(), w, h);
> +	   return FloatSize(static_cast<float>(w), static_cast<float>(h));
> +    }

These can be const member functions.

> +    PassRefPtr<SharedBitmap> getTransparentLayerBitmap(IntRect& origRect,
AlphaPaintType alphaPaint, RECT& bmpRect, bool checkClipBox, bool force)
> +    {
> +	   if (m_opacity <= 0)
> +	       return 0;
> +
> +	   if (force || m_opacity < 1.)  {
> +	       if (checkClipBox) {
> +		   RECT clipBox;
> +		   int clipType = GetClipBox(m_dc, &clipBox);
> +		   if (clipType == SIMPLEREGION || clipType == COMPLEXREGION)
> +		       origRect.intersect(clipBox);
> +		   if (origRect.isEmpty())
> +		       return 0;
> +	       }
> +
> +	       RefPtr<SharedBitmap> bmp =
SharedBitmap::createInstance(alphaPaint == AlphaPaintNone, origRect.width(),
origRect.height(), false);
> +	       SetRect(&bmpRect, 0, 0, origRect.width(), origRect.height());
> +	       if (bmp) {
> +		   switch (alphaPaint) {
> +		   case AlphaPaintNone:
> +		   case AlphaPaintImage:
> +		       {
> +			   SharedBitmap::DCHolder dc(bmp.get());
> +			   if (dc.get()) {
> +			       BitBlt(dc.get(), 0, 0, origRect.width(),
origRect.height(), m_dc, origRect.x(), origRect.y(), SRCCOPY);
> +			       if (bmp->is32bit() && (!m_bitmap ||
m_bitmap->is16bit())) {
> +				   // Set alpha channel
> +				   unsigned* pixels = (unsigned*)bmp->bytes();

Please use a C++-style cast here.

> +template <typename PixelType, bool Is16bit> void _rotateBitmap(SharedBitmap*
destBmp, const SharedBitmap* sourceBmp, const RotationTransform& transform)

This can be marked "static" so it gets internal linkage.

> +{
> +    int destW = destBmp->width();
> +    int destH = destBmp->height();
> +    int sourceW = sourceBmp->width();
> +    int sourceH = sourceBmp->height();
> +    PixelType* dest = (PixelType*)destBmp->bytes();
> +    const PixelType* source = (const PixelType*)sourceBmp->bytes();

Please use C++-style casts here.

> +void rotateBitmap(SharedBitmap* destBmp, const SharedBitmap* sourceBmp,
const RotationTransform& transform)

This can be marked "static" so it gets internal linkage.

> +void GraphicsContext::drawRect(const IntRect& rect)
> +{
> +    if (!m_data->m_opacity || paintingDisabled() || rect.width() <= 0 ||
rect.height() <= 0)
> +	   return;

I thought you were going to use rect.isEmpty() instead of the width() and
height() checks?

> +#define EQUAL_ANGLE(a, b) (fabs((a) - (b)) < 1e-5)

An inline function would be nicer.

> +void GraphicsContext::setPlatformShouldAntialias(bool should)
> +{
> +    (void)should;
> +    notImplemented();
> +}
> +
> +void GraphicsContext::setLineDash(const DashArray&, float dashOffset)
> +{
> +    (void)dashOffset;
> +    notImplemented();
> +}

As I said before, you should either use UNUSED_PARAM from wtf/UnusedParam.h, or
omit the parameter names entirely.

This patch still has many instances of names like "transparentDc" instead of
"transparentDC", and still has the single-parameter-per-line style for some
function calls. Please fix those.