[Webkit-unassigned] [Bug 37440] [WINCE] Port tiled backing store
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu May 20 01:27:14 PDT 2010
https://bugs.webkit.org/show_bug.cgi?id=37440
--- Comment #9 from Young Han Lee <joybro at company100.net> 2010-05-20 01:27:12 PST ---
(In reply to comment #7)
> (From update of attachment 53156 [details])
> > + * platform/graphics/wince/TileWince.cpp: Added.
>
> This file should be named TileWinCE.cpp (see bug 37287).
>
> > + (WebCore::checkeredBitmap):
> > + (WebCore::Tile::Tile):
> > + (WebCore::Tile::~Tile):
> > + (WebCore::Tile::isDirty):
> > + (WebCore::Tile::isReadyToPaint):
> > + (WebCore::Tile::invalidate):
> > + (WebCore::enclosingRect):
> > + (WebCore::getUpdateRects):
> > + (WebCore::Tile::updateBackBuffer):
> > + (WebCore::Tile::swapBackBufferToFront):
> > + (WebCore::Tile::paint):
> > + (WebCore::Tile::paintCheckerPattern):
>
> Function-level comments are helpful to anyone trying to understand the patch. That includes reviewers and people in the future who are trying to figure out what the point of this patch was (if, e.g., it caused a regression or contained a bug). It's especially good if the comments explain the "why" of the patch, not just the "how".
>
> > +static PassRefPtr<SharedBitmap> checkeredBitmap()
> > +{
> > + static RefPtr<SharedBitmap> bitmap;
> > + if (!bitmap) {
> > + bitmap = SharedBitmap::createInstance(true, checkerSize, checkerSize, false);
> > + unsigned key1, key2;
> > + HDC dc = bitmap->getDC(&key1, &key2);
> > + GraphicsContext context(dc);
> > +
> > + Color color1(checkerColor1);
> > + Color color2(checkerColor2);
> > + for (unsigned y = 0; y < checkerSize; y += checkerSize / 2) {
> > + bool alternate = y % checkerSize;
> > + for (unsigned x = 0; x < checkerSize; x += checkerSize / 2) {
> > + context.fillRect(FloatRect(x, y, checkerSize / 2, checkerSize / 2),
> > + alternate ? color1 : color2, DeviceColorSpace);
> > + alternate = !alternate;
> > + }
> > + }
> > +
> > + bitmap->releaseDC(dc, key1, key2);
> > + }
> > + return bitmap.release();
> > +}
>
> It looks like this function is trying to create the bitmap just once and then return that single cached instance. But that's not the behavior it will have. RefPtr::release nulls out the RefPtr, so if (!bitmap) will always be true.
>
> I think instead you should change this function to return a bare pointer. It's a also slightly nicer style to have two separate functions: a "create" function that always allocates a new bitmap, and a getter that returns a single cached instance. Like this:
>
> static PassRefPtr<SharedBitmap> createCheckeredBitmap()
> {
> RefPtr<SharedBitmap> bitmap = SharedBitmap::createInstance(...);
> // Draw the checker pattern
> return bitmap.release();
> }
>
> static SharedBitmap* checkeredBitmap()
> {
> static SharedBitmap* bitmap = createCheckeredBitmap().releaseRef();
> return bitmap;
> }
>
> > +Tile::Tile(TiledBackingStore* backingStore, const Coordinate& tileCoordinate)
> > + : m_backingStore(backingStore)
> > + , m_coordinate(tileCoordinate)
> > + , m_rect(m_backingStore->tileRectForCoordinate(tileCoordinate))
> > + , m_buffer(0)
> > + , m_backBuffer(0)
>
> No need to initialize m_buffer or m_backBuffer here. RefPtr does this for you automatically.
>
> > +static IntRect enclosingRect(HRGN hrgn)
> > +{
> > + RECT r;
> > + GetRgnBox(hrgn, &r);
> > + return r;
> > +}
> > +
>
> This function is only called once. I'm not sure it's worth having a separate function for this.
>
> > +static void getUpdateRects(HRGN region, Vector<IntRect>& rects)
> > +{
> > + const int rectThreshold = 10;
> > + const float wastedSpaceThreshold = 0.75f;
> > +
> > + IntRect dirtyRect(enclosingRect(region));
> > +
> > + DWORD regionDataSize = GetRegionData(region, sizeof(RGNDATA), 0);
> > + if (!regionDataSize) {
> > + rects.append(dirtyRect);
> > + return;
> > + }
> > +
> > + Vector<unsigned char> buffer(regionDataSize);
> > + RGNDATA* regionData = reinterpret_cast<RGNDATA*>(buffer.data());
> > + GetRegionData(region, regionDataSize, regionData);
> > + if (regionData->rdh.nCount > rectThreshold) {
> > + rects.append(dirtyRect);
> > + return;
> > + }
> > +
> > + double singlePixels = 0.0;
> > + unsigned i;
> > + RECT* rect;
> > + for (i = 0, rect = reinterpret_cast<RECT*>(regionData->Buffer); i < regionData->rdh.nCount; i++, rect++)
> > + singlePixels += (rect->right - rect->left) * (rect->bottom - rect->top);
> > +
> > + double unionPixels = dirtyRect.width() * dirtyRect.height();
> > + double wastedSpace = 1.0 - (singlePixels / unionPixels);
> > + if (wastedSpace <= wastedSpaceThreshold) {
> > + rects.append(dirtyRect);
> > + return;
> > + }
> > +
> > + for (i = 0, rect = reinterpret_cast<RECT*>(regionData->Buffer); i < regionData->rdh.nCount; i++, rect++)
> > + rects.append(*rect);
> > +}
>
> This looks to be a copy of the function of the same name in WebKit/win/WebView.cpp. Can we share the code instead of duplicating it?
>
> > +void Tile::updateBackBuffer()
> > +{
> > + if (m_buffer && !isDirty())
> > + return;
> > +
> > + if (!m_backBuffer) {
> > + if (!m_buffer)
> > + m_backBuffer = SharedBitmap::createInstance(
> > + true, m_backingStore->m_tileSize.width(), m_backingStore->m_tileSize.height(), false);
>
> We normally would just put this all on one line. I think you should do that here.
>
> It would be nice if there were an overload of SharedBitmap::createInstance that took a const IntSize&. It would also be nice if it used enums instead of booleans, as I have no idea what "true" and "false" mean here.
>
> > + else {
> > + // Currently all buffers are updated synchronously at the same time so there is no real need
> > + // to have separate back and front buffers. Just use the existing buffer.
>
> If there's no need for two buffers, why do we have them?
>
> > + m_backBuffer = m_buffer;
> > + m_buffer = 0;
>
> You can do this more efficiently like this:
>
> m_backBuffer = m_buffer.release();
>
> > + HRGN nullRegion = CreateRectRgn(0, 0, 0, 0);
> > + m_dirtyRegion.set(nullRegion);
>
> No need for the local nullRegion variable.
>
> > + int size = dirtyRects.size();
> > + for (int n = 0; n < size; ++n) {
>
> It's a little better to use size_t instead of int when iterating over a Vector.
>
> > +void Tile::swapBackBufferToFront()
> > +{
> > + if (!m_backBuffer)
> > + return;
> > + m_buffer = m_backBuffer;
> > + m_backBuffer = 0;
> > +}
>
> You can do this more efficiently like this:
>
> m_buffer = m_backBuffer.release();
>
> > +void Tile::paint(GraphicsContext* context, const IntRect& rect)
> > +{
> > + if (!m_buffer)
> > + return;
> > +
> > + IntRect target = intersection(rect, m_rect);
> > + IntRect source((target.x() - m_rect.x()),
> > + (target.y() - m_rect.y()),
>
> No need for the parentheses around these expressions. I'm also not sure all the newlines are making this more readable.
>
> > +void Tile::paintCheckerPattern(GraphicsContext* context, const FloatRect& target)
> > +{
> > + FloatRect tileRectIn(0, 0, checkerSize, checkerSize);
> > + context->drawBitmapPattern(checkeredBitmap().get(),
> > + tileRectIn,
> > + AffineTransform(),
> > + FloatPoint(0, 0),
>
> You can just say FloatPoint()
>
> > + CompositeCopy,
> > + target,
> > + IntSize(checkerSize, checkerSize));
>
> Is there no way to get the size from checkeredBitmap? It seems unfortunate to have to use the constants again here.
>
> Again, I don't think all the newlines are helping here.
I accepted all your points except a few works seems like overwork. We could consider it later.
--
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