[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