44771 2010-08-27 09:15:48 -0700 [GTK] Windowless Plugins - Initialize XEvent before passing to plugin 2010-09-03 20:40:59 -0700 1 1 1 Unclassified WebKit Plug-ins 528+ (Nightly build) PC Linux RESOLVED FIXED P2 Normal --- 0 bharathwaaj.s webkit-unassigned commit-queue eric xan.lopez oldest_to_newest 270534 0 bharathwaaj.s 2010-08-27 09:15:48 -0700 Initialize keyboard events before passing plugins. This causes a crash if not initialized. 270538 1 65720 bharathwaaj.s 2010-08-27 09:24:47 -0700 Created attachment 65720 Patch to initialize keyboard event This patch initializes keyboard event in windowless plugins 270551 2 65720 xan.lopez 2010-08-27 09:44:31 -0700 Comment on attachment 65720 Patch to initialize keyboard event LGTM. 270935 3 65720 commit-queue 2010-08-27 23:26:20 -0700 Comment on attachment 65720 Patch to initialize keyboard event Rejecting patch 65720 from commit-queue. Failed to run "[u'git', u'svn', u'dcommit']" exit_code: 1 Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/plugins/gtk/PluginViewGtk.cpp A repository hook failed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output: svnlook: Can't write to stream: Broken pipe The following ChangeLog files contain OOPS: trunk/WebCore/ChangeLog Please don't ever say "OOPS" in a ChangeLog file. at /usr/local/git/libexec/git-core/git-svn line 572 Full output: http://queues.webkit.org/results/3866052 271000 4 65720 eric 2010-08-28 09:18:34 -0700 Comment on attachment 65720 Patch to initialize keyboard event The queue only replaces the OOPS in the Reviewed by line. You should explain what new tests your adding or why testing is impossible/impractical. 271899 5 66009 bharathwaaj.s 2010-08-30 20:56:13 -0700 Created attachment 66009 Patch to initialize keyboard event - Modified changelog This is the same patch with the ChangeLog modified to explain why no new tests were needed. This is a trivial fix. This is done to support https://bugs.webkit.org/show_bug.cgi?id=44613 It just initializes keyboard event.This patch should be applied first else it might cause a crash when bug #44613 patch is applied. 273859 6 bharathwaaj.s 2010-09-03 04:07:55 -0700 Please review this and provide comments. 273883 7 66009 xan.lopez 2010-09-03 04:51:31 -0700 Comment on attachment 66009 Patch to initialize keyboard event - Modified changelog LGTM. 273889 8 66009 commit-queue 2010-09-03 05:13:18 -0700 Comment on attachment 66009 Patch to initialize keyboard event - Modified changelog Clearing flags on attachment: 66009 Committed r66723: <http://trac.webkit.org/changeset/66723> 273890 9 commit-queue 2010-09-03 05:13:23 -0700 All reviewed patches have been landed. Closing bug. 274016 10 ap 2010-09-03 10:28:09 -0700 + No new tests needed since this is a trivial fix. I don't see how one follows from another. Any patch that changes observable behavior needs a test. 274056 11 xan.lopez 2010-09-03 11:24:17 -0700 (In reply to comment #10) > + No new tests needed since this is a trivial fix. > > I don't see how one follows from another. Any patch that changes observable behavior needs a test. This is correct, and now and in the past I have hold patches that were useful and needed of going into WebKit(GTK+) because they lacked the necessary tests. In this case the patch seems so obviously correct and trivial that I don't think we need to do this. I believe this is routinely done by everyone, and far more complicated patches have been landed without tests in WebKit by every single person commenting on this bug. If we are going to enforce this rule absolutely everywhere then let's do it, but please let's not enforce it randomly and without reason. 274061 12 eric 2010-09-03 11:29:24 -0700 I support Alexey's request. Changes are universally required to have tests. We make occasional exceptions where testing is impossible/impractical. Any change you see landing w/o tests, please comment in the bug and remind folks of this requirement. In particularly if there is no keyboard event testing for plugins, that's a huge hole which should be easy to plug. We have a test plugin in DRT for just this purpose. We also have the ability to synthesize key events in DRT. This change should be very testable. 274063 13 eric 2010-09-03 11:32:41 -0700 In further defense of testing: I've had numerous discussions with other webkit engineers over the years, where we've agreed that the tests are the most important part of our codebase. It's very difficult (impossible) to write a web engine that works w/o testing. However any monkey can eventually write a decent web engine by making all our tests pass. I'm constantly fixing old bugs, in old code, where if the person had simply written a test when they made the change, we would have better understood, and never regressed the behavior in the first place. 274338 14 xan.lopez 2010-09-03 19:58:09 -0700 I don't disagree with anything of what you have said, but I think this is one of those exceptional cases where to hold off the patch until the test is written is impractical given that it's an obviously correct one-liner that fixes a bug. In any case, if anyone disagrees strongly with that judgment feel free to revert the patch. 274342 15 xan.lopez 2010-09-03 20:40:59 -0700 Just checked, there's already a test for windowless plugins & key events, plugins/keyboard-events.html, skipped in GTK+. With this patch and the patch from https://bugs.webkit.org/show_bug.cgi?id=44613 we at least receive the events themselves, although there seems to still be a couple of bugs wrt the focus event and the keycodes received. We'll iterate the other patch until those are gone. 65720 2010-08-27 09:24:47 -0700 2010-08-30 20:56:13 -0700 Patch to initialize keyboard event keyboardinitialize.patch text/plain 1169 bharathwaaj.s SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA2NjIyNSkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTUgQEAKKzIwMTAtMDgtMjcgIEJoYXJhdGh3YWFqIFNyaW5pdmFzYW4gIDxiaGFy YXRod2Fhai5zQGdtYWlsLmNvbT4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMh KS4KKworICAgICAgICBJbml0aWFsaXplIGtleWJvYXJkIGV2ZW50cyBiZWZvcmUgcGFzc2luZyBw bHVnaW5zLgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9 NDQ3NzEKKworICAgICAgICBObyBuZXcgdGVzdHMuIChPT1BTISkKKworICAgICAgICAqIHBsdWdp bnMvZ3RrL1BsdWdpblZpZXdHdGsuY3BwOgorICAgICAgICAoV2ViQ29yZTo6UGx1Z2luVmlldzo6 aGFuZGxlS2V5Ym9hcmRFdmVudCk6CisKIDIwMTAtMDgtMjcgIFBhdmVsIEZlbGRtYW4gIDxwZmVs ZG1hbkBjaHJvbWl1bS5vcmc+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgWXVyeSBTZW1pa2hhdHNr eS4KSW5kZXg6IFdlYkNvcmUvcGx1Z2lucy9ndGsvUGx1Z2luVmlld0d0ay5jcHAKPT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PQotLS0gV2ViQ29yZS9wbHVnaW5zL2d0ay9QbHVnaW5WaWV3R3RrLmNwcAkocmV2aXNpb24gNjU4 NTkpCisrKyBXZWJDb3JlL3BsdWdpbnMvZ3RrL1BsdWdpblZpZXdHdGsuY3BwCSh3b3JraW5nIGNv cHkpCkBAIC0yOTksNiArMjk5LDcgQEAgdm9pZCBQbHVnaW5WaWV3OjpoYW5kbGVLZXlib2FyZEV2 ZW50KEtleQogCiAgICAgTlBFdmVudCB4RXZlbnQ7CiAjaWYgZGVmaW5lZChYUF9VTklYKQorICAg IGluaXRYRXZlbnQoJnhFdmVudCk7CiAgICAgR2RrRXZlbnRLZXkqIGdka0V2ZW50ID0gZXZlbnQt PmtleUV2ZW50KCktPmdka0V2ZW50S2V5KCk7CiAKICAgICB4RXZlbnQudHlwZSA9IChldmVudC0+ dHlwZSgpID09IGV2ZW50TmFtZXMoKS5rZXlkb3duRXZlbnQpID8gMiA6IDM7IC8vIEtleVByZXNz L1JlbGVhc2UgZ2V0IHVuc2V0IHNvbWV3aGVyZQo= 66009 2010-08-30 20:56:13 -0700 2010-09-03 05:13:18 -0700 Patch to initialize keyboard event - Modified changelog keyboardinitialize.patch text/plain 1190 bharathwaaj.s SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA2NjQ0OSkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTUgQEAKKzIwMTAtMDgtMzAgIEJoYXJhdGh3YWFqIFNyaW5pdmFzYW4gIDxiaGFy YXRod2Fhai5zQGdtYWlsLmNvbT4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMh KS4KKworICAgICAgICBJbml0aWFsaXplIGtleWJvYXJkIGV2ZW50cyBiZWZvcmUgcGFzc2luZyBw bHVnaW5zLgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9 NDQ3NzEKKworICAgICAgICBObyBuZXcgdGVzdHMgbmVlZGVkIHNpbmNlIHRoaXMgaXMgYSB0cml2 aWFsIGZpeC4KKworICAgICAgICAqIHBsdWdpbnMvZ3RrL1BsdWdpblZpZXdHdGsuY3BwOgorICAg ICAgICAoV2ViQ29yZTo6UGx1Z2luVmlldzo6aGFuZGxlS2V5Ym9hcmRFdmVudCk6CisKIDIwMTAt MDgtMzAgIEVyaWMgU2VpZGVsICA8ZXJpY0B3ZWJraXQub3JnPgogCiAgICAgICAgIFVucmV2aWV3 ZWQsIHJvbGxpbmcgb3V0IHI2NjQxOC4KSW5kZXg6IFdlYkNvcmUvcGx1Z2lucy9ndGsvUGx1Z2lu Vmlld0d0ay5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2ViQ29yZS9wbHVnaW5zL2d0ay9QbHVnaW5WaWV3 R3RrLmNwcAkocmV2aXNpb24gNjU4NTkpCisrKyBXZWJDb3JlL3BsdWdpbnMvZ3RrL1BsdWdpblZp ZXdHdGsuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0yOTksNiArMjk5LDcgQEAgdm9pZCBQbHVnaW5W aWV3OjpoYW5kbGVLZXlib2FyZEV2ZW50KEtleQogCiAgICAgTlBFdmVudCB4RXZlbnQ7CiAjaWYg ZGVmaW5lZChYUF9VTklYKQorICAgIGluaXRYRXZlbnQoJnhFdmVudCk7CiAgICAgR2RrRXZlbnRL ZXkqIGdka0V2ZW50ID0gZXZlbnQtPmtleUV2ZW50KCktPmdka0V2ZW50S2V5KCk7CiAKICAgICB4 RXZlbnQudHlwZSA9IChldmVudC0+dHlwZSgpID09IGV2ZW50TmFtZXMoKS5rZXlkb3duRXZlbnQp ID8gMiA6IDM7IC8vIEtleVByZXNzL1JlbGVhc2UgZ2V0IHVuc2V0IHNvbWV3aGVyZQo=