[webkit-reviews] review granted: [Bug 23433] Add InputElement abstraction : [Attachment 26863] Initial patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jan 20 11:34:44 PST 2009
Adam Roben (aroben) <aroben at apple.com> has granted Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 23433: Add InputElement abstraction
https://bugs.webkit.org/show_bug.cgi?id=23433
Attachment 26863: Initial patch
https://bugs.webkit.org/attachment.cgi?id=26863&action=review
------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
> + A lot of code from HTMLInputElement now lives in InputElement, as
static member functions - the
> + InputElement class itself is an abstract virtual class, just like
ScriptElement. HTML/WMLInputElement
> + derive from InputElement, and hold a InputElementData member
variable, that they pass to the
> + static functions in InputElement. The abstraction is equal to the
one chosen for HTML/SVGScriptElement.
Why do HTML/WMLInputElement hold the InputElementData, rather than InputElement
itself? Is it just from a desire to keep InputElement an abstract base class?
> + * WebCore.xcodeproj/project.pbxproj:
> + * dom/FormControlElement.cpp: Added.
> + (WebCore::formControlElementForElement):
> + * dom/FormControlElement.h:
> + * dom/InputElement.cpp: Added.
> + (WebCore::InputElement::dispatchFocusEvent):
> + (WebCore::InputElement::dispatchBlurEvent):
> + (WebCore::InputElement::updatePlaceholderVisibility):
> + (WebCore::InputElement::updateFocusAppearance):
> + (WebCore::InputElement::updateSelectionRange):
> + (WebCore::InputElement::aboutToUnload):
> + (WebCore::InputElement::setValueFromRenderer):
> + (WebCore::numCharactersInGraphemeClusters):
> + (WebCore::InputElement::constrainValue):
> + (WebCore::numGraphemeClusters):
> + (WebCore::InputElement::handleBeforeTextInsertedEvent):
> + (WebCore::InputElement::parseSizeAttribute):
> + (WebCore::InputElement::parseMaxLengthAttribute):
> + (WebCore::InputElement::updateValueIfNeeded):
> + (WebCore::InputElement::notifyFormStateChanged):
> + (WebCore::InputElementData::InputElementData):
> + (WebCore::InputElementData::~InputElementData):
> + (WebCore::InputElementData::name):
> + (WebCore::inputElementForElement):
> + * dom/InputElement.h: Added.
> + (WebCore::InputElement::~InputElement):
> + (WebCore::InputElement::InputElement):
> + (WebCore::InputElementData::inputElement):
> + (WebCore::InputElementData::element):
> + (WebCore::InputElementData::placeholderShouldBeVisible):
> + (WebCore::InputElementData::setPlaceholderShouldBeVisible):
> + (WebCore::InputElementData::setName):
> + (WebCore::InputElementData::value):
> + (WebCore::InputElementData::setValue):
> + (WebCore::InputElementData::size):
> + (WebCore::InputElementData::setSize):
> + (WebCore::InputElementData::maxLength):
> + (WebCore::InputElementData::setMaxLength):
> + (WebCore::InputElementData::cachedSelectionStart):
> + (WebCore::InputElementData::setCachedSelectionStart):
> + (WebCore::InputElementData::cachedSelectionEnd):
> + (WebCore::InputElementData::setCachedSelectionEnd):
> + * html/HTMLInputElement.cpp:
> + (WebCore::HTMLInputElement::HTMLInputElement):
> + (WebCore::HTMLInputElement::name):
> + (WebCore::HTMLInputElement::updateFocusAppearance):
> + (WebCore::HTMLInputElement::aboutToUnload):
> + (WebCore::HTMLInputElement::dispatchFocusEvent):
> + (WebCore::HTMLInputElement::dispatchBlurEvent):
> + (WebCore::HTMLInputElement::setInputType):
> + (WebCore::HTMLInputElement::selectionStart):
> + (WebCore::HTMLInputElement::selectionEnd):
> + (WebCore::HTMLInputElement::setSelectionRange):
> + (WebCore::HTMLInputElement::parseMappedAttribute):
> + (WebCore::HTMLInputElement::appendFormData):
> + (WebCore::HTMLInputElement::size):
> + (WebCore::HTMLInputElement::copyNonAttributeProperties):
> + (WebCore::HTMLInputElement::value):
> + (WebCore::HTMLInputElement::setValue):
> + (WebCore::HTMLInputElement::placeholderValue):
> + (WebCore::HTMLInputElement::searchEventsShouldBeDispatched):
> + (WebCore::HTMLInputElement::setValueFromRenderer):
> + (WebCore::HTMLInputElement::setFileListFromRenderer):
> + (WebCore::HTMLInputElement::defaultEventHandler):
> + (WebCore::HTMLInputElement::setDefaultName):
> + (WebCore::HTMLInputElement::maxLength):
> + (WebCore::HTMLInputElement::constrainValue):
> + (WebCore::HTMLInputElement::cacheSelection):
> + (WebCore::HTMLInputElement::selection):
> + (WebCore::HTMLInputElement::placeholderShouldBeVisible):
> + * html/HTMLInputElement.h:
> + (WebCore::HTMLInputElement::isTextField):
> + (WebCore::HTMLInputElement::isSearchField):
> + (WebCore::HTMLInputElement::isAutofilled):
> + * html/HTMLIsIndexElement.cpp:
> + (WebCore::HTMLIsIndexElement::HTMLIsIndexElement):
> + * rendering/RenderTextControl.cpp:
> + (WebCore::RenderTextControl::formControlElement):
> + * rendering/RenderTextControlSingleLine.cpp:
> + (WebCore::RenderTextControlSingleLine::placeholderShouldBeVisible):
> + (WebCore::RenderTextControlSingleLine::addSearchResult):
> + (WebCore::RenderTextControlSingleLine::stopSearchEventTimer):
> + (WebCore::RenderTextControlSingleLine::showPopup):
> + (WebCore::RenderTextControlSingleLine::hidePopup):
> + (WebCore::RenderTextControlSingleLine::subtreeHasChanged):
> + (WebCore::RenderTextControlSingleLine::capsLockStateMayHaveChanged):
> + (WebCore::RenderTextControlSingleLine::preferredContentWidth):
> + (WebCore::RenderTextControlSingleLine::createSubtreeIfNeeded):
> + (WebCore::RenderTextControlSingleLine::updateFromElement):
> + (WebCore::RenderTextControlSingleLine::cacheSelection):
> + (WebCore::RenderTextControlSingleLine::createInnerBlockStyle):
> + (WebCore::RenderTextControlSingleLine::createResultsButtonStyle):
> + (WebCore::RenderTextControlSingleLine::createCancelButtonStyle):
> +
(WebCore::RenderTextControlSingleLine::updateCancelButtonVisibility):
> + (WebCore::RenderTextControlSingleLine::startSearchEventTimer):
> + (WebCore::RenderTextControlSingleLine::searchEventTimerFired):
> + (WebCore::RenderTextControlSingleLine::valueChanged):
> + (WebCore::RenderTextControlSingleLine::setTextFromItem):
> + (WebCore::RenderTextControlSingleLine::inputElement):
> + * rendering/RenderTextControlSingleLine.h:
It would be useful to have function-level comments explaining how the code was
moved around.
> +void InputElement::dispatchFocusEvent(InputElementData& data, Document*
document)
> +{
> + if (!data.inputElement()->isTextField())
> + return;
> +
> + InputElement::updatePlaceholderVisibility(data, document);
I don't think "InputElement::" is needed here or in the other member functions
in this file.
> +void InputElement::dispatchBlurEvent(InputElementData& data, Document*
document)
> +{
> + if (!data.inputElement()->isTextField())
> + return;
> +
> + if (Frame* frame = document->frame()) {
You could make this an early return to remove some nesting.
> +void InputElement::updatePlaceholderVisibility(InputElementData& data,
Document* document, bool placeholderValueChanged)
> +{
> + ASSERT(data.inputElement()->isTextField());
> +
> + bool oldPlaceholderShouldBeVisible = data.placeholderShouldBeVisible();
> + Element* element = data.element();
> +
> +
data.setPlaceholderShouldBeVisible(data.inputElement()->value().isEmpty()
> + && document->focusedNode() != element
> + &&
!data.inputElement()->placeholderValue().isEmpty());
> +
> + if ((oldPlaceholderShouldBeVisible != data.placeholderShouldBeVisible()
|| placeholderValueChanged) && element->renderer())
> +
static_cast<RenderTextControlSingleLine*>(element->renderer())->updatePlacehold
erVisibility();
What guarantees that element->renderer() will be a RenderTextControlSingleLine?
> +void InputElement::updateSelectionRange(InputElementData& data, int start,
int end)
> +{
> + if (!data.inputElement()->isTextField())
> + return;
> +
> + if (RenderTextControl* renderer =
static_cast<RenderTextControl*>(data.element()->renderer()))
What guarantees that data.element()->renderer() is a RenderTextControl?
> +void InputElement::setValueFromRenderer(InputElementData& data, Document*
document, const String& value)
> +{
> + // Renderer and our event handler are responsible for constraining
values.
> + ASSERT(value == data.inputElement()->constrainValue(value) ||
data.inputElement()->constrainValue(value).isEmpty());
> +
> + if (data.inputElement()->isTextField())
> + InputElement::updatePlaceholderVisibility(data, document);
> +
> + // Workaround for bug where trailing \n is included in the result of
textContent.
> + // The assert macro above may also be simplified to: value ==
constrainValue(value)
> + // http://bugs.webkit.org/show_bug.cgi?id=9661
> + if (value == "\n")
> + data.setValue("");
> + else
> + data.setValue(value);
> +
> + Element* element = data.element();
> + formControlElementForElement(element)->setValueMatchesRenderer();
Should we assert that formControlElementForElement returns a non-null value?
> +String InputElement::constrainValue(const InputElementData& data, const
String& proposedValue, int maxLength)
> +{
> + String string = proposedValue;
> + if (!data.inputElement()->isTextField())
> + return string;
> +
> + string.replace("\r\n", " ");
> + string.replace('\r', ' ');
> + string.replace('\n', ' ');
> +
> + StringImpl* s = string.impl();
> + int newLength = numCharactersInGraphemeClusters(s, maxLength);
> + for (int i = 0; i < newLength; ++i) {
> + const UChar& current = (*s)[i];
> + if (current < ' ' && current != '\t') {
> + newLength = i;
> + break;
> + }
> + }
> +
> + if (newLength < static_cast<int>(string.length()))
> + return string.substring(0, newLength);
I believe String::left could be used here, but I know this isn't new code.
> +void InputElement::parseMaxLengthAttribute(InputElementData& data,
MappedAttribute* attribute)
> +{
> + int maxLength = attribute->isNull() ? InputElement::s_maximumLength :
attribute->value().toInt();
> + if (maxLength <= 0 || maxLength > InputElement::s_maximumLength)
> + maxLength = InputElement::s_maximumLength;
Often we do this kind of clamping using max() and min(): maxLength = max(0,
min(maxLength, InputElement::s_maximumLength))
> @@ -348,9 +353,6 @@ void RenderTextControlSingleLine::styleD
>
> void RenderTextControlSingleLine::capsLockStateMayHaveChanged()
> {
> - if (!node() || !document())
> - return;
> -
Why remove these null-checks?
r=me, but please consider these comments.
More information about the webkit-reviews
mailing list