Bug 44771 - [GTK] Windowless Plugins - Initialize XEvent before passing to plugin
Summary: [GTK] Windowless Plugins - Initialize XEvent before passing to plugin
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-08-27 09:15 PDT by Bharathwaaj
Modified: 2010-09-03 20:40 PDT (History)
3 users (show)

See Also:


Attachments
Patch to initialize keyboard event (1.14 KB, patch)
2010-08-27 09:24 PDT, Bharathwaaj
eric: review-
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch to initialize keyboard event - Modified changelog (1.16 KB, patch)
2010-08-30 20:56 PDT, Bharathwaaj
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Bharathwaaj 2010-08-27 09:15:48 PDT
Initialize keyboard events before passing plugins. This causes a crash if not initialized.
Comment 1 Bharathwaaj 2010-08-27 09:24:47 PDT
Created attachment 65720 [details]
Patch to initialize keyboard event

This patch initializes keyboard event in windowless plugins
Comment 2 Xan Lopez 2010-08-27 09:44:31 PDT
Comment on attachment 65720 [details]
Patch to initialize keyboard event

LGTM.
Comment 3 WebKit Commit Bot 2010-08-27 23:26:20 PDT
Comment on attachment 65720 [details]
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
Comment 4 Eric Seidel (no email) 2010-08-28 09:18:34 PDT
Comment on attachment 65720 [details]
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.
Comment 5 Bharathwaaj 2010-08-30 20:56:13 PDT
Created attachment 66009 [details]
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.
Comment 6 Bharathwaaj 2010-09-03 04:07:55 PDT
Please review this and provide comments.
Comment 7 Xan Lopez 2010-09-03 04:51:31 PDT
Comment on attachment 66009 [details]
Patch to initialize keyboard event - Modified changelog

LGTM.
Comment 8 WebKit Commit Bot 2010-09-03 05:13:18 PDT
Comment on attachment 66009 [details]
Patch to initialize keyboard event - Modified changelog

Clearing flags on attachment: 66009

Committed r66723: <http://trac.webkit.org/changeset/66723>
Comment 9 WebKit Commit Bot 2010-09-03 05:13:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Alexey Proskuryakov 2010-09-03 10:28:09 PDT
+        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.
Comment 11 Xan Lopez 2010-09-03 11:24:17 PDT
(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.
Comment 12 Eric Seidel (no email) 2010-09-03 11:29:24 PDT
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.
Comment 13 Eric Seidel (no email) 2010-09-03 11:32:41 PDT
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.
Comment 14 Xan Lopez 2010-09-03 19:58:09 PDT
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.
Comment 15 Xan Lopez 2010-09-03 20:40:59 PDT
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.