Bug 83474 – [Chromium] External popup menus should support early cancelation

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Jay Civelli]

In Chromium for Mac and Android, we can end up in cases where we end up with 2 select popups showing (if the user presses a select and then quickly presses another before the 1st one has shown).
We need a way for the browser to cancel a select popup early to prevent that case.

[Jay Civelli]

Created attachment 136230 [details]
Patch

[Ilya Sherman]

Comment on attachment 136230 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136230&action=review

> Source/WebKit/chromium/src/ExternalPopupMenu.cpp:75
> +        // The client might refuse to create a popup (when there is already one pending to be shown for example).

Hmm, can you point me to the code that might refuse to create a popup?  The implementation that I found for createExternalPopupMenu() [1] looks like it will always create a new one, but sometimes destroy the old one (if the DCHECK fails).  Does this patch assume a Chromium change as well?
[1] https://code.google.com/searchframe#OAMlx_jo-ck/src/content/renderer/render_view_impl.cc&l=1577-1583

[Jay Civelli]

Yes there is an CL on the Chromium side to make this work:
https://chromiumcodereview.appspot.com/10392093/

[Ilya Sherman]

Ok, looks good when coupled with the Chromium CL.

tkent, can you double check for an official review, since I'm not a reviewer?

[Kent Tamura]

Comment on attachment 136230 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136230&action=review

> Source/WebKit/chromium/src/ExternalPopupMenu.cpp:76
> +    else
> +        // The client might refuse to create a popup (when there is already one pending to be shown for example).
> +        didCancel();

nit: This else clause contains two physical lines.  So we need to enclose it with {}.

[Jay Civelli]

Created attachment 142003 [details]
Patch

[Jay Civelli]

tkent, I fixed the nit, could you R+ CQ+ this?
Thanks!

[WebKit Review Bot]

Comment on attachment 142003 [details]
Patch

Clearing flags on attachment: 142003

Committed r118258: <http://trac.webkit.org/changeset/118258>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.