[webkit-reviews] review denied: [Bug 29682] [Qt] QWebElement lacks hasFocus and SetFocus apis : [Attachment 40000] Proposed patch, adds setFocus, hasFocus to QWebElement

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has denied Joseph Ligman
<joseph.ligman at nokia.com>'s request for review:
Bug 29682: [Qt] QWebElement lacks hasFocus and SetFocus apis
https://bugs.webkit.org/show_bug.cgi?id=29682

Attachment 40000: Proposed patch, adds setFocus, hasFocus to QWebElement
https://bugs.webkit.org/attachment.cgi?id=40000&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
In principle this looks good. I think only a few small fixes are necessary.


>  /*!
> +    \since 4.6
> +    Returns true if this QWebElement has keyboard input focus; otherwise,
returns false

In the documentation you can simply use "this element" instead of repeating the
class name.
Just like in the other methods of this class :)

> +
> +    \sa setFocus()
> +*/
> +bool QWebElement::hasFocus() const
> +{
> +    if (m_element->document() && m_element->isFocusable())
> +	   return m_element == m_element->document()->focusedNode();
> +    return false;
> +}

Please add a null pointer check for m_element.

> +/*!
> +    \since 4.6
> +    Gives keyboard input focus to this QWebElement
> +
> +    \sa hasFocus()
> +*/
> +void QWebElement::setFocus()
> +{
> +    if (m_element->document() && m_element->isFocusable())
> +	   m_element->document()->setFocusedNode(m_element);
> +}

Same here :)