[webkit-reviews] review denied: [Bug 81081] [Chromium] Add PagePopup implementation : [Attachment 132234] Patch 2

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Kent Tamura <tkent at chromium.org>'s
request for review:
Bug 81081: [Chromium] Add PagePopup implementation
https://bugs.webkit.org/show_bug.cgi?id=81081

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

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


This patch doesn't seem well factored.	We're trying to do a bunch of work in
the WebKit layer that doesn't really seem to belong here (e.g., V8 bindings). 
We're also duplicating a bunch of code from elsewhere in the WebKit layer.  I
need to study how the select popup works.  It seems like it's trying to do a
similar thing, but I don't remember it having all these messy parts.

My overall sense is that this approach feels "glued on" rather than naturally
fitting into the overall design of WebKit.

> Source/WebKit/chromium/src/WebPagePopupImpl.cpp:172
> +    static EditorClient* emptyEditorClient = new EmptyEditorClient;
> +    pageClients.editorClient = emptyEditorClient;
> +#if ENABLE(CONTEXT_MENUS)
> +    static ContextMenuClient* emptyContextMenuClient = new
EmptyContextMenuClient;
> +    pageClients.contextMenuClient = emptyContextMenuClient;
> +#endif
> +#if ENABLE(DRAG_SUPPORT)
> +    static DragClient* emptyDragClient = new EmptyDragClient;
> +    pageClients.dragClient = emptyDragClient;
> +#endif
> +#if ENABLE(INSPECTOR)
> +    static InspectorClient* emptyInspectorClient = new EmptyInspectorClient;

> +    pageClients.inspectorClient = emptyInspectorClient;
> +#endif

This all feel reminiscent of the code for SVGImage, which is a pain to
maintain.  Maybe WebCore should have a notion of a FakePage that we can use
both for SVGImage and here?

> Source/WebKit/chromium/src/WebPagePopupImpl.cpp:199
> +    {
> +	   v8::HandleScope handleScope;
> +	   v8::Handle<v8::Context> context =
V8Proxy::mainWorldContext(frame.get());
> +	   if (context.IsEmpty())
> +	       return false;
> +	   v8::Context::Scope scope(context);
> +	   m_v8this =
v8::Persistent<v8::External>::New(v8::External::New(this));
> +	   v8::Local<v8::FunctionTemplate> templ =
v8::FunctionTemplate::New(setValueAndClosePopupCallback, m_v8this);
> +	   context->Global()->Set(v8::String::New("setValueAndClosePopup"),
v8::Handle<v8::Function>(templ->GetFunction()));
> +    }

It feels wrong to have bindings code in the WebKit layer.  Wouldn't it be
better to have the bindings code in WebCore/bindings and enable it via some
sort of setting?

> Source/WebKit/chromium/src/WebPagePopupImpl.cpp:238
> +void WebPagePopupImpl::paint(WebCanvas* canvas, const WebRect& rect)

Is this code copy/pasted from elsewhere as well?

> Source/WebKit/chromium/src/WebPagePopupImpl.cpp:256
> +bool WebPagePopupImpl::handleInputEvent(const WebInputEvent& event)

This feels like an "on the cheap" re-implementation of
WebViewImpl::handleInputEvent.	It seems like there should be some way of
refactoring the code so we don't need to repeat ourselves.