[webkit-reviews] review denied: [Bug 36328] Missing plug-ins should be represented by text only, instead of lego block : [Attachment 51167] FrameLoader refactoring patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Kevin Decker <kdecker at apple.com>'s
request for review:
Bug 36328: Missing plug-ins should be represented by text only, instead of lego
block
https://bugs.webkit.org/show_bug.cgi?id=36328

Attachment 51167: FrameLoader refactoring patch
https://bugs.webkit.org/attachment.cgi?id=51167&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Thanks. I think this is a good approach.

> -bool FrameLoader::requestObject(RenderPart* renderer, const String& url,
const AtomicString& frameName,
> +bool FrameLoader::requestObject(RenderEmbeddedObject* requestEmbeddedObject,
const String& url, const AtomicString& frameName,

The type change here seems good, but I don't understand why the argument is now
named "request embedded object". That sounds like a sentence, with "request"
being a verb. I think that renderer was actually a fine name for the argument.
Or we could use "container" or "object".

The function name "requestObject", is not good, but you didn't change that.

> +class RenderEmbeddedObject;
>  class RenderPart;

These functions were the only thing using RenderPart, so I suggest you remove
the forward declaration of RenderPart too.

> -    bool requestObject(RenderPart* frame, const String& url, const
AtomicString& frameName,
> +    bool requestObject(RenderEmbeddedObject*, const String& url, const
AtomicString& frameName,
>	   const String& serviceType, const Vector<String>& paramNames, const
Vector<String>& paramValues);

> -    bool loadPlugin(RenderPart*, const KURL&, const String& mimeType,
> +    bool loadPlugin(RenderEmbeddedObject*, const KURL&, const String&
mimeType,
>      const Vector<String>& paramNames, const Vector<String>& paramValues,
bool useFallback);

Indenting is wrong here. You should fix that.

Since the purpose of the patch is primarily renaming and I didn't like the name
change, I'll say review-, but lets keep going!