Bug 58503 – Make more functions static local in EventHandlers.cpp

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

Some functions in EventHandler.cpp do not need to be member functions. Since EventHandler is implemented in two files for each port, we should localize functions as much as possible to reduce the dependencies between the two.

[Ryosuke Niwa]

Created attachment 89514 [details]
cleanup

[Build Bot]

Attachment 89514 [details] did not build on win:
Build output: http://queues.webkit.org/results/8404480

[Ryosuke Niwa]

Created attachment 89520 [details]
Fixed Windows build

[Build Bot]

Attachment 89520 [details] did not build on win:
Build output: http://queues.webkit.org/results/8400552

[Ryosuke Niwa]

Created attachment 89598 [details]
cleanup

[Hajime Morrita]

Comment on attachment 89598 [details]
cleanup

View in context: https://bugs.webkit.org/attachment.cgi?id=89598&action=review

> Source/WebCore/page/EventHandler.cpp:-1726
> -    bool canHandle = false;

"accepted" should be always assigned. (as original did.)

> Source/WebCore/page/EventHandler.cpp:1822
> +    bool accepted;

Always assigning the initial value is good idea in general, Especially the argument is not pointer 
At a glance, it is not clear if this code is correct because readers don't know "accept" is an output variable or not.

> Source/WebCore/page/EventHandler.cpp:2544
> +    SelectionDirection direction = DirectionForward;

It might be possible to extract following switch statement into a conversion function like as focusDirectionForKey().

> Source/WebCore/page/EventHandler.cpp:2568
> +

This code always calls modify(), which looks different from original. Is it intentional?

[Nate Chapin]

Comment on attachment 89598 [details]
cleanup

View in context: https://bugs.webkit.org/attachment.cgi?id=89598&action=review

r- based mostly on the previous comments

> Source/WebCore/page/EventHandler.cpp:2519
> +    DEFINE_STATIC_LOCAL(AtomicString, Down, ("Down"));
> +    DEFINE_STATIC_LOCAL(AtomicString, Up, ("Up"));
> +    DEFINE_STATIC_LOCAL(AtomicString, Left, ("Left"));
> +    DEFINE_STATIC_LOCAL(AtomicString, Right, ("Right"));

I know you're just moving this code, but these variable names should be lowercase.

Also, is it worth putting these in a map?

[Ryosuke Niwa]

