Multi-select <select> in 'external popup window'

Multi-select <select> in 'external popup window'

We use an 'external popup window' for both single and multi selection
<select> tag popups.  This also supports <optgroup> and disabled <option>
items and multiple selections.  On all other chrome platforms a multi-select
<select> tag is rendered in page, so this CL adds a bit of plumbing and IPC
stuff to get the multi-select popups working as well.

WebKit side of this CL is at http://trac.webkit.org/changeset/94600

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=139169

[aruslan, 2012-05-23 23:58:47]

Please let me know if anything doesn't look right! Thanks!

[Ben Goodger (Google), 2012-05-24 15:35:13]

Replacing myself with Avi on review.

[Avi (use Gerrit), 2012-05-24 18:41:41]

https://chromiumcodereview.appspot.com/10436010/diff/17002/content/browser/we...
File content/browser/web_contents/interstitial_page_impl.cc (right):

https://chromiumcodereview.appspot.com/10436010/diff/17002/content/browser/we...
content/browser/web_contents/interstitial_page_impl.cc:103: virtual void
ShowPopupMenu(const gfx::Rect& bounds,
ShowPopupMenu was the only function you added to RenderViewHostDelegateView. Why
are you adding the rest of these functions here? It looks like a bad merge of a
change made to this file not long ago. Also, if this is an interface
implementation, use OVERRIDE everywhere. (It would have caught this mismerge.)

https://chromiumcodereview.appspot.com/10436010/diff/17002/content/browser/we...
content/browser/web_contents/interstitial_page_impl.cc:752: }
Bad merge.

https://chromiumcodereview.appspot.com/10436010/diff/17002/content/browser/we...
File content/browser/web_contents/web_contents_view_gtk.cc (right):

https://chromiumcodereview.appspot.com/10436010/diff/17002/content/browser/we...
content/browser/web_contents/web_contents_view_gtk.cc:344: NOTREACHED();
Standardize the implementations of ShowPopupMenu across all the platforms that
don't use it.

https://chromiumcodereview.appspot.com/10436010/diff/17002/content/common/vie...
File content/common/view_messages.h (right):

https://chromiumcodereview.appspot.com/10436010/diff/17002/content/common/vie...
content/common/view_messages.h:964: int /* selected index, -1 means no selection
*/)
If ViewMsg_SelectPopupMenuItems gets an platform #if, give this message one as
well.

https://chromiumcodereview.appspot.com/10436010/diff/17002/content/port/brows...
File content/port/browser/render_view_host_delegate_view.h (right):

https://chromiumcodereview.appspot.com/10436010/diff/17002/content/port/brows...
content/port/browser/render_view_host_delegate_view.h:38: // This method should
call RenderViewHost::DidSelectPopupMenuItemAt() or
// This method should call RenderViewHost::DidSelectPopupMenuItem[s]() or

(The method name is wrong.)

https://chromiumcodereview.appspot.com/10436010/diff/17002/content/renderer/e...
File content/renderer/external_popup_menu.cc (right):

https://chromiumcodereview.appspot.com/10436010/diff/17002/content/renderer/e...
content/renderer/external_popup_menu.cc:48: }
ditto platform mark. Plus, blank line please before the #if.

https://chromiumcodereview.appspot.com/10436010/diff/17002/content/renderer/e...
File content/renderer/external_popup_menu.h (right):

https://chromiumcodereview.appspot.com/10436010/diff/17002/content/renderer/e...
content/renderer/external_popup_menu.h:29: void DidSelectItem(int
selected_index);
Again, if we mark one for a platform, mark them all.

