[Webkit-unassigned] [Bug 27349] [WINCE] Add GraphicsContextWince implementation for WinCE.
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sat Jul 18 09:24:10 PDT 2009
https://bugs.webkit.org/show_bug.cgi?id=27349
--- Comment #7 from Yong Li <yong.li at torchmobile.com> 2009-07-18 09:24:06 PDT ---
(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.
--
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