[Webkit-unassigned] [Bug 16815] Crash with navigator.plugins and navigator.mimeTypes after plugins.refresh

[bugzilla-daemon at webkit.org]

http://bugs.webkit.org/show_bug.cgi?id=16815


darin at apple.com changed:

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




------- Comment #10 from darin at apple.com  2008-01-14 23:32 PDT -------
(From update of attachment 18436)
Basic approach looks great.

Here are a few comments.

For new names, please use "PlugIn" rather than "Plugin". Because it's plug-in,
not plugin. I know we're inconsistent about this, but it would be good to be
consistent in the new code. Unless the real DOM goes the other way, in which
case maybe we should rename it in the other direction.

I'm not a big fan of the name PlugInData, because of the vagueness of the word
"data". Maybe PlugInSpecifications? What type of data is it? I'm especially
annoyed that we have both PlugInData and PlugInInfo, and they are two different
things!

Formatting has a lot of strange things in it.

+        inline Navigator* clientInformation() const
+        { return navigator(); }

Functions in class declarations are automatically inline, so no need to say
"inline" here. We normally put stuff like this all on one line, or format it as
a multiple line function declaration. This style, where the body is all on one
line, but a line after the declaration, is not the usual.

+ *  This file is part of the KDE libraries

Please don't include that in new source files.

When moving a large block of code to a new file, please use a "svn copy" so we
don't lose all the history of the code. Also it makes it easier to review
because I can see the changes.

It seems a little strange to have almost all the functions of Navigator be
const members, and almost all the data members be mutable. How about just
skipping "const" for this object, since it's a DOM object and the DOM has no
real notion of const.

+        ~Navigator();
+        ~MimeType();

We normally repeat the "virtual" when overriding things like the virtual
destructor.

+        /*readonly attribute MimeTypeArray mimeTypes; */

We normally don't check in commented-out code, even in IDL files, if it can be
avoided.

+    const Vector<MimeClassInfo*> &mimes = m_pluginData->mimes();

Misplaced & character here. Should go next to the type.

+Plugin *MimeType::enabledPlugin() const
+        Plugin *enabledPlugin() const;

Misplaced * character here.

+        MimeType(const RefPtr<PluginData>& pluginData, int index);

If you're passing ownership, the parameter type should be
PassRefPtr<PlugInData>, not const RefPtr&. And the argument name should be
omitted in cases like this one.

+#include "RefPtr.h"

It's <wtf/RefPtr.h>.

+#include "Shared.h"

It hasn't been "Shared.h" for a while. It's <wtf/RefCounted.h>.

Headers with functions that return a String don't need to include
PlatformString.h. They can just forward-declare String.

+    class PluginData : public Shared<PluginData>
+    {

Brace goes on the same line as the class name.

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

Mixing int with size_t is going to create a warning. Better to either put the
size into an int local variable or use size_t consistently.

For the various places where we are looking for MIME types, etc. the code
should probably use more modern data structures like hashes rather than
iterating over lists doing == on each element. Maybe the MIME types should be
AtomicString to save memory if the same type appears over and over again?

It seems really poor for each frame to have its own entire copy of the plug-in
database. Is there any way to allow sharing for the common cases where all
frames are identical?

When you refresh, I don't think it is correct to leave all other frames with
old data about plug-ins. It's good that you're fixing this so frames can be
different from each other, but moving from too much sharing to no sharing at
all doesn't seem good.

+    // ### hack

First, we use "FIXME" for such things. Second, we try to be more specific about
what's up. Why is this code in the DOM? Why not elsewhere?


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