Bug 58503

Created attachment 89514 [details] cleanup

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

Created attachment 89520 [details] Fixed Windows build

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

Created attachment 89598 [details] cleanup

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

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

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.

Created attachment 91839 [details] Fixed per comments

Created attachment 91842 [details] Fixed a regression

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

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

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

(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 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 on attachment 91842 [details] Fixed a regression Seems OK.

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

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