[webkit-reviews] review denied: [Bug 34721] [Chromium] Refactor AutocompletePopupMenuClient into a base class, SuggestionsPopupMenuClient, and two derived classes, AutocompletePopupMenuClient and AutoFillPopupMenuClient : [Attachment 48371] [Chromium] Refactor AutocompletePopupMenuClient [try4]

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied James Hawkins
<jhawkins at google.com>'s request for review:
Bug 34721: [Chromium] Refactor AutocompletePopupMenuClient into a base class,
SuggestionsPopupMenuClient, and two derived classes,
AutocompletePopupMenuClient and AutoFillPopupMenuClient
https://bugs.webkit.org/show_bug.cgi?id=34721

Attachment 48371: [Chromium] Refactor AutocompletePopupMenuClient [try4]
https://bugs.webkit.org/attachment.cgi?id=48371&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> Index: WebKit/chromium/src/AutocompletePopupMenuClient.h
> Index: WebKit/chromium/src/SuggestionsPopupMenuClient.cpp
...
> +// FIXME: Make sure we need all of these includes.
> +#include "CSSStyleSelector.h"
> +#include "CSSValueKeywords.h"
> +#include "Chrome.h"
> +#include "FrameView.h"
> +#include "HTMLInputElement.h"
> +#include "RenderTheme.h"
> +#include "WebVector.h"
> +#include "WebViewImpl.h"

How about processing that FIXME?


> +void SuggestionsPopupMenuClient::selectionChanged(unsigned listIndex, bool
fireEvents)
> +{
> +    if (listIndex != static_cast<unsigned>(-1)) {
> +	   const WebString& suggestion = getSuggestion(listIndex);
> +	   setSuggestedValue(suggestion);
> +    } else {
> +	 m_textField->setValue(m_typedFieldValue);
> +	 if (m_lastFieldValues->contains(m_textField->name()))
> +	     m_lastFieldValues->set(m_textField->name(), m_typedFieldValue);
> +	 else
> +	     m_lastFieldValues->add(m_textField->name(), m_typedFieldValue);
> +    }
> +}

^^^ bad indentation


> +    if (!hasPreviousValue || m_typedFieldValue.length() >
m_lastFieldValues->get(m_textField->name()).length()) {
> +	     if (suggestion.startsWith(m_typedFieldValue))
> +		 m_selectedIndex = newIndex;

^^^ bad indentation


> Index: WebKit/chromium/src/SuggestionsPopupMenuClient.h
...
> +template <typename T> class WebVector;
> +
> +// 
> +class SuggestionsPopupMenuClient : public WebCore::PopupMenuClient {

^^^ same nit as before about the spurious '//' without any comments.


What about my question re: "WebViewImpl* m_webView" from before?