Bug 58503 - Make more functions static local in EventHandlers.cpp
Summary: Make more functions static local in EventHandlers.cpp
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 56410
  Show dependency treegraph
 
Reported: 2011-04-13 18:47 PDT by Ryosuke Niwa
Modified: 2011-06-02 14:41 PDT (History)
8 users (show)

See Also:


Attachments
cleanup (15.74 KB, patch)
2011-04-13 18:54 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixed Windows build (15.78 KB, patch)
2011-04-13 20:25 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
cleanup (14.15 KB, patch)
2011-04-14 09:52 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixed per comments (13.57 KB, patch)
2011-05-01 13:47 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixed a regression (13.85 KB, patch)
2011-05-01 14:09 PDT, Ryosuke Niwa
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2011-04-13 18:47:15 PDT
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.
Comment 1 Ryosuke Niwa 2011-04-13 18:54:02 PDT
Created attachment 89514 [details]
cleanup
Comment 2 Build Bot 2011-04-13 19:27:52 PDT
Attachment 89514 [details] did not build on win:
Build output: http://queues.webkit.org/results/8404480
Comment 3 Ryosuke Niwa 2011-04-13 20:25:31 PDT
Created attachment 89520 [details]
Fixed Windows build
Comment 4 Build Bot 2011-04-13 20:55:42 PDT
Attachment 89520 [details] did not build on win:
Build output: http://queues.webkit.org/results/8400552
Comment 5 Ryosuke Niwa 2011-04-14 09:52:58 PDT
Created attachment 89598 [details]
cleanup
Comment 6 Hajime Morrita 2011-04-14 15:38:32 PDT
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?
Comment 7 Nate Chapin 2011-04-26 15:40:58 PDT
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?
Comment 8 Ryosuke Niwa 2011-04-26 19:56:04 PDT
(In reply to comment #7)
> Also, is it worth putting these in a map?

No.
Comment 9 Ryosuke Niwa 2011-05-01 12:29:34 PDT
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.
Comment 10 Ryosuke Niwa 2011-05-01 13:47:00 PDT
Created attachment 91839 [details]
Fixed per comments
Comment 11 Ryosuke Niwa 2011-05-01 14:09:28 PDT
Created attachment 91842 [details]
Fixed a regression
Comment 12 Eric Seidel (no email) 2011-05-01 14:41:41 PDT
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?
Comment 13 Ryosuke Niwa 2011-05-01 19:07:47 PDT
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.
Comment 14 Ryosuke Niwa 2011-05-02 12:45:48 PDT
(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.
Comment 15 Ryosuke Niwa 2011-05-02 13:08:50 PDT
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.
Comment 16 Luiz Agostini 2011-05-02 16:56:36 PDT
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.
Comment 17 Ryosuke Niwa 2011-05-02 16:58:45 PDT
(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.
Comment 18 Luiz Agostini 2011-05-06 18:50:42 PDT
(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. :)
Comment 19 Brent Fulgham 2011-05-31 20:23:25 PDT
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 ...
Comment 20 Eric Seidel (no email) 2011-06-02 07:54:33 PDT
Comment on attachment 91842 [details]
Fixed a regression

Seems OK.
Comment 21 Ryosuke Niwa 2011-06-02 14:29:51 PDT
Committed r87951: <http://trac.webkit.org/changeset/87951>
Comment 22 Ryosuke Niwa 2011-06-02 14:41:07 PDT
Thanks for the review, landed after fixing the change log entry.