[Webkit-unassigned] [Bug 106539] PDFPlugin: Tab between text annotations

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=106539


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #186973|review?                     |review+
               Flag|                            |




--- Comment #7 from Darin Adler <darin at apple.com>  2013-02-25 10:03:16 PST ---
(From update of attachment 186973)
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(ScriptExecutionContext*, 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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.