[webkit-reviews] review granted: [Bug 106539] PDFPlugin: Tab between text annotations : [Attachment 186973] patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Feb 25 10:00:52 PST 2013
Darin Adler <darin at apple.com> has granted Tim Horton
<timothy_horton at apple.com>'s request for review:
Bug 106539: PDFPlugin: Tab between text annotations
https://bugs.webkit.org/show_bug.cgi?id=106539
Attachment 186973: patch
https://bugs.webkit.org/attachment.cgi?id=186973&action=review
------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=186973&action=review
review+ but only if you add a check of isKeyboardEvent to fix the bad cast
> Source/WebCore/ChangeLog:9
> + * WebCore.exp.in: Export KeyboardEvent::getModifierState.
Why? I don’t see it being used in this patch. Is it used indirectly?
> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginTextAnnotation.h:63
> + virtual bool operator==(const EventListener& listener) OVERRIDE {
return this == &listener; }
Normally a virtual == operator is an anti-pattern. Not sure if there’s any real
problem with it here.
> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginTextAnnotation.mm:131
> +void
PDFPluginTextAnnotation::PDFPluginTextAnnotationEventListener::handleEvent(Scri
ptExecutionContext*, Event* event)
The code in EventHandler::defaultTabEventHandler checks
Page::tabKeyCyclesThroughElements. Is that something we need to do too?
> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginTextAnnotation.mm:134
> + if (event->type() == eventNames().keydownEvent) {
> + KeyboardEvent* keyboardEvent = static_cast<KeyboardEvent*>(event);
You need to check the type of the event before casting to KeyboardEvent by
calling isKeyboardEvent. A script can create an event with a given event name
that is not a “real” event of that type and if so this will be a bad cast.
> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginTextAnnotation.mm:136
> + if (keyboardEvent->keyCode() == VK_TAB) {
The code in EventHandler::defaultKeyboardEventHandler uses keyIdentifier rather
than keyCode. Any reason you chose differently?
> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginTextAnnotation.mm:138
> + if (keyboardEvent->ctrlKey() || keyboardEvent->metaKey() ||
keyboardEvent->altKey() || keyboardEvent->altGraphKey())
> + return;
The code in EventHandler::defaultTabEventHandler does not check altKey. Any
reason for the difference?
More information about the webkit-reviews
mailing list