63249 2011-06-23 05:33:57 -0700 [UNIX] Fix compile warnings in NetscapePluginX11.cpp 2011-06-23 09:42:06 -0700 1 1 1 Unclassified WebKit WebKit2 528+ (Nightly build) PC Linux RESOLVED FIXED Gtk, Qt P2 Normal --- 1 cgarcia webkit-unassigned oldest_to_newest 426099 0 cgarcia 2011-06-23 05:33:57 -0700 ../Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp: In function ‘void WebKit::setXButtonEventFields(XEvent&, const WebKit::WebMouseEvent&, const WebCore::IntPoint&)’: ../Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:405:12: warning: enumeration value ‘NoButton’ not handled in switch ../Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp: In member function ‘bool WebKit::NetscapePlugin::platformHandleMouseEvent(const WebKit::WebMouseEvent&)’: ../Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:457:12: warning: enumeration value ‘NoType’ not handled in switch ../Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:457:12: warning: enumeration value ‘Wheel’ not handled in switch ../Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:457:12: warning: enumeration value ‘KeyDown’ not handled in switch ../Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:457:12: warning: enumeration value ‘KeyUp’ not handled in switch ../Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:457:12: warning: enumeration value ‘RawKeyDown’ not handled in switch ../Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:457:12: warning: enumeration value ‘Char’ not handled in switch 426100 1 98337 cgarcia 2011-06-23 05:35:31 -0700 Created attachment 98337 Patch 426185 2 98337 mrobinson 2011-06-23 08:10:41 -0700 Comment on attachment 98337 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=98337&action=review Looks good, but I have a few minor requests before landing. > Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:362 > + case WebMouseEvent::LeftButton: > + default: > + xButton.button = Button1; > + break; If we are receiving other types of events shouldn't we be passing them properly to the plugin. I think it's more correct to simply have ASSERT_NOT_REACHED() for default:. This makes me wonder whether wheel events are supported or not. > Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:414 > + default: > + return false; I would rather you list them explicitly here. Do you mind changing that before landing? 426198 3 cgarcia 2011-06-23 08:25:30 -0700 (In reply to comment #2) > (From update of attachment 98337 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=98337&action=review > > Looks good, but I have a few minor requests before landing. > > > Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:362 > > + case WebMouseEvent::LeftButton: > > + default: > > + xButton.button = Button1; > > + break; > > If we are receiving other types of events shouldn't we be passing them properly to the plugin. I think it's more correct to simply have ASSERT_NOT_REACHED() for default:. This makes me wonder whether wheel events are supported or not. Since this code is shared between Gtk and Qt ports I looked at current WebKi1 code of both ports and followed it. There's another method for wheel events platformHandleWheelEvent(). > > Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:414 > > + default: > > + return false; > > I would rather you list them explicitly here. Do you mind changing that before landing? Sure. Thanks for reviewing. 426232 4 cgarcia 2011-06-23 09:42:06 -0700 Committed r89580: <http://trac.webkit.org/changeset/89580> 98337 2011-06-23 05:35:31 -0700 2011-06-23 08:10:41 -0700 Patch wk2-compile-warnings.diff text/plain 1898 cgarcia ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQyL0No YW5nZUxvZwppbmRleCA5Mzc2MDcyLi5jNmM0M2Q1IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0 Mi9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTQg QEAKKzIwMTEtMDYtMjMgIENhcmxvcyBHYXJjaWEgQ2FtcG9zICA8Y2dhcmNpYUBpZ2FsaWEuY29t PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFtVTklY XSBGaXggY29tcGlsZSB3YXJuaW5ncyBpbiBOZXRzY2FwZVBsdWdpblgxMS5jcHAKKyAgICAgICAg aHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTYzMjQ5CisKKyAgICAgICAg KiBXZWJQcm9jZXNzL1BsdWdpbnMvTmV0c2NhcGUveDExL05ldHNjYXBlUGx1Z2luWDExLmNwcDoK KyAgICAgICAgKFdlYktpdDo6c2V0WEJ1dHRvbkV2ZW50RmllbGRzKToKKyAgICAgICAgKFdlYktp dDo6TmV0c2NhcGVQbHVnaW46OnBsYXRmb3JtSGFuZGxlTW91c2VFdmVudCk6CisKIDIwMTEtMDYt MjIgIENhcmxvcyBHYXJjaWEgQ2FtcG9zICA8Y2dhcmNpYUBpZ2FsaWEuY29tPgogCiAgICAgICAg IFJldmlld2VkIGJ5IE1hcnRpbiBSb2JpbnNvbi4KZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQy L1dlYlByb2Nlc3MvUGx1Z2lucy9OZXRzY2FwZS94MTEvTmV0c2NhcGVQbHVnaW5YMTEuY3BwIGIv U291cmNlL1dlYktpdDIvV2ViUHJvY2Vzcy9QbHVnaW5zL05ldHNjYXBlL3gxMS9OZXRzY2FwZVBs dWdpblgxMS5jcHAKaW5kZXggZmVjYTVhMi4uOWU3YTQ2MSAxMDA2NDQKLS0tIGEvU291cmNlL1dl YktpdDIvV2ViUHJvY2Vzcy9QbHVnaW5zL05ldHNjYXBlL3gxMS9OZXRzY2FwZVBsdWdpblgxMS5j cHAKKysrIGIvU291cmNlL1dlYktpdDIvV2ViUHJvY2Vzcy9QbHVnaW5zL05ldHNjYXBlL3gxMS9O ZXRzY2FwZVBsdWdpblgxMS5jcHAKQEAgLTM1MCwxNSArMzUwLDE2IEBAIHN0YXRpYyBpbmxpbmUg dm9pZCBzZXRYQnV0dG9uRXZlbnRGaWVsZHMoWEV2ZW50JiB4RXZlbnQsIGNvbnN0IFdlYk1vdXNl RXZlbnQmIHdlCiAKICAgICB4QnV0dG9uLnR5cGUgPSAod2ViRXZlbnQudHlwZSgpID09IFdlYkV2 ZW50OjpNb3VzZURvd24pID8gQnV0dG9uUHJlc3MgOiBCdXR0b25SZWxlYXNlOwogICAgIHN3aXRj aCAod2ViRXZlbnQuYnV0dG9uKCkpIHsKLSAgICBjYXNlIFdlYk1vdXNlRXZlbnQ6OkxlZnRCdXR0 b246Ci0gICAgICAgIHhCdXR0b24uYnV0dG9uID0gQnV0dG9uMTsKLSAgICAgICAgYnJlYWs7CiAg ICAgY2FzZSBXZWJNb3VzZUV2ZW50OjpNaWRkbGVCdXR0b246CiAgICAgICAgIHhCdXR0b24uYnV0 dG9uID0gQnV0dG9uMjsKICAgICAgICAgYnJlYWs7CiAgICAgY2FzZSBXZWJNb3VzZUV2ZW50OjpS aWdodEJ1dHRvbjoKICAgICAgICAgeEJ1dHRvbi5idXR0b24gPSBCdXR0b24zOwogICAgICAgICBi cmVhazsKKyAgICBjYXNlIFdlYk1vdXNlRXZlbnQ6OkxlZnRCdXR0b246CisgICAgZGVmYXVsdDoK KyAgICAgICAgeEJ1dHRvbi5idXR0b24gPSBCdXR0b24xOworICAgICAgICBicmVhazsKICAgICB9 CiB9CiAKQEAgLTQwOSw2ICs0MTAsOCBAQCBib29sIE5ldHNjYXBlUGx1Z2luOjpwbGF0Zm9ybUhh bmRsZU1vdXNlRXZlbnQoY29uc3QgV2ViTW91c2VFdmVudCYgZXZlbnQpCiAgICAgY2FzZSBXZWJF dmVudDo6TW91c2VNb3ZlOgogICAgICAgICBzZXRYTW90aW9uRXZlbnRGaWVsZHMoeEV2ZW50LCBl dmVudCwgbV9mcmFtZVJlY3QubG9jYXRpb24oKSk7CiAgICAgICAgIGJyZWFrOworICAgIGRlZmF1 bHQ6CisgICAgICAgIHJldHVybiBmYWxzZTsKICAgICB9CiAKICAgICByZXR1cm4gTlBQX0hhbmRs ZUV2ZW50KCZ4RXZlbnQpOwo=