https://chromiumcodereview.appspot.com/10436010/diff/17002/content/renderer/r...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/10436010/diff/17002/content/renderer/r...
content/renderer/render_view_impl.cc:5461: std::vector<int> selected_indices) {
const std::vector<int>& ?

https://chromiumcodereview.appspot.com/10436010/diff/17002/content/renderer/r...
File content/renderer/render_view_impl.h (right):

https://chromiumcodereview.appspot.com/10436010/diff/17002/content/renderer/r...
content/renderer/render_view_impl.h:965: void OnSelectPopupMenuItems(bool
canceled, std::vector<int> selected_indices);
const std::vector<int>& ?

[aruslan, 2012-05-24 19:53:15]

Thank you, Avi; that was quite a bad merge on my side, should review it myself
before letting other seeing it the next time.

https://chromiumcodereview.appspot.com/10436010/diff/17002/content/browser/we...
File content/browser/web_contents/interstitial_page_impl.cc (right):

https://chromiumcodereview.appspot.com/10436010/diff/17002/content/browser/we...
content/browser/web_contents/interstitial_page_impl.cc:103: virtual void
ShowPopupMenu(const gfx::Rect& bounds,
Stupid me. Thanks!!
On 2012/05/24 18:41:41, Avi wrote:
> ShowPopupMenu was the only function you added to RenderViewHostDelegateView.
Why
> are you adding the rest of these functions here? It looks like a bad merge of
a
> change made to this file not long ago. Also, if this is an interface
> implementation, use OVERRIDE everywhere. (It would have caught this mismerge.)

https://chromiumcodereview.appspot.com/10436010/diff/17002/content/browser/we...
content/browser/web_contents/interstitial_page_impl.cc:752: }
On 2012/05/24 18:41:41, Avi wrote:
> Bad merge.

Done.

https://chromiumcodereview.appspot.com/10436010/diff/17002/content/browser/we...
File content/browser/web_contents/web_contents_view_gtk.cc (right):

https://chromiumcodereview.appspot.com/10436010/diff/17002/content/browser/we...
content/browser/web_contents/web_contents_view_gtk.cc:344: NOTREACHED();
On 2012/05/24 18:41:41, Avi wrote:
> Standardize the implementations of ShowPopupMenu across all the platforms that
> don't use it.

Done.

https://chromiumcodereview.appspot.com/10436010/diff/17002/content/common/vie...
File content/common/view_messages.h (right):

https://chromiumcodereview.appspot.com/10436010/diff/17002/content/common/vie...
content/common/view_messages.h:964: int /* selected index, -1 means no selection
*/)
On 2012/05/24 18:41:41, Avi wrote:
> If ViewMsg_SelectPopupMenuItems gets an platform #if, give this message one as
> well.

Done.

https://chromiumcodereview.appspot.com/10436010/diff/17002/content/port/brows...
File content/port/browser/render_view_host_delegate_view.h (right):

https://chromiumcodereview.appspot.com/10436010/diff/17002/content/port/brows...
content/port/browser/render_view_host_delegate_view.h:38: // This method should
call RenderViewHost::DidSelectPopupMenuItemAt() or
On 2012/05/24 18:41:41, Avi wrote:
> // This method should call RenderViewHost::DidSelectPopupMenuItem[s]() or
> 
> (The method name is wrong.)

Done.

https://chromiumcodereview.appspot.com/10436010/diff/17002/content/renderer/e...
File content/renderer/external_popup_menu.cc (right):

https://chromiumcodereview.appspot.com/10436010/diff/17002/content/renderer/e...
content/renderer/external_popup_menu.cc:48: }
On 2012/05/24 18:41:41, Avi wrote:
> ditto platform mark. Plus, blank line please before the #if.

Done.

https://chromiumcodereview.appspot.com/10436010/diff/17002/content/renderer/e...
File content/renderer/external_popup_menu.h (right):

https://chromiumcodereview.appspot.com/10436010/diff/17002/content/renderer/e...
content/renderer/external_popup_menu.h:29: void DidSelectItem(int
selected_index);
On 2012/05/24 18:41:41, Avi wrote:
> Again, if we mark one for a platform, mark them all.

Done.

https://chromiumcodereview.appspot.com/10436010/diff/17002/content/renderer/r...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/10436010/diff/17002/content/renderer/r...
content/renderer/render_view_impl.cc:5461: std::vector<int> selected_indices) {
On 2012/05/24 18:41:41, Avi wrote:
> const std::vector<int>& ?

Done.

https://chromiumcodereview.appspot.com/10436010/diff/17002/content/renderer/r...
File content/renderer/render_view_impl.h (right):

https://chromiumcodereview.appspot.com/10436010/diff/17002/content/renderer/r...
content/renderer/render_view_impl.h:965: void OnSelectPopupMenuItems(bool
canceled, std::vector<int> selected_indices);
On 2012/05/24 18:41:41, Avi wrote:
> const std::vector<int>& ?

Done.

[Avi (use Gerrit), 2012-05-24 20:20:34]

On 2012/05/24 19:53:15, aruslan wrote:
> Thank you, Avi; that was quite a bad merge on my side, should review it myself
> before letting other seeing it the next time.

No problem; I've done it more than once. That's what reviews are for. :)

[Avi (use Gerrit), 2012-05-24 20:22:05]

On 2012/05/24 20:20:34, Avi wrote:
> On 2012/05/24 19:53:15, aruslan wrote:
> > Thank you, Avi; that was quite a bad merge on my side, should review it
myself
> > before letting other seeing it the next time.
> 
> No problem; I've done it more than once. That's what reviews are for. :)

Re the build failure because something is no longer overriding the way it used
to, you can take the OVERRIDE off, but please follow up.

[Avi (use Gerrit), 2012-05-24 20:24:41]

LGTM

[commit-bot: I haz the power, 2012-05-25 22:42:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aruslan@chromium.org/10436010/17006

[commit-bot: I haz the power, 2012-05-26 00:54:16]

Change committed as 139169