[Webkit-unassigned] [Bug 84291] [BlackBerry] MediaPlayerPrivate should not reference HTMLMediaElement directly

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=84291


Eric Carlson <eric.carlson at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #159698|review?                     |review-
               Flag|                            |




--- Comment #6 from Eric Carlson <eric.carlson at apple.com>  2012-08-21 10:45:25 PST ---
(From update of attachment 159698)
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:775
> +    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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.