Bug 34721 – [Chromium] Refactor AutocompletePopupMenuClient into a base class, SuggestionsPopupMenuClient, and two derived classes, AutocompletePopupMenuClient and AutoFillPopupMenuClient

• Component: WebKit API
• Status: RESOLVED FIXED

[James Hawkins]

The Chromium AutoFill implementation needs to override the behavior of the suggestions popup.  This initial version just appends the profile label to the suggestion, but eventually we'll change this to add a right-justified, gray label.  Attachment will follow.

[James Hawkins]

Created attachment 48358 [details]
[Chromium] Refactor AutocompletePopupMenuClient

[WebKit Review Bot]

Attachment 48358 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/src/WebViewImpl.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/chromium/src/WebViewImpl.cpp:1596:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebKit/chromium/src/SuggestionsPopupMenuClient.h:116:  One space before end of line comments  [whitespace/comments] [5]
WebKit/chromium/src/SuggestionsPopupMenuClient.cpp:48:  Use 0 instead of NULL.  [readability/null] [5]
WebKit/chromium/src/SuggestionsPopupMenuClient.cpp:241:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/chromium/src/SuggestionsPopupMenuClient.cpp:242:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/chromium/src/AutoFillPopupMenuClient.cpp:57:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 7


If any of these errors are false positives, please file a bug against check-webkit-style.

[James Hawkins]

Created attachment 48359 [details]
[Chromium] Refactor AutocompletePopupMenuClient [try2]

Style fixes.

[Darin Fisher (:fishd, Google)]

Comment on attachment 48359 [details]
[Chromium] Refactor AutocompletePopupMenuClient [try2]

> Index: WebKit/chromium/ChangeLog
...
> +        * src/AutoFillPopupMenuClient.cpp: Added.
> +        (WebKit::AutoFillPopupMenuClient::AutoFillPopupMenuClient):
> +        (WebKit::AutoFillPopupMenuClient::~AutoFillPopupMenuClient):
> +        (WebKit::AutoFillPopupMenuClient::getSuggestionsSize):
> +        (WebKit::AutoFillPopupMenuClient::getSuggestion):
> +        (WebKit::AutoFillPopupMenuClient::removeSuggestionAtIndex):
> +        (WebKit::selectionChanged):
> +        (WebKit::AutoFillPopupMenuClient::initialize):
> +        (WebKit::AutoFillPopupMenuClient::setSuggestions):

^^^ nit: when adding a new file, it is considered good form to hand edit
the ChangeLog to remove the list of functions being added.


> Index: WebKit/chromium/public/WebView.h
> ===================================================================
> --- WebKit/chromium/public/WebView.h	(revision 54501)
> +++ WebKit/chromium/public/WebView.h	(working copy)
> @@ -225,12 +225,27 @@ public:
>  
>      // Autofill ------------------------------------------------------------

^^^ Autofill / autocomplete


