Bug 66679

Summary: [chromium] DRT linux should pass nativeKeyCode to plugins
Product: WebKit Reporter: Tony Chang <tony>
Component: Tools / TestsAssignee: noel gordon <noel.gordon>
Status: RESOLVED FIXED    
Severity: Normal CC: fsamuel, noel.gordon, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
Patch
none
Patch for landing
none
Patch none

Description Tony Chang 2011-08-22 10:09:12 PDT
From https://bugs.webkit.org/show_bug.cgi?id=65964#c4 :

> Do we know why we get an empty keycode with Chromium's DRT?  It seems to be working for GTK+.

The GTK port creates an honest to goodness GdkEvent keypress event
  http://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/gtk/EventSender.cpp?rev=85516#L612

Chromium's DRT generates a WebKeyboardEvent, defined by the webkit api
  http://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/chromium/EventSender.cpp?rev=85516#L566

and fills in the WebKeyboardEvent.windowsKeyCode field, but not the nativeKeyCode field
  http://trac.webkit.org/browser/trunk/WebKit/chromium/public/WebInputEvent.h?rev=50721#L153

The plugin is sent the nativeKeyCode value.  That works in the real browser case, but not with
the Chromium DRT.  Is there a table somewhere that maps webkit::VKEY_CODE to native key codes?

--

I don't think a table like this exists, but adding one for DRT would be fine.
Comment 1 noel gordon 2011-08-25 01:07:51 PDT
Will require two-sided patch, chromium side is http://crbug/66679
Comment 2 noel gordon 2011-08-25 01:08:51 PDT
Meant http://crbug.com/66679
Comment 3 noel gordon 2011-08-25 01:11:13 PDT
copy/paste-fu-day, it's http://crbug.com/94211
Comment 4 noel gordon 2011-08-25 01:17:57 PDT
Created attachment 105142 [details]
patch
Comment 5 Tony Chang 2011-08-25 09:34:51 PDT
Comment on attachment 105142 [details]
patch

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

> Tools/DumpRenderTree/chromium/EventSender.cpp:580
> +    eventDown.nativeKeyCode = ui::NativeKeyCodeFromWindowsKeyCode(keyCode, needsShiftKeyModifier);

We don't want to call code in ui directly.  Instead, we want all methods to go through the webkit_support API.  You can add a function to webkit_support.h that does this conversion.  In general, this approach seems fine.
Comment 6 noel gordon 2011-08-25 20:48:20 PDT
Created attachment 105300 [details]
Patch

Right, move the helper into the webkit_support namespace.
Comment 7 noel gordon 2011-08-29 00:32:55 PDT
Chrome side part #1 done.
  http://src.chromium.org/viewvc/chrome?view=rev&revision=98599
Comment 8 noel gordon 2011-08-29 21:42:17 PDT
Chrome side part #2 done.
  http://src.chromium.org/viewvc/chrome?view=rev&revision=98771
Comment 9 noel gordon 2011-08-29 21:43:56 PDT
Wait for chrome rev 98771 to roll into webkit chromium DEPS.
Comment 10 noel gordon 2011-08-30 21:47:40 PDT
DEPS rolled rev 98771 in http://trac.webkit.org/changeset/94086
Comment 11 noel gordon 2011-08-30 21:52:05 PDT
Created attachment 105738 [details]
Patch for landing
Comment 12 WebKit Review Bot 2011-08-31 10:43:22 PDT
Comment on attachment 105738 [details]
Patch for landing

Clearing flags on attachment: 105738

Committed r94203: <http://trac.webkit.org/changeset/94203>
Comment 13 WebKit Review Bot 2011-08-31 10:43:27 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Fady Samuel 2011-11-05 19:24:05 PDT
(In reply to comment #13)
> All reviewed patches have been landed.  Closing bug.

DumpRenderTree seems to be broken now. 

CXX(target) out/Debug/obj.target/DumpRenderTree/third_party/WebKit/Tools/DumpRenderTree/chromium/WebViewHost.o
third_party/WebKit/Tools/DumpRenderTree/chromium/EventSender.cpp: In member function 'void EventSender::keyDown(const CppArgumentList&, CppVariant*)':
third_party/WebKit/Tools/DumpRenderTree/chromium/EventSender.cpp:580: error: 'NativeKeyCodeForWindowsKeyCode' is not a member of 'webkit_support'
make: *** [out/Debug/obj.target/DumpRenderTree/third_party/WebKit/Tools/DumpRenderTree/chromium/EventSender.o] Error 1
Comment 16 noel gordon 2011-11-05 23:19:32 PDT
Created attachment 113778 [details]
Patch
Comment 17 noel gordon 2011-11-06 00:38:03 PDT
webkit DEPS rolled past chromium r104013 4 weeks ago
  http://trac.webkit.org/changeset/96899/trunk/Source/WebKit/chromium/DEPS

And you'd think we would have heard about Linux DumpRenderTree compilation
failures 4 weeks ago.

Checked Linux DumpRenderTree is compiling on queues.webkit.org as we speak
  http://queues.webkit.org/results/10338386

I don't see a problem, possible false alarm?
Comment 18 Tony Chang 2011-11-07 11:19:17 PST
(In reply to comment #14)
> (In reply to comment #13)
> > All reviewed patches have been landed.  Closing bug.
> 
> DumpRenderTree seems to be broken now. 
> 
> CXX(target) out/Debug/obj.target/DumpRenderTree/third_party/WebKit/Tools/DumpRenderTree/chromium/WebViewHost.o
> third_party/WebKit/Tools/DumpRenderTree/chromium/EventSender.cpp: In member function 'void EventSender::keyDown(const CppArgumentList&, CppVariant*)':
> third_party/WebKit/Tools/DumpRenderTree/chromium/EventSender.cpp:580: error: 'NativeKeyCodeForWindowsKeyCode' is not a member of 'webkit_support'
> make: *** [out/Debug/obj.target/DumpRenderTree/third_party/WebKit/Tools/DumpRenderTree/chromium/EventSender.o] Error 1

Fady, which builder do you see this on?
Comment 19 noel gordon 2011-12-01 16:51:31 PST
Fady worked it out in http://trac.webkit.org/changeset/101464