Bug 79729 – [BlackBerry] upstream MediaPlayerPrivateBlackBerry.[cpp|h]

• Component: Platform
• Status: RESOLVED FIXED

[Jonathan Dong]

file: Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.[cpp|h]

[Jonathan Dong]

Created attachment 129160 [details]
Patch

[Rob Buis]

Comment on attachment 129160 [details]
Patch

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:292
> +    if (bufferLoaded)

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

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

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

> Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:379
> +            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:380
> +            int newHeight = (rect.height() > PLAYBOOK_MIN_AUDIO_ELEMENT_HEIGHT) ? rect.height() : PLAYBOOK_MIN_AUDIO_ELEMENT_HEIGHT;

Ditto.

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

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

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

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

> Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:617
> +    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.

[Jonathan Dong]

Created attachment 129195 [details]
Patch

[Antonio Gomes]

Comment on attachment 129195 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129195&action=review

r=me, but please fix it up before landing.

When you have the revised patch, please re-upload and ask Leo or Charles to cq+ it. No need to r? again :-)

> Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:53
> +/////////////////////////////////////////////////////////////////////////////

Lets remove this "///*///" line.

> Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:60
> +// static

remove

> Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:66
> +// static

ditto

> Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:75
> +// static

ditto

> Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:92
> +// static

ditto

> Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:98
> +// static

ditto

> Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:104
> +/////////////////////////////////////////////////////////////////////////////

ditto

> Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:172
> +}
> +
> +
> +void MediaPlayerPrivate::prepareToPlay()

extra line

> Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:178
> +}
> +
> +
> +void MediaPlayerPrivate::play()

ditto

> Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:389
> +    // If we have an HTMLVideoElement but the source has no video, then we need to resize the media element.
> +    if (client && client->isVideo() && !hasVideo()) {
> +        if (RenderObject* ro = client->renderer()) {
> +            IntRect rect = ro->enclosingBox()->contentBoxRect();
> +
> +            static const int playbookMinAudioElementWidth = 300;
> +            static const int playbookMinAudioElementHeight = 32;
> +            // If the rect dimensions are less than the allowed minimum, use the minimum instead.
> +            int newWidth = max(rect.width(), playbookMinAudioElementWidth);
> +            int newHeight = max(rect.height(), playbookMinAudioElementHeight);
> +
> +            char attrString[12];
> +
> +            sprintf(attrString, "%d", newWidth);
> +            client->setAttribute(HTMLNames::widthAttr, attrString);
> +
> +            sprintf(attrString, "%d", newHeight);
> +            client->setAttribute(HTMLNames::heightAttr, attrString);
> +        }
> +    }
> +
> +    // If we don't know what the width and height of the video source is, then we need to set it to something sane.
> +    if (client && client->isVideo() && !(m_platformPlayer->sourceWidth() && m_platformPlayer->sourceHeight())) {
> +        if (RenderObject* ro = client->renderer()) {
> +            IntRect rect = ro->enclosingBox()->contentBoxRect();
> +            m_platformPlayer->setSourceDimension(rect.width(), rect.height());
> +        }

maybe it would look cleaner this:

if (!client || !client->isVideo())
    return;

RenderObject* o = client->renderer();
if (!o)
    return;

....

> Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:419
> +int MediaPlayerPrivate::windowPositionGet(unsigned& x, unsigned& y, unsigned& width, unsigned& height) const

"Get" as the suffix is not common in WebKit. In fact, "get" is only used in cases exactly like this, where you "getting" values passed as references. However, I think it should be "getWindowPosition".

> Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:424
> +const char* MediaPlayerPrivate::mmrContextNameGet()

rename it to mmrContextName()? i.e. no "get"..

> Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:532
> +//////////////////////////////////////////////////////////////////

Drop the "//////*////" line.

> Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:687
> +}
> +
> +
> +#if USE(ACCELERATED_COMPOSITING)
> +static const double BufferingAnimationDelay = 1.0 / 24;

drop extra blank line.

[Jonathan Dong]

Created attachment 130023 [details]
Patch for cq

[WebKit Review Bot]

Comment on attachment 130023 [details]
Patch for cq

Clearing flags on attachment: 130023

Committed r109677: <http://trac.webkit.org/changeset/109677>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.