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

[bugzilla-daemon at webkit.org]

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





--- Comment #11 from Tim Horton <timothy_horton at apple.com>  2013-02-25 12:46:24 PST ---
(In reply to comment #7)
> (From update of attachment 186973 [details])
> 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?

Good call. Not being used anymore.

> > 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.

Interesting! I was just following all the other EventListener subclasses I saw.

> > 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?

Looking at the history of tabKeyCyclesThroughElements (r15093!), I don't really think we care in the PDFPlugin case. Even if it were embedded in another app, if a PDFPlugin form has focus, tab should tab to the next field, not insert a tab (IMO tabKeyCyclesThroughElements only makes sense for contenteditable areas and such, not PDF forms).

> > 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.

Ooh, that's unfortunate. Ok, will fix.

> > 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?

Not in particular, I'll match them.

> > 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?

Nope, makes sense to match them. Will fix.

> Tim, lets fix that stale pointer issue by having ~PDFPluginAnnotation call a function that zeroes the pointer, and make sure you null check. We should do it mechanically and make sure to not have pointer lifetime depend on high level understanding of the code.

Okie.

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