[webkit-reviews] review denied: [Bug 122431] Move FocusController from Page to MainFrame : [Attachment 213541] Patch

[bugzilla-daemon at webkit.org]

Andreas Kling <akling at apple.com> has denied Darin Adler <darin at apple.com>'s
request for review:
Bug 122431: Move FocusController from Page to MainFrame
https://bugs.webkit.org/show_bug.cgi?id=122431

Attachment 213541: Patch
https://bugs.webkit.org/attachment.cgi?id=213541&action=review

------- Additional Comments from Andreas Kling <akling at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=213541&action=review


> Source/WebCore/dom/Element.cpp:1959
> +	   if (!frame->mainFrame().focus().setFocusedElement(this,
*document().frame(), direction))

*document().frame() -> *frame

> Source/WebCore/page/EventHandler.cpp:3319
> -	   bool changedFocusedFrame = m_frame.page() && &m_frame !=
&m_frame.page()->focusController().focusedOrMainFrame();
> +	   bool changedFocusedFrame = m_frame.page() && &m_frame !=
&m_frame.mainFrame().focus().focusedOrMainFrame();

I think we can lose the m_frame.page() check here. I suspect it was just there
due to pointer paranoia.

> Source/WebCore/page/EventHandler.cpp:3341
> -    bool changedFocusedFrame = m_frame.page() && &m_frame !=
&m_frame.page()->focusController().focusedOrMainFrame();
> +    bool changedFocusedFrame = m_frame.page() && &m_frame !=
&m_frame.mainFrame().focus().focusedOrMainFrame();

Ditto.

> Source/WebCore/page/FocusController.cpp:200
> -    m_focusedFrame = newFrame;
> +    if (oldFrame != &m_mainFrame)
> +	   oldFrame->deref();
> +    if (newFrame != &m_mainFrame)
> +	   newFrame->ref();
> +    m_focusedFrame = newFrame.get();

This could probably use a comment.

> Source/WebCore/page/FocusController.cpp:357
> +	   document.setFocusedElement(0);

nullptr

> Source/WebCore/page/FocusController.cpp:429
> +// @param start The node from which to start searching. The node after this
will be focused. May be null.
> +//
> +// @return The focus node that comes after/before start node.

I know you are just moving them, but these comments sure look out-of-place in
WebKit.

> Source/WebCore/page/FocusController.cpp:784
> +    Node* focusedNode = (m_focusedFrame && m_focusedFrame->document()) ?
m_focusedFrame->document()->focusedElement() : 0;

Looks like this should be an Element*.

> Source/WebCore/page/FocusController.h:86
> +    Frame* m_focusedFrame;

Bug! This is missing from the constructor initializer list.