[webkit-reviews] review denied: [Bug 32865] [Qt] QWebElement::attribute always returns empty result for input's values : [Attachment 91644] I'm awfully sorry for all these junk files

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied wolfy <wolfram at ritsuka.org>'s
request for review:
Bug 32865: [Qt] QWebElement::attribute always returns empty result for input's
values
https://bugs.webkit.org/show_bug.cgi?id=32865

Attachment 91644: I'm awfully sorry for all these junk files
https://bugs.webkit.org/attachment.cgi?id=91644&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=91644&action=review

> Source/WebKit/qt/Api/qwebelement.cpp:392
> +	  HTMLInputElement* htmlElement =
static_cast<HTMLInputElement*>(m_element);
> +	   if (HTMLNames::valueAttr == name)
> +	     htmlElement->setValue(value, true /* sendChangeEvent */);
> +	   else if (HTMLNames::checkedAttr == name) {
> +	     bool checked = !QString::compare(value,
QString::fromLatin1("checked"), Qt::CaseInsensitive);
> +	     htmlElement->setChecked(checked, true /* sendChangeEvent */);
> +	   }

Invalid indentation.

> Source/WebKit/qt/Api/qwebelement.cpp:428
> +    if (m_element->hasTagName(HTMLNames::inputTag)) {
> +	 HTMLInputElement* htmlElement =
static_cast<HTMLInputElement*>(m_element);
> +	 if (HTMLNames::valueAttr == name)
> +	   return htmlElement->value();
> +	 else if (HTMLNames::checkedAttr == name)
> +	   return htmlElement->checked() ? QString::fromLatin1("checked") :
QString();
> +    }

This is not indented correctly.

> Source/WebKit/qt/ChangeLog:1
> +2011-04-29  wolfy  <wolfram at ritsuka.org>

You should probably put your name in here.

> Source/WebKit/qt/ChangeLog:15
> +	   [Qt] QWebElement::attribute always returns empty result for input's
values
> +	   https://bugs.webkit.org/show_bug.cgi?id=32865

This, the title and url, usually goes above the description, not after.

> Source/WebKit/qt/tests/qwebelement/tst_qwebelement.cpp:1045
> +    // No initial value

You should have a period at the end of sentences. (This one and the other
comments).

> Source/WebKit/qt/tests/qwebelement/tst_qwebelement.cpp:1049
> +    QCOMPARE(inputElement.attribute("value"), QString());

I would think the correct value for inputElement.attribute("value") is empty
but not null.
While a null string would be if there is no value attribute.

> Source/WebKit/qt/tests/qwebelement/tst_qwebelement.cpp:1076
> +    QCOMPARE(inputElement.attribute("value"), QString(""));

Kind of reverse from the comment for the text, I would expect value to be null
in this case, not empty.