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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Mar 3 17:47:53 PST 2012


Antonio Gomes <tonikitoo at webkit.org> has granted 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 129195: Patch
https://bugs.webkit.org/attachment.cgi?id=129195&action=review

------- Additional Comments from Antonio Gomes <tonikitoo at webkit.org>
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:10
4
>
+/////////////////////////////////////////////////////////////////////////////

ditto

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

extra line

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

ditto

>
Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:38
9
> +    // 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:41
9
> +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:42
4
> +const char* MediaPlayerPrivate::mmrContextNameGet()

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

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

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

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

drop extra blank line.


More information about the webkit-reviews mailing list