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

[bugzilla-daemon at webkit.org]

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
https://bugs.webkit.org/show_bug.cgi?id=73668

Attachment 117912: Updated patch
https://bugs.webkit.org/attachment.cgi?id=117912&action=review

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

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?r
ev=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.