83474 2012-04-09 08:57:22 -0700 [Chromium] External popup menus should support early cancelation 2012-05-23 15:27:43 -0700 1 1 1 Unclassified WebKit WebKit Misc. 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 jcivelli jcivelli dglazkov isherman tkent webkit.review.bot oldest_to_newest 598303 0 jcivelli 2012-04-09 08:57:22 -0700 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. 598306 1 136230 jcivelli 2012-04-09 09:02:11 -0700 Created attachment 136230 Patch 623253 2 136230 isherman 2012-05-14 13:25:12 -0700 Comment on attachment 136230 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 623399 3 jcivelli 2012-05-14 15:56:34 -0700 Yes there is an CL on the Chromium side to make this work: https://chromiumcodereview.appspot.com/10392093/ 623421 4 isherman 2012-05-14 16:27:33 -0700 Ok, looks good when coupled with the Chromium CL. tkent, can you double check for an official review, since I'm not a reviewer? 623651 5 136230 tkent 2012-05-14 22:41:26 -0700 Comment on attachment 136230 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 {}. 624263 6 142003 jcivelli 2012-05-15 10:52:19 -0700 Created attachment 142003 Patch 631784 7 jcivelli 2012-05-23 10:42:19 -0700 tkent, I fixed the nit, could you R+ CQ+ this? Thanks! 632114 8 142003 webkit.review.bot 2012-05-23 15:27:39 -0700 Comment on attachment 142003 Patch Clearing flags on attachment: 142003 Committed r118258: <http://trac.webkit.org/changeset/118258> 632115 9 webkit.review.bot 2012-05-23 15:27:43 -0700 All reviewed patches have been landed. Closing bug. 136230 2012-04-09 09:02:11 -0700 2012-05-15 10:52:15 -0700 Patch bug-83474-20120409090215.patch text/plain 1641 jcivelli U3VidmVyc2lvbiBSZXZpc2lvbjogMTEzMzgwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L2No cm9taXVtL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCmluZGV4 IDFlOTM3NDRjZjg5YzljY2Q3MDYxZjQyNTM5ZjQ4MmZhYWM3ZTM0ZjcuLjlhOGZlMWFmNTM1OGE1 OTVkMWFhNjkxZjUzYjExOTVjNWE3NmEwZGUgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvY2hy b21pdW0vQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCkBA IC0xLDMgKzEsMTMgQEAKKzIwMTItMDQtMDkgIEpheSBDaXZlbGxpICA8amNpdmVsbGlAY2hyb21p dW0ub3JnPgorCisgICAgICAgIFtjaHJvbWl1bV0gUHJvdmlkZXMgYSB3YXkgdG8gY2FuY2VsIGFu IGV4dGVybmFsIHBvcHVwIG1lbnUgYmVmb3JlIGl0IGlzIHNob3duLgorICAgICAgICBodHRwczov L2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9ODM0NzQKKworICAgICAgICBSZXZpZXdl ZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIHNyYy9FeHRlcm5hbFBvcHVwTWVudS5j cHA6CisgICAgICAgIChXZWJLaXQ6OkV4dGVybmFsUG9wdXBNZW51OjpzaG93KToKKwogMjAxMi0w NC0wNSAgRGFuYSBKYW5zZW5zICA8ZGFuYWtqQGNocm9taXVtLm9yZz4KIAogICAgICAgICBbY2hy b21pdW1dIFJlY29yZCB0aGUgbnVtYmVyIG9mIHRpbGVzIHBhaW50LWN1bGxlZCBpbnN0ZWFkIG9m IHRoZSBpbmNvcnJlY3QgbnVtYmVyIG9mIHBpeGVscwpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktp dC9jaHJvbWl1bS9zcmMvRXh0ZXJuYWxQb3B1cE1lbnUuY3BwIGIvU291cmNlL1dlYktpdC9jaHJv bWl1bS9zcmMvRXh0ZXJuYWxQb3B1cE1lbnUuY3BwCmluZGV4IDYzZmEzYWVhYzllMmI0YmJjMWU5 ZDRjZmVlMDlmMmM1NjBlODIyMGEuLmQ4NGViNDhmNTJlZmMyOTkxNmRlNjg2NTVhOWJmZDViNzNj OWFiYWMgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vc3JjL0V4dGVybmFsUG9w dXBNZW51LmNwcAorKysgYi9Tb3VyY2UvV2ViS2l0L2Nocm9taXVtL3NyYy9FeHRlcm5hbFBvcHVw TWVudS5jcHAKQEAgLTY5LDcgKzY5LDExIEBAIHZvaWQgRXh0ZXJuYWxQb3B1cE1lbnU6OnNob3co Y29uc3QgSW50UmVjdCYgcmVjdCwgRnJhbWVWaWV3KiB2LCBpbnQgaW5kZXgpCiAgICAgICAgIHJl dHVybjsKICAgICBtX3dlYkV4dGVybmFsUG9wdXBNZW51ID0KICAgICAgICAgbV93ZWJWaWV3Q2xp ZW50LT5jcmVhdGVFeHRlcm5hbFBvcHVwTWVudShpbmZvLCB0aGlzKTsKLSAgICBtX3dlYkV4dGVy bmFsUG9wdXBNZW51LT5zaG93KHYtPmNvbnRlbnRzVG9XaW5kb3cocmVjdCkpOworICAgIGlmICht X3dlYkV4dGVybmFsUG9wdXBNZW51KQorICAgICAgICBtX3dlYkV4dGVybmFsUG9wdXBNZW51LT5z aG93KHYtPmNvbnRlbnRzVG9XaW5kb3cocmVjdCkpOworICAgIGVsc2UKKyAgICAgICAgLy8gVGhl IGNsaWVudCBtaWdodCByZWZ1c2UgdG8gY3JlYXRlIGEgcG9wdXAgKHdoZW4gdGhlcmUgaXMgYWxy ZWFkeSBvbmUgcGVuZGluZyB0byBiZSBzaG93biBmb3IgZXhhbXBsZSkuCisgICAgICAgIGRpZENh bmNlbCgpOwogfQogCiB2b2lkIEV4dGVybmFsUG9wdXBNZW51OjpoaWRlKCkK 142003 2012-05-15 10:52:19 -0700 2012-05-23 15:27:39 -0700 Patch bug-83474-20120515105218.patch text/plain 1639 jcivelli U3VidmVyc2lvbiBSZXZpc2lvbjogMTE3MDA2CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L2No cm9taXVtL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCmluZGV4 IDU1OWJhNjM4Njg0MTE4ZGM3YTlhZTIwMmY2YWQ3NTUwZGUwZTNkMWEuLmRhMWNkMDVjZTcwODAz MGZmMmNjNWI5ZDE2MmU2MDhmNTkyOTM5ZWIgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvY2hy b21pdW0vQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCkBA IC0xLDMgKzEsMTMgQEAKKzIwMTItMDUtMTQgIEpheSBDaXZlbGxpICA8amNpdmVsbGlAY2hyb21p dW0ub3JnPgorCisgICAgICAgIFtjaHJvbWl1bV0gUHJvdmlkZXMgYSB3YXkgdG8gY2FuY2VsIGFu IGV4dGVybmFsIHBvcHVwIG1lbnUgYmVmb3JlIGl0IGlzIHNob3duLgorICAgICAgICBodHRwczov L2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9ODM0NzQKKworICAgICAgICBSZXZpZXdl ZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIHNyYy9FeHRlcm5hbFBvcHVwTWVudS5j cHA6CisgICAgICAgIChXZWJLaXQ6OkV4dGVybmFsUG9wdXBNZW51OjpzaG93KToKKwogMjAxMi0w NS0xNCAgQWRhbSBCYXJ0aCAgPGFiYXJ0aEB3ZWJraXQub3JnPgogCiAgICAgICAgIFtDaHJvbWl1 bV0gYW5kcm9pZC9XZWJJbnB1dEV2ZW50RmFjdG9yeSBzaG91bGQgaGFuZGxlIHdoZWVsIGV2ZW50 cyBhbmQgZ2VzdHVyZSBldmVudHMKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0v c3JjL0V4dGVybmFsUG9wdXBNZW51LmNwcCBiL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vc3JjL0V4 dGVybmFsUG9wdXBNZW51LmNwcAppbmRleCA2M2ZhM2FlYWM5ZTJiNGJiYzFlOWQ0Y2ZlZTA5ZjJj NTYwZTgyMjBhLi4xODU1NDAxYzRhNmIxY2M4MmI3OGZkMzU3MDA4OTkzMzFiOWU4NjcwIDEwMDY0 NAotLS0gYS9Tb3VyY2UvV2ViS2l0L2Nocm9taXVtL3NyYy9FeHRlcm5hbFBvcHVwTWVudS5jcHAK KysrIGIvU291cmNlL1dlYktpdC9jaHJvbWl1bS9zcmMvRXh0ZXJuYWxQb3B1cE1lbnUuY3BwCkBA IC02OSw3ICs2OSwxMiBAQCB2b2lkIEV4dGVybmFsUG9wdXBNZW51OjpzaG93KGNvbnN0IEludFJl Y3QmIHJlY3QsIEZyYW1lVmlldyogdiwgaW50IGluZGV4KQogICAgICAgICByZXR1cm47CiAgICAg bV93ZWJFeHRlcm5hbFBvcHVwTWVudSA9CiAgICAgICAgIG1fd2ViVmlld0NsaWVudC0+Y3JlYXRl RXh0ZXJuYWxQb3B1cE1lbnUoaW5mbywgdGhpcyk7Ci0gICAgbV93ZWJFeHRlcm5hbFBvcHVwTWVu dS0+c2hvdyh2LT5jb250ZW50c1RvV2luZG93KHJlY3QpKTsKKyAgICBpZiAobV93ZWJFeHRlcm5h bFBvcHVwTWVudSkKKyAgICAgICAgbV93ZWJFeHRlcm5hbFBvcHVwTWVudS0+c2hvdyh2LT5jb250 ZW50c1RvV2luZG93KHJlY3QpKTsKKyAgICBlbHNlIHsKKyAgICAgICAgLy8gVGhlIGNsaWVudCBt aWdodCByZWZ1c2UgdG8gY3JlYXRlIGEgcG9wdXAgKHdoZW4gdGhlcmUgaXMgYWxyZWFkeSBvbmUg cGVuZGluZyB0byBiZSBzaG93biBmb3IgZXhhbXBsZSkuCisgICAgICAgIGRpZENhbmNlbCgpOwor ICAgIH0KIH0KIAogdm9pZCBFeHRlcm5hbFBvcHVwTWVudTo6aGlkZSgpCg==