[webkit-reviews] review denied: [Bug 79729] [BlackBerry] upstream MediaPlayerPrivateBlackBerry.[cpp|h] : [Attachment 129160] Patch

[bugzilla-daemon at webkit.org]

Rob Buis <rwlbuis at gmail.com> has denied Jonathan Dong
<jonathan.dong at torchmobile.com.cn>'s request for review:
Bug 79729: [BlackBerry] upstream MediaPlayerPrivateBlackBerry.[cpp|h]
https://bugs.webkit.org/show_bug.cgi?id=79729

Attachment 129160: Patch
https://bugs.webkit.org/attachment.cgi?id=129160&action=review

------- Additional Comments from Rob Buis <rwlbuis at gmail.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=129160&action=review


Looks good, but I think it can be cleaned up some more.

>
Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:29
2
> +    if (bufferLoaded)

CAn combine to if (float bufferLoaded = m_platformPlayer->bufferLoaded())

>
Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:37
5
> +	   if (ro) {

Can be combined to if (RenderObject* ro = client->renderer())

>
Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:37
9
> +	       int newWidth = (rect.width() > PLAYBOOK_MIN_AUDIO_ELEMENT_WIDTH)
? rect.width() : PLAYBOOK_MIN_AUDIO_ELEMENT_WIDTH;

Can use max here.

>
Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:38
0
> +	       int newHeight = (rect.height() >
PLAYBOOK_MIN_AUDIO_ELEMENT_HEIGHT) ? rect.height() :
PLAYBOOK_MIN_AUDIO_ELEMENT_HEIGHT;

Ditto.

>
Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:39
5
> +	   if (ro) {

Can be combined to if (RenderObject* ro = client->renderer())

>
Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:61
0
> +    if (element)

Can be combined to if (HTMLMediaElement* element =
static_cast<HTMLMediaElement*>(m_webCorePlayer->mediaPlayerClient()))

>
Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:61
7
> +    if (element)

Ditto.

> Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.h:39

> +#define PLAYBOOK_MIN_AUDIO_ELEMENT_HEIGHT 32

Could be put in the cpp

>
Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.h:141
> +    virtual BlackBerry::Platform::Graphics::Window* platformWindow();

Should enter empty line here.

>
Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.h:149
> +private:

private can be removed, already is in private section.