[Webkit-unassigned] [Bug 27349] [WINCE] Add GraphicsContextWince implementation for WinCE.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jul 17 19:59:27 PDT 2009


https://bugs.webkit.org/show_bug.cgi?id=27349


Adam Roben (aroben) <aroben at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #32884|review?                     |review-
               Flag|                            |




--- Comment #5 from Adam Roben (aroben) <aroben at apple.com>  2009-07-17 19:59:26 PDT ---
(From update of attachment 32884)
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 @@
> +2009-07-16  Yong Li  <yong.li at torchmobile.com>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=27349
> +        Add GraphicsContext implementation for the WinCE port.
> +
> +        Written by Yong Li <yong.li at torchmobile.com> and George Staikos <george.staikos at torchmobile.com>
> +        with trivial style fixes by Adam Treat <adam.treat at torchmobile.com>
> +
> +        * 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!

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.



More information about the webkit-unassigned mailing list