[webkit-reviews] review denied: [Bug 84291] [BlackBerry] MediaPlayerPrivate should not reference HTMLMediaElement directly : [Attachment 159698] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has denied John Griggs
<jgriggs at rim.com>'s request for review:
Bug 84291: [BlackBerry] MediaPlayerPrivate should not reference
HTMLMediaElement directly
https://bugs.webkit.org/show_bug.cgi?id=84291

Attachment 159698: Patch
https://bugs.webkit.org/attachment.cgi?id=159698&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=159698&action=review


Thank you for working on this! r- for now because of the relatively minor
issues noted, but this is close.

> Source/WebCore/html/HTMLMediaElement.cpp:4398
> +    sprintf(attrString, "%d", width);
> +    setAttribute(widthAttr, attrString);

sprintf is unsafe so you should always use snprintf instead, but in this case
you can use String::number :

    setAttribute(widthAttr, String::number(width));

> Source/WebCore/platform/graphics/MediaPlayer.h:195
> +    virtual void mediaPlayerSetSize(int , int) { }

mediaPlayerContentBoxRect returns a LayoutRect, why not have this take a
LayoutSize?

>
Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:77
5
> +    return
m_webCorePlayer->mediaPlayerClient()->mediaPlayerOwningDocument()->view();

Accessing Document and FrameView from platform is also a layering violation. I
see that you only use it to get to the HostWindow and the ClipRect, so why
don't you add accessors for those instead?