Summary: | [Chromium] Refactor AutocompletePopupMenuClient into a base class, SuggestionsPopupMenuClient, and two derived classes, AutocompletePopupMenuClient and AutoFillPopupMenuClient | ||
---|---|---|---|
Product: | WebKit | Reporter: | James Hawkins <jhawkins> |
Component: | WebKit API | Assignee: | Nobody <webkit-unassigned> |
Status: | RESOLVED FIXED | ||
Severity: | Normal | CC: | commit-queue, fishd, tony, webkit.review.bot |
Priority: | P2 | ||
Version: | 528+ (Nightly build) | ||
Hardware: | PC | ||
OS: | All | ||
Attachments: |
Description
James Hawkins
2010-02-08 13:09:27 PST
Created attachment 48358 [details]
[Chromium] Refactor AutocompletePopupMenuClient
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.
Created attachment 48359 [details]
[Chromium] Refactor AutocompletePopupMenuClient [try2]
Style fixes.
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. Created attachment 48368 [details]
[Chromium] Refactor AutocompletePopupMenuClient [try3]
All comments addressed.
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.
Created attachment 48371 [details]
[Chromium] Refactor AutocompletePopupMenuClient [try4]
Style fix.
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? 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.
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 Created attachment 48446 [details]
[Chromium] Refactor AutocompletePopupMenuClient [try6]
Added NULL checks in getWebView() and callers of the method. Now using RenderTheme::defaultTheme().
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.
Created attachment 48448 [details]
[Chromium] Refactor AutocompletePopupMenuClient [try7]
Fixes style nits.
Created attachment 48450 [details]
[Chromium] Refactor AutocompletePopupMenuClient [try8]
Fixed indentation.
Created attachment 48451 [details]
[Chromium] Refactor AutocompletePopupMenuClient [try9]
Another indentation fix.
Comment on attachment 48451 [details] [Chromium] Refactor AutocompletePopupMenuClient [try9] Clearing flags on attachment: 48451 Committed r54586: <http://trac.webkit.org/changeset/54586> All reviewed patches have been landed. Closing bug. (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 . |