Created attachment 129160 [details] Patch

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.

Created attachment 129195 [details] Patch

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.

Created attachment 130023 [details] Patch for cq

Comment on attachment 130023 [details] Patch for cq Clearing flags on attachment: 130023 Committed r109677: <http://trac.webkit.org/changeset/109677>

All reviewed patches have been landed. Closing bug.