[webkit-reviews] review denied: [Bug 16815] Crash with navigator.plugins and navigator.mimeTypes after plugins.refresh : [Attachment 19433] patch including support for Windows

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Simon Hausmann <hausmann at kde.org>'s
request for review:
Bug 16815: Crash with navigator.plugins and navigator.mimeTypes after
plugins.refresh
http://bugs.webkit.org/show_bug.cgi?id=16815

Attachment 19433: patch including support for Windows
http://bugs.webkit.org/attachment.cgi?id=19433&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Great patch! Seems almost ready to go.

+    const String &userAgent = m_frame->loader()->userAgent(m_frame->document()
? m_frame->document()->url() : KURL());

Should be const String&, with the space after the &.

+  return m_frame->settings()->isJavaEnabled();

Should be indented four spaces.

+    for (unsigned int i = 0; i < framesNeedingReload.size(); ++i)

We normally just use "unsigned", but also since this is a Vector, should be
size_t.

+	 m_pluginData = new PluginData(this);

Should be using PluginData::create instead of calling new directly. I don't
think this will even compile.

+MimeType::MimeType(PassRefPtr<PluginData> pluginData, unsigned index)
+    : RefCounted<MimeType>(0)

This is going to be a problem. The constructor here starts with a count of 0,
but the create function does an adoptRef. Need to remove the initialization of
the refcount to 0. Same problem with PluginArray and perhaps other classes.
Please double check this.

+    for (unsigned int i = 0; i < plugins.size(); ++i)
+	 if (plugins[i] == mimes[m_index]->plugin)
+	     return Plugin::create(m_pluginData.get(), i);

Same comment about unsigned int and size_t as above. Also, we use braces around
multi-line bodies, even if they are only a single statement, so the for should
have braces.

Could evaluate mimes[m_index]->plugin outside the loop.

+	 String type() const;
+	 String suffixes() const;
+	 String description() const;

Could return const String& from these for better efficiency since these are
stored in a persistent data structure.

+#include "PluginData.h"

No need to include the entire PluginData header just to compile a RefPtr to
one.

+	 unsigned long length() const;

unsigned long in IDL is the same as unsigned in C++, not unsigned long. So this
should just be unsigned, not unsigned long.

+    // FIXME: Generated JSPlugin.cpp doesn't include JSMimeType.h for toJS
+    KJS::JSValue* toJS(KJS::ExecState*, MimeType*);

What's the deal with this? Have you discussed it with Sam Weinig?

It'd be more efficient to use a constructor for MimeClassInfo rather than
constructing one with null strings and then assigning to it.

The class MimeClassInfo uses the mixed class Mime as in Qt rather than the all
capitals MIME used elsewhere in WebKit.

review- because of the thing that won't compile (new with a private
constructor) and the refcount problems (0 with adoptRef).