Bug 46016 - [chromium] There should be an API for showing an external popup that does not require creation of a WebWidget
Summary: [chromium] There should be an API for showing an external popup that does not...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Jay Civelli
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-17 17:25 PDT by Jay Civelli
Modified: 2010-10-21 04:17 PDT (History)
5 users (show)

See Also:


Attachments
Initial patch (17.45 KB, patch)
2010-09-21 17:02 PDT, Jay Civelli
no flags Details | Formatted Diff | Diff
Fixed style issues (17.44 KB, patch)
2010-09-21 17:17 PDT, Jay Civelli
no flags Details | Formatted Diff | Diff
Fixing style take 2 (17.44 KB, patch)
2010-09-21 17:24 PDT, Jay Civelli
fishd: review-
Details | Formatted Diff | Diff
Applied fishd suggested changes (17.71 KB, patch)
2010-10-07 14:59 PDT, Jay Civelli
no flags Details | Formatted Diff | Diff
Adding an static method to WebView to set whether to use external popups or not. (19.91 KB, patch)
2010-10-12 09:55 PDT, Jay Civelli
no flags Details | Formatted Diff | Diff
Added the bounds where the popup should be shown to WebExternalPopup::show() (20.74 KB, patch)
2010-10-15 09:39 PDT, Jay Civelli
no flags Details | Formatted Diff | Diff
Patch for landing (20.60 KB, patch)
2010-10-20 10:18 PDT, Jay Civelli
no flags Details | Formatted Diff | Diff
Patch for landing (20.62 KB, patch)
2010-10-20 14:55 PDT, Jay Civelli
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jay Civelli 2010-09-17 17:25:34 PDT
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.
Comment 1 Jay Civelli 2010-09-21 17:02:33 PDT
Created attachment 68310 [details]
Initial patch
Comment 2 WebKit Review Bot 2010-09-21 17:08:39 PDT
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.
Comment 3 Jay Civelli 2010-09-21 17:17:28 PDT
Created attachment 68313 [details]
Fixed style issues
Comment 4 WebKit Review Bot 2010-09-21 17:21:11 PDT
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.
Comment 5 Jay Civelli 2010-09-21 17:24:44 PDT
Created attachment 68316 [details]
Fixing style take 2
Comment 6 Darin Fisher (:fishd, Google) 2010-10-06 11:04:43 PDT
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.
Comment 7 Jay Civelli 2010-10-07 14:59:08 PDT
Created attachment 70156 [details]
Applied fishd suggested changes
Comment 8 Darin Fisher (:fishd, Google) 2010-10-08 00:00:07 PDT
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?
Comment 9 Jay Civelli 2010-10-12 09:55:46 PDT
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 10 Darin Fisher (:fishd, Google) 2010-10-12 22:37:49 PDT
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
Comment 11 Eric Seidel (no email) 2010-10-13 12:25:39 PDT
Attachment 70538 [details] was posted by a committer and has review+, assigning to Jay Civelli for commit.
Comment 12 Jay Civelli 2010-10-15 09:39:57 PDT
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 13 Eric Seidel (no email) 2010-10-19 08:07:52 PDT
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 14 Darin Fisher (:fishd, Google) 2010-10-19 23:40:15 PDT
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));
Comment 15 Jay Civelli 2010-10-20 10:18:13 PDT
Created attachment 71304 [details]
Patch for landing
Comment 16 WebKit Commit Bot 2010-10-20 11:55:38 PDT
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 17 Eric Seidel (no email) 2010-10-20 11:58:15 PDT
Comment on attachment 71304 [details]
Patch for landing

I think this was a false rejection from a run-away commit-node.
Comment 18 WebKit Commit Bot 2010-10-20 14:28:31 PDT
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
Comment 19 Jay Civelli 2010-10-20 14:55:58 PDT
Created attachment 71342 [details]
Patch for landing
Comment 20 WebKit Commit Bot 2010-10-21 03:11:44 PDT
Comment on attachment 71342 [details]
Patch for landing

Clearing flags on attachment: 71342

Committed r70222: <http://trac.webkit.org/changeset/70222>
Comment 21 WebKit Commit Bot 2010-10-21 03:11:50 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 WebKit Review Bot 2010-10-21 04:17:32 PDT
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