Bug 28272 – WINCE PORT: graphics files only for WINCE

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Yong Li]

derived from 27511 because there are too many comments in there.

patches are going to be post soon

[Yong Li]

Created attachment 34768 [details]
1) SharedBitmap

createInstance() should be changed to create().

But this needs modifications on some landed files. Can we do this afterward?

[Yong Li]

Created attachment 34769 [details]
2) ImageWince.cpp

implement some platform-specific BitmapImage methods.

[Adam Barth]

Comment on attachment 34768 [details]
1) SharedBitmap

+ BitmapMap g_bitmapMap

Static initializers are forbidden.

+ int discardBitmapHandles()

Functions like this should be static.

+ SharedBitmap::SharedBitmap
+ SharedBitmap::SharedBitmap

Initializer list as wrong indent.

+ SharedBitmap::~SharedBitmap

Missing blank line before this function.

+ delete[] m_pixels

Why not OwnArray?

+ memset(m_pixels, 0, bufferSize * (is16bit() ? 2 : 4));

Size calculation should be factored into an separate function.

+ void _clear()

This and its ilk are improper style.

+ explicit SharedBitmap

The explicit keyword is needed only for one-argument constructors.

[Adam Barth]

Comment on attachment 34769 [details]
2) ImageWince.cpp

+ static inline int stableRound

Why is this function WINCE specific?

+ PassRefPtr<WebCore::SharedBuffer> loadResourceIntoBuffer(const char* name);

Why are we declaring a function in a cpp file?

Also, these things should be happening in the WebCore namespace.

Aside from these minor issues, this patch actually looks somewhat reasonable.  I'm not a graphics expert, however.

[Yong Li]

(In reply to comment #3)
> (From update of attachment 34768 [details])
> + BitmapMap g_bitmapMap
> 
> Static initializers are forbidden.

Forbidden by you?

> 
> + int discardBitmapHandles()
> 
> Functions like this should be static.

it's supposed to be called from outside

[Kwang Yul Seo]

Created attachment 53310 [details]
1) SharedBitmap

Change g_drawPattern and g_drawImage to s_drawPattern and s_drawImage respectively.
Change g_dcProvider to s_dcProvider ane make it static as it is not used elsewhere.
Change g_bitmapMap to bitmapMap function as static initializers are forbidden.
Use AffineTransform instead of TransformationMatrix.
Change methods names which start with underscore: _clear -> clearInternal, _set -> setInternal.
Remove explicit keyword from SharedBitmap constructors as they take multiple arguments.
Indent initializer list properly.
Use C++ style casts.
Fix other miscellaneous style errors.

[Kent Tamura]

Comment on attachment 53310 [details]
1) SharedBitmap

WebCore/platform/graphics/wince/SharedBitmap.cpp:86
 +  PassRefPtr<SharedBitmap> SharedBitmap::createInstance(bool is16bit, int w, int h, bool initPixels)
One letter variable names are not good.


WebCore/platform/graphics/wince/SharedBitmap.cpp:88
 +      SharedBitmap* rtn = new SharedBitmap(is16bit, w, h, initPixels);
"rtn" is not good name.  resultantBitmap?


WebCore/platform/graphics/wince/SharedBitmap.cpp:109
 +  SharedBitmap::SharedBitmap(bool _is16bit, int w, int h, bool initPixels)
_is16bit should be is16bit.  w and h should be width and height.


WebCore/platform/graphics/wince/SharedBitmap.cpp:143
 +      if (m_pixels) {
We prefer early exit in WebKit. So this should be:
if (!m_pixels)
    return;


WebCore/platform/graphics/wince/SharedBitmap.cpp:246
 +  HBITMAP SharedBitmap::createHandle(void** pixels, BitmapInfo* bmpInfo, int h, bool use16bit) const
One letter variable name is not good.

[Patrick R. Gansterer]

Created attachment 67058 [details]
2) ImageWinCE.cpp

