Bug 37013 – [Chromium] The popup type (select or suggestion) should be passed to the WebClient::createPopupMenu() method

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Jay Campan]

Chromium on Linux needs to know when it displays a popup whether it is a select or a autofill/autocomplete.
For that reason, the popup type (select or suggestion) should be passed to the WebClient::createPopupMenu() method.

[Jay Campan]

Created attachment 52389 [details]
Adding the popup type to the WebClient::createPopupMenu() method

[WebKit Review Bot]

Attachment 52389 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/src/ChromeClientImpl.cpp:705:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebKit/chromium/public/WebPopupType.h:1:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Total errors found: 45 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 52389 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1658102

[Adam Barth]

Comment on attachment 52389 [details]
Adding the popup type to the WebClient::createPopupMenu() method

Looks like this patch could use a tune-up.

[Jay Campan]

Created attachment 52395 [details]
Fixed style issues

[WebKit Review Bot]

Attachment 52395 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1614186

[Adam Barth]

Comment on attachment 52395 [details]
Fixed style issues

This patch doesn't compile.

Also, we'll need fishd to review changes to the WebKit API.

[Jay Campan]

Created attachment 52418 [details]
Fixed compilation issue and cleaned-up

[Jay Campan]

Created attachment 52419 [details]
Added missing file...

[Darin Fisher (:fishd, Google)]

Comment on attachment 52419 [details]
Added missing file...

> 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.

[Jay Campan]

Created attachment 52420 [details]
Applied fishd suggested changes

[WebKit Commit Bot]

Comment on attachment 52420 [details]
Applied fishd suggested changes

Clearing flags on attachment: 52420

Committed r57014: <http://trac.webkit.org/changeset/57014>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.