Bug 73668

Created attachment 117648 [details] The patch

Comment on attachment 117648 [details] The patch 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::shouldSuppressNonVisibleRegularRenderJobs): > + (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::copyPreviousContentsToBackSurfaceOfWindow): > + (BlackBerry::WebKit::BackingStorePrivate::copyPreviousContentsToBackSurfaceOfTile): > + (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::updateTilesForScrollOrNotRenderedRegion): > + (BlackBerry::WebKit::BackingStorePrivate::resetTile): > + (BlackBerry::WebKit::BackingStorePrivate::updateTile): > + (BlackBerry::WebKit::BackingStorePrivate::mapFromTilesToTransformedContents): > + (BlackBerry::WebKit::BackingStorePrivate::mapFromTransformedContentsToAbsoluteTileBoundaries): > + (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*/

(In reply to comment #2) > (From update of attachment 117648 [details]) > 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). > Will do, thanks for your review Antonio!

Created attachment 117904 [details] Updated patch Updated the patch according to Antonio's comments.

Created attachment 117912 [details] Updated patch Updated more style fixes.

Comment on attachment 117912 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=117912&action=review > Source/WebKit/blackberry/Api/BackingStore.cpp:27 > +#include "FloatRect.h" This include is unnecessary as GraphicsContext.h (below) includes this file. > Source/WebKit/blackberry/Api/BackingStore.cpp:33 > +#include "PlatformString.h" This file is unnecessary as Page.h (above) includes this file. > Source/WebKit/blackberry/Api/BackingStore.cpp:77 > +// Compute divisor pairs. This comment is inane. I suggest we remove it. > Source/WebKit/blackberry/Api/BackingStore.cpp:80 > +// TODO: cache this and/or use a smarter algorithm. Nit: TODO => FIXME cache => Cache > Source/WebKit/blackberry/Api/BackingStore.cpp:84 > + for (unsigned int i = 1; i <= n; ++i) unsigned int => unsigned > Source/WebKit/blackberry/Api/BackingStore.cpp:153 > +struct BackingStoreMutexLocker { Nit: I would the keyword "struct" to "class" since we have private instance data here and then explicitly add a "public:" keyword. Also, this class should be non copyable since it doesn't make sense to copy this RAII object. Using WTF_MAKE_NONCOPYABLE() defined in <http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/wtf/Noncopyable.h?rev=76248#L29>, I would write this class to have the form: class BackingStoreMutexLocker { WTF_MAKE_NONCOPYABLE(BackingStoreMutexLocker); public: ... private: ... }; > Source/WebKit/blackberry/Api/BackingStore.cpp:218 > + delete m_renderQueue; > + m_renderQueue = 0; > + delete m_renderTimer; > + m_renderTimer = 0; We should make both m_renderQueue and m_renderTimer OwnPtrs. Then we don't need to explicitly delete them here. > Source/WebKit/blackberry/Api/BackingStore.cpp:335 > + render(rect, false /*renderContentOnly*/); We should make render() take an enum value for its second argument instead of a boolean so that we can remove the inline comment. We can do this in a separate patch. > Source/WebKit/blackberry/Api/BackingStore.cpp:656 > + // Record the current backingstore rect. This comment is inane. Please remove it. > Source/WebKit/blackberry/Api/BackingStore.cpp:677 > + // The list of indexes we need to fill. Ditto. > Source/WebKit/blackberry/Api/BackingStore.cpp:686 > + // our new backingstore rect. Nit: backingstore => backing store > Source/WebKit/blackberry/Api/BackingStore.cpp:748 > + if (!(i < indexesToFill.size())) { Nit: Negating an expression or sub-expression tends to be more difficult to reason about than reading the same expression with the negation pushed through. I suggest pushing the negation through and hence write the if-statement expression as: "i >= indexesToFill.size()" . > Source/WebKit/blackberry/Api/BackingStore.cpp:757 > + // Origin of the new index for the new backingstore rect. Nit: backingstore => backing store > Source/WebKit/blackberry/Api/BackingStore.cpp:761 > + updateTile(originOfNew, false /*immediate*/); Nit: We should make the second argument of updateTile() be an enum so that we don't need an inline comment. We can do this in a follow-up patch. > Source/WebKit/blackberry/Api/BackingStore.cpp:763 > + // Do some bookkeeping with shifting tiles... This comment is inane. Please remove it. > Source/WebKit/blackberry/Api/BackingStore.cpp:777 > + // Actually set state. This comment is inane. Please remove it. > Source/WebKit/blackberry/Api/BackingStore.cpp:783 > + // Swap the front/back state. Ditto. > Source/WebKit/blackberry/Api/BackingStore.cpp:868 > + // Find the origin of this tile. This comment is inane. Please remove it. > Source/WebKit/blackberry/Api/BackingStore.cpp:874 > + // Check if it is current. Ditto. > Source/WebKit/blackberry/Api/BackingStore.cpp:915 > + // Set the state. Ditto. > Source/WebKit/blackberry/Api/BackingStore.cpp:940 > + // Request a layout now which might change the contents rect. Ditto. > Source/WebKit/blackberry/Api/BackingStore.cpp:957 > + renderContents(0 /*not needed since we're direct rendering to window*/, > + origin, dirtyRect); I would write this on one line. Also, I would move the inline comment so that it's above this line and update it so that it reads: "We don't need a buffer since we're direct rendering to window." > Source/WebKit/blackberry/Api/BackingStore.cpp:980 > + // Request a layout now which might change the contents rect. This comment is inane. Please remove it. > Source/WebKit/blackberry/Api/BackingStore.cpp:1206 > + clearWindow(overScrollRect, > + color.red(), color.green(), color.blue(), color.alpha()); Nit: I would write this on one line.

Created attachment 118047 [details] Updated patch Updated the patch according to Dan's comments.

Comment on attachment 118047 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=118047&action=review > Source/WebKit/blackberry/Api/BackingStore.cpp:2437 > +#if OS(QNX) Remove this if-define as it's unnecessary. > Source/WebKit/blackberry/Api/BackingStore.cpp:2438 > + // Store temps. Remove this comment as it's inane. > Source/WebKit/blackberry/Api/BackingStore.h:23 > + Remove this empty line as it doesn't improve the readability of this file. > Source/WebKit/blackberry/Api/BackingStore.h:25 > +#include <BlackBerryPlatformGraphics.h> > +#include <BlackBerryPlatformPrimitives.h> As far as I can tell we neither of these includes are necessary. Instead, we should just forward declare BlackBerry::Platform::IntRect. > Source/WebKit/blackberry/Api/BackingStore_p.h:27 > + Remove this empty line as it doesn't improve the readability of this file. > Source/WebKit/blackberry/Api/BackingStore_p.h:354 > +} Please add an inline comment on this line: // namespace WebKit > Source/WebKit/blackberry/Api/BackingStore_p.h:355 > +} Please add an inline comment on this line: // namespace BlackBerry

Created attachment 118601 [details] Updated patch Updated the patch according to Dan's comments.

Comment on attachment 118601 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=118601&action=review > Source/WebKit/blackberry/Api/BackingStore.cpp:1957 > +void BackingStorePrivate::actualVisibleSizeChanged(const Platform::IntSize& size) We should ultimately remove this method since it has an empty implementation. We can do this in a follow up bug. > Source/WebKit/blackberry/Api/BackingStore.cpp:2054 > + static int tw = 0; I would write lines 2054-2056 as: static int tileWidth = BlackBerry::Platform::Graphics::Screen::landscapeWidth(); > Source/WebKit/blackberry/Api/BackingStore.cpp:2062 > + static int th = 0; I would use a similar approach here.

Created attachment 118667 [details] Updated patch Updated the tileWidth() and tileHeight().

Comment on attachment 118667 [details] Updated patch Thanks Jacky for the updated patch. We should look to further clean up this file and break it into smaller chunks in a follow up patch.

(In reply to comment #12) > (From update of attachment 118667 [details]) > Thanks Jacky for the updated patch. We should look to further clean up this file and break it into smaller chunks in a follow up patch. OK, thanks for your review Dan!

Comment on attachment 118667 [details] Updated patch Clearing flags on attachment: 118667 Committed r102514: <http://trac.webkit.org/changeset/102514>

All reviewed patches have been landed. Closing bug.