WebViewClient has a method createPopupMenu(const WebPopupMenuInfo&) that is used to create a popup menu (such as the select menu) externally. However it still involves creating a WebWidget that is kept hidden. We should instead have a clean API that does not require that WebWidget creation.
Created attachment 68310 [details] Initial patch
Attachment 68310 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebKit/chromium/src/ExternalPopupMenu.h:53: This { should be at the end of the previous line [whitespace/braces] [4] WebKit/chromium/src/ExternalPopupMenu.h:54: Tab found; better to use spaces [whitespace/tab] [1] WebKit/chromium/src/ExternalPopupMenu.h:57: Tab found; better to use spaces [whitespace/tab] [1] WebKit/chromium/src/ExternalPopupMenu.cpp:38: Alphabetical sorting problem. [build/include_order] [4] WebKit/chromium/src/ExternalPopupMenu.cpp:97: Tab found; better to use spaces [whitespace/tab] [1] Total errors found: 5 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 68313 [details] Fixed style issues
Attachment 68313 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebKit/chromium/src/ExternalPopupMenu.h:56: Tab found; better to use spaces [whitespace/tab] [1] Total errors found: 1 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 68316 [details] Fixing style take 2
Comment on attachment 68316 [details] Fixing style take 2 View in context: https://bugs.webkit.org/attachment.cgi?id=68316&action=review > WebKit/chromium/public/WebExternalPopupMenu.h:38 > + virtual void showExternalPopupMenu() = 0; nit: these method names can use shorter names like "show" and "close" they are already defined within the context of WebExternalPopupMenu, so there is no need to re-type that context. > WebKit/chromium/public/WebExternalPopupMenuClient.h:40 > + virtual void selectionChanged(int index) = 0; style-nit: selectionChanged -> didChangeSelection > WebKit/chromium/public/WebExternalPopupMenuClient.h:45 > + virtual void acceptedIndex(int index) = 0; style-nit: acceptedIndex -> didAcceptIndex > WebKit/chromium/public/WebExternalPopupMenuClient.h:51 > + virtual void canceled() = 0; style-nit: canceled -> didCancel > WebKit/chromium/src/ChromeClientImpl.cpp:853 > +#if defined(EXTERNAL_POPUP) I don't see where this is defined. Is this something you want to alter in the build system to engage this new API? An alternative way to accomplish this for migration purposes is to call createExternalPopupMenu here, and if that returns non-null, then pass the WebExternalPopupMenu instance to the ExternalPopupMenu constructor, etc. Otherwise, create a PopupMenuChromium as the old code was doing.
Created attachment 70156 [details] Applied fishd suggested changes
Comment on attachment 70156 [details] Applied fishd suggested changes View in context: https://bugs.webkit.org/attachment.cgi?id=70156&action=review > WebKit/chromium/public/WebExternalPopupMenuClient.h:49 > + // Note that it is not safe to access this WebExternalPopupClientMenu after nit: WebExternalPopupClientMenu -> WebExternalPopupMenuClient > WebKit/chromium/public/WebViewClient.h:263 > + virtual bool shouldUseExternalPopups() { return false; } is there a real good reason to make this a WebViewClient method? isn't this effectively a static / compile-time option?
Created attachment 70538 [details] Adding an static method to WebView to set whether to use external popups or not. @fishd: per our discussion, added a static method to WebView to set whether to use external popup menus, so that Chromium directly controls it.
Comment on attachment 70538 [details] Adding an static method to WebView to set whether to use external popups or not. View in context: https://bugs.webkit.org/attachment.cgi?id=70538&action=review > WebKit/chromium/public/WebView.h:311 > + // Popup menu ---------------------------------------------------------- nit: please preserve the two lines of whitespace above a section divider comment. > WebKit/chromium/src/ExternalPopupMenu.cpp:41 > + nit: I recommend adding a 'using namespace WebCore' here. then kill off the WebCore:: prefixes down below. otherwise, R=me
Attachment 70538 [details] was posted by a committer and has review+, assigning to Jay Civelli for commit.
Created attachment 70876 [details] Added the bounds where the popup should be shown to WebExternalPopup::show() Sorry for the extra small new change on this CL. In order to know where to show the popup, the WebExternalPopup needs to be passed bounds. I added them as a parameter to the WebExternalPopup::show().
Comment on attachment 70538 [details] Adding an static method to WebView to set whether to use external popups or not. Cleared Darin Fisher's review+ from obsolete attachment 70538 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment on attachment 70876 [details] Added the bounds where the popup should be shown to WebExternalPopup::show() View in context: https://bugs.webkit.org/attachment.cgi?id=70876&action=review > WebKit/chromium/src/ExternalPopupMenu.cpp:70 > + IntPoint location = v->contentsToWindow(rect.location()); nit: there is also a version of contentsToWindow that takes an IntRect, and a WebRect can be implicitly created from an IntRect. in other words, you can rewrite this as: m_webExternalPopupMenu->show(v->contentsToWindow(rect));
Created attachment 71304 [details] Patch for landing
Comment on attachment 71304 [details] Patch for landing Rejecting patch 71304 from commit-queue. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'apply-attachment', '--force-clean', '--non-interactive', '--quiet', 71304]" exit_code: 2 Cleaning working directory Updating working directory Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2 Full output: http://queues.webkit.org/results/4545010
Comment on attachment 71304 [details] Patch for landing I think this was a false rejection from a run-away commit-node.
Comment on attachment 71304 [details] Patch for landing Rejecting patch 71304 from commit-queue. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'apply-attachment', '--force-clean', '--non-interactive', '--quiet', 71304]" exit_code: 2 Cleaning working directory Updating working directory Logging in as commit-queue@webkit.org... Fetching: https://bugs.webkit.org/attachment.cgi?id=71304&action=edit Fetching: https://bugs.webkit.org/show_bug.cgi?id=46016&ctype=xml Processing 1 patch from 1 bug. Processing patch 71304 from bug 46016. Failed to run "[u'/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Darin Fisher', u'--force']" exit_code: 1 Full output: http://queues.webkit.org/results/4696001
Created attachment 71342 [details] Patch for landing
Comment on attachment 71342 [details] Patch for landing Clearing flags on attachment: 71342 Committed r70222: <http://trac.webkit.org/changeset/70222>
All reviewed patches have been landed. Closing bug.
http://trac.webkit.org/changeset/70222 might have broken GTK Linux 64-bit Debug The following tests are not passing: mathml/presentation/fractions-vertical-alignment.xhtml mathml/presentation/mo.xhtml mathml/presentation/row-alignment.xhtml mathml/presentation/row.xhtml