Summary: | [Qt] QWebElement lacks hasFocus and SetFocus apis | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joseph Ligman <joseph.ligman> | ||||||
Component: | WebKit Qt | Assignee: | Joseph Ligman <joseph.ligman> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Enhancement | CC: | commit-queue, eric, kenneth, laszlo.gombos, tonikitoo, vestbo | ||||||
Priority: | P2 | Keywords: | Qt | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | PC | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Joseph Ligman
2009-09-23 07:00:35 PDT
Created attachment 40000 [details]
Proposed patch, adds setFocus, hasFocus to QWebElement
This patch adds setFocus and hasFocus Api to QWebElement. And a test case.
good stuff Are these standard Qt apis? Please explain (ideally in the ChangeLog) why you're adding these, and why this particular API makes sense. Comment on attachment 40000 [details] Proposed patch, adds setFocus, hasFocus to QWebElement 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 :)
> > /*!
> > + \since 4.6
> > + Returns true if this QWebElement has keyboard input focus; otherwise, returns false
>
The QWebElement class is new in 4.6 so I believe we should leave out the \since 4.6
Created attachment 40127 [details]
Proposed patch, adds API setFocus, hasFocus to QWebElement
Fixed the comments and added the null pointer check for m_element
Comment on attachment 40127 [details]
Proposed patch, adds API setFocus, hasFocus to QWebElement
r=me, thanks!
Comment on attachment 40127 [details] Proposed patch, adds API setFocus, hasFocus to QWebElement Clearing flags on attachment: 40127 Committed r48805: <http://trac.webkit.org/changeset/48805> All reviewed patches have been landed. Closing bug. |