Bug 79729 - [BlackBerry] upstream MediaPlayerPrivateBlackBerry.[cpp|h]
Summary: [BlackBerry] upstream MediaPlayerPrivateBlackBerry.[cpp|h]
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 73144
  Show dependency treegraph
 
Reported: 2012-02-27 18:29 PST by Jonathan Dong
Modified: 2012-03-04 09:12 PST (History)
7 users (show)

See Also:


Attachments
Patch (38.37 KB, patch)
2012-02-27 18:38 PST, Jonathan Dong
no flags Details | Formatted Diff | Diff
Patch (37.90 KB, patch)
2012-02-27 23:15 PST, Jonathan Dong
no flags Details | Formatted Diff | Diff
Patch for cq (37.52 KB, patch)
2012-03-04 05:47 PST, Jonathan Dong
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Dong 2012-02-27 18:29:33 PST
file: Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.[cpp|h]
Comment 1 Jonathan Dong 2012-02-27 18:38:59 PST
Created attachment 129160 [details]
Patch
Comment 2 Rob Buis 2012-02-27 19:25:52 PST
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.
Comment 3 Jonathan Dong 2012-02-27 23:15:56 PST
Created attachment 129195 [details]
Patch
Comment 4 Antonio Gomes 2012-03-03 17:47:54 PST
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.
Comment 5 Jonathan Dong 2012-03-04 05:47:52 PST
Created attachment 130023 [details]
Patch for cq
Comment 6 WebKit Review Bot 2012-03-04 09:12:01 PST
Comment on attachment 130023 [details]
Patch for cq

Clearing flags on attachment: 130023

Committed r109677: <http://trac.webkit.org/changeset/109677>
Comment 7 WebKit Review Bot 2012-03-04 09:12:06 PST
All reviewed patches have been landed.  Closing bug.