[webkit-reviews] review denied: [Bug 6665] Implement NSView-less version of <input type="text"> : [Attachment 5774] initial implementation

[bugzilla-request-daemon at opendarwin.org]

Dave Hyatt <hyatt at apple.com> has denied Adele Peterson <adele at apple.com>'s
request for review:
Bug 6665: Implement NSView-less version of <input type="text">
http://bugzilla.opendarwin.org/show_bug.cgi?id=6665

Attachment 5774: initial implementation
http://bugzilla.opendarwin.org/attachment.cgi?id=5774&action=edit

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
(1) Make a note about focusability.  By making the shadow div focusable, the
input field won't have the focus (and so onfocus/blur won't work).  You will
have to do something similar to XBL, making both the div and the input element
focused together (changes in focus that remain inside shadow content don't
affect the focus on the outer element).

(2) Make a note about mouse events as well.  Shadow content cannot be exposed
to the user/author by default, so the engine should internally know when it's
over shadow content but will have to retarget DOM events when it crosses shadow
scope boundaries.

(3) In places where you rely on falling through to a subsequent case statement,
it might be nice to have a "// Fall through" comment to make that clear.

(4) For destroyLeftoverChildren, I'd add a comment along the lines of:

"Destroy any anonymous children remaining in the render tree, as well as
implicit (shadow) DOM elements like those used in the engine-based textfields."


(5) You don't need "using namespace DOM in RenderTextField.cpp, since DOM =
khtml = WebCore now.  Also go ahead and put the header and implementation files
in namespace WebCore rather than khtml, since we're switching to that.

(6) I would actually add a comment to the RenderTextField destructor that the
renderer m_div has already been destroyed by destroyLeftoverChildren.

(7) You do not need to override addChild and removeChild.  An input field
doesn't have child content from the DOM.  I assume you copied that from button,
but you shouldn't need it.

(8) I would change your "getDivStyle" method to be "createDivStyle", since it
always makes a new one.

(9) Single line ifs should not have braces (as in RenderTextField::setStyle).

(10) I'm not sure why button-bevel has been added to cssvalues.in.  I assume
that's a typo.

Looks great!  It's awesome that it's pretty functional without a huge amount of
code!