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

[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 48443: [Chromium] Refactor AutocompletePopupMenuClient [try5]
https://bugs.webkit.org/attachment.cgi?id=48443&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> 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