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

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=34721


Darin Fisher (:fishd, Google) <fishd at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #48443|review?                     |review-
               Flag|                            |




--- Comment #10 from Darin Fisher (:fishd, Google) <fishd at chromium.org>  2010-02-09 15:13:46 PST ---
(From update of attachment 48443)
> 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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.