Bug 46016 – [chromium] There should be an API for showing an external popup that does not require creation of a WebWidget

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Jay Civelli]

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.

[Jay Civelli]

Created attachment 68310 [details]
Initial patch

[WebKit Review Bot]

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.

[Jay Civelli]

Created attachment 68313 [details]
Fixed style issues

[WebKit Review Bot]

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.

[Jay Civelli]

Created attachment 68316 [details]
Fixing style take 2

[Darin Fisher (:fishd, Google)]

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.

[Jay Civelli]

Created attachment 70156 [details]
Applied fishd suggested changes

[Darin Fisher (:fishd, Google)]

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?

[Jay Civelli]

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.

[Darin Fisher (:fishd, Google)]

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

[Eric Seidel (no email)]

Attachment 70538 [details] was posted by a committer and has review+, assigning to Jay Civelli for commit.

[Jay Civelli]

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().

[Eric Seidel (no email)]

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.

[Darin Fisher (:fishd, Google)]

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));

[Jay Civelli]

Created attachment 71304 [details]
Patch for landing

[WebKit Commit Bot]

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

[Eric Seidel (no email)]

Comment on attachment 71304 [details]
Patch for landing

I think this was a false rejection from a run-away commit-node.

[WebKit Commit Bot]

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 [e-mail]...
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

[Jay Civelli]

Created attachment 71342 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 71342 [details]
Patch for landing

Clearing flags on attachment: 71342

Committed r70222: <http://trac.webkit.org/changeset/70222>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

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