[webkit-reviews] review denied: [Bug 73668] Upstream BlackBerry API backing store files : [Attachment 117912] Updated patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Dec 5 14:11:25 PST 2011

Daniel Bates <dbates at webkit.org> has denied Jacky Jiang
<zkjiang008 at gmail.com>'s request for review:
Bug 73668: Upstream BlackBerry API backing store files

Attachment 117912: Updated patch

------- Additional Comments from Daniel Bates <dbates at webkit.org>
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.


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
ev=76248#L29>, I would write this class to have the form:

class BackingStoreMutexLocker {

> 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

> 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.


> 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.


> 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.


> Source/WebKit/blackberry/Api/BackingStore.cpp:915
> +    // Set the state.


> Source/WebKit/blackberry/Api/BackingStore.cpp:940
> +    // Request a layout now which might change the contents rect.


> 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(),

Nit: I would write this on one line.

More information about the webkit-reviews mailing list