[Webkit-unassigned] [Bug 73668] Upstream BlackBerry API backing store files

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=73668


Daniel Bates <dbates at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #117912|review?                     |review-
               Flag|                            |




--- Comment #6 from Daniel Bates <dbates at webkit.org>  2011-12-05 14:11:25 PST ---
(From update of attachment 117912)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.