[webkit-reviews] review denied: [Bug 16815] Crash with navigator.plugins and navigator.mimeTypes after plugins.refresh : [Attachment 19188] updated patch that compiles against trunk

[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 19188: updated patch that compiles against trunk
http://bugs.webkit.org/attachment.cgi?id=19188&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
This patch is great. Sorry I didn't review it earlier.

The patch will need to be revised to preserve the new Windows-only quirk in
appVersion, which will require a custom binding.

+	 MimeType(PluginData* pluginData, unsigned index);

This should be a PassRefPtr<PluginData>, not a raw pointer.

+	 Plugin* enabledPlugin() const;

Since this produces a new reference counted object, the return type should be a
PassRefPtr.

+	 m_navigator = new Navigator(m_frame);

New refcounted classes should use the "create" idiom, where there's a function
to create a new object that returns a PassRefPtr and the constructor should be
private.

+	 readonly attribute Navigator navigator;
+	 readonly attribute Navigator clientInformation;

Is it correct that these are not [Replaceable]? Does this change behavior from
the old version or preserve it?

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

There's no good reason to use "const String" here. We could declare many of our
local variables const, but we normally don't. But instead of String, you could
use const String&. I don't think it would be more efficient, but it's often a
good idiom. It's probably even better to just use "this->userAgent()" to
initialize userAgent; if you did that you could even skip the m_frame null
check.

+    class Frame;
+    class String;
+    class PluginData;
+    class PluginArray;
+    class MimeTypeArray;

We normally use alphabetical order for these things.

+    HashSet<Page*>::iterator end = allPages->end();
+    for (HashSet<Page*>::iterator it = allPages->begin(); it != end; ++it) {

Can calling reload() trigger anything that changes the contents of allPages?
Like maybe open a new window? If it can, then this code needs to be written so
it can cope with that. The behavior of iterators in a hash table that changes
is undefined -- could crash. A safe idiom is to copy the set into a vector and
iterate the vector.

+	 for (Frame* frame = (*it)->mainFrame(); frame; frame =
frame->tree()->traverseNext()) {
+	     if (reload && frame->loader()->containsPlugins())

I suggest putting the "reload" if statement outside the loop.

If reload() can change the state of frames then I think you need to use a
RefPtr<Frame> to avoid a possible null-dereference. We might also need to copy
the frames into a vector to avoid problems in case the frame tree changes.

+    : RefCounted<MimeType>(0)

Please use a count of 1 for new classes you're adding in the future. this is OK
on this patch, but should be revised for future patches.

Sorry, I have to go now and I only got down to PluginData.h -- still got about
a third of the patch to review.

review- because of the above issues.