[Webkit-unassigned] [Bug 27349] [WINCE] Add GraphicsContextWince implementation for WinCE.
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jul 20 07:06:00 PDT 2009
https://bugs.webkit.org/show_bug.cgi?id=27349
--- Comment #10 from Adam Roben (aroben) <aroben at apple.com> 2009-07-20 07:05:58 PDT ---
(In reply to comment #7)
> (In reply to comment #5)
> > > +static inline int Round(double d)
> >
> > Function names start with a lowercase letter in WebKit. But you can probably
> > just use lround from wtf/MathExtras.h instead.
>
> I just reviewed lround(). It does the same thing, but it calls "ceil()" for
> negative values, which is probably not good in term of performance.
If your version is better, shouldn't we change lround() in MathExtras.h so that
everyone will benefit?
> > > +static inline IntRect roundRect(const FloatRect& r)
> > > +{
> > > + return IntRect(Round(r.x()), Round(r.y()), Round(r.right()) - Round(r.x()), Round(r.bottom()) - Round(r.y()));
> > > +}
> >
> > Are you sure you don't want to use enclosingIntRect instead?
>
> I'm sure. enclosingIntRect() can result more rendering problems when page is
> zoomed.
OK. A comment explaining this would be good.
> > > + FloatSize mapSize(const FloatSize& size)
> > > + {
> > > + double w, h;
> > > + m_transform.map(size.width(), size.height(), w, h);
> > > + return FloatSize(w, h);
> > > + }
> >
> > Earlier you used static_cast when passing doubles to the FloatSize constructor.
> > It would be good to be consistent one way or the other.
>
> how about static_cast<float>(w)?
Sounds fine.
> > > + PassRefPtr<SharedBitmap> getTransparentLayerBitmap(IntRect& origRect, bool uses16bit, RECT& bmpRect, bool checkClipBox, bool force)
> >
> > It seems strange to use a mix of IntRect and RECT in this declaration. (applies
> > elsewhere)
>
> To avoid converting IntRect and RECT too frequently.
Have you noticed IntRect <-> RECT conversions showing up on performance
profiles? It seems like it would be dwarfed by the actual painting calls. But I
don't know the performance characteristics of your platform(s).
> > > +#if !defined(NO_ALPHABLEND)
> > > + if (m_opacity < 1. || !use16bit) {
> > > + const BLENDFUNCTION blend = { AC_SRC_OVER, 0
> > > + , m_opacity >= 1. ? 255 : (BYTE)(m_opacity * 255)
> > > + , use16bit ? 0 : AC_SRC_ALPHA };
> > > + AlphaBlend
> > > + (m_dc
> > > + , origRect.x()
> > > + , origRect.y()
> > > + , origRect.width()
> > > + , origRect.height()
> > > + , hdc
> > > + , 0
> > > + , 0
> > > + , bmpRect.right
> > > + , bmpRect.bottom
> > > + , blend
> > > + );
> >
> > I've never seen this one-parameter-per-line style anywhere else in WebKit. I'm
> > not sure it's a good idea to introduce it here. (applies elsewhere)
>
> It makes code much more readable when there are too many arguments.
It makes it more readable if you're used to this style and if it's used
elsewhere in the code. We try to maintain a consistent coding style in WebKit
because we think it's a lot easier to read the code if the same style is used
everywhere. I won't r- the patch just for this issue, but I strongly recommend
that you format your code to match the WebKit coding style, so that WebKit's
code can continue being consistent (and therefore easier to read as a whole).
> > > + *dest++ = source[srcY * paddedSourceW + srcX] | 0xFF000000;
> > > + else
> > > + *dest++ |= 0xFF;
> > > + }
> > > + dest += padding;
> > > + }
> > > + }
> > > +}
> >
> > It would be nice to have a little less duplication in this function.
>
> Performance vs code size... that's a fight.
Would an inline function really affect performance?
> > > + : m_data(data)
> > > + , m_origRect(origRect)
> > > + , m_oldOpacity(data->m_opacity)
> >
> > It seems error-prone to leave m_key1 and m_key2 uninitialized here.
>
> m_key1 and m_key2 will be set up by ->getDC(). In many cases, they are not used
> at all. Performance consideration.
I saw that they get initialized in getDC. My concern was if someone were to add
code to use these members elsewhere, and didn't realize they don't get
initialized by the constructor. Have you seen evidence that initializing these
members affects performance? It would surprise me if so, but again I don't know
the details of your platform(s).
> > > + if (fillColor().alpha()) {
> > > + brush = createBrush(fillColor());
> > > + oldBrush = SelectObject(dc, brush);
> > > + } else {
> > > + SelectObject(dc, GetStockObject(NULL_BRUSH));
> > > + }
> >
> > We don't put braces around the bodies of single-line branches.
>
> We will follow the rules if we have to. But I really don't like this rule.
> consider this:
>
> if () {
> 2 lines here
> } else
> one line code;
> else if {
> 2 lines here
> }
>
> It looks so terrible. Even this case
>
> if () {
> one line code;
> }
>
> I cannot see nothing wrong with it.
I agree that there are some cases where omitting the braces looks funny/bad.
But see my comments above about maintaining a consistent coding style
throughout WebKit.
> > > +void GraphicsContext::fillRect(const FloatRect& rect, const Color& color)
> > > +{
> > > + if (paintingDisabled() || !m_data->m_opacity)
> > > + return;
> > > +
> > > + if (int alpha = color.alpha()) {
> > > + OwnPtr<HBRUSH> hbrush(CreateSolidBrush(RGB(color.red(), color.green(), color.blue())));
> > > + if (hbrush) {
> > > + ScopeDCProvider dcProvider(m_data);
> > > + if (m_data->m_dc) {
> > > + IntRect intRect = enclosingIntRect(rect);
> > > + TransparentLayerDC transparentDc(m_data, m_data->mapRect(intRect), &intRect);
> > > + if (transparentDc.hdc())
> > > + if (alpha == 0xFF)
> > > + FillRect(transparentDc.hdc(), &transparentDc.rect(), hbrush.get());
> > > + else
> > > + fillRectWithAlpha(transparentDc.hdc(), &transparentDc.rect(), hbrush.get(), alpha);
> > > + }
> > > + }
> > > + }
> > > +}
> >
> > This function would benefit from reducing nesting by using early returns.
>
> By benefit, do you mean it runs faster?
No, I meant that it would be more readable for someone who is accustomed to
WebKit's coding style.
> Personally, I don't like too many
> "return" in a function. it's just like "goto", which makes code more difficult
> to maintain & review.
Early returns are an idiom we use throughout WebKit. One specific reason we use
them is to avoid nesting and all the extra indentation it brings.
> > > +#define isCharVisible(c) ((c) && (c) != zeroWidthSpace)
> >
> > This would be better as an inline helper function.
>
> Only concern is inline function is not inline in debug version, and it makes
> debug version run much slower.
One advantage of an function instead of a macro is that it gives you stronger
type safety.
> > > +
> > > + if (hFont) {
> >
> > If you put the !hFont case first, you can add an early return at the end of it,
> > and add early returns throughout the rest of the function to reduce nesting.
> > i.e.,:
> >
> > if (!hFont) {
> > ....the current else case goes here....
> > return;
> > }
> >
> > ....the rest of the code goes here, hopefully with more early returns....
>
> Later, if we want to add more code at the end to clean up something, we have to
> make it nested again.
Are you expecting to add something like that in the future? If not, it seems a
bit premature to be structuring the code in ways that make certain unplanned
future changes easier.
> > > + HGDIOBJ hOldFont = SelectObject(m_data->m_dc, hFont);
> > > + // transparent always...
> > > + int oldBkMode = GetBkMode(m_data->m_dc);
> >
> > What does this comment mean?
>
> BkMode is always transparent.
oldBkMode? Or the new one that you're setting?
--
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