Bug 81081

Created attachment 131800 [details] Patch

Comment on attachment 131800 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131800&action=review > Source/WebKit/chromium/src/WebPagePopupImpl.cpp:2 > - * Copyright (C) 2009 Google Inc. All rights reserved. > + * Copyright (C) 2012 Google Inc. All rights reserved. '2009' was incorrect. This file was committed in 2012.

Add reviewers suggested by webkit-patch.

Comment on attachment 131800 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131800&action=review > Source/WebKit/chromium/src/ChromeClientImpl.cpp:994 > + if (!client) > + return 0; Should it be an error to call this function with a null client? > Source/WebKit/chromium/src/ChromeClientImpl.cpp:1001 > + ASSERT(popupWidget); > + if (!popupWidget) > + return 0; Should we just crash in this case rather than returning zero? If this case can happen, we should remove the asert. > Source/WebKit/chromium/src/WebPagePopupImpl.cpp:57 > +class PagePopupChromeClient : public EmptyChromeClient { Hum... So, the PagePopup has fake clients like an SVGImage? How does this compare to the way <select> drop-downs are implemented? > Source/WebKit/chromium/src/WebPagePopupImpl.cpp:63 > + { Please add the explicit keywork to one-argument constructors. > Source/WebKit/chromium/src/WebPagePopupImpl.cpp:156 > +bool WebPagePopupImpl::initPage() This is all reminding me of SVGImage, which is a less-than-ideal approach. Rather than re-embedding WebCore at the WebKit layer, would it make more sense for the embedder to create a real WebView to hold the PagePopup? Consider, for example, the case of showing <video> in the page popup. That's something that doesn't work properly in an SVGImage and probably wouldn't work properly here either. If the embedder provided a real WebView, then everything would be wired up normally... I'm sure there's a trade-off, but it might be worth considering. > Source/WebKit/chromium/src/WebPagePopupImpl.cpp:258 > +bool WebPagePopupImpl::handleInputEvent(const WebInputEvent& event) This function looks like another maintenance burden. At least SVGImage is non-interactive...

Comment on attachment 131800 [details] Patch I'm marking this R- for now. I'm not sure this is the best approach. Maybe there are downsides to other approaches that I haven't considered...

Have you looked at how the <select> popup works? It seems you're trying to solve a very similar problem.

What's this for?

> What's this for? tkent can probably give you more details, but my understanding is that this for things like <input type="calendar"> to have a nice calendar picker popup.

Comment on attachment 131800 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131800&action=review >> Source/WebKit/chromium/src/WebPagePopupImpl.cpp:57 >> +class PagePopupChromeClient : public EmptyChromeClient { > > Hum... So, the PagePopup has fake clients like an SVGImage? How does this compare to the way <select> drop-downs are implemented? <select> popup doesn't need ChromeClient because it doesn't have a Page. >> Source/WebKit/chromium/src/WebPagePopupImpl.cpp:156 >> +bool WebPagePopupImpl::initPage() > > This is all reminding me of SVGImage, which is a less-than-ideal approach. Rather than re-embedding WebCore at the WebKit layer, would it make more sense for the embedder to create a real WebView to hold the PagePopup? > > Consider, for example, the case of showing <video> in the page popup. That's something that doesn't work properly in an SVGImage and probably wouldn't work properly here either. If the embedder provided a real WebView, then everything would be wired up normally... I'm sure there's a trade-off, but it might be worth considering. Yes, using a real WebView is worth considering. I decided not to use WebView because - I don't want to change focus on the main WebView when a popup is opened, but I want to support keyboard operation in a PagePopup. This is very similar to <select> popup. So I think WebWidget is more suitable than WebView. - PagePopup doesn't need many things which WebView provides. It doesn't need to support <video>, x-webkit-speech, geolocation, etc. I was afraid code duplication with WebViewImpl, but the code of WebpagePopupImpl is not so large. Surprisingly, WebPagePopupImpl.cpp is smaller than WebPopupMenuImpl.cpp.

Created attachment 132234 [details] Patch 2 0-check and explicit

Comment on attachment 131800 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131800&action=review >> Source/WebKit/chromium/src/ChromeClientImpl.cpp:994 >> + return 0; > > Should it be an error to call this function with a null client? I changed this ASSERT(client) because passing 0 is a wrong usage. >> Source/WebKit/chromium/src/ChromeClientImpl.cpp:1001 >> + return 0; > > Should we just crash in this case rather than returning zero? If this case can happen, we should remove the asert. I removed 0-check and return. AFAIK, createPopupMenu() doesn't return 0. >> Source/WebKit/chromium/src/WebPagePopupImpl.cpp:63 >> + { > > Please add the explicit keywork to one-argument constructors. Done.

Any other comments?

Comment on attachment 132234 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=132234&action=review > Source/WebKit/chromium/src/WebPagePopupImpl.cpp:210 > + // setValeuAndClosePopup() deletes the popup object. Do not access it. Typo: setValeuAndClosePopup > Source/WebKit/chromium/src/WebPagePopupImpl.cpp:337 > + m_v8this.Dispose(); Please Clear() the handle after Disposing it. > Source/WebKit/chromium/src/WebPagePopupImpl.cpp:365 > + return adoptRef(new WebPagePopupImpl(client)).leakRef(); Can we hold this reference somewhere rather than calling leakRef() and deref() manually? This ownership model is pretty fragile.

Comment on attachment 132234 [details] Patch 2 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.

Comment on attachment 132234 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=132234&action=review I'll work on code sharing with WebVIewImpl. >> Source/WebKit/chromium/src/WebPagePopupImpl.cpp:199 >> + } > > 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? Supplimental=DOMWindow and V8EnabledAtRuntime might help. >> Source/WebKit/chromium/src/WebPagePopupImpl.cpp:365 >> + return adoptRef(new WebPagePopupImpl(client)).leakRef(); > > Can we hold this reference somewhere rather than calling leakRef() and deref() manually? This ownership model is pretty fragile. WebViewImpl might be able to own this. I'll try it.

(In reply to comment #15) > > 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? > > Supplimental=DOMWindow and V8EnabledAtRuntime might help. I found V8EnabledAtRuntime didn't work in this case. It's per-process setting, and I want per-page or per-window setting.

(In reply to comment #15) > I'll work on code sharing with WebVIewImpl. Is it acceptable to use virtual inheritance? I'd like to do: class WebView : virtual public WebWidget { ... class PageView : virtual public WebWidget { ... // Common implementation of WebViewImpl and WebPagePopupImpl class WebViewImpl : public WebView, public PageView, ... { ... class WebPagePopupImpl: public PageView, ... { ...

(In reply to comment #17) > (In reply to comment #15) > > I'll work on code sharing with WebVIewImpl. > > Is it acceptable to use virtual inheritance? I'd like to do: > > class WebView : virtual public WebWidget { ... > > class PageView : virtual public WebWidget { ... // Common implementation of WebViewImpl and WebPagePopupImpl > > class WebViewImpl : public WebView, public PageView, ... { ... > class WebPagePopupImpl: public PageView, ... { ... I found we couldn't use virtual inheritance. We sometimes use WebWidget -> WebView cast. content/renderer/render_view_impl.cc:668:10: error: cannot cast 'WebKit::WebWidget *' to 'WebKit::WebView *' via virtual base 'WebKit::WebWidget' return static_cast<WebKit::WebView*>(webwidget()); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Created attachment 136409 [details] WIP Sharing some code in WebViewImpl.cpp and WebPagePopupImpl.cpp

Created attachment 136626 [details] WIP 2 It depends on Bug 83561

Comment on attachment 136626 [details] WIP 2 Attachment 136626 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12383687

Created attachment 136813 [details] Patch 3

Comment on attachment 136813 [details] Patch 3 Attachment 136813 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12387439

(In reply to comment #15) > >> Source/WebKit/chromium/src/WebPagePopupImpl.cpp:365 > >> + return adoptRef(new WebPagePopupImpl(client)).leakRef(); > > > > Can we hold this reference somewhere rather than calling leakRef() and deref() manually? This ownership model is pretty fragile. > > WebViewImpl might be able to own this. I'll try it. I tried to make WebViewImpl::m_pagePopup OwnPtr<>. But I decided I didn't do it. We need to close the popup before deleting the WebPagePopupImpl object. However WebWidgetClient::closeWidgetSoon() closes the widget asynchronously. We can know the deletable timing by WebPagePopupImpl::close() callback. I think deleting WebPagePopupImpl itself in close() is much simpler than notifying the timing to WebViewImpl.

Created attachment 136818 [details] Patch 4 fix release build

Comment on attachment 136818 [details] Patch 4 Attachment 136818 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12387458 New failing tests: compositing/checkerboard.html compositing/absolute-position-changed-in-composited-layer.html compositing/geometry/fixed-in-composited.html compositing/layer-creation/fixed-position-scroll.html compositing/iframes/nested-composited-iframe.html compositing/iframes/overlapped-nested-iframes.html compositing/geometry/limit-layer-bounds-fixed-positioned.html compositing/geometry/vertical-scroll-composited.html compositing/geometry/horizontal-scroll-composited.html accessibility/aria-disabled.html compositing/overflow/fixed-position-ancestor-clip.html http/tests/cache/history-only-cached-subresource-loads.html accessibility/aria-roles.html http/tests/appcache/crash-when-navigating-away-then-back.html accessibility/img-alt-tag-only-whitespace.html http/tests/cache/history-only-cached-subresource-loads-max-age-https.html compositing/geometry/fixed-position.html compositing/iframes/fixed-position-iframe.html compositing/scroll-painted-composited-content.html compositing/absolute-position-changed-with-composited-parent-layer.html

Created attachment 136820 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 136822 [details] Patch 5 fix test failures

I can separate the PageWidgetDelegate part to another patch if this way to share code looks good.

Comment on attachment 136822 [details] Patch 5 View in context: https://bugs.webkit.org/attachment.cgi?id=136822&action=review > Source/WebKit/chromium/src/PageWidgetDelegate.cpp:51 > + if (!page->mainFrame()) > + return 0; Can a Page fail to have a mainFrame ? > Source/WebKit/chromium/src/PageWidgetDelegate.cpp:93 > + if (view && page->mainFrame()->document()) { You don't need to null-check from Frame to Document. A Frame always has a Document. > Source/WebKit/chromium/src/WebPagePopupImpl.cpp:60 > + explicit PagePopupChromeClient(WebPagePopupImpl* popup) : m_popup(popup) ": m_popup(popup)" should be on its own line. > Source/WebKit/chromium/src/WebPagePopupImpl.cpp:162 > + m_page->settings()->setScriptEnabled(true); > + m_page->settings()->setAllowScriptsToCloseWindows(true); What happens to the rest of the settings? They get whatever random defaults we happen to set in Settings.cpp? > Source/WebKit/chromium/src/WebPagePopupImpl.cpp:261 > + delete this; There isn't a better object to own this one? > Source/WebKit/chromium/src/WebPagePopupImpl.cpp:286 > + ASSERT(client); > + if (!client) > + return 0; Why do we both ASSERT client and then handle the case when client is 0. It seems like we should just crash in that case and be happy for the bug report.

tkent! This is so much better.

Comment on attachment 136822 [details] Patch 5 View in context: https://bugs.webkit.org/attachment.cgi?id=136822&action=review > Source/WebKit/chromium/src/WebPagePopupImpl.cpp:176 > + DocumentWriter* writer = frame->loader()->activeDocumentLoader()->writer(); > + writer->setMIMEType("text/html"); > + writer->setEncoding("UTF-8", false); > + writer->begin(); > + m_popupClient->writeDocument(*writer); > + writer->end(); I wonder if it would be easier to use the DocumentWriter::replaceDocument function. What you really want to do is use "srcdoc", but that's a feature of iframes, and not something that works on the main frame.

Comment on attachment 136822 [details] Patch 5 There's a lot going on in this patch. I don't feel like I'm an expert in all of it. Everything looks reasonable, and you say that you're confident in the details, so I'm inclined to R+ the patch. It's very possible that there are bugs in here that we've missed, but we can iterate if that's the case.

Comment on attachment 136822 [details] Patch 5 View in context: https://bugs.webkit.org/attachment.cgi?id=136822&action=review > Source/WebKit/chromium/src/PageWidgetDelegate.cpp:190 > + // We call mouseMoved here instead of handleMouseMovedEvent because we need > + // our ChromeClientImpl to receive changes to the mouse position and tooltip > + // text, and mouseMoved handles all of that. > + mainFrame.eventHandler()->mouseMoved(PlatformMouseEventBuilder(mainFrame.view(), event)); This makes me wonder if this isn't 100% factored perfectly, but I'm not sure what to recommend.

Comment on attachment 136822 [details] Patch 5 View in context: https://bugs.webkit.org/attachment.cgi?id=136822&action=review >> Source/WebKit/chromium/src/PageWidgetDelegate.cpp:51 >> + return 0; > > Can a Page fail to have a mainFrame ? Maybe no? This is equivalent to the original code in WebViewImpl and WebFrameImpl. I'll add a FIXME comment here. >> Source/WebKit/chromium/src/PageWidgetDelegate.cpp:93 >> + if (view && page->mainFrame()->document()) { > > You don't need to null-check from Frame to Document. A Frame always has a Document. The original code in WebFrameImpl had the document check. I'm not confident Frame::document() never be null. I'll add a FIXME comment. >> Source/WebKit/chromium/src/PageWidgetDelegate.cpp:190 >> + mainFrame.eventHandler()->mouseMoved(PlatformMouseEventBuilder(mainFrame.view(), event)); > > This makes me wonder if this isn't 100% factored perfectly, but I'm not sure what to recommend. This was copied from WebViewImpl.cpp. >> Source/WebKit/chromium/src/WebPagePopupImpl.cpp:60 >> + explicit PagePopupChromeClient(WebPagePopupImpl* popup) : m_popup(popup) > > ": m_popup(popup)" should be on its own line. will do. >> Source/WebKit/chromium/src/WebPagePopupImpl.cpp:162 >> + m_page->settings()->setAllowScriptsToCloseWindows(true); > > What happens to the rest of the settings? They get whatever random defaults we happen to set in Settings.cpp? Because this page will contain only specific documents and is not exposed to web developers, I don't care settings much. >> Source/WebKit/chromium/src/WebPagePopupImpl.cpp:261 >> + delete this; > > There isn't a better object to own this one? See https://bugs.webkit.org/show_bug.cgi?id=81081#c24 >> Source/WebKit/chromium/src/WebPagePopupImpl.cpp:286 >> + return 0; > > Why do we both ASSERT client and then handle the case when client is 0. It seems like we should just crash in that case and be happy for the bug report. will do.

(In reply to comment #29) > I can separate the PageWidgetDelegate part to another patch if this way to share code looks good. Though the patch already has r+, I'll separate the patch into two parts and commit them without additional review.

Created attachment 136853 [details] Patch for landing

Comment on attachment 136853 [details] Patch for landing Rejecting attachment 136853 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/Source/WebKit/chromium/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/12391325

Created attachment 136857 [details] Patch for landing fill out the reviewer

Comment on attachment 136857 [details] Patch for landing Clearing flags on attachment: 136857 Committed r113956: <http://trac.webkit.org/changeset/113956>

All reviewed patches have been landed. Closing bug.