[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