[webkit-reviews] review denied: [Bug 27511] Add WinCE specific platform/graphics files to WebCore : [Attachment 34396] 5) MediaPlayer

[bugzilla-daemon at webkit.org]

George Staikos <staikos at kde.org> has denied Yong Li <yong.li at torchmobile.com>'s
request for review:
Bug 27511: Add WinCE specific platform/graphics files to WebCore
https://bugs.webkit.org/show_bug.cgi?id=27511

Attachment 34396: 5) MediaPlayer
https://bugs.webkit.org/attachment.cgi?id=34396&action=review

------- Additional Comments from George Staikos <staikos at kde.org>
> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 3afc825..2facda8 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,16 @@
> +2009-08-08  Yong Li	<yong.li at torchmobile.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   WINCE PORT: MediaPlayerProxy and MediaPlayerPrivate
> +	   https://bugs.webkit.org/show_bug.cgi?id=27511
> +
> +	   Written by Crystal Zhang <crystal.zhang at torchmobile.com>
> +
> +	   * platform/graphics/wince/MediaPlayerPrivateWince.h: Added.
> +	   * platform/graphics/wince/MediaPlayerProxy.cpp: Added.
> +	   * platform/graphics/wince/MediaPlayerProxy.h: Added.


  Should probably put Crystal's name in the ChangeLog entry instead.


> +WebMediaPlayerProxy::WebMediaPlayerProxy(MediaPlayer* player)
> +: m_mediaPlayer(player)
> +, m_init(false)
> +, m_hasSentResponseToPlugin(false)

   Need indenting


> +WebMediaPlayerProxy::~WebMediaPlayerProxy()
> +{
> +    m_instance.release();
> +}
> +ScriptInstance WebMediaPlayerProxy::pluginInstance()

  Needs a blank line


> +}
> +void WebMediaPlayerProxy::load(const String& url)
> +{
> +    if (!m_init)
> +	   initEngine();
> +    if (m_init)
> +	   invokeMethod("play");
> +}
> +void WebMediaPlayerProxy::initEngine()

  More blank lines missing


> +    NamedNodeMap* a = element->attributes();
> +    if (a) {
> +	   for (unsigned i = 0; i < a->length(); ++i) {
> +	       Attribute* it = a->attributeItem(i);
> +	       paramNames.append(it->name().localName().string());
> +	       paramValues.append(it->value().string());
> +	   }
> +    }

   I guess you should remove the braces around the if, as ugly as it becomes.


> +HTMLMediaElement* WebMediaPlayerProxy::element()
> +{
> +    return
static_cast<HTMLMediaElement*>(m_mediaPlayer->mediaPlayerClient());
> +
> +}

   Extra blank line here

> +
> +void WebMediaPlayerProxy::invokeMethod(const String& methodName)
> +{
> +    Frame* frame = element()->document()->frame();
> +    RootObject *root = frame->script()->bindingRootObject();
> +    if (!root)
> +	   return;
> +    ExecState *exec = root->globalObject()->globalExec();
> +    Instance* instance = pluginInstance().get();
> +    if (!instance)
> +	   return;
> +
> +    instance->begin();
> +    Class *aClass = instance->getClass();
> +    Identifier iden(exec, methodName);
> +    MethodList methodList = aClass->methodsNamed(iden, instance);
> +    ArgList args;
> +    /*JSValuePtr result =*/
> +    instance->invokeMethod(exec, methodList , args);
> +    instance->end();


   This needs a comment to indicate why the result variable is commented out I
think.


   After those minor cleanups it will be good to go.