(In reply to comment #7)
> Also, is it worth putting these in a map?

No.

[Ryosuke Niwa]

Comment on attachment 89598 [details]
cleanup

View in context: https://bugs.webkit.org/attachment.cgi?id=89598&action=review

>> Source/WebCore/page/EventHandler.cpp:1822

> 
> Always assigning the initial value is good idea in general, Especially the argument is not pointer 
> At a glance, it is not clear if this code is correct because readers don't know "accept" is an output variable or not.

Okay but compiler will give you a warning if I didn't initialize a variable and passed it into a function by value.

>> Source/WebCore/page/EventHandler.cpp:2568
>> +
> 
> This code always calls modify(), which looks different from original. Is it intentional?

There's no behavioral change.

[Ryosuke Niwa]

Created attachment 91839 [details]
Fixed per comments

[Ryosuke Niwa]

Created attachment 91842 [details]
Fixed a regression

[Eric Seidel (no email)]

Comment on attachment 91842 [details]
Fixed a regression

View in context: https://bugs.webkit.org/attachment.cgi?id=91842&action=review

> Source/WebCore/page/EventHandler.cpp:1783
> +        if (targetIsFrame(newTarget, targetFrame)) {
> +            if (targetFrame)

The case where targetIsFrame returns true and yet targetFrame is 0 seems odd.  Do we test that case?

[Ryosuke Niwa]

Comment on attachment 91842 [details]
Fixed a regression

View in context: https://bugs.webkit.org/attachment.cgi?id=91842&action=review

>> Source/WebCore/page/EventHandler.cpp:1783
>> +            if (targetFrame)
> 
> The case where targetIsFrame returns true and yet targetFrame is 0 seems odd.  Do we test that case?

How about when iframe doesn't have src attribute?  I'm certain this will occur based on the code in HTMLFrameElementBase and the original code here.

[Ryosuke Niwa]

(In reply to comment #13)
> How about when iframe doesn't have src attribute?  I'm certain this will occur based on the code in HTMLFrameElementBase and the original code here.

This still creates the content frame.  The only case I can think of is when a frame is in construction or destruction but it's extremely hard to test this.

[Ryosuke Niwa]

Comment on attachment 91842 [details]
Fixed a regression

View in context: https://bugs.webkit.org/attachment.cgi?id=91842&action=review

>>> Source/WebCore/page/EventHandler.cpp:1783
>>> +            if (targetFrame)
>> 
>> The case where targetIsFrame returns true and yet targetFrame is 0 seems odd.  Do we test that case?
> 
> How about when iframe doesn't have src attribute?  I'm certain this will occur based on the code in HTMLFrameElementBase and the original code here.

editing/pasteboard/drag-drop-dead-frame.html crashes without this if statement.

[Luiz Agostini]

Comment on attachment 91842 [details]
Fixed a regression

View in context: https://bugs.webkit.org/attachment.cgi?id=91842&action=review

LGTM.

> Source/WebCore/ChangeLog:17
> +        Removed canHandleDragAndDropForTarget and made focusDirectionForKey local to EventHandler.cpp.
> +
> +        * page/EventHandler.cpp:
> +        (WebCore::targetIsFrame): Extracted from canHandleDragAndDropForTarget.
> +        (WebCore::EventHandler::updateDragAndDrop): Calls contentFrameForTarget instead of canHandleDragAndDropForTarget.
> +        (WebCore::EventHandler::cancelDragAndDrop): Ditto.
> +        (WebCore::EventHandler::performDragAndDrop): Ditto.
> +        (WebCore::focusDirectionForKey): No longer a member function of EventHandler class.
> +        (WebCore::EventHandler::handleKeyboardSelectionMovement): Calls focusDirectionForKey instead of manually comparing
> +        strings.

You did not mention in the change log that handleKeyboardSelectionMovement was made local to EventHandler.cpp and is no longer a member of class EventHandler.

[Ryosuke Niwa]

(In reply to comment #16)
> (From update of attachment 91842 [details])
> You did not mention in the change log that handleKeyboardSelectionMovement was made local to EventHandler.cpp and is no longer a member of class EventHandler.

Oh, that's a good point.  Will fix it before landing.

[Luiz Agostini]

(In reply to comment #17)
> (In reply to comment #16)
> > (From update of attachment 91842 [details] [details])
> > You did not mention in the change log that handleKeyboardSelectionMovement was made local to EventHandler.cpp and is no longer a member of class EventHandler.
> 
> Oh, that's a good point.  Will fix it before landing.

Well, I am not an official reviewer. I should have mentioned it in my last comment.
I hope I have not disturbed too much. :)

[Brent Fulgham]

Comment on attachment 91842 [details]
Fixed a regression

View in context: https://bugs.webkit.org/attachment.cgi?id=91842&action=review

> Source/WebCore/page/EventHandler.cpp:1745
> +static bool targetIsFrame(Node* target, Frame*& frame)

What if 'targetIsFrame' returned a Frame*?  Then the test for "isFrame" and null could be one and the same.  Do we ever have a case where the target is NOT a frame, and yet we get a valid Frame*?

Then calls like:
if (targetIsFrame(newTarget, targetFrame))
    if (targetFrame) {
        // ... do stuff ...

Would become:
Frame* frame = targetIsFrame(newTarget);
if (frame) {
    // ... do stuff ...

[Eric Seidel (no email)]

Comment on attachment 91842 [details]
Fixed a regression

Seems OK.

[Ryosuke Niwa]

Committed r87951: <http://trac.webkit.org/changeset/87951>

[Ryosuke Niwa]

Thanks for the review, landed after fixing the change log entry.