[webkit-reviews] review denied: [Bug 73668] Upstream BlackBerry API backing store files : [Attachment 117648] The patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sun Dec 4 20:30:02 PST 2011
Antonio Gomes <tonikitoo at webkit.org> has denied Jacky Jiang
<zkjiang008 at gmail.com>'s request for review:
Bug 73668: Upstream BlackBerry API backing store files
https://bugs.webkit.org/show_bug.cgi?id=73668
Attachment 117648: The patch
https://bugs.webkit.org/attachment.cgi?id=117648&action=review
------- Additional Comments from Antonio Gomes <tonikitoo at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=117648&action=review
Patch looks good, as I know the code, but we have too many minor issues, that
another round is needed.
Jacky, please fix all comment styles (including the ones I did not highlight).
> Source/WebKit/ChangeLog:156
> + (BlackBerry::WebKit::divisors):
> + (BlackBerry::WebKit::bestDivisor):
> +
(BlackBerry::WebKit::BackingStoreMutexLocker::BackingStoreMutexLocker):
> +
(BlackBerry::WebKit::BackingStoreMutexLocker::~BackingStoreMutexLocker):
> + (BlackBerry::WebKit::BackingStoreGeometry::backingStoreRect):
> + (BlackBerry::WebKit::BackingStoreGeometry::backingStoreSize):
> + (BlackBerry::WebKit::BackingStorePrivate::BackingStorePrivate):
> + (BlackBerry::WebKit::BackingStorePrivate::~BackingStorePrivate):
> +
(BlackBerry::WebKit::BackingStorePrivate::shouldDirectRenderingToWindow):
> +
(BlackBerry::WebKit::BackingStorePrivate::suspendScreenAndBackingStoreUpdates):
> +
(BlackBerry::WebKit::BackingStorePrivate::resumeScreenAndBackingStoreUpdates):
> + (BlackBerry::WebKit::BackingStorePrivate::repaint):
> + (BlackBerry::WebKit::BackingStorePrivate::slowScroll):
> + (BlackBerry::WebKit::BackingStorePrivate::scroll):
> + (BlackBerry::WebKit::BackingStorePrivate::scrollingStartedHelper):
> +
(BlackBerry::WebKit::BackingStorePrivate::shouldSuppressNonVisibleRegularRender
Jobs):
> + (BlackBerry::WebKit::BackingStorePrivate::shouldPerformRenderJobs):
> +
(BlackBerry::WebKit::BackingStorePrivate::shouldPerformRegularRenderJobs):
> + (BlackBerry::WebKit::BackingStorePrivate::startRenderTimer):
> + (BlackBerry::WebKit::BackingStorePrivate::stopRenderTimer):
> + (BlackBerry::WebKit::BackingStorePrivate::renderOnTimer):
> + (BlackBerry::WebKit::BackingStorePrivate::renderOnIdle):
> + (BlackBerry::WebKit::BackingStorePrivate::willFireTimer):
> + (BlackBerry::WebKit::BackingStorePrivate::expandedContentsRect):
> + (BlackBerry::WebKit::BackingStorePrivate::visibleContentsRect):
> +
(BlackBerry::WebKit::BackingStorePrivate::unclippedVisibleContentsRect):
> + (BlackBerry::WebKit::BackingStorePrivate::shouldMoveLeft):
> + (BlackBerry::WebKit::BackingStorePrivate::shouldMoveRight):
> + (BlackBerry::WebKit::BackingStorePrivate::shouldMoveUp):
> + (BlackBerry::WebKit::BackingStorePrivate::shouldMoveDown):
> + (BlackBerry::WebKit::BackingStorePrivate::canMoveX):
> + (BlackBerry::WebKit::BackingStorePrivate::canMoveY):
> + (BlackBerry::WebKit::BackingStorePrivate::canMoveLeft):
> + (BlackBerry::WebKit::BackingStorePrivate::canMoveRight):
> + (BlackBerry::WebKit::BackingStorePrivate::canMoveUp):
> + (BlackBerry::WebKit::BackingStorePrivate::canMoveDown):
> +
(BlackBerry::WebKit::BackingStorePrivate::backingStoreRectForScroll):
> + (BlackBerry::WebKit::BackingStorePrivate::setBackingStoreRect):
> +
(BlackBerry::WebKit::BackingStorePrivate::indexesForBackingStoreRect):
> +
(BlackBerry::WebKit::BackingStorePrivate::originOfLastRenderForTile):
> + (BlackBerry::WebKit::BackingStorePrivate::indexOfLastRenderForTile):
> + (BlackBerry::WebKit::BackingStorePrivate::indexOfTile):
> +
(BlackBerry::WebKit::BackingStorePrivate::clearAndUpdateTileOfNotRenderedRegion
):
> + (BlackBerry::WebKit::BackingStorePrivate::isCurrentVisibleJob):
> + (BlackBerry::WebKit::BackingStorePrivate::scrollBackingStore):
> + (BlackBerry::WebKit::BackingStorePrivate::render):
> + (BlackBerry::WebKit::BackingStorePrivate::renderDirectToWindow):
> + (BlackBerry::WebKit::BackingStorePrivate::requestLayoutIfNeeded):
> + (BlackBerry::WebKit::BackingStorePrivate::renderVisibleContents):
> + (BlackBerry::WebKit::BackingStorePrivate::renderBackingStore):
> + (BlackBerry::WebKit::BackingStorePrivate::blitVisibleContents):
> +
(BlackBerry::WebKit::BackingStorePrivate::copyPreviousContentsToBackSurfaceOfWi
ndow):
> +
(BlackBerry::WebKit::BackingStorePrivate::copyPreviousContentsToBackSurfaceOfTi
le):
> + (BlackBerry::WebKit::BackingStorePrivate::paintDefaultBackground):
> + (BlackBerry::WebKit::BackingStorePrivate::blitContents):
> + (BlackBerry::WebKit::BackingStorePrivate::blitTileRect):
> + (BlackBerry::WebKit::BackingStorePrivate::blendCompositingSurface):
> + (BlackBerry::WebKit::BackingStorePrivate::clearCompositingSurface):
> + (BlackBerry::WebKit::BackingStorePrivate::blitHorizontalScrollbar):
> + (BlackBerry::WebKit::BackingStorePrivate::blitVerticalScrollbar):
> + (BlackBerry::WebKit::BackingStorePrivate::isTileVisible):
> + (BlackBerry::WebKit::BackingStorePrivate::visibleTilesRect):
> + (BlackBerry::WebKit::BackingStorePrivate::tileVisibleContentsRect):
> +
(BlackBerry::WebKit::BackingStorePrivate::tileUnclippedVisibleContentsRect):
> + (BlackBerry::WebKit::BackingStorePrivate::tileContentsRect):
> + (BlackBerry::WebKit::BackingStorePrivate::resetRenderQueue):
> + (BlackBerry::WebKit::BackingStorePrivate::clearVisibleZoom):
> + (BlackBerry::WebKit::BackingStorePrivate::resetTiles):
> + (BlackBerry::WebKit::BackingStorePrivate::updateTiles):
> +
(BlackBerry::WebKit::BackingStorePrivate::updateTilesForScrollOrNotRenderedRegi
on):
> + (BlackBerry::WebKit::BackingStorePrivate::resetTile):
> + (BlackBerry::WebKit::BackingStorePrivate::updateTile):
> +
(BlackBerry::WebKit::BackingStorePrivate::mapFromTilesToTransformedContents):
> +
(BlackBerry::WebKit::BackingStorePrivate::mapFromTransformedContentsToAbsoluteT
ileBoundaries):
> +
(BlackBerry::WebKit::BackingStorePrivate::mapFromTransformedContentsToTiles):
> + (BlackBerry::WebKit::BackingStorePrivate::updateTileMatrixIfNeeded):
> + (BlackBerry::WebKit::BackingStorePrivate::contentsSizeChanged):
> + (BlackBerry::WebKit::BackingStorePrivate::scrollChanged):
> + (BlackBerry::WebKit::BackingStorePrivate::transformChanged):
> + (BlackBerry::WebKit::BackingStorePrivate::orientationChanged):
> + (BlackBerry::WebKit::BackingStorePrivate::actualVisibleSizeChanged):
> + (BlackBerry::WebKit::createVisibleTileBufferForWebPage):
> + (BlackBerry::WebKit::BackingStorePrivate::createSurfaces):
> + (BlackBerry::WebKit::BackingStorePrivate::createVisibleTileBuffer):
> + (BlackBerry::WebKit::BackingStorePrivate::originOfTile):
> + (BlackBerry::WebKit::BackingStorePrivate::minimumNumberOfTilesWide):
> + (BlackBerry::WebKit::BackingStorePrivate::minimumNumberOfTilesHigh):
> + (BlackBerry::WebKit::BackingStorePrivate::expandedContentsSize):
> + (BlackBerry::WebKit::BackingStorePrivate::tileWidth):
> + (BlackBerry::WebKit::BackingStorePrivate::tileHeight):
> + (BlackBerry::WebKit::BackingStorePrivate::tileSize):
> + (BlackBerry::WebKit::BackingStorePrivate::tileRect):
> + (BlackBerry::WebKit::BackingStorePrivate::renderContents):
> + (BlackBerry::WebKit::drawDebugRect):
> + (BlackBerry::WebKit::BackingStorePrivate::blitToWindow):
> + (BlackBerry::WebKit::BackingStorePrivate::checkerWindow):
> + (BlackBerry::WebKit::BackingStorePrivate::invalidateWindow):
> + (BlackBerry::WebKit::BackingStorePrivate::clearWindow):
> + (BlackBerry::WebKit::BackingStorePrivate::isScrollingOrZooming):
> + (BlackBerry::WebKit::BackingStorePrivate::setScrollingOrZooming):
> + (BlackBerry::WebKit::BackingStorePrivate::lockBackingStore):
> + (BlackBerry::WebKit::BackingStorePrivate::unlockBackingStore):
> + (BlackBerry::WebKit::BackingStorePrivate::frontState):
> + (BlackBerry::WebKit::BackingStorePrivate::backState):
> + (BlackBerry::WebKit::BackingStorePrivate::swapState):
> + (BlackBerry::WebKit::BackingStorePrivate::windowFrontBufferState):
> + (BlackBerry::WebKit::BackingStorePrivate::windowBackBufferState):
> + (BlackBerry::WebKit::BackingStorePrivate::drawSubLayers):
> + (BlackBerry::WebKit::BackingStorePrivate::isActive):
> + (BlackBerry::WebKit::BackingStore::BackingStore):
> + (BlackBerry::WebKit::BackingStore::~BackingStore):
> + (BlackBerry::WebKit::BackingStore::createSurface):
> +
(BlackBerry::WebKit::BackingStore::suspendScreenAndBackingStoreUpdates):
> +
(BlackBerry::WebKit::BackingStore::resumeScreenAndBackingStoreUpdates):
> + (BlackBerry::WebKit::BackingStore::isScrollingOrZooming):
> + (BlackBerry::WebKit::BackingStore::setScrollingOrZooming):
> + (BlackBerry::WebKit::BackingStore::blitContents):
> + (BlackBerry::WebKit::BackingStore::repaint):
> + (BlackBerry::WebKit::BackingStore::hasRenderJobs):
> + (BlackBerry::WebKit::BackingStore::renderOnIdle):
> + (BlackBerry::WebKit::BackingStore::isDirectRenderingToWindow):
> + (BlackBerry::WebKit::BackingStore::defersBlit):
> + (BlackBerry::WebKit::BackingStore::setDefersBlit):
> + (BlackBerry::WebKit::BackingStore::hasBlitJobs):
> + (BlackBerry::WebKit::BackingStore::blitOnIdle):
> + * blackberry/Api/BackingStore.h: Added.
> + * blackberry/Api/BackingStore_p.h: Added.
> + (BlackBerry::WebKit::BackingStoreGeometry::BackingStoreGeometry):
> + (BlackBerry::WebKit::BackingStoreGeometry::numberOfTilesWide):
> + (BlackBerry::WebKit::BackingStoreGeometry::setNumberOfTilesWide):
> + (BlackBerry::WebKit::BackingStoreGeometry::numberOfTilesHigh):
> + (BlackBerry::WebKit::BackingStoreGeometry::setNumberOfTilesHigh):
> + (BlackBerry::WebKit::BackingStoreGeometry::backingStoreOffset):
> + (BlackBerry::WebKit::BackingStoreGeometry::setBackingStoreOffset):
> + (BlackBerry::WebKit::BackingStoreGeometry::tileAt):
> + (BlackBerry::WebKit::BackingStoreGeometry::tileMap):
> + (BlackBerry::WebKit::BackingStoreGeometry::setTileMap):
> + (BlackBerry::WebKit::BackingStoreWindowBufferState::blittedRegion):
> +
(BlackBerry::WebKit::BackingStoreWindowBufferState::addBlittedRegion):
> +
(BlackBerry::WebKit::BackingStoreWindowBufferState::clearBlittedRegion):
> + (BlackBerry::WebKit::BackingStoreWindowBufferState::isRendered):
> +
(BlackBerry::WebKit::BackingStorePrivate::setCurrentBackingStoreOwner):
> + (BlackBerry::WebKit::BackingStorePrivate::currentBackingStoreOwner):
This whole block can be omitted since it is a new file.
> Source/WebKit/blackberry/Api/BackingStore.cpp:302
> + /*
> + * If immediate is true, then we're being asked to perform synchronously
> + *
> + * NOTE: WebCore::ScrollView will call this method with immediate:true
and contentChanged:false.
> + * This is a special case introduced specifically for the Apple's
windows port and can be safely ignored I believe.
> + *
> + * Now this method will be called from WebPagePrivate::repaint()
> + */
Lets use "//" comments.
> Source/WebKit/blackberry/Api/BackingStore.cpp:308
> + rect.inflate(1 /*dx*/, 1 /*dy*/); // account for anti-aliasing of
previous rendering runs
comments start with capital letter, and dot at the end.
> Source/WebKit/blackberry/Api/BackingStore.cpp:483
> + // so it can determine if it is under pressure
missing "." at the end.
> Source/WebKit/blackberry/Api/BackingStore.cpp:660
> + // Record the current backingstore rect
ditto
> Source/WebKit/blackberry/Api/BackingStore.cpp:686
> + TileMap tiles; // our new tile map
instead of a comment like this, we should have a better variable name
> Source/WebKit/blackberry/Api/BackingStore.cpp:687
> + TileMap leftOverTiles; // tiles that are left over
unneeded comment, given the variable name
> Source/WebKit/blackberry/Api/BackingStore.cpp:690
> + // Iterate through our current tile map and add tiles that are rendered
with
> + // our new backingstore rect
missing "." in the end.
> Source/WebKit/blackberry/Api/BackingStore.cpp:696
> + // Reset the old index
ditto
> Source/WebKit/blackberry/Api/BackingStore.cpp:699
> + // Origin of last committed render for tile in transformed content
coordinates
ditto
> Source/WebKit/blackberry/Api/BackingStore.cpp:703
> + // If the new backing store rect contains this origin, then insert
the tile there
> + // and mark it as no longer shifted. Note:
Platform::IntRect::contains checks for a 1x1 rect
ditto
> Source/WebKit/blackberry/Api/BackingStore.cpp:717
> + // Intersect the tile with the not rendered region to get
the areas
> + // of the tile that we need to clear
ditto
> Source/WebKit/blackberry/Api/BackingStore.cpp:746
> + ASSERT((int)indexesToFill.size() == leftOverTiles.size());
c-cast is to be avoided.
> Source/WebKit/blackberry/Api/BackingStore.cpp:759
> + // Origin of last committed render for tile in transformed content
coordinates
period
> Source/WebKit/blackberry/Api/BackingStore.cpp:778
> + // Checks to make sure we haven't lost any tiles
ditto
> Source/WebKit/blackberry/Api/BackingStore.cpp:781
> + // Actually set state
ditto
> Source/WebKit/blackberry/Api/BackingStore.cpp:787
> + // Swap the front/back state
ditto
> Source/WebKit/blackberry/Api/BackingStore.cpp:835
> + // Intersect the tile with the not rendered region to get the areas
> + // of the tile that we need to clear
ditto
> Source/WebKit/blackberry/Api/BackingStore.cpp:847
> + // Find the origin of this tile
ditto
> Source/WebKit/blackberry/Api/BackingStore.cpp:850
> + // Map to tile coordinates
ditto
> Source/WebKit/blackberry/Api/BackingStore.cpp:1073
> + // FIXME: modify render to take a Vector<IntRect> parameter so we're
not recreating
> + // GraphicsContext on the stack each time
ditto
> Source/WebKit/blackberry/Api/BackingStore.cpp:1076
> + // Add the newly rendered region to the tile so it can keep track
for blits
ditto
> Source/WebKit/blackberry/Api/BackingStore.cpp:1088
> + // We will need a swap here because of the shared back buffer
ditto.
> Source/WebKit/blackberry/Api/BackingStore.cpp:1098
> + // Clip to the visible content including overscroll
ditto
> Source/WebKit/blackberry/Api/BackingStore.cpp:2431
> +#if ENABLE_SCROLLBARS
Is ENABLE_SCROLLBARS really needed?
> Source/WebKit/blackberry/Api/BackingStore.cpp:2626
> + d->blitVisibleContents(true /* force */);
/*force*/
More information about the webkit-reviews
mailing list