> Index: WebKit/chromium/src/AutoFillPopupMenuClient.cpp
...
> +WebString AutoFillPopupMenuClient::getSuggestion(unsigned listIndex) const
> +{
> +    // TODO(jhawkins): Modify the PopupMenu to add the label in gray

nit: TODO(jhawkins) -> FIXME


> +    // right-justified.
> +    ASSERT(listIndex >= 0 && listIndex < m_names.size());
> +    return m_names[listIndex] + WebString(" (") + m_labels[listIndex] + WebString(")");

^^^ Please use String instead of WebString here.  Otherwise, code is being
wastefully generated to convert the WebString to a String in order to perform
the concatenation.


> +void AutoFillPopupMenuClient::removeSuggestionAtIndex(unsigned listIndex)
> +{
> +    // TODO(jhawkins): Do we want to remove AutoFill suggestions?

same nit about TODO


> Index: WebKit/chromium/src/AutoFillPopupMenuClient.h
...
> +// 

^^^ errant comment, or did you intend to write something about this class?

> +class AutoFillPopupMenuClient : public SuggestionsPopupMenuClient {
> +public:
> +    AutoFillPopupMenuClient(WebViewImpl* webView);

nit: please leave off the parameter name when it is just a variation
on the type name.  i.e., if it doesn't add any information, then just
leave it off.


> Index: WebKit/chromium/src/AutocompletePopupMenuClient.h

> +    // SuggestionsPopupMenuClient implementation:
> +    virtual size_t getSuggestionsSize() const;
> +    virtual WebString getSuggestion(unsigned listIndex) const;
> +    virtual void removeSuggestionAtIndex(unsigned listIndex);
>  
>      void initialize(WebCore::HTMLInputElement*,
>                      const WebVector<WebString>& suggestions,
>                      int defaultSuggestionIndex);
>  
> -    WebCore::HTMLInputElement* textField() const { return m_textField.get(); }
> -
>      void setSuggestions(const WebVector<WebString>&);
> -    void removeItemAtIndex(int index);
> -
> -    // WebCore::PopupMenuClient methods:
> -    virtual void valueChanged(unsigned listIndex, bool fireEvents = true);
> -    virtual WebCore::String itemText(unsigned listIndex) const;
> -    virtual WebCore::String itemToolTip(unsigned lastIndex) const { return WebCore::String(); }
> -    virtual bool itemIsEnabled(unsigned listIndex) const { return true; }
> -    virtual WebCore::PopupMenuStyle itemStyle(unsigned listIndex) const;
> -    virtual WebCore::PopupMenuStyle menuStyle() const;
> -    virtual int clientInsetLeft() const { return 0; }
> -    virtual int clientInsetRight() const { return 0; }
> -    virtual int clientPaddingLeft() const;
> -    virtual int clientPaddingRight() const;
> -    virtual int listSize() const { return m_suggestions.size(); }
> -    virtual int selectedIndex() const { return m_selectedIndex; }
> -    virtual void popupDidHide();
> -    virtual bool itemIsSeparator(unsigned listIndex) const { return false; }
> -    virtual bool itemIsLabel(unsigned listIndex) const { return false; }
> -    virtual bool itemIsSelected(unsigned listIndex) const { return false; }
> -    virtual bool shouldPopOver() const { return false; }
> -    virtual bool valueShouldChangeOnHotTrack() const { return false; }
> -    virtual void setTextFromItem(unsigned listIndex);
> -    virtual WebCore::FontSelector* fontSelector() const;
> -    virtual WebCore::HostWindow* hostWindow() const;
> -    virtual PassRefPtr<WebCore::Scrollbar> createScrollbar(
> -        WebCore::ScrollbarClient* client,
> -        WebCore::ScrollbarOrientation orientation,
> -        WebCore::ScrollbarControlSize size);
>  
>  private:
> -    WebCore::RenderStyle* textFieldStyle() const;
> -
> -    RefPtr<WebCore::HTMLInputElement> m_textField;
>      Vector<WebCore::String> m_suggestions;
> -    int m_selectedIndex;
> -    WebViewImpl* m_webView;
> -    OwnPtr<WebCore::PopupMenuStyle> m_style;
>  };
>  
>  } // namespace WebKit
> +
> +#endif
> Index: WebKit/chromium/src/SuggestionsPopupMenuClient.cpp
> ===================================================================
> --- WebKit/chromium/src/SuggestionsPopupMenuClient.cpp	(revision 0)
> +++ WebKit/chromium/src/SuggestionsPopupMenuClient.cpp	(revision 0)
> @@ -0,0 +1,246 @@
> +/*
> + * Copyright (C) 2010 Google Inc. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are
> + * met:
> + *
> + *     * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following disclaimer
> + * in the documentation and/or other materials provided with the
> + * distribution.
> + *     * Neither the name of Google Inc. nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#include "config.h"
> +#include "SuggestionsPopupMenuClient.h"
> +
> +// TODO(jhawkins): Make sure we need all of these includes.
> +#include "CSSStyleSelector.h"
> +#include "CSSValueKeywords.h"
> +#include "FrameView.h"
> +#include "HTMLInputElement.h"
> +#include "RenderTheme.h"
> +#include "WebVector.h"
> +#include "WebViewImpl.h"
> +
> +using namespace WebCore;
> +
> +namespace WebKit {
> +
> +SuggestionsPopupMenuClient::SuggestionsPopupMenuClient(WebViewImpl* webView)
> +    : m_textField(0)
> +    , m_selectedIndex(0)
> +    , m_webView(webView)
> +{
> +}
> +
> +SuggestionsPopupMenuClient::~SuggestionsPopupMenuClient()
> +{
> +}
> +
> +// TODO(jhawkins): Implement this per child class?
> +void SuggestionsPopupMenuClient::valueChanged(unsigned listIndex, bool fireEvents)
> +{
> +    m_textField->setValue(getSuggestion(listIndex));
> +
> +    EditorClientImpl* editor =
> +        static_cast<EditorClientImpl*>(m_webView->page()->editorClient());
> +    ASSERT(editor);
> +    editor->onAutofillSuggestionAccepted(
> +        static_cast<HTMLInputElement*>(m_textField.get()));
> +}
> +
> +void SuggestionsPopupMenuClient::selectionChanged(unsigned listIndex, bool fireEvents)
> +{
> +    printf("selectionChanged\n");
> +    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);
> +    }
> +}
> +
> +String SuggestionsPopupMenuClient::itemText(unsigned listIndex) const
> +{
> +    return getSuggestion(listIndex);
> +}
> +
> +PopupMenuStyle SuggestionsPopupMenuClient::itemStyle(unsigned listIndex) const
> +{
> +    return *m_style;
> +}
> +
> +PopupMenuStyle SuggestionsPopupMenuClient::menuStyle() const
> +{
> +    return *m_style;
> +}
> +
> +int SuggestionsPopupMenuClient::clientPaddingLeft() const
> +{
> +    // Bug http://crbug.com/7708 seems to indicate the style can be 0.
> +    RenderStyle* style = textFieldStyle();
> +    return style ? m_webView->theme()->popupInternalPaddingLeft(style) : 0;
> +}
> +
> +int SuggestionsPopupMenuClient::clientPaddingRight() const
> +{
> +    // Bug http://crbug.com/7708 seems to indicate the style can be 0.
> +    RenderStyle* style = textFieldStyle();
> +    return style ? m_webView->theme()->popupInternalPaddingRight(style) : 0;
> +}
> +
> +void SuggestionsPopupMenuClient::popupDidHide()
> +{
> +#if 0
> +    if (acceptSuggestions) {
> +        String suggestedValue = m_textField->suggestedValue();
> +        if (!suggestedValue.isNull())
> +            m_textField->setValue(suggestedValue);
> +    } else
> +#endif
> +        m_textField->setValue(m_typedFieldValue);
> +
> +    resetLastSuggestion();
> +    m_webView->suggestionsPopupDidHide();
> +}
> +
> +void SuggestionsPopupMenuClient::setTextFromItem(unsigned listIndex)
> +{
> +    m_textField->setValue(getSuggestion(listIndex));
> +    resetLastSuggestion();
> +}
> +
> +void SuggestionsPopupMenuClient::resetLastSuggestion()
> +{
> +    if (m_lastFieldValues->contains(m_textField->name()))
> +        m_lastFieldValues->set(m_textField->name(), m_textField->value());
> +    else
> +        m_lastFieldValues->add(m_textField->name(), m_textField->value());
> +}
> +
> +FontSelector* SuggestionsPopupMenuClient::fontSelector() const
> +{
> +    return m_textField->document()->styleSelector()->fontSelector();
> +}
> +
> +HostWindow* SuggestionsPopupMenuClient::hostWindow() const
> +{
> +    return m_textField->document()->view()->hostWindow();
> +}
> +
> +PassRefPtr<Scrollbar> SuggestionsPopupMenuClient::createScrollbar(
> +    ScrollbarClient* client,
> +    ScrollbarOrientation orientation,
> +    ScrollbarControlSize size)
> +{
> +    return Scrollbar::createNativeScrollbar(client, orientation, size);
> +}
> +
> +RenderStyle* SuggestionsPopupMenuClient::textFieldStyle() const
> +{
> +    RenderStyle* style = m_textField->computedStyle();
> +    if (!style) {
> +        // It seems we can only have a 0 style in a TextField if the
> +        // node is detached, in which case we the popup shoud not be
> +        // showing.  Please report this in http://crbug.com/7708 and
> +        // include the page you were visiting.
> +        ASSERT_NOT_REACHED();
> +    }
> +    return style;
> +}
> +
> +void SuggestionsPopupMenuClient::initialize(HTMLInputElement* textField,
> +                                            int defaultSuggestionIndex)
> +{
> +    if (!m_lastFieldValues)
> +        m_lastFieldValues.set(new FieldValuesMap);
> +
> +    m_textField = textField;
> +    m_typedFieldValue = textField->value();
> +    m_selectedIndex = defaultSuggestionIndex;
> +
> +    setInitialSuggestion();
> +
> +    FontDescription fontDescription;
> +    m_webView->theme()->systemFont(CSSValueWebkitControl, fontDescription);
> +    // Use a smaller font size to match IE/Firefox.
> +    // FIXME: http://crbug.com/7376 use the system size instead of a
> +    //        fixed font size value.
> +    fontDescription.setComputedSize(12.0);
> +    Font font(fontDescription, 0, 0);
> +    font.update(textField->document()->styleSelector()->fontSelector());
> +    // The direction of text in popup menu is set the same as the direction of
> +    // the input element: textField.
> +    m_style.set(new PopupMenuStyle(Color::black, Color::white, font, true,
> +                                   Length(WebCore::Fixed),
> +                                   textField->renderer()->style()->direction()));
> +}
> +
> +void SuggestionsPopupMenuClient::setInitialSuggestion()
> +{
> +    if (!getSuggestionsSize() || !m_textField->name().length() || !m_typedFieldValue.length())
> +        return;
> +
> +    int newIndex = m_selectedIndex >= 0 ? m_selectedIndex : 0;
> +    const String& suggestion = getSuggestion(newIndex);
> +    bool hasPreviousValue = m_lastFieldValues->contains(m_textField->name());
> +
> +    String prevValue;
> +    if (hasPreviousValue)
> +        prevValue = m_lastFieldValues->get(m_textField->name());
> +
> +    if (!hasPreviousValue || m_typedFieldValue.length() > m_lastFieldValues->get(m_textField->name()).length()) {
> +          if (suggestion.startsWith(m_typedFieldValue))
> +              m_selectedIndex = newIndex;
> +
> +          int start = 0;
> +          String newSuggestion = suggestion;
> +          if (suggestion.startsWith(m_typedFieldValue, false)) {
> +              newSuggestion = m_typedFieldValue;
> +              if (suggestion.length() > m_typedFieldValue.length()) {
> +                  newSuggestion.append(suggestion.substring(m_typedFieldValue.length(),
> +                      suggestion.length() - m_typedFieldValue.length()));
> +              }
> +              start = m_typedFieldValue.length();
> +          }
> +
> +          m_textField->setSuggestedValue(newSuggestion);
> +          m_textField->setSelectionRange(start, newSuggestion.length());
> +    }
> +
> +    if (hasPreviousValue)
> +        m_lastFieldValues->set(m_textField->name(), m_typedFieldValue);
> +    else
> +        m_lastFieldValues->add(m_textField->name(), m_typedFieldValue);
> +}
> +
> +void SuggestionsPopupMenuClient::setSuggestedValue(const WebString& suggestion)
> +{
> +    m_textField->setSuggestedValue(suggestion);
> +    m_textField->setSelectionRange(m_typedFieldValue.length(),
> +                                   suggestion.length());
> +}
> +
> +} // namespace WebKit
> Index: WebKit/chromium/src/SuggestionsPopupMenuClient.h
> ===================================================================
> --- WebKit/chromium/src/SuggestionsPopupMenuClient.h	(revision 0)
> +++ WebKit/chromium/src/SuggestionsPopupMenuClient.h	(revision 0)
> @@ -0,0 +1,118 @@
> +/*
> + * Copyright (C) 2010 Google Inc. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are
> + * met:
> + *
> + *     * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following disclaimer
> + * in the documentation and/or other materials provided with the
> + * distribution.
> + *     * Neither the name of Google Inc. nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#include "PopupMenuClient.h"
> +
> +#ifndef SuggestionsPopupMenuClient_h
> +#define SuggestionsPopupMenuClient_h
> +
> +namespace WebCore {
> +class HTMLInputElement;
> +class PopupMenuStyle;
> +class RenderStyle;
> +}
> +
> +namespace WebKit {
> +class WebString;
> +class WebViewImpl;
> +template <typename T> class WebVector;
> +
> +// 
> +class SuggestionsPopupMenuClient : public WebCore::PopupMenuClient {
> +public:
> +    SuggestionsPopupMenuClient(WebViewImpl* webview);
> +    virtual ~SuggestionsPopupMenuClient();
> +
> +    // Returns the number of suggestions available.
> +    virtual size_t getSuggestionsSize() const = 0;
> +
> +    // Returns the suggestion at |listIndex|.
> +    virtual WebString getSuggestion(unsigned listIndex) const = 0;

getSuggestionsSize should probably be named getSuggestionsCount to
indicate that it is a count of available suggestions.  also,
getSuggestion should probably have its listIndex parameter type
changed to size_t to match, or change getSuggestionsCount to 
return unsigned instead of size_t.


> +    RefPtr<WebCore::HTMLInputElement> m_textField;
> +    int m_selectedIndex;
> +    WebViewImpl* m_webView;

Do we have to worry about this weak pointer to the WebViewImpl becoming
invalid?  Maybe it would be better to access the WebViewImpl via the
m_textField member, which we have retained in a RefPtr.

  cast<chromeclientimpl>(element->document->frame->page->chrome->client)->webview


> Index: WebKit/chromium/src/WebViewImpl.cpp
...
> +#include "AutoFillPopupMenuClient.h"
>  #include "AutocompletePopupMenuClient.h"
>  #include "AXObjectCache.h"
>  #include "Chrome.h"
> @@ -81,6 +82,7 @@
>  #include "SecurityOrigin.h"
>  #include "SelectionController.h"
>  #include "Settings.h"
> +#include "SuggestionsPopupMenuClient.h"

^^^ this include is unnecessary given that we include the subclass headers, right?


> +    if (!m_autoFillPopup.get())
> +        m_autoFillPopup = PopupContainer::create(m_suggestionsPopupClient,
> +                                                 suggestionsPopupSettings);

nit: multi-line, so it should have {}'s


> Index: WebKit/chromium/src/WebViewImpl.h
...
> +    // A pointer to the current suggestions popup.  We do not own this pointer.
> +    WebCore::PopupContainer* m_suggestionsPopup;

^^^ need to initialize this to 0 in the ctor.

[James Hawkins]

Created attachment 48368 [details]
[Chromium] Refactor AutocompletePopupMenuClient [try3]

All comments addressed.

[WebKit Review Bot]

Attachment 48368 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/src/SuggestionsPopupMenuClient.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1


If any of these errors are false positives, please file a bug against check-webkit-style.

[James Hawkins]

Created attachment 48371 [details]
[Chromium] Refactor AutocompletePopupMenuClient [try4]

Style fix.

[Darin Fisher (:fishd, Google)]

Comment on attachment 48371 [details]
[Chromium] Refactor AutocompletePopupMenuClient [try4]

> 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?

[James Hawkins]

Created attachment 48443 [details]
[Chromium] Refactor AutocompletePopupMenuClient [try5]

Fixed indentation.  Removed spurious headers.  m_webView was replaced with getWebView method in my previous patch, but I forgot to remove the member variable.  Done in this patch.

[Darin Fisher (:fishd, Google)]

Comment on attachment 48443 [details]
[Chromium] Refactor AutocompletePopupMenuClient [try5]

> Index: WebKit/chromium/src/SuggestionsPopupMenuClient.cpp
...
> +int SuggestionsPopupMenuClient::clientPaddingLeft() const
> +{
> +    // Bug http://crbug.com/7708 seems to indicate the style can be 0.
> +    RenderStyle* style = textFieldStyle();
> +    if (!style)
> +       return 0;
> +
> +    return getWebView()->theme()->popupInternalPaddingLeft(style);

Technically, there's no need to use WebViewImpl::theme().  I've been
meaning to delete that function since we don't need it.  We only have
a single global RenderTheme implementation, which you can access via
RenderTheme::defaultTheme().  I suggest using that function instead.

I note, however, that you are just copying the existing code, so if
you prefer to leave this as-is for now that's fine.


> +WebViewImpl* SuggestionsPopupMenuClient::getWebView() const
> +{
> +    return static_cast<ChromeClientImpl*>(
> +        m_textField->document()->frame()->page()->chrome()->client())->webView();

Note: when a frame is being torn down, it is possible for Frame::page()
to return null.

Document::frame() also has a comment saying that the result can be null.

The point of not holding a direct reference to m_webView was to deal
well with these odd shutdown cases, so it is probably worth adding
some null checks.

I think you don't have to worry about m_textField->document() ever
being null, and a Page always has a Chrome object, which will have
a ChromeClient.  ChromeClientImpl is allocated as a member variable
of WebViewImpl, so you don't have to worry about the backpointer
from ChromeClientImpl to WebViewImpl ever being null.

R- because I think we should add some null checks in getWebView()
and at the callsites.

-Darin

[James Hawkins]

Created attachment 48446 [details]
[Chromium] Refactor AutocompletePopupMenuClient [try6]

Added NULL checks in getWebView() and callers of the method.  Now using RenderTheme::defaultTheme().

[WebKit Review Bot]

Attachment 48446 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/src/SuggestionsPopupMenuClient.cpp:253:  Use 0 instead of NULL.  [readability/null] [5]
WebKit/chromium/src/SuggestionsPopupMenuClient.cpp:257:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 2


If any of these errors are false positives, please file a bug against check-webkit-style.

[James Hawkins]

Created attachment 48448 [details]
[Chromium] Refactor AutocompletePopupMenuClient [try7]

Fixes style nits.

[James Hawkins]

Created attachment 48450 [details]
[Chromium] Refactor AutocompletePopupMenuClient [try8]

Fixed indentation.

[James Hawkins]

Created attachment 48451 [details]
[Chromium] Refactor AutocompletePopupMenuClient [try9]

Another indentation fix.

[WebKit Commit Bot]

Comment on attachment 48451 [details]
[Chromium] Refactor AutocompletePopupMenuClient [try9]

Clearing flags on attachment: 48451

Committed r54586: <http://trac.webkit.org/changeset/54586>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Tony Chang]

(In reply to comment #16)
> (From update of attachment 48451 [details])
> Clearing flags on attachment: 48451
> 
> Committed r54586: <http://trac.webkit.org/changeset/54586>

The Chromium mac build appears to be broken with the following error:
cc1plus: warnings being treated as errors
/Users/cltbld/Desktop/BuildSlaveData/WebKit-BuildSlave/chromium-mac-release/build/WebKit/chromium/src/AutoFillPopupMenuClient.cpp: In member function ‘virtual void WebKit::AutoFillPopupMenuClient::removeSuggestionAtIndex(unsigned int)’:
/Users/cltbld/Desktop/BuildSlaveData/WebKit-BuildSlave/chromium-mac-release/build/WebKit/chromium/src/AutoFillPopupMenuClient.cpp:57: warning: comparison between signed and unsigned integer expressions


However, the canary buildbot on build.chromium.org compiles just fine.  I think it's just different versions of gcc and 4.2 gives the warning and 4.2.1 doesn't.

Anyway, fixed in http://trac.webkit.org/changeset/54589 .