[Webkit-unassigned] [Bug 34721] [Chromium] Refactor AutocompletePopupMenuClient into a base class, SuggestionsPopupMenuClient, and two derived classes, AutocompletePopupMenuClient and AutoFillPopupMenuClient
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Feb 8 14:33:41 PST 2010
https://bugs.webkit.org/show_bug.cgi?id=34721
Darin Fisher (:fishd, Google) <fishd at chromium.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #48359|review? |review-
Flag| |
--- Comment #4 from Darin Fisher (:fishd, Google) <fishd at chromium.org> 2010-02-08 14:33:40 PST ---
(From update of attachment 48359)
> 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.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list