Bug 28272 - WINCE PORT: graphics files only for WINCE
Summary: WINCE PORT: graphics files only for WINCE
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Normal
Assignee: Patrick R. Gansterer
URL:
Keywords:
Depends on:
Blocks: 37440 27511
  Show dependency treegraph
 
Reported: 2009-08-13 12:08 PDT by Yong Li
Modified: 2010-11-03 07:41 PDT (History)
6 users (show)

See Also:


Attachments
1) SharedBitmap (29.12 KB, patch)
2009-08-13 12:28 PDT, Yong Li
abarth: review-
Details | Formatted Diff | Diff
2) ImageWince.cpp (8.07 KB, patch)
2009-08-13 12:29 PDT, Yong Li
abarth: review-
Details | Formatted Diff | Diff
1) SharedBitmap (30.95 KB, patch)
2010-04-13 21:31 PDT, Kwang Yul Seo
tkent: review-
Details | Formatted Diff | Diff
2) ImageWinCE.cpp (8.11 KB, patch)
2010-09-09 10:51 PDT, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff
1) SharedBitmap (29.53 KB, patch)
2010-10-23 05:43 PDT, Patrick R. Gansterer
aroben: review-
aroben: commit-queue-
Details | Formatted Diff | Diff
1) SharedBitmap (28.95 KB, patch)
2010-11-01 16:59 PDT, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff
1) SharedBitmap (28.92 KB, patch)
2010-11-01 17:01 PDT, Patrick R. Gansterer
aroben: review-
aroben: commit-queue-
Details | Formatted Diff | Diff
Patch (28.47 KB, patch)
2010-11-03 05:26 PDT, Patrick R. Gansterer
aroben: review+
aroben: commit-queue-
Details | Formatted Diff | Diff
1) SharedBitmap (32.56 KB, patch)
2010-11-03 07:07 PDT, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yong Li 2009-08-13 12:08:47 PDT
derived from 27511 because there are too many comments in there.

patches are going to be post soon
Comment 1 Yong Li 2009-08-13 12:28:35 PDT
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?
Comment 2 Yong Li 2009-08-13 12:29:47 PDT
Created attachment 34769 [details]
2) ImageWince.cpp

implement some platform-specific BitmapImage methods.
Comment 3 Adam Barth 2009-09-01 18:45:06 PDT
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.
Comment 4 Adam Barth 2009-09-01 18:49:21 PDT
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.
Comment 5 Yong Li 2009-09-01 18:51:12 PDT
(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
Comment 6 Kwang Yul Seo 2010-04-13 21:31:35 PDT
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.
Comment 7 Kent Tamura 2010-06-19 09:29:54 PDT
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.
Comment 8 Patrick R. Gansterer 2010-09-09 10:51:07 PDT
Created attachment 67058 [details]
2) ImageWinCE.cpp
Comment 9 WebKit Review Bot 2010-09-09 10:53:54 PDT
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.
Comment 10 Adam Roben (:aroben) 2010-09-14 09:39:06 PDT
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.
Comment 11 Patrick R. Gansterer 2010-09-16 09:00:20 PDT
Comment on attachment 67058 [details]
2) ImageWinCE.cpp

Clearing flags on attachment: 67058

Committed r67624: <http://trac.webkit.org/changeset/67624>
Comment 12 Patrick R. Gansterer 2010-10-23 05:43:26 PDT
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.
Comment 13 WebKit Review Bot 2010-10-23 05:46:00 PDT
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.
Comment 14 Adam Roben (:aroben) 2010-10-27 05:06:49 PDT
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.
Comment 15 Patrick R. Gansterer 2010-10-27 05:13:59 PDT
(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.
Comment 16 Adam Roben (:aroben) 2010-10-27 05:20:19 PDT
(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.
Comment 17 Yong Li 2010-10-27 08:12:43 PDT
(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
Comment 18 Adam Roben (:aroben) 2010-11-01 14:19:11 PDT
(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.
Comment 19 Adam Roben (:aroben) 2010-11-01 14:47:31 PDT
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.
Comment 20 Patrick R. Gansterer 2010-11-01 16:59:17 PDT
Created attachment 72602 [details]
1) SharedBitmap
Comment 21 Patrick R. Gansterer 2010-11-01 17:01:36 PDT
Created attachment 72603 [details]
1) SharedBitmap
Comment 22 WebKit Review Bot 2010-11-01 17:02:12 PDT
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.
Comment 23 WebKit Review Bot 2010-11-01 17:03:19 PDT
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.
Comment 24 Adam Roben (:aroben) 2010-11-03 04:22:19 PDT
(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...
Comment 25 Adam Roben (:aroben) 2010-11-03 04:36:52 PDT
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?
Comment 26 Patrick R. Gansterer 2010-11-03 05:26:35 PDT
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?
Comment 27 Adam Roben (:aroben) 2010-11-03 06:15:08 PDT
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.
Comment 28 Patrick R. Gansterer 2010-11-03 07:07:10 PDT
Created attachment 72819 [details]
1) SharedBitmap

Added GraphicsContext changes
Comment 29 Adam Roben (:aroben) 2010-11-03 07:11:22 PDT
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.
Comment 30 Patrick R. Gansterer 2010-11-03 07:40:26 PDT
Comment on attachment 72819 [details]
1) SharedBitmap

Clearing flags on attachment: 72819

Manually committed r71237: <http://trac.webkit.org/changeset/71237>
Comment 31 Patrick R. Gansterer 2010-11-03 07:41:02 PDT
All reviewed patches have been landed.  Closing bug.