[webkit-reviews] review denied: [Bug 26952] WebCore Support for the Haiku WebKit port : [Attachment 32860] Patch to add FrameLoaderClient for Haiku WebCore support.

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Maxime Simon
<simon.maxime at gmail.com>'s request for review:
Bug 26952: WebCore Support for the Haiku WebKit port
https://bugs.webkit.org/show_bug.cgi?id=26952

Attachment 32860: Patch to add FrameLoaderClient for Haiku WebCore support.
https://bugs.webkit.org/attachment.cgi?id=32860&action=review

------- Additional Comments from David Levin <levin at chromium.org>
Just a few issues to address.


> diff --git a/WebKit/haiku/WebCoreSupport/FrameLoaderClientHaiku.cpp
b/WebKit/haiku/WebCoreSupport/FrameLoaderClientHaiku.cpp
> +/*
> + * Copyright (C) 2006 Don Gibson <dgibson77 at gmail.com>
> + * Copyright (C) 2006 Zack Rusin <zack at kde.org>
> + * Copyright (C) 2006 Apple Computer, Inc.  All rights reserved.
> + * Copyright (C) 2007 Trolltech ASA
> + * Copyright (C) 2007 Ryan Leavengood <leavengood at gmail.com>
> + *
> + * All rights reserved.

This is an odd location for "All rights reserved."  It typically follows the
copyright lines.

> +#include "config.h"
> +#include "FrameLoaderClientHaiku.h"
> +
> +#include "DocumentLoader.h"
> +#include "FrameLoader.h"
> +#include "FrameTree.h"
> +#include "Frame.h"
> +#include "FrameView.h"
> +#include "HTMLFrameOwnerElement.h"
> +#include "Page.h"
> +#include "PlatformString.h"
> +#include "ResourceRequest.h"
> +
> +#include "NotImplemented.h"
This appears out of sort order.

> +void FrameLoaderClientHaiku::setWebView(WebView* webview)
webView

> +bool FrameLoaderClientHaiku::hasWebView() const
> +{
> +    return m_webView != NULL;
Avoid NULL (use 0), but also avoid comparisons to 0/NULL.

    return m_webView;

> +void FrameLoaderClientHaiku::dispatchWillPerformClientRedirect(const KURL&,
> +								  double
interval,
> +								  double
fireDate)
Feel free to unwrap these parameters into one line.


> +void FrameLoaderClientHaiku::didChangeTitle(DocumentLoader *docLoader)

"*" in wrong location. (DocumentLoader* docLoader)



> +    if (m_firstData) {
> +	   FrameLoader *frameLoader = loader->frameLoader();

"*" in wrong location.

> +{
> +	// FIXME:

Indentation is off.


> diff --git a/WebKit/haiku/WebCoreSupport/FrameLoaderClientHaiku.h
b/WebKit/haiku/WebCoreSupport/FrameLoaderClientHaiku.h
> @@ -0,0 +1,248 @@
> +/*
> + * Copyright (C) 2006 Zack Rusin <zack at kde.org>
> + * Copyright (C) 2006 Apple Computer, Inc.  All rights reserved.
> + * Copyright (C) 2007 Ryan Leavengood <leavengood at gmail.com>
> + *
> + * All rights reserved.

Same comment as before.

> +#ifndef FrameLoaderClientHaiku_H

Use lowercase h like this:
FrameLoaderClientHaiku_h

> +#include "FrameLoaderClient.h"
> +#include "FrameLoader.h"

Out of sort order.

> +    class NavigationAction;
> +    class String;
> +    class ResourceLoader;

Out of sort order.

> +	   void setFrame(Frame *frame);

Remove parameter names if they don't add any information.
This should be 
  "void setFrame(Frame*);"

> +	   void setWebView(WebView *webview);

Remove parameter names if they don't add any information.


> +	   virtual void dispatchDecidePolicyForMIMEType(FramePolicyFunction
function,

Remove parameter names if they don't add any information.

> +	   virtual void
dispatchDecidePolicyForNewWindowAction(FramePolicyFunction function,

Remove parameter names if they don't add any information.

> +	   virtual void
dispatchDecidePolicyForNavigationAction(FramePolicyFunction function,

Remove parameter names if they don't add any information.


> +	   // FIXME: This should probably not be here, but it's needed for the
tests currently
Add "."


> +	   virtual PassRefPtr<Frame> createFrame(const KURL& url, const String&
name,
> +						 HTMLFrameOwnerElement*
ownerElement,

Remove parameter names if they don't add any information: ownerElement.


> +	   virtual PassRefPtr<Widget> createPlugin(const IntSize&,
HTMLPlugInElement*, const KURL&,
> +					const Vector<String>&, const
Vector<String>&, const String&,

indentation is off.

> +					bool loadManually) ;

> +
> +	   virtual PassRefPtr<Widget> createJavaAppletWidget(const IntSize&,
HTMLAppletElement*,
> +						  const KURL& baseURL, const
Vector<String>& paramNames,

indentation is off.

> +						  const Vector<String>&
paramValues);

> +	   Frame *m_frame;

"*" in wrong location.

> +	   WebView *m_webView;

"*" in wrong location.