[webkit-reviews] review denied: [Bug 47980] Add the feature "Add as search engine..." in a search text field context menu for chromium : [Attachment 72654] Proposed Patch V6

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied
philippe.beauchamp at gmail.com's request for review:
Bug 47980: Add the feature "Add as search engine..." in a search text field
context menu for chromium
https://bugs.webkit.org/show_bug.cgi?id=47980

Attachment 72654: Proposed Patch V6
https://bugs.webkit.org/attachment.cgi?id=72654&action=review

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=72654&action=review

> WebKit/chromium/public/WebSearchableFormData.h:48
> +    WEBKIT_API WebSearchableFormData(const WebFormElement&, const
WebCore::HTMLInputElement* selectedInputElement = 0);

Please don't pass WebCore types into WebKit API. That's what
WebInputElement/WebElement wrappers are for.

> WebKit/chromium/src/ContextMenuClientImpl.cpp:253
> +	   WebCore::HTMLInputElement* selectedElement =
static_cast<WebCore::HTMLInputElement*>(r.innerNonSharedNode());

What if it's not an HTMLInputElement?

> WebKit/chromium/src/WebSearchableFormData.cpp:147
> +WebCore::HTMLInputElement* FindSuitableTextElement(const HTMLFormElement*
form)

Why do we have Capitalized methods in WebKit? That's not WebKit style
http://webkit.org/coding/coding-style.html

> WebKit/chromium/src/WebSearchableFormData.cpp:187
> +bool BuildSearchString(const HTMLFormElement* form, Vector<char>*
encodedString, TextEncoding* encoding, const WebCore::HTMLInputElement*
textElement)

Ditto.

> WebKit/chromium/src/WebSearchableFormData.cpp:222
> +{
> +    bool isElementFound = false;   
> +
> +    // FIXME: Consider refactoring this code so that we don't call
form->associatedElements() twice.
> +    for (Vector<HTMLFormControlElement*>::const_iterator
i(form->associatedElements().begin()); i != form->associatedElements().end();
++i) {
> +	   HTMLFormControlElement* formElement = *i;
> +	   if (formElement->disabled() || formElement->name().isNull())
> +	       continue;
> +
> +	   FormDataList dataList(*encoding);
> +	   if (!formElement->appendFormData(dataList, false))
> +	       continue;
> +
> +	   const Vector<FormDataList::Item>& items = dataList.items();
> +
> +	   for (Vector<FormDataList::Item>::const_iterator j(items.begin()); j
!= items.end(); ++j) {
> +	       // Handle ISINDEX / <input name=isindex> specially, but only if
it's
> +	       // the first entry.
> +	       if (!encodedString->isEmpty() || j->data() != "isindex") {
> +		   if (!encodedString->isEmpty())
> +		       encodedString->append('&');
> +		   FormDataBuilder::encodeStringAsFormData(*encodedString,
j->data());
> +		   encodedString->append('=');
> +	       }
> +	       ++j;
> +	       if (formElement == textElement) {
> +		   encodedString->append("{searchTerms}", 13);
> +		   isElementFound = true;
> +	       } else {
> +		   FormDataBuilder::encodeStringAsFormData(*encodedString,
j->data());
> +	       }
> +	   }
> +    }
> +    return isElementFound;
> +}

Can this somehow not be a mostly duplication of FormData logic?

> WebKit/chromium/src/WebSearchableFormData.cpp:284
> +    m_encoding = (String) encoding.name();

Whoa. Why are we casting things here like that?