[webkit-reviews] review granted: [Bug 37013] [Chromium] The popup type (select or suggestion) should be passed to the WebClient::createPopupMenu() method : [Attachment 52419] Added missing file...

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has granted Jay Campan
<jcampan at google.com>'s request for review:
Bug 37013: [Chromium] The popup type (select or suggestion) should be passed to
the WebClient::createPopupMenu() method
https://bugs.webkit.org/show_bug.cgi?id=37013

Attachment 52419: Added missing file...
https://bugs.webkit.org/attachment.cgi?id=52419&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> Index: WebKit/chromium/public/WebPopupType.h
...
> + * Copyright (C) 2009 Google Inc. All rights reserved.

2009 -> 2010


> Index: WebKit/chromium/src/ChromeClientImpl.cpp
...
> +namespace {
> +
> +// Converts a WebCore::PopupContainerType to a WebKit::WebPopupType.
> +WebKit::WebPopupType convertPopupType(WebCore::PopupContainer::PopupType
type)
> +{
> +    switch (type) {
> +    case PopupContainer::Select:
> +	   return WebKit::WebPopupTypeSelect;
> +    case PopupContainer::Suggestion:
> +	   return WebKit::WebPopupTypeSuggestion;
> +    default:
> +	   ASSERT_NOT_REACHED();
> +	   return WebKit::WebPopupTypeNone;
> +    }
> +}
> +
> +} // namespace
> +
>  namespace WebKit {

In webkit code, it would be more commonplace for convertPopupType to be
a static method in the WebKit namespace.  Then you can do away with the
WebKit:: prefix, and you can also do away with the WebCore:: prefix due
to the using declaration at the top of this file.


> +	   // TODO(jcivelli): Remove the deprecated methods once the Chromium
side
> +	   //		      use the new method.

nit: WebKit style is to just add a FIXME comment without the (username) bit.

R=me otherwise.  Please fix these nits before committing.