[WebKit Review Bot]

Attachment 67058 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/graphics/wince/ImageWinCE.cpp:40:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Roben (:aroben)]

Comment on attachment 67058 [details]
2) ImageWinCE.cpp

View in context: https://bugs.webkit.org/attachment.cgi?id=67058&action=prettypatch

> WebCore/platform/graphics/wince/ImageWinCE.cpp:46
> +    NativeImagePtr ret = SharedBitmap::createInstance(false, width(), height(), false);
A non-abbreviated variable name (like "image") would be better.

> WebCore/platform/graphics/wince/ImageWinCE.cpp:100
> +        if (bmp && bmp->height() == (size_t)srcSize.height() && bmp->width() == (size_t)srcSize.width()) {
I suggest using an early-continue and static_cast/

> WebCore/platform/graphics/wince/ImageWinCE.cpp:125
> +    RefPtr<SharedBitmap> bmp = frameAtIndex(m_currentFrame);
> +    if (mayFillWithSolidColor())
> +        fillWithSolidColor(ctxt, dstRect, solidColor(), styleColorSpace, compositeOp);
> +    else {
> +        IntRect intSrcRect(srcRectIn);
> +
> +        if (bmp->width() != m_source.size().width()) {
It looks like bmp is only used in the else case. Can it be declared inside the else?

> WebCore/platform/graphics/wince/ImageWinCE.cpp:126
> +            double rate = (double)bmp->width() / m_source.size().width();
static_cast would be better.

"scaleFactor" seems like a better name than "rate".

> WebCore/platform/graphics/wince/ImageWinCE.cpp:129
> +            int temp = stableRound(srcRectIn.right() * rate);
> +            intSrcRect.setX(stableRound(srcRectIn.x() * rate));
> +            intSrcRect.setWidth(temp - intSrcRect.x());
Why is temp needed? Can't you do this instead?

intSrcRect.setWidth(stableRound(srcRectIn.width() * rate));

> WebCore/platform/graphics/wince/ImageWinCE.cpp:132
> +            temp = stableRound(srcRectIn.bottom() * rate);
> +            intSrcRect.setY(stableRound(srcRectIn.y() * rate));
> +            intSrcRect.setHeight(temp - intSrcRect.y());
Same question here.

> WebCore/platform/graphics/wince/ImageWinCE.cpp:151
> +    RefPtr<SharedBitmap> bmp = nativeImageForCurrentFrame();
> +    if (bmp)
> +        bmp->drawPattern(ctxt, tileRectIn, patternTransform, phase, styleColorSpace, op, destRect, m_source.size());
An early return would be nicer here.

> WebCore/platform/graphics/wince/ImageWinCE.cpp:171
> +    if (bmp->width() == 1 && bmp->validHeight() == 1) {
I think this would be cleaner if you reversed the condition and used an early return.

> WebCore/platform/graphics/wince/ImageWinCE.cpp:175
> +            unsigned short c = ((unsigned short *)bmp->bytes())[0];
reinterpret_cast would be better.

> WebCore/platform/graphics/wince/ImageWinCE.cpp:184
> +            unsigned c = ((unsigned *)bmp->bytes())[0];
reinterpret_cast would be better.

The code looks fine, but you should consider these comments.

[Patrick R. Gansterer]

Comment on attachment 67058 [details]
2) ImageWinCE.cpp

Clearing flags on attachment: 67058

Committed r67624: <http://trac.webkit.org/changeset/67624>

[Patrick R. Gansterer]

Created attachment 71636 [details]
1) SharedBitmap

There are some parts of uncool code in the patch:
E.g: HDC SharedBitmap::DCProvider::getDC(SharedBitmap* bmp, unsigned* key1, unsigned* key2)

I'd like to commit the patch as it is at the moment, because many code in the other WinCE files require this constructs.
E.g: ScopeDCProvider in GraphicsContextWinCE

If it's ok, I want to clean this up in further patches, so we can see the codechanges of the already comitted code in context with the other changes.

[WebKit Review Bot]

Attachment 71636 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/graphics/wince/SharedBitmap.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/graphics/wince/SharedBitmap.h:145:  Could not find a newline character at the end of the file.  [whitespace/ending_newline] [5]
WebCore/platform/graphics/wince/SharedBitmap.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/graphics/wince/SharedBitmap.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 4 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Roben (:aroben)]

Comment on attachment 71636 [details]
1) SharedBitmap

Can this class be unified with ImageBuffer? It seems to me they serve essentially the same purpose, i.e., providing some memory into which you can draw.

[Patrick R. Gansterer]

(In reply to comment #14)
> (From update of attachment 71636 [details])
> Can this class be unified with ImageBuffer? It seems to me they serve essentially the same purpose, i.e., providing some memory into which you can draw.
I don't think so, because SharedBitmap works on top of HBITMAP.

[Adam Roben (:aroben)]

(In reply to comment #15)
> (In reply to comment #14)
> > (From update of attachment 71636 [details] [details])
> > Can this class be unified with ImageBuffer? It seems to me they serve essentially the same purpose, i.e., providing some memory into which you can draw.
> I don't think so, because SharedBitmap works on top of HBITMAP.

Presumably ImageBuffer would, too. I don't know how ImageBuffer is implemented currently on WinCE. But it should just wrap an HBITMAP. You'll need this for things like <canvas> anyway.

[Yong Li]

(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > (From update of attachment 71636 [details] [details] [details])
> > > Can this class be unified with ImageBuffer? It seems to me they serve essentially the same purpose, i.e., providing some memory into which you can draw.
> > I don't think so, because SharedBitmap works on top of HBITMAP.
> Presumably ImageBuffer would, too. I don't know how ImageBuffer is implemented currently on WinCE. But it should just wrap an HBITMAP. You'll need this for things like <canvas> anyway.

ImageBuffer uses SharedBitmap. SharedBitmap is a helper class for HBITMAP

[Adam Roben (:aroben)]

(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > (In reply to comment #14)
> > > > (From update of attachment 71636 [details] [details] [details] [details])
> > > > Can this class be unified with ImageBuffer? It seems to me they serve essentially the same purpose, i.e., providing some memory into which you can draw.
> > > I don't think so, because SharedBitmap works on top of HBITMAP.
> > Presumably ImageBuffer would, too. I don't know how ImageBuffer is implemented currently on WinCE. But it should just wrap an HBITMAP. You'll need this for things like <canvas> anyway.
> 
> ImageBuffer uses SharedBitmap. SharedBitmap is a helper class for HBITMAP

I guess what I really mean is that the code seems misplaced. A class named "SharedBitmap" sounds like it would be just that: a ref-counted wrapper around an HBITMAP. But this class is a lot more than that. If you try to rename the class to reflect this, you start coming up with names like GraphicsContext and ImageBuffer. Which makes me wonder: why is this code not in those classes directly? Why is his helper class needed at all? It seems like it's adding an unnecessary extra layer to the already-complicated graphics code.

[Adam Roben (:aroben)]

Comment on attachment 71636 [details]
1) SharedBitmap

View in context: https://bugs.webkit.org/attachment.cgi?id=71636&action=review

> WebCore/platform/graphics/wince/SharedBitmap.cpp:49
> +    SharedBitmap* resultantBitmap = new SharedBitmap(size, bitCount, initPixels);
> +    if (resultantBitmap && !resultantBitmap->bytes()) {
> +        delete resultantBitmap;
> +        return 0;
> +    }
> +    return adoptRef(resultantBitmap);

This should be:

RefPtr<SharedBitmap> resultantBitmap = adoptRef(new ...);
if (...)
    return 0;
return resultantBitmap.release();

> WebCore/platform/graphics/wince/SharedBitmap.cpp:59
> +    PassRefPtr<SharedBitmap> result = create(size, BitmapInfo::BitCount32, false);
> +    if (result) {
> +        memcpy(result->bytes(), data.data(), data.size() * sizeof(unsigned));
> +        result->setHasAlpha(hasAlpha);
> +    }
> +    return result;

result should be a RefPtr and you should return result.release().

> WebCore/platform/graphics/wince/SharedBitmap.cpp:62
> +SharedBitmap::SharedBitmap(const BitmapInfo& bmpInfo, HBITMAP hbmp, void* pixels)

This constructor doesn't seem to be used.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:79
> +    , m_reference(1)

This member doesn't seem to be used.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:111
> +    if (m_hbitmap)
> +        DeleteObject(m_hbitmap);
> +    else
> +        delete[] m_pixels;

Using OwnPtr/OwnArrayPtr would be better.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:165
> +    unsigned short* p16 = reinterpret_cast<unsigned short*>(newPixels);
> +    const unsigned* p32 = reinterpret_cast<const unsigned*>(m_pixels);

static_cast should work here.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:167
> +    const bool skips = paddedWidth != width;

We don't normally mark locals const like this.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:169
> +    const unsigned short* const p16end = p16 + bufferSize;

...or this (second const).

> WebCore/platform/graphics/wince/SharedBitmap.cpp:288
> +    if (!hbitmap) {

An early-return would be better.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:296
> +        StretchDIBits(hdc, dstRect.x(), dstRect.y(), dstRect.width(), dstRect.height()
> +            , srcRect.x(), srcRect.y(), srcRect.width(), srcRect.height(), m_pixels, &m_bmpInfo, DIB_RGB_COLORS, rop);

This comma should be on the previous line.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:304
> +            bool success = alphaBlendIfSupported(hdc, dstRect.x(), dstRect.y(), dstRect.width(), dstRect.height(), hmemdc.get()
> +                , srcRect.x(), srcRect.y(), srcRect.width(), srcRect.height(), blend);

and this one

> WebCore/platform/graphics/wince/SharedBitmap.cpp:308
> +            TransparentBlt(hdc, dstRect.x(), dstRect.y(), dstRect.width(), dstRect.height(), hmemdc.get()
> +                , srcRect.x(), srcRect.y(), srcRect.width(), srcRect.height(), transparentColor());

and this one

> WebCore/platform/graphics/wince/SharedBitmap.cpp:331
> +    if (newBmp) {

An early-return would be better.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:336
> +        StretchDIBits(dcNew.get(), 0, 0, copyWidth, copyHeight, rect.x(), rect.y(), copyWidth, copyHeight
> +            , bytes(), &bitmapInfo(), DIB_RGB_COLORS, SRCCOPY);

and this one

> WebCore/platform/graphics/wince/SharedBitmap.cpp:353
> +    PassRefPtr<SharedBitmap> newBmp = create(IntSize(copyWidth, copyHeight), useAlpha && is32bit() ? BitmapInfo::BitCount32 : BitmapInfo::BitCount16, false);

This should be a RefPtr and you should return newBmp.release().

> WebCore/platform/graphics/wince/SharedBitmap.cpp:361
> +    StretchDIBits(dcNew.get(), 0, 0, copyWidth, copyHeight, rect.x(), rect.y(), copyWidth, copyHeight
> +        , bytes(), &bitmapInfo(), DIB_RGB_COLORS, SRCCOPY);

Comma should be on the previous line.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:369
> +    if (hBrush) {

Early-return would be better. Do we really need the null-check at all?

> WebCore/platform/graphics/wince/SharedBitmap.cpp:388
> +            StretchDIBits(hdc, dstX, dstY, sourceW, sourceH, sourceX, sourceY, sourceW, sourceH
> +                , bmp->bytes(), &bmp->bitmapInfo(), DIB_RGB_COLORS, SRCCOPY);

Comma should be on the previous line.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:521
> +    {

Why is this extra scope needed?

> WebCore/platform/graphics/wince/SharedBitmap.cpp:536
> +        if (hbmpTemp) {

Early-return would be better.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:574
> +                bool success = alphaBlendIfSupported(hdc, dstRectWin.left, dstRectWin.top, dstRectWin.right - dstRectWin.left, dstRectWin.bottom - dstRectWin.top
> +                    , hmemdc.get(), 0, 0, srcRectWin.right, srcRectWin.bottom, blend);
> +                ASSERT_UNUSED(success, success);
> +            } else if (useAlpha && !hasAlphaBlendSupport() || op == CompositeSourceOver && usesTransparentColor())
> +                TransparentBlt(hdc, dstRectWin.left, dstRectWin.top, dstRectWin.right - dstRectWin.left
> +                    , dstRectWin.bottom - dstRectWin.top, hmemdc.get(), 0, 0, srcRectWin.right, srcRectWin.bottom, transparentColor());
> +            else {
> +                DWORD bmpOp = op == CompositeCopy ? SRCCOPY
> +                            : op == CompositeSourceOver ? SRCCOPY
> +                            : op == CompositeXOR ? PATINVERT
> +                            : op == CompositeClear ? WHITENESS
> +                            : SRCCOPY; // FIXEME: other types?
> +
> +                StretchDIBits(hdc, dstRectWin.left, dstRectWin.top, dstRectWin.right - dstRectWin.left
> +                    , dstRectWin.bottom - dstRectWin.top, 0, 0, srcRectWin.right, srcRectWin.bottom
> +                    , pixels, &bmpInfo, DIB_RGB_COLORS, bmpOp);

Please fix the leading commas.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:582
> +SharedBitmap::DCProvider s_dcProvider;
> +SharedBitmap::DCProvider* SharedBitmap::m_dcProvider = &s_dcProvider;

Why is the s_dcProvider needed? Why not just do m_dcProvider = new DCProvider?

The current s_dcProvider should be marked static if it needs to stay. m_dcProvider should be named s_dcProvider.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:584
> +HDC SharedBitmap::DCProvider::getDC(SharedBitmap* bmp, unsigned* key1, unsigned* key2)

Why do we have key2 if it's never used?

> WebCore/platform/graphics/wince/SharedBitmap.cpp:590
> +    if (hdc) {

Early-return would be better. Is the null-check really needed?

> WebCore/platform/graphics/wince/SharedBitmap.h:44
> +    virtual ~SharedBitmap();

Why is this virtual?

> WebCore/platform/graphics/wince/SharedBitmap.h:146
> +#endif // SharedBitmap_h
>  \ No newline at end of file

Please add a newline.

[Patrick R. Gansterer]

Created attachment 72602 [details]
1) SharedBitmap

[Patrick R. Gansterer]

Created attachment 72603 [details]
1) SharedBitmap

[WebKit Review Bot]

Attachment 72602 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/graphics/wince/SharedBitmap.h:28:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/graphics/wince/SharedBitmap.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/graphics/wince/SharedBitmap.h:144:  Could not find a newline character at the end of the file.  [whitespace/ending_newline] [5]
WebCore/platform/graphics/wince/SharedBitmap.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/graphics/wince/SharedBitmap.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 5 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 72603 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/graphics/wince/SharedBitmap.h:28:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/graphics/wince/SharedBitmap.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/graphics/wince/SharedBitmap.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/graphics/wince/SharedBitmap.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 4 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Roben (:aroben)]

(In reply to comment #23)
> Attachment 72603 [details] did not pass style-queue:
> 
> Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
> WebCore/platform/graphics/wince/SharedBitmap.h:28:  Alphabetical sorting problem.  [build/include_order] [4]
> WebCore/platform/graphics/wince/SharedBitmap.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
> WebCore/platform/graphics/wince/SharedBitmap.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
> WebCore/platform/graphics/wince/SharedBitmap.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
> Total errors found: 4 in 3 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

You should fix these style errors. You can run check-webkit-style yourself from the command line:

check-webkit-style file1 file2...

[Adam Roben (:aroben)]

Comment on attachment 72603 [details]
1) SharedBitmap

View in context: https://bugs.webkit.org/attachment.cgi?id=72603&action=review

I think this is worth one more pass.


> WebCore/platform/graphics/wince/SharedBitmap.cpp:47
> +    return resultantBitmap;

This should be: return resultantBitmap.release();

> WebCore/platform/graphics/wince/SharedBitmap.cpp:57
> +    return result;

Ditto.

By using RefPtr::release you save an extra ref/deref.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:92
> +    if (m_hbitmap)
> +        DeleteObject(m_hbitmap);

I still think it would be good to use an OwnPtr for m_hbitmap.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:109
> +        wmemset(reinterpret_cast<wchar_t*>(m_pixels), 0xFFFF, bufferSize);

static_cast should work fine here.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:114
> +    unsigned* pixel = reinterpret_cast<unsigned*>(m_pixels);

And here.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:115
> +    const unsigned* const bufferEnd = pixel + bufferSize;

I think you can omit the second const here, to match standard WebKit style.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:164
> +        m_pixelData.swap(newPixelData);

Another way to write this: m_pixelData = newPixelData.release();

I don't think one way is clearly better than the other.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:194
> +HBITMAP SharedBitmap::createHandle(void** pixels, BitmapInfo* bmpInfo, int height, bool use16bit) const

This should return a PassOwnPtr<HBITMAP>. You can do this even if you don't make m_hbitmap an OwnPtr.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:203
> +    HBITMAP hbmp = CreateDIBSection(0, bmpInfo, DIB_RGB_COLORS, pixels, 0, 0);

This should be an OwnPtr. Then you can return hbmp.release() at the end.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:292
> +    } else
> +        TransparentBlt(hdc, dstRect.x(), dstRect.y(), dstRect.width(), dstRect.height(), hmemdc.get(),
> +            srcRect.x(), srcRect.y(), srcRect.width(), srcRect.height(), transparentColor());

You should use braces around the body of the else because it is more than one line (even though it's only a single statement).

> WebCore/platform/graphics/wince/SharedBitmap.cpp:351
> +    HBRUSH hBrush = CreatePatternBrush(hbmp);

You could use an OwnPtr for this.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:383
> +static void normalizePhase(LONG& phase, int limit)

Since this uses an out-parameter, it should have a name like "getNormalizedPhase". But why does it use an out-parameter? A return value seems better. That would let you shorten this function quite a bit.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:489
> +    RECT dstRectWin =
> +    {

The brace should be on the previous line.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:549
> +    } else if (useAlpha && !hasAlphaBlendSupport() || op == CompositeSourceOver && usesTransparentColor())
> +        TransparentBlt(hdc, dstRectWin.left, dstRectWin.top, dstRectWin.right - dstRectWin.left,
> +            dstRectWin.bottom - dstRectWin.top, hmemdc.get(), 0, 0, srcRectWin.right, srcRectWin.bottom, transparentColor());
> +    else {

Need braces here.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:565
> +HDC SharedBitmap::DCProvider::getDC(SharedBitmap* bmp, unsigned* key1)

key1 could just be key now.

Maybe this function should return an OwnPtr<HDC>...

> WebCore/platform/graphics/wince/SharedBitmap.cpp:582
> +void SharedBitmap::DCProvider::releaseDC(SharedBitmap*, HDC hdc, unsigned key1)

...and this function should take a PassOwnPtr<HDC>?

> WebCore/platform/graphics/wince/SharedBitmap.cpp:600
> +        unsigned short* dst = reinterpret_cast<unsigned short*>(m_pixels);

static_cast should be fine here.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:612
> +    unsigned* dst = reinterpret_cast<unsigned*>(m_pixels);

static_cast should be fine here.

> WebCore/platform/graphics/wince/SharedBitmap.h:35
> +enum CompositeOperator;

Foroward-declaring enums doesn't normally work. Is this declaration really helping?

[Patrick R. Gansterer]

Created attachment 72813 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=72603&action=review

>> WebCore/platform/graphics/wince/SharedBitmap.cpp:57
>> +    return result;
> 
> Ditto.
> 
> By using RefPtr::release you save an extra ref/deref.

Ohh, good to know.

>> WebCore/platform/graphics/wince/SharedBitmap.cpp:92
>> +        DeleteObject(m_hbitmap);
> 
> I still think it would be good to use an OwnPtr for m_hbitmap.

Upps, missed this point.

>> WebCore/platform/graphics/wince/SharedBitmap.cpp:565
>> +HDC SharedBitmap::DCProvider::getDC(SharedBitmap* bmp, unsigned* key1)
> 
> key1 could just be key now.
> 
> Maybe this function should return an OwnPtr<HDC>...

They do nearly the same as GraphicsContext::getWindowsContext().
Maybe we should change them too?

[Adam Roben (:aroben)]

Comment on attachment 72813 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=72813&action=review

> WebCore/platform/graphics/wince/SharedBitmap.cpp:54
> +    if (result) {

This could be an early return instead.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:79
> +    m_pixelData.set(new unsigned[bufferSize]);

I think the more modern way to do this is:

m_pixelData = adoptArrayPtr(new unsigned[bufferSize]);

> WebCore/platform/graphics/wince/SharedBitmap.cpp:176
> +        m_hbitmap.clear();

Some people have started to use: m_hbitmap = nullptr;

I don't know if that's the recommended style for all new code.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:199
> +    OwnPtr<HBITMAP> hbmp(CreateDIBSection(0, bmpInfo, DIB_RGB_COLORS, pixels, 0, 0));

You could write this as: OwnPtr<HBITMAP> hbmp = adoptPtr(CreateDIBSection(...));

I don't know whether that's the recommended style for all new code. I find it slightly more readable.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:296
> +HBITMAP SharedBitmap::clipBitmap(const IntRect& rect, bool useAlpha, BitmapInfo& bmpInfo, void*& pixels)

This could return a PassOwnPtr<HBITMAP>.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:309
> +    HBITMAP newBmp = CreateDIBSection(0, &bmpInfo, DIB_RGB_COLORS, &pixels, 0, 0);

...and you could store this in an OwnPtr.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:343
> +    return newBmp;

This should be: return newBmp.release();

> WebCore/platform/graphics/wince/SharedBitmap.h:146
> +#endif // SharedBitmap_h
>  \ No newline at end of file

You should add a newline.

[Patrick R. Gansterer]

Created attachment 72819 [details]
1) SharedBitmap

Added GraphicsContext changes

[Adam Roben (:aroben)]

Comment on attachment 72819 [details]
1) SharedBitmap

View in context: https://bugs.webkit.org/attachment.cgi?id=72819&action=review

> WebCore/platform/graphics/wince/GraphicsContextWinCE.cpp:467
> -TransparentLayerDC::TransparentLayerDC(GraphicsContextPlatformPrivate* data, IntRect& origRect, const IntRect* rectBeforeTransform, int alpha, bool paintImage)
> +TransparentLayerDC::TransparentLayerDC(GraphicsContextPlatformPrivate* data, IntRect& origRect, const IntRect* rectBeforeTransform, int alpha, bool paintImage, bool paintGradient)

This change seems to have snuck in.

[Patrick R. Gansterer]

Comment on attachment 72819 [details]
1) SharedBitmap

Clearing flags on attachment: 72819

Manually committed r71237: <http://trac.webkit.org/changeset/71237>

[Patrick R. Gansterer]

All reviewed patches have been landed.  Closing bug.