Bug 27349 – [WINCE] Add GraphicsContextWince implementation for WinCE.

• Component: Platform
• Status: RESOLVED FIXED

[Adam Treat]

The following patch adds the GraphicsContextWince implementation for WinCE.

[Adam Treat]

Created attachment 32884 [details]
Adds GraphicsContextWince.cpp

[Adam Roben (:aroben)]

Comment on attachment 32884 [details]
Adds GraphicsContextWince.cpp

I'm sorry I didn't get to this today. I will look at it tomorrow!

Before I review this, could you run the code through the new cpplint.py script in WebKitTools/Scripts/modules? That should point out a number of style issues that it would be nice to fix before a real review happens.

[Adam Treat]

I've already run it through cpplint :)

[Adam Roben (:aroben)]

(In reply to comment #3)
> I've already run it through cpplint :)

Excellent!

[Adam Roben (:aroben)]

Comment on attachment 32884 [details]
Adds GraphicsContextWince.cpp

There are many comments below that apply to many parts of the code. I've marked these with "(applies elsewhere)" instead of repeating the comments in each case.

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 3b18cab..a0708ac 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,115 @@
> [phone]  Yong Li  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=27349
> +        Add GraphicsContext implementation for the WinCE port.
> +
> +        Written by Yong Li <[e-mail]> and George Staikos <[e-mail]>
> +        with trivial style fixes by Adam Treat <[e-mail]>
> +
> +        * platform/graphics/wince/GraphicsContextWince.cpp: Added.
> +        (WebCore::isZero):
> +        (WebCore::Round):
> +        (WebCore::roundRect):
> +        (WebCore::RotationTransform::RotationTransform):
> +        (WebCore::RotationTransform::operator-):
> +        (WebCore::RotationTransform::map):
> +        (WebCore::mapPoint):
> +        (WebCore::mapRect):
> +        (WebCore::GraphicsContextPlatformPrivateData::GraphicsContextPlatformPrivateData):
> +        (WebCore::GraphicsContextPlatformPrivate::GraphicsContextPlatformPrivate):
> +        (WebCore::GraphicsContextPlatformPrivate::~GraphicsContextPlatformPrivate):
> +        (WebCore::GraphicsContextPlatformPrivate::origin):
> +        (WebCore::GraphicsContextPlatformPrivate::translate):
> +        (WebCore::GraphicsContextPlatformPrivate::scale):
> +        (WebCore::GraphicsContextPlatformPrivate::rotate):
> +        (WebCore::GraphicsContextPlatformPrivate::concatCTM):
> +        (WebCore::GraphicsContextPlatformPrivate::mapRect):
> +        (WebCore::GraphicsContextPlatformPrivate::mapPoint):
> +        (WebCore::GraphicsContextPlatformPrivate::mapSize):
> +        (WebCore::GraphicsContextPlatformPrivate::save):
> +        (WebCore::GraphicsContextPlatformPrivate::restore):
> +        (WebCore::GraphicsContextPlatformPrivate::getTransparentLayerBitmap):
> +        (WebCore::GraphicsContextPlatformPrivate::paintBackTransparentLayerBitmap):
> +        (WebCore::createPen):
> +        (WebCore::createBrush):
> +        (WebCore::rotateBitmap):
> +        (WebCore::TransparentLayerDC::TransparentLayerDC):
> +        (WebCore::TransparentLayerDC::~TransparentLayerDC):
> +        (WebCore::TransparentLayerDC::hdc):
> +        (WebCore::TransparentLayerDC::rect):
> +        (WebCore::TransparentLayerDC::toShift):
> +        (WebCore::ScopeDCProvider::ScopeDCProvider):
> +        (WebCore::ScopeDCProvider::~ScopeDCProvider):
> +        (WebCore::GraphicsContext::GraphicsContext):
> +        (WebCore::GraphicsContext::~GraphicsContext):
> +        (WebCore::GraphicsContext::setBitmap):
> +        (WebCore::GraphicsContext::getWindowsContext):
> +        (WebCore::GraphicsContext::releaseWindowsContext):
> +        (WebCore::GraphicsContext::savePlatformState):
> +        (WebCore::GraphicsContext::restorePlatformState):
> +        (WebCore::GraphicsContext::drawRect):
> +        (WebCore::GraphicsContext::drawLine):
> +        (WebCore::GraphicsContext::drawEllipse):
> +        (WebCore::getEllipsePointByAngle):
> +        (WebCore::getEllipseY):
> +        (WebCore::getEllipseX):
> +        (WebCore::GraphicsContext::strokeArc):
> +        (WebCore::GraphicsContext::drawConvexPolygon):
> +        (WebCore::fillRectWithAlpha):
> +        (WebCore::GraphicsContext::fillRect):
> +        (WebCore::GraphicsContext::clip):
> +        (WebCore::GraphicsContext::clipOut):
> +        (WebCore::GraphicsContext::drawFocusRing):
> +        (WebCore::GraphicsContext::drawLineForText):
> +        (WebCore::GraphicsContext::drawLineForMisspellingOrBadGrammar):
> +        (WebCore::GraphicsContext::setPlatformFillColor):
> +        (WebCore::GraphicsContext::setPlatformStrokeColor):
> +        (WebCore::GraphicsContext::setPlatformStrokeThickness):
> +        (WebCore::GraphicsContext::setURLForRect):
> +        (WebCore::GraphicsContext::addInnerRoundedRectClip):
> +        (WebCore::GraphicsContext::clearRect):
> +        (WebCore::GraphicsContext::strokeRect):
> +        (WebCore::GraphicsContext::beginTransparencyLayer):
> +        (WebCore::GraphicsContext::endTransparencyLayer):
> +        (WebCore::GraphicsContext::concatCTM):
> +        (WebCore::GraphicsContext::affineTransform):
> +        (WebCore::GraphicsContext::resetAffineTransform):
> +        (WebCore::GraphicsContext::translate):
> +        (WebCore::GraphicsContext::rotate):
> +        (WebCore::GraphicsContext::origin):
> +        (WebCore::GraphicsContext::scale):
> +        (WebCore::GraphicsContext::mapRect):
> +        (WebCore::GraphicsContext::mapPoint):
> +        (WebCore::GraphicsContext::mapSize):
> +        (WebCore::GraphicsContext::setLineCap):
> +        (WebCore::GraphicsContext::setLineJoin):
> +        (WebCore::GraphicsContext::setMiterLimit):
> +        (WebCore::GraphicsContext::setAlpha):
> +        (WebCore::GraphicsContext::setCompositeOperation):
> +        (WebCore::GraphicsContext::beginPath):
> +        (WebCore::GraphicsContext::addPath):
> +        (WebCore::GraphicsContext::clipOutEllipseInRect):
> +        (WebCore::GraphicsContext::fillRoundedRect):
> +        (WebCore::GraphicsContext::roundToDevicePixels):
> +        (WebCore::GraphicsContext::fillPath):
> +        (WebCore::GraphicsContext::strokePath):
> +        (WebCore::GraphicsContext::getCTM):
> +        (WebCore::GraphicsContext::clipToImageBuffer):
> +        (WebCore::GraphicsContext::setPlatformShadow):
> +        (WebCore::GraphicsContext::clearPlatformShadow):
> +        (WebCore::GraphicsContext::setImageInterpolationQuality):
> +        (WebCore::GraphicsContext::drawText):
> +        (WebCore::GraphicsContext::drawFrameControl):
> +        (WebCore::GraphicsContext::drawFocusRect):
> +        (WebCore::GraphicsContext::paintTextField):
> +        (WebCore::GraphicsContext::drawBitmap):
> +        (WebCore::GraphicsContext::drawBitmapPattern):
> +        (WebCore::GraphicsContext::setPlatformShouldAntialias):
> +        (WebCore::GraphicsContext::setLineDash):
> +        (WebCore::GraphicsContext::clipPath):

It's probably better to omit the function list if you aren't adding comments about each function.

> +#include "config.h"
> +#include "GraphicsContext.h"
> +#include "TransformationMatrix.h"
> +#include "NotImplemented.h"
> +#include "wince/MemoryManager.h"
> +#include "OwnPtr.h"
> +#include "SharedBitmap.h"
> +#include "Path.h"
> +#include "PlatformPathWince.h"
> +#include "Gradient.h"
> +#include "GraphicsContextPrivate.h"
> +#include "SimpleFontData.h"
> +#include "GlyphBuffer.h"
> +#include "CharacterNames.h"
> +#include <windows.h>

These should be sorted in ASCII order. You should add a blank line after #include "GraphicsContext.h". "OwnPtr.h" should be <wtf/OwnPtr.h>.

> +#define PI        3.14159265358979323846

wtf/MathExtras.h defines piFloat and piDouble. You should probably use those instead.

> +#define RADIAN(a)    ((a) * PI / 180.)
> +#define DEGREE(a)    ((a) * 180. / PI)

wtf/MathExtras.h has deg2rad and rad2deg functions you can use instead.

> +static inline int Round(double d)

Function names start with a lowercase letter in WebKit. But you can probably just use lround from wtf/MathExtras.h instead.

> +{
> +    if (d > 0)
> +        return int(d + 0.5);

We normally use C++ cast operators instead of function-style casts. So static_cast would be better here. (applies elsewhere)

> +    else {

We don't write "else" after "return". (applies elsewhere)

> +static inline IntRect roundRect(const FloatRect& r)
> +{
> +    return IntRect(Round(r.x()), Round(r.y()), Round(r.right()) - Round(r.x()), Round(r.bottom()) - Round(r.y()));
> +}

Are you sure you don't want to use enclosingIntRect instead?

> +struct RotationTransform { // Origin (0, 0)
> +    double m_cosA;
> +    double m_sinA;
> +    int m_preShiftX;
> +    int m_preShiftY;
> +    int m_postShiftX;
> +    int m_postShiftY;

We often omit the "m_" prefix on struct data members. (applies elsewhere)

> +    void map(double x1, double y1, double* x2, double* y2) const
> +    {
> +        x1 += m_preShiftX;
> +        y1 += m_preShiftY;
> +        *x2 = x1 * m_cosA + y1 * m_sinA + m_postShiftX;
> +        *y2 = y1 * m_cosA - x1 * m_sinA + m_postShiftY;
> +    }

Maybe this would be better as: IntPoint map(const IntPoint&) const

> +    void map(int x1, int y1, int* x2, int* y2) const
> +    {
> +        x1 += m_preShiftX;
> +        y1 += m_preShiftY;
> +        *x2 = Round(x1 * m_cosA + y1 * m_sinA) + m_postShiftX;
> +        *y2 = Round(y1 * m_cosA - x1 * m_sinA) + m_postShiftY;
> +    }

Maybe this would be better as: FloatPoint map(const FloatPoint&) const

> +template<class T> static inline
> +IntPoint mapPoint(const IntPoint& p, const T& t)

I think we normally put the entire function declaration on a single line. (applies elsewhere)

> +template<class Transform, class Rect, class Value> static inline
> +Rect mapRect(const Rect& rect, const Transform& transform)

If you switched the template parameter order to Value, Transform, Rect, then I think you could normally call the function like this: mapRect<Value>(...). The Transform and Rect parameters would be inferred from the parameter types.

> +{
> +    Value x[4], y[4];
> +    Value l, t, r, b;

It might be clearer to give these variables full-word names ("left", "top", etc.).
We don't normally declare multiple variables on a single line. (applies elsewhere)

> +    GraphicsContextPlatformPrivateData()
> +        : m_transform()
> +        , m_opacity(1.0)
> +    {}

Please put the braces on their own lines, or put the entire constructor on a single line (including the initializer list).

> +    GraphicsContextPlatformPrivate(HDC dc)
> +        : m_dc(dc)
> +    {
> +        HDC hScreenDC = ::GetDC(0);
> +        int rtn = ::GetDeviceCaps(hScreenDC, RASTERCAPS);
> +        ::ReleaseDC(0, hScreenDC);
> +    }

What's the point of the three lines in the body of the constructor? They don't seem to do anything. Does calling GetDeviceCaps have some side-effect?

> +    IntPoint origin()
> +    {
> +        return IntPoint(Round(-m_transform.e()),
> +                Round(-m_transform.f()));
> +    }

This could be a const member function.

> +    FloatSize mapSize(const FloatSize& size)
> +    {
> +        double w, h;
> +        m_transform.map(size.width(), size.height(), w, h);
> +        return FloatSize(w, h);
> +    }

Earlier you used static_cast when passing doubles to the FloatSize constructor. It would be good to be consistent one way or the other.

> +    void restore()
> +    {
> +        if (m_backupData.isEmpty())
> +            return;
> +
> +        if (m_dc)
> +            RestoreDC(m_dc, -1);
> +
> +        GraphicsContextPlatformPrivateData::operator=(m_backupData.last());

Does *this = m_backupData.last() not work?

> +    PassRefPtr<SharedBitmap> getTransparentLayerBitmap(IntRect& origRect, bool uses16bit, RECT& bmpRect, bool checkClipBox, bool force)

It seems strange to use a mix of IntRect and RECT in this declaration. (applies elsewhere)

> +    {
> +        if (m_opacity <= 0)
> +            return 0;
> +        else if (force || m_opacity < 1.)  {

You could negate this condition and use an early return.

> +#if !defined(NO_ALPHABLEND)
> +        if (m_opacity < 1. || !use16bit) {
> +            const BLENDFUNCTION blend = { AC_SRC_OVER, 0
> +                , m_opacity >= 1. ? 255 : (BYTE)(m_opacity * 255)
> +                , use16bit ? 0 : AC_SRC_ALPHA };
> +            AlphaBlend
> +                (m_dc
> +                , origRect.x()
> +                , origRect.y()
> +                , origRect.width()
> +                , origRect.height()
> +                , hdc
> +                , 0
> +                , 0
> +                , bmpRect.right
> +                , bmpRect.bottom
> +                , blend
> +                );

I've never seen this one-parameter-per-line style anywhere else in WebKit. I'm not sure it's a good idea to introduce it here. (applies elsewhere)

> +// Only for 16bit
> +void rotateBitmap(SharedBitmap* destBmp, const SharedBitmap* sourceBmp, const RotationTransform& transform)

This should be marked "static" so it gets internal linkage. (applies elsewhere)

> +{
> +    ASSERT(destBmp->is16bit() && sourceBmp->is16bit());

It would be better to use ASSERT_ARG. It would be better to separate this into two assertions.

> +    int destW = destBmp->width();
> +    int destH = destBmp->height();
> +    int sourceW = sourceBmp->width();
> +    int sourceH = sourceBmp->height();
> +    unsigned short* dest = (unsigned short*)destBmp->bytes();
> +    const unsigned short* source = (const unsigned short*)sourceBmp->bytes();
> +    int padding = destW & 1;
> +    int paddedSourceW = sourceW + (sourceW & 1);
> +    if (isZero(transform.m_sinA)) {
> +        int cosA = transform.m_cosA > 0 ? 1 : -1;
> +        for (int y = 0; y < destH; ++y) {
> +            for (int x = 0; x < destW; ++x) {
> +                int x1 = x + transform.m_preShiftX;
> +                int y1 = y + transform.m_preShiftY;
> +                int srcX = x1 * cosA + transform.m_postShiftX;
> +                int srcY = y1 * cosA - transform.m_postShiftY;
> +                if (srcX >= 0 && srcX <= sourceW && srcY >= 0 && srcY <= sourceH)
> +                    *dest++ = source[srcY * paddedSourceW + srcX] | 0xFF000000;
> +                else
> +                    *dest++ |= 0xFF;
> +            }
> +            dest += padding;
> +        }
> +    } else if (isZero(transform.m_cosA)) {
> +        int sinA = transform.m_sinA > 0 ? 1 : -1;
> +        for (int y = 0; y < destH; ++y) {
> +            for (int x = 0; x < destW; ++x) {
> +                int x1 = x + transform.m_preShiftX;
> +                int y1 = y + transform.m_preShiftY;
> +                int srcX = y1 * sinA + transform.m_postShiftX;
> +                int srcY = -x1 * sinA + transform.m_postShiftY;
> +                if (srcX >= 0 && srcX <= sourceW && srcY >= 0 && srcY <= sourceH)
> +                    *dest++ = source[srcY * paddedSourceW + srcX] | 0xFF000000;
> +                else
> +                    *dest++ |= 0xFF;
> +            }
> +            dest += padding;
> +        }
> +    } else {
> +        for (int y = 0; y < destH; ++y) {
> +            for (int x = 0; x < destW; ++x) {
> +                // FIXME: for best quality, we should get weighted sum of four neighbours,
> +                // but that will be too expensive
> +                int srcX, srcY;
> +                transform.map(x, y, &srcX, &srcY);
> +                if (srcX >= 0 && srcX <= sourceW && srcY >= 0 && srcY <= sourceH)
> +                    *dest++ = source[srcY * paddedSourceW + srcX] | 0xFF000000;
> +                else
> +                    *dest++ |= 0xFF;
> +            }
> +            dest += padding;
> +        }
> +    }
> +}

It would be nice to have a little less duplication in this function.

> +    explicit TransparentLayerDC(GraphicsContextPlatformPrivate* data,
> +                                IntRect& origRect,
> +                                const IntRect* rectBeforeTransform = 0,
> +                                int alpha = 255)

"explicit" doesn't really add anything here, since this constructor requires more than one parameter.

> +        : m_data(data)
> +        , m_origRect(origRect)
> +        , m_oldOpacity(data->m_opacity)

It seems error-prone to leave m_key1 and m_key2 uninitialized here.

> +#if 1
> +                FloatPoint topLeft = m_data->m_transform.mapPoint(FloatPoint(rectBeforeTransform->topLeft()));
> +                FloatPoint topRight(rectBeforeTransform->right() - 1, rectBeforeTransform->y());
> +                topRight = m_data->m_transform.mapPoint(topRight);
> +                FloatPoint bottomLeft(rectBeforeTransform->x(), rectBeforeTransform->bottom() - 1);
> +                bottomLeft = m_data->m_transform.mapPoint(bottomLeft);
> +                FloatSize sideTop = topRight - topLeft;
> +                FloatSize sideLeft = bottomLeft - topLeft;
> +                float width = _hypot(sideTop.width() + 1, sideTop.height() + 1);
> +                float height = _hypot(sideLeft.width() + 1, sideLeft.height() + 1);
> +
> +                origRect.inflateX(Round((width - origRect.width()) * 0.5));
> +                origRect.inflateY(Round((height - origRect.height()) * 0.5));
> +#else
> +                FloatRect rotatedBeforeTransform = mapRect(FloatRect(*rectBeforeTransform), -m_rotation);
> +                double shrinkRateX = (1. - rectBeforeTransform->width() / (double)rotatedBeforeTransform.width()) * 0.5;
> +                double shrinkRateY = (1. - rectBeforeTransform->height() / (double)rotatedBeforeTransform.height()) * 0.5;
> +                origRect.inflateX(-Round(shrinkRateX * origRect.width()));
> +                origRect.inflateY(-Round(shrinkRateY * origRect.height()));
> +#endif

We don't like to check in commented-out code. (applies elsewhere)

> +        if (m_bitmap)
> +            m_memDc = m_bitmap->getDC(&m_key1, &m_key2);

m_memDC would match our variable naming conventions better.

> +HDC GraphicsContext::getWindowsContext(const IntRect& dstRect, bool supportAlphaBlend, bool mayCreateBitmap)
> +{
> +    // FIXME: the implementation is buggy. Currently the function seems not being used. Just make an assertion
> +    ASSERT(0);
> +    return 0;

ASSERT_NOT_REACHED() would be better here.

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

You can use rect.isEmpty() instead of the width() and height() checks.

> +    TransparentLayerDC transparentDc(m_data, trRect, &rect);

transparentDC would match our variable naming conventions better. (applies elsewhere)

> +    if (fillColor().alpha()) {
> +        brush = createBrush(fillColor());
> +        oldBrush = SelectObject(dc, brush);
> +    } else {
> +        SelectObject(dc, GetStockObject(NULL_BRUSH));
> +    }

We don't put braces around the bodies of single-line branches.

> +void GraphicsContext::drawEllipse(const IntRect& rect)
> +{
> +    if (!m_data->m_opacity || paintingDisabled() || (!fillColor().alpha() && strokeStyle() == NoStroke))
> +        return;

Should you check that strokeColor().alpha() is non-zero, too?

> +    HGDIOBJ oldBrush = SelectObject(dc, GetStockObject(NULL_BRUSH));
> +    Ellipse(dc, trRect.x(), trRect.y(), trRect.right(), trRect.bottom());
> +    SelectObject(dc, oldBrush);
> +
> +    if (newClip)
> +        SelectClipRgn(dc, 0);

Why don't we need to select the old clip region back in in this case?

> +void GraphicsContext::drawConvexPolygon(size_t npoints, const FloatPoint* points, bool shouldAntialias)
> +{
> +    if (!m_data->m_opacity || paintingDisabled() || npoints <= 1 || !points)
> +        return;
> +
> +    ScopeDCProvider dcProvider(m_data);
> +    if (!m_data->m_dc)
> +        return;
> +
> +    Vector<POINT, 20> winPoints;
> +    winPoints.resize(npoints);

You can just pass npoints to the Vector constructor instead of calling resize. (applies elsewhere)

> +void GraphicsContext::fillRect(const FloatRect& rect, const Color& color)
> +{
> +    if (paintingDisabled() || !m_data->m_opacity)
> +        return;
> +
> +    if (int alpha = color.alpha()) {
> +        OwnPtr<HBRUSH> hbrush(CreateSolidBrush(RGB(color.red(), color.green(), color.blue())));
> +        if (hbrush) {
> +            ScopeDCProvider dcProvider(m_data);
> +            if (m_data->m_dc) {
> +                IntRect intRect = enclosingIntRect(rect);
> +                TransparentLayerDC transparentDc(m_data, m_data->mapRect(intRect), &intRect);
> +                if (transparentDc.hdc())
> +                    if (alpha == 0xFF)
> +                        FillRect(transparentDc.hdc(), &transparentDc.rect(), hbrush.get());
> +                    else
> +                        fillRectWithAlpha(transparentDc.hdc(), &transparentDc.rect(), hbrush.get(), alpha);
> +            }
> +        }
> +    }
> +}

This function would benefit from reducing nesting by using early returns.

> +void GraphicsContext::drawFocusRing(const Color& color)
> +{
> +    if (!m_data->m_opacity || paintingDisabled())
> +        return;
> +
> +    ScopeDCProvider dcProvider(m_data);
> +    if (!m_data->m_dc)
> +        return;
> +
> +    int radius = (focusRingWidth() - 1) / 2;
> +    int offset = radius + focusRingOffset();
> +
> +    const Vector<IntRect>& rects = focusRingRects();
> +    unsigned rectCount = rects.size();
> +    IntRect finalFocusRect;
> +    for (unsigned i = 0; i < rectCount; i++) {
> +        IntRect focusRect = rects[i];
> +        focusRect.inflate(offset);
> +        finalFocusRect.unite(focusRect);
> +    }
> +
> +    RECT rect = { finalFocusRect.x(), finalFocusRect.y(), finalFocusRect.right(), finalFocusRect.bottom() };

There's an operator RECT() member of IntRect, so you shouldn't have to do this explicitly.

> +void GraphicsContext::addInnerRoundedRectClip(const IntRect& rect, int thickness)
> +{
> +    notImplemented();
> +    // FIX ME:
> +    clip(rect);

Our style is "FIXME:". It would be good to add a slightly more descriptive comment.

> +void GraphicsContext::fillPath()
> +{
> +    if (!fillColor().alpha() || !m_data->m_opacity)
> +        return;
> +    ScopeDCProvider dcProvider(m_data);
> +    if (!m_data->m_dc)
> +        return;
> +
> +    if (m_data->m_opacity < 1.0f) {
> +        HGDIOBJ brush = createBrush(fillColor());
> +        for (Vector<Path>::const_iterator i = m_data->m_paths.begin(); i != m_data->m_paths.end(); ++i) {
> +            IntRect trRect = roundRect(m_data->mapRect(i->boundingRect()));
> +            TransparentLayerDC transparentDc(m_data, trRect);
> +            HDC dc = transparentDc.hdc();
> +            if (!dc)
> +                continue;
> +
> +            TransformationMatrix tr = m_data->m_transform;
> +            tr.translate(transparentDc.toShift().width(), transparentDc.toShift().height());
> +
> +            SelectObject(dc, GetStockObject(NULL_PEN));
> +            HGDIOBJ oldBrush = SelectObject(dc, brush);
> +            i->platformPath()->fillPath(dc, &tr);
> +            SelectObject(dc, oldBrush);

Don't you need to restore the old pen, too? (Ditto for the other branch in this function.)

> +void GraphicsContext::strokePath()
> +{
> +    if (!m_data->m_opacity)
> +        return;
> +
> +    ScopeDCProvider dcProvider(m_data);
> +    if (!m_data->m_dc)
> +        return;
> +
> +    if (m_data->m_opacity < 1.0f) {
> +        HGDIOBJ pen = createPen(strokeColor(), strokeThickness(), strokeStyle());
> +        for (Vector<Path>::const_iterator i = m_data->m_paths.begin(); i != m_data->m_paths.end(); ++i) {
> +            IntRect trRect = roundRect(m_data->mapRect(i->boundingRect()));
> +            TransparentLayerDC transparentDc(m_data, trRect);
> +            HDC dc = transparentDc.hdc();
> +            if (!dc)
> +                continue;
> +
> +            TransformationMatrix tr = m_data->m_transform;
> +            tr.translate(transparentDc.toShift().width(), transparentDc.toShift().height());
> +
> +            SelectObject(dc, GetStockObject(NULL_BRUSH));
> +            HGDIOBJ oldPen = SelectObject(dc, pen);
> +            i->platformPath()->strokePath(dc, &m_data->m_transform);
> +            SelectObject(dc, oldPen);

Don't you need to restore the old brush? (Ditto for the other branch of this function.)

> +void GraphicsContext::fillRect(const FloatRect& r, const Gradient* gradient)
> +{
> +    if (!m_data->m_opacity)
> +        return;
> +
> +    const Vector<Gradient::ColorStop>& stops = gradient->getStops();
> +    int numStops = stops.size();
> +    if (numStops == 0) {
> +    } else if (numStops == 1) {

I think this would be clearer as:

if (stops.isEmpty())
    return;
size_t numStops = stops.size();

> +#define isCharVisible(c) ((c) && (c) != zeroWidthSpace)

This would be better as an inline helper function.

> +void GraphicsContext::drawText(const Font& font, const TextRun& run, const IntPoint& point, int from, int to)
> +{
> +    if (paintingDisabled() || fillColor().alpha() == 0 || !m_data->m_opacity)
> +        return;

Please use !fillColor().alpha() instead of comparing to 0. (applies elsewhere)

> +    if (fillColor().alpha() == 0xFF && m_data->m_opacity >= 1.0)
> +        font.drawText(this, run, point, from, to);
> +    else {

You should add an early return in the if case to reduce nesting.

> +        float oldOpacity = m_data->m_opacity;
> +        m_data->m_opacity *= fillColor().alpha() / 255.0;
> +
> +        FloatRect textRect = font.selectionRectForText(run, point, font.height(), from, to);
> +        textRect.setY(textRect.y() - font.ascent());
> +        IntRect trRect = enclosingIntRect(m_data->mapRect(textRect));
> +        RECT bmpRect;
> +        RefPtr<SharedBitmap> bmp = m_data->getTransparentLayerBitmap(trRect, true, bmpRect, true, false);
> +        if (bmp) {

You can declare the bmp variable inside the if condition:

if (RefPtr<SharedBitmap> bmp = ...) {

> +    // TransparentLayerDC ...

What does this comment mean?

> +    HFONT hFont = height > 1
> +        ? fontData->platformData().getScaledFontHandle(height, scaleX == scaleY ? 0 : width)
> +        : 0;
> +
> +    FloatPoint startPoint(point.x(), point.y() - fontData->ascent());
> +    FloatPoint trPoint = m_data->mapPoint(startPoint);
> +    int y = Round(trPoint.y());
> +
> +    Color color = fillColor();
> +    if (color.alpha() == 0) // hack
> +        return;
> +
> +    COLORREF fontColor = RGB(color.red(), color.green(), color.blue());
> +
> +    if (hFont) {

If you put the !hFont case first, you can add an early return at the end of it, and add early returns throughout the rest of the function to reduce nesting. i.e.,:

if (!hFont) {
    ....the current else case goes here....
    return;
}

....the rest of the code goes here, hopefully with more early returns....

> +        COLORREF shadowRgbColor;

shadowRGBColor would match our variable naming conventions better.

> +        HGDIOBJ hOldFont = SelectObject(m_data->m_dc, hFont);
> +        // transparent always...
> +        int oldBkMode = GetBkMode(m_data->m_dc);

What does this comment mean?

> +    if ((rectWin.right - rectWin.left) < boxWidthBest) {
> +        RefPtr<SharedBitmap> bmp1 = SharedBitmap::createInstance(true, boxWidthBest, boxHeightBest, true);
> +        SharedBitmap::DCHolder dc1(bmp1.get());

Is there a better way to name these variables than with a "1" suffix?

> +void GraphicsContext::setPlatformShouldAntialias(bool should)
> +{
> +    (void)should;
> +    notImplemented();
> +}

You can use the UNUSED_PARAM macro from wtf/UnusedParam.h here, or just omit the parameter name entirely. (applies elsewhere)

I think there's enough style clean-up here that we should do another pass at this. Hopefully we can make cpplint.py catch more of these style issues!

[Adam Treat]

Thanks Adam for the thorough review!  I'm going to let Yong and the others comment on the substantive parts as they are the original authors.

> I think there's enough style clean-up here that we should do another pass at
> this. Hopefully we can make cpplint.py catch more of these style issues!

As part of this upstreaming process I'm using cpplint and adding improvements and enhancements to the tool as they appear.  At least some of the things your review caught could be automated I think such as the include order and so on.  Thanks again!

[Yong Li]

(In reply to comment #5)

Hi, Adam,

Thanks a lot for your review. See my comments below.

Best regards,
-Yong

> (From update of attachment 32884 [details])
> There are many comments below that apply to many parts of the code. I've marked
> these with "(applies elsewhere)" instead of repeating the comments in each
> case.
> 
> > +#define PI        3.14159265358979323846
> 
> wtf/MathExtras.h defines piFloat and piDouble. You should probably use those
> instead.
> 
> > +#define RADIAN(a)    ((a) * PI / 180.)
> > +#define DEGREE(a)    ((a) * 180. / PI)
> 
> wtf/MathExtras.h has deg2rad and rad2deg functions you can use instead.

Agree. At the time I created this file, they were not there. Now we should use them

> 
> > +static inline int Round(double d)
> 
> Function names start with a lowercase letter in WebKit. But you can probably
> just use lround from wtf/MathExtras.h instead.

I just reviewed lround(). It does the same thing, but it calls "ceil()" for negative values, which is probably not good in term of performance.

> 
> > +{
> > +    if (d > 0)
> > +        return int(d + 0.5);
> 
> We normally use C++ cast operators instead of function-style casts. So
> static_cast would be better here. (applies elsewhere)
> 

Agree.

> 
> > +static inline IntRect roundRect(const FloatRect& r)
> > +{
> > +    return IntRect(Round(r.x()), Round(r.y()), Round(r.right()) - Round(r.x()), Round(r.bottom()) - Round(r.y()));
> > +}
> 
> Are you sure you don't want to use enclosingIntRect instead?

I'm sure. enclosingIntRect() can result more rendering problems when page is zoomed.

> > +    void map(double x1, double y1, double* x2, double* y2) const
> > +    {
> 
> Maybe this would be better as: FloatPoint map(const FloatPoint&) const

then I have to construct FloatPoint in many cases. another problem is that FloatPoint is float, but this function uses double.

> > +template<class Transform, class Rect, class Value> static inline
> > +Rect mapRect(const Rect& rect, const Transform& transform)
> 
> If you switched the template parameter order to Value, Transform, Rect, then I
> think you could normally call the function like this: mapRect<Value>(...). The
> Transform and Rect parameters would be inferred from the parameter types.

Good idea.
> 
> > +    GraphicsContextPlatformPrivate(HDC dc)
> > +        : m_dc(dc)
> > +    {
> > +        HDC hScreenDC = ::GetDC(0);
> > +        int rtn = ::GetDeviceCaps(hScreenDC, RASTERCAPS);
> > +        ::ReleaseDC(0, hScreenDC);
> > +    }
> 
> What's the point of the three lines in the body of the constructor? They don't
> seem to do anything. Does calling GetDeviceCaps have some side-effect?

This is a piece of garbage. I thought I removed them... probably we lost a commit in this branch :(

> 
> > +    IntPoint origin()
> > +    {
> > +        return IntPoint(Round(-m_transform.e()),
> > +                Round(-m_transform.f()));
> > +    }
> 
> This could be a const member function.

right.

> 
> > +    FloatSize mapSize(const FloatSize& size)
> > +    {
> > +        double w, h;
> > +        m_transform.map(size.width(), size.height(), w, h);
> > +        return FloatSize(w, h);
> > +    }
> 
> Earlier you used static_cast when passing doubles to the FloatSize constructor.
> It would be good to be consistent one way or the other.

how about static_cast<float>(w)?

> > +
> > +        GraphicsContextPlatformPrivateData::operator=(m_backupData.last());
> 
> Does *this = m_backupData.last() not work?

I usually like to use operator=() rather than *this =. Personal habit.

> 
> > +    PassRefPtr<SharedBitmap> getTransparentLayerBitmap(IntRect& origRect, bool uses16bit, RECT& bmpRect, bool checkClipBox, bool force)
> 
> It seems strange to use a mix of IntRect and RECT in this declaration. (applies
> elsewhere)

To avoid converting IntRect and RECT too frequently.

> 
> > +#if !defined(NO_ALPHABLEND)
> > +        if (m_opacity < 1. || !use16bit) {
> > +            const BLENDFUNCTION blend = { AC_SRC_OVER, 0
> > +                , m_opacity >= 1. ? 255 : (BYTE)(m_opacity * 255)
> > +                , use16bit ? 0 : AC_SRC_ALPHA };
> > +            AlphaBlend
> > +                (m_dc
> > +                , origRect.x()
> > +                , origRect.y()
> > +                , origRect.width()
> > +                , origRect.height()
> > +                , hdc
> > +                , 0
> > +                , 0
> > +                , bmpRect.right
> > +                , bmpRect.bottom
> > +                , blend
> > +                );
> 
> I've never seen this one-parameter-per-line style anywhere else in WebKit. I'm
> not sure it's a good idea to introduce it here. (applies elsewhere)

It makes code much more readable when there are too many arguments.

sourceH)
> > +                    *dest++ = source[srcY * paddedSourceW + srcX] | 0xFF000000;
> > +                else
> > +                    *dest++ |= 0xFF;
> > +            }
> > +            dest += padding;
> > +        }
> > +    }
> > +}
> 
> It would be nice to have a little less duplication in this function.

Performance vs code size... that's a fight.

> > +        : m_data(data)
> > +        , m_origRect(origRect)
> > +        , m_oldOpacity(data->m_opacity)
> 
> It seems error-prone to leave m_key1 and m_key2 uninitialized here.

m_key1 and m_key2 will be set up by ->getDC(). In many cases, they are not used at all. Performance consideration.

> 
> > +#if 1
...
> > +#else
...
> > +#endif
> 
> We don't like to check in commented-out code. (applies elsewhere)
>

this piece garbage needs to be cleaned up

> > +    if (!m_data->m_opacity || paintingDisabled() || rect.width() <= 0 || rect.height() <= 0)
> > +        return;
> 
> You can use rect.isEmpty() instead of the width() and height() checks.

well, rect.isEmpty() cheated before. it was equivalent to rect.width() <= 0 && rect.height() <= 0. Seems it has been fixed in upstream.

> > +    if (fillColor().alpha()) {
> > +        brush = createBrush(fillColor());
> > +        oldBrush = SelectObject(dc, brush);
> > +    } else {
> > +        SelectObject(dc, GetStockObject(NULL_BRUSH));
> > +    }
> 
> We don't put braces around the bodies of single-line branches.

We will follow the rules if we have to. But I really don't like this rule. consider this:

if () {
 2 lines here
} else
 one line code;
else if {
 2 lines here
}

It looks so terrible. Even this case

if () {
 one line code;
}

I cannot see nothing wrong with it.

> 
> > +    HGDIOBJ oldBrush = SelectObject(dc, GetStockObject(NULL_BRUSH));
> > +    Ellipse(dc, trRect.x(), trRect.y(), trRect.right(), trRect.bottom());
> > +    SelectObject(dc, oldBrush);
> > +
> > +    if (newClip)
> > +        SelectClipRgn(dc, 0);
> 
> Why don't we need to select the old clip region back in in this case?

no. newClip is set to true, only when there's no old clip region.


> 
> > +void GraphicsContext::fillRect(const FloatRect& rect, const Color& color)
> > +{
> > +    if (paintingDisabled() || !m_data->m_opacity)
> > +        return;
> > +
> > +    if (int alpha = color.alpha()) {
> > +        OwnPtr<HBRUSH> hbrush(CreateSolidBrush(RGB(color.red(), color.green(), color.blue())));
> > +        if (hbrush) {
> > +            ScopeDCProvider dcProvider(m_data);
> > +            if (m_data->m_dc) {
> > +                IntRect intRect = enclosingIntRect(rect);
> > +                TransparentLayerDC transparentDc(m_data, m_data->mapRect(intRect), &intRect);
> > +                if (transparentDc.hdc())
> > +                    if (alpha == 0xFF)
> > +                        FillRect(transparentDc.hdc(), &transparentDc.rect(), hbrush.get());
> > +                    else
> > +                        fillRectWithAlpha(transparentDc.hdc(), &transparentDc.rect(), hbrush.get(), alpha);
> > +            }
> > +        }
> > +    }
> > +}
> 
> This function would benefit from reducing nesting by using early returns.

By benefit, do you mean it runs faster? Personally, I don't like too many "return" in a function. it's just like "goto", which makes code more difficult to maintain & review.
> 
> > +
> > +            SelectObject(dc, GetStockObject(NULL_PEN));
> > +            HGDIOBJ oldBrush = SelectObject(dc, brush);
> > +            i->platformPath()->fillPath(dc, &tr);
> > +            SelectObject(dc, oldBrush);
> 
> Don't you need to restore the old pen, too? (Ditto for the other branch in this
> function.)

No. Leaving NULL_PEN to dc is safe.

> > +
> > +            SelectObject(dc, GetStockObject(NULL_BRUSH));
> > +            HGDIOBJ oldPen = SelectObject(dc, pen);
> > +            i->platformPath()->strokePath(dc, &m_data->m_transform);
> > +            SelectObject(dc, oldPen);
> 
> Don't you need to restore the old brush? (Ditto for the other branch of this
> function.)

No. Leaving NULL_BRUSH to dc is safe.

> > +#define isCharVisible(c) ((c) && (c) != zeroWidthSpace)
> 
> This would be better as an inline helper function.

Only concern is inline function is not inline in debug version, and it makes debug version run much slower.

> > +
> > +    if (hFont) {
> 
> If you put the !hFont case first, you can add an early return at the end of it,
> and add early returns throughout the rest of the function to reduce nesting.
> i.e.,:
> 
> if (!hFont) {
>     ....the current else case goes here....
>     return;
> }
> 
> ....the rest of the code goes here, hopefully with more early returns....

Later, if we want to add more code at the end to clean up something, we have to make it nested again.

> 
> > +        COLORREF shadowRgbColor;
> 
> shadowRGBColor would match our variable naming conventions better.
> 
> > +        HGDIOBJ hOldFont = SelectObject(m_data->m_dc, hFont);
> > +        // transparent always...
> > +        int oldBkMode = GetBkMode(m_data->m_dc);
> 
> What does this comment mean?

BkMode is always transparent.

[Yong Li]

Oops: lround does different thing:

inline long lround(double num) { return static_cast<long>(num > 0 ? num + 0.5 : ceil(num - 0.5)); }

What we want in GraphicsContextWince.cpp is to round -0.5 to 0, as 0.5 is rounded to 1, so that the rendering behavior is consistent when offset varies.

but lround(-0.5) will return -1.

-Yong

[Adam Roben (:aroben)]

(In reply to comment #6)
> > I think there's enough style clean-up here that we should do another pass at
> > this. Hopefully we can make cpplint.py catch more of these style issues!
> 
> As part of this upstreaming process I'm using cpplint and adding improvements
> and enhancements to the tool as they appear.  At least some of the things your
> review caught could be automated I think such as the include order and so on. 
> Thanks again!

Yeah, that comment was definitely meant for you, since I see you've been working on cpplint (yay!).

[Adam Roben (:aroben)]

(In reply to comment #7)
> (In reply to comment #5)
> > > +static inline int Round(double d)
> > 
> > Function names start with a lowercase letter in WebKit. But you can probably
> > just use lround from wtf/MathExtras.h instead.
> 
> I just reviewed lround(). It does the same thing, but it calls "ceil()" for
> negative values, which is probably not good in term of performance.

If your version is better, shouldn't we change lround() in MathExtras.h so that everyone will benefit?

> > > +static inline IntRect roundRect(const FloatRect& r)
> > > +{
> > > +    return IntRect(Round(r.x()), Round(r.y()), Round(r.right()) - Round(r.x()), Round(r.bottom()) - Round(r.y()));
> > > +}
> > 
> > Are you sure you don't want to use enclosingIntRect instead?
> 
> I'm sure. enclosingIntRect() can result more rendering problems when page is
> zoomed.

OK. A comment explaining this would be good.

> > > +    FloatSize mapSize(const FloatSize& size)
> > > +    {
> > > +        double w, h;
> > > +        m_transform.map(size.width(), size.height(), w, h);
> > > +        return FloatSize(w, h);
> > > +    }
> > 
> > Earlier you used static_cast when passing doubles to the FloatSize constructor.
> > It would be good to be consistent one way or the other.
> 
> how about static_cast<float>(w)?

Sounds fine.

> > > +    PassRefPtr<SharedBitmap> getTransparentLayerBitmap(IntRect& origRect, bool uses16bit, RECT& bmpRect, bool checkClipBox, bool force)
> > 
> > It seems strange to use a mix of IntRect and RECT in this declaration. (applies
> > elsewhere)
> 
> To avoid converting IntRect and RECT too frequently.

Have you noticed IntRect <-> RECT conversions showing up on performance profiles? It seems like it would be dwarfed by the actual painting calls. But I don't know the performance characteristics of your platform(s).

> > > +#if !defined(NO_ALPHABLEND)
> > > +        if (m_opacity < 1. || !use16bit) {
> > > +            const BLENDFUNCTION blend = { AC_SRC_OVER, 0
> > > +                , m_opacity >= 1. ? 255 : (BYTE)(m_opacity * 255)
> > > +                , use16bit ? 0 : AC_SRC_ALPHA };
> > > +            AlphaBlend
> > > +                (m_dc
> > > +                , origRect.x()
> > > +                , origRect.y()
> > > +                , origRect.width()
> > > +                , origRect.height()
> > > +                , hdc
> > > +                , 0
> > > +                , 0
> > > +                , bmpRect.right
> > > +                , bmpRect.bottom
> > > +                , blend
> > > +                );
> > 
> > I've never seen this one-parameter-per-line style anywhere else in WebKit. I'm
> > not sure it's a good idea to introduce it here. (applies elsewhere)
> 
> It makes code much more readable when there are too many arguments.

It makes it more readable if you're used to this style and if it's used elsewhere in the code. We try to maintain a consistent coding style in WebKit because we think it's a lot easier to read the code if the same style is used everywhere. I won't r- the patch just for this issue, but I strongly recommend that you format your code to match the WebKit coding style, so that WebKit's code can continue being consistent (and therefore easier to read as a whole).

> > > +                    *dest++ = source[srcY * paddedSourceW + srcX] | 0xFF000000;
> > > +                else
> > > +                    *dest++ |= 0xFF;
> > > +            }
> > > +            dest += padding;
> > > +        }
> > > +    }
> > > +}
> > 
> > It would be nice to have a little less duplication in this function.
> 
> Performance vs code size... that's a fight.

Would an inline function really affect performance?

> > > +        : m_data(data)
> > > +        , m_origRect(origRect)
> > > +        , m_oldOpacity(data->m_opacity)
> > 
> > It seems error-prone to leave m_key1 and m_key2 uninitialized here.
> 
> m_key1 and m_key2 will be set up by ->getDC(). In many cases, they are not used
> at all. Performance consideration.

I saw that they get initialized in getDC. My concern was if someone were to add code to use these members elsewhere, and didn't realize they don't get initialized by the constructor. Have you seen evidence that initializing these members affects performance? It would surprise me if so, but again I don't know the details of your platform(s).

> > > +    if (fillColor().alpha()) {
> > > +        brush = createBrush(fillColor());
> > > +        oldBrush = SelectObject(dc, brush);
> > > +    } else {
> > > +        SelectObject(dc, GetStockObject(NULL_BRUSH));
> > > +    }
> > 
> > We don't put braces around the bodies of single-line branches.
> 
> We will follow the rules if we have to. But I really don't like this rule.
> consider this:
> 
> if () {
>  2 lines here
> } else
>  one line code;
> else if {
>  2 lines here
> }
> 
> It looks so terrible. Even this case
> 
> if () {
>  one line code;
> }
> 
> I cannot see nothing wrong with it.

I agree that there are some cases where omitting the braces looks funny/bad. But see my comments above about maintaining a consistent coding style throughout WebKit.

> > > +void GraphicsContext::fillRect(const FloatRect& rect, const Color& color)
> > > +{
> > > +    if (paintingDisabled() || !m_data->m_opacity)
> > > +        return;
> > > +
> > > +    if (int alpha = color.alpha()) {
> > > +        OwnPtr<HBRUSH> hbrush(CreateSolidBrush(RGB(color.red(), color.green(), color.blue())));
> > > +        if (hbrush) {
> > > +            ScopeDCProvider dcProvider(m_data);
> > > +            if (m_data->m_dc) {
> > > +                IntRect intRect = enclosingIntRect(rect);
> > > +                TransparentLayerDC transparentDc(m_data, m_data->mapRect(intRect), &intRect);
> > > +                if (transparentDc.hdc())
> > > +                    if (alpha == 0xFF)
> > > +                        FillRect(transparentDc.hdc(), &transparentDc.rect(), hbrush.get());
> > > +                    else
> > > +                        fillRectWithAlpha(transparentDc.hdc(), &transparentDc.rect(), hbrush.get(), alpha);
> > > +            }
> > > +        }
> > > +    }
> > > +}
> > 
> > This function would benefit from reducing nesting by using early returns.
> 
> By benefit, do you mean it runs faster?

No, I meant that it would be more readable for someone who is accustomed to WebKit's coding style.

> Personally, I don't like too many
> "return" in a function. it's just like "goto", which makes code more difficult
> to maintain & review.

Early returns are an idiom we use throughout WebKit. One specific reason we use them is to avoid nesting and all the extra indentation it brings.

> > > +#define isCharVisible(c) ((c) && (c) != zeroWidthSpace)
> > 
> > This would be better as an inline helper function.
> 
> Only concern is inline function is not inline in debug version, and it makes
> debug version run much slower.

One advantage of an function instead of a macro is that it gives you stronger type safety.

> > > +
> > > +    if (hFont) {
> > 
> > If you put the !hFont case first, you can add an early return at the end of it,
> > and add early returns throughout the rest of the function to reduce nesting.
> > i.e.,:
> > 
> > if (!hFont) {
> >     ....the current else case goes here....
> >     return;
> > }
> > 
> > ....the rest of the code goes here, hopefully with more early returns....
> 
> Later, if we want to add more code at the end to clean up something, we have to
> make it nested again.

Are you expecting to add something like that in the future? If not, it seems a bit premature to be structuring the code in ways that make certain unplanned future changes easier.

> > > +        HGDIOBJ hOldFont = SelectObject(m_data->m_dc, hFont);
> > > +        // transparent always...
> > > +        int oldBkMode = GetBkMode(m_data->m_dc);
> > 
> > What does this comment mean?
> 
> BkMode is always transparent.

oldBkMode? Or the new one that you're setting?

[Adam Roben (:aroben)]

(In reply to comment #8)
> Oops: lround does different thing:
> 
> inline long lround(double num) { return static_cast<long>(num > 0 ? num + 0.5 :
> ceil(num - 0.5)); }
> 
> What we want in GraphicsContextWince.cpp is to round -0.5 to 0, as 0.5 is
> rounded to 1, so that the rendering behavior is consistent when offset varies.
> 
> but lround(-0.5) will return -1.

In that case I'd recommend giving your function a different name, so that its difference in behavior from the standard functions will be more clear.

[Yong Li]

(In reply to comment #11)
> (In reply to comment #8)
> > Oops: lround does different thing:
> > 
> > inline long lround(double num) { return static_cast<long>(num > 0 ? num + 0.5 :
> > ceil(num - 0.5)); }
> > 
> > What we want in GraphicsContextWince.cpp is to round -0.5 to 0, as 0.5 is
> > rounded to 1, so that the rendering behavior is consistent when offset varies.
> > 
> > but lround(-0.5) will return -1.
> 
> In that case I'd recommend giving your function a different name, so that its
> difference in behavior from the standard functions will be more clear.

yes. I'm going to change it to safeRound() or something else

[Yong Li]

(In reply to comment #10)

I'm modifying it to match the webkit coding styles. Thanks a lot.

[Yong Li]

(In reply to comment #9)
> (In reply to comment #6)


> > > > +    PassRefPtr<SharedBitmap> getTransparentLayerBitmap(IntRect& origRect, bool uses16bit, RECT& bmpRect, bool checkClipBox, bool force)
> > > 
> > > It seems strange to use a mix of IntRect and RECT in this declaration. (applies
> > > elsewhere)
> > 
> > To avoid converting IntRect and RECT too frequently.
> 
> Have you noticed IntRect <-> RECT conversions showing up on performance
> profiles? It seems like it would be dwarfed by the actual painting calls. But I
> don't know the performance characteristics of your platform(s).
> 

IntRect contains x, y, width and height, where RECT contains top, left, right, bottom. Obviously every conversion involves "+" or "-" operations.

> > > > +                    *dest++ = source[srcY * paddedSourceW + srcX] | 0xFF000000;
> > > > +                else
> > > > +                    *dest++ |= 0xFF;
> > > > +            }
> > > > +            dest += padding;
> > > > +        }
> > > > +    }
> > > > +}
> > > 
> > > It would be nice to have a little less duplication in this function.
> > 
> > Performance vs code size... that's a fight.
> 
> Would an inline function really affect performance?

Those 2 blocks are not exactly same. If I want to make them share one piece of code, I have to introduce some Boolean variables and check them in every cycle (for every pixel). This has nothing to do with inline.

> 
> > > > +        : m_data(data)
> > > > +        , m_origRect(origRect)
> > > > +        , m_oldOpacity(data->m_opacity)
> > > 
> > > It seems error-prone to leave m_key1 and m_key2 uninitialized here.
> > 
> > m_key1 and m_key2 will be set up by ->getDC(). In many cases, they are not used
> > at all. Performance consideration.
> 
> I saw that they get initialized in getDC. My concern was if someone were to add
> code to use these members elsewhere, and didn't realize they don't get
> initialized by the constructor. Have you seen evidence that initializing these
> members affects performance? It would surprise me if so, but again I don't know
> the details of your platform(s).
> 

They are supposed to be used only in special cases. I'll add some comments for that.

[Yong Li]

Created attachment 33091 [details]
coding style modifications and latest fixes

modified to follow WEBKIT coding style rules

[Adam Roben (:aroben)]

Comment on attachment 33091 [details]
coding style modifications and latest fixes

> +#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.

[Yong Li]

(In reply to comment #16)
> (From update of attachment 33091 [details])
>
> 
> > +    IntPoint origin()
> 
> This can be a const member function.
> 

Sorry, I forgot to check complete list in last update. I'm cleaning it up carefully now.

BTW, in GraphicsContext.h

class GraphicsContext {
...
IntPoint origin();
...

Should origin() be "const" here?

[Adam Roben (:aroben)]

(In reply to comment #17)
> (In reply to comment #16)
> > (From update of attachment 33091 [details] [details])
> >
> > 
> > > +    IntPoint origin()
> > 
> > This can be a const member function.
> > 
> 
> Sorry, I forgot to check complete list in last update. I'm cleaning it up
> carefully now.

Thanks!

> BTW, in GraphicsContext.h
> 
> class GraphicsContext {
> ...
> IntPoint origin();
> ...
> 
> Should origin() be "const" here?

Probably. It certainly seems like it should be "const". It could be that there's something preventing it from being const, or it could just be an oversight.

[Yong Li]

Created attachment 33098 [details]
more modifications for coding style

more modifications for coding style

[Adam Roben (:aroben)]

Comment on attachment 33098 [details]
more modifications for coding style

> +#include "wtf/OwnPtr.h"

This should be <wtf/OwnPtr.h>, with angle brackets.

r=me. Maybe the committer can fix the above before committing.

[Adam Treat]

Landed with r46129.