[webkit-reviews] review denied: [Bug 90501] Implement HTML5 6.5.2 "The External interface" : [Attachment 170740] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied otcheung at rim.com's request for
review:
Bug 90501: Implement HTML5 6.5.2 "The External interface"
https://bugs.webkit.org/show_bug.cgi?id=90501

Attachment 170740: Patch
https://bugs.webkit.org/attachment.cgi?id=170740&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=170740&action=review


Below is a technical review of your patch.  I'm not sure whether WebKit should
implement this feature or not.	My understanding is that we've historically
believed that embedders of WebKit should implement this feature themselves
rather than implementing it in the engine directly.

> Source/WebCore/Modules/external/External.cpp:33
> +: DOMWindowProperty(frame)

Bad indent

> Source/WebCore/Modules/external/External.h:27
> +#include "PassRefPtr.h"
> +#include "RefCounted.h"

Usually we include things from wtf in the following style:

#include <wtf/RefCounted.h>

> Source/WebCore/Modules/external/External.h:31
> +namespace WTF {
> +class String;
> +}

I would just #include <wtf/text/WTFString.h>.  There isn't much to be gained
from forward declaring this class since essentially every compilation unit
needs to include it anyway.

> Source/WebCore/Modules/external/External.h:36
> +class DOMWindow;
> +class Frame;

By contrast, these are well worth forward declaring.  :)

> Source/WebCore/Modules/external/External.idl:26
> +	   void AddSearchProvider(in DOMString engineURL) raises(DOMException);


Yuck.  These functions are in the wrong style.	:(

> Source/WebCore/page/Chrome.cpp:569
> +#if ENABLE(WINDOW_EXTERNAL)
> +void Chrome::addSearchProvider(const String& url)
> +{
> +    m_client->addSearchProvider(url);
> +}
> +
> +unsigned long Chrome::isSearchProviderInstalled(const String& url)
> +{
> +    return m_client->isSearchProviderInstalled(url);
> +}
> +#endif

There is no reason to have these functions.  Callers cal just talk to
chrome()->client() directly.

> Source/WebCore/page/DOMWindow.idl:225
> +#if defined(ENABLE_WINDOW_EXTERNAL) && ENABLE_WINDOW_EXTERNAL
> +    attribute [Replaceable] External external;
> +#endif

You've half made your feature into a module (a la
http://trac.webkit.org/wiki/Modules ) and half not.  You should pick one
approach or the other.	In the case of this feature, I wouldn't bother making
it a module since it is only one file and it is defined in the core HTML5
specification.

Rather than using #if, you can use the Conditional IDL attribute.