Bug 35919 – [Qt] [Symbian] Use native popup menu in Symbian

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Chang Shu]

When displaying combo box dropdown list, the current behavior on S60 devices is the same as desktop browsers: drawing inline selection list. This may not suit for mobile devices which have limited screen size. 
It is preferred to use Symbian native popup menu on latest S60 devices.

[Chang Shu]

Created attachment 50328 [details]
fix patch

[Simon Hausmann]

Comment on attachment 50328 [details]
fix patch

I think in general this patch looks good, a few comments and coding style nitpicks though.


> +contains(DEFINES, ENABLE_SYMBIAN_DIALOG_PROVIDERS) {
> +    FEATURE_DEFINES_JAVASCRIPT += ENABLE_SYMBIAN_DIALOG_PROVIDERS=1

I don't think the above line is necessary, this feature is not tested for
in IDL files.

> +#if ENABLE(SYMBIAN_DIALOG_PROVIDERS)
> +#include <BrCtlDialogsProvider.h>
> +#include <BrowserDialogsProvider.h>
> +#include <e32base.h>
> +#endif
> +

Perhaps it would make sense to add a comment here (or in the .pro file?) that this
feature requires the S60 platform private BrowserDialogsProvider.h header file and
is therefore not enabled by default but only meant for platform builds. A little
explanation can't hurt here :)

> +static void ResetAndDestroy(TAny *aPtr)

Coding style, * placement.

> +    RPointerArray<HBufC>* items = (RPointerArray<HBufC>*) aPtr;

Coding style (space after closing parentheses) and should probably use a reinterpret_cast instead.

> +    items->ResetAndDestroy();
> +}
> +
> +void QtFallbackWebPopup::showL()
> +{
> +    static MBrCtlDialogsProvider* dialogs = CBrowserDialogsProvider::NewL(0);
> +    if (!dialogs)
> +        return;
> +
> +    int size = itemCount();
> +    CArrayFix<TBrCtlSelectOptionData>* options = new CArrayFixFlat<TBrCtlSelectOptionData>((size > 0) ? size : 1);
> +    RPointerArray<HBufC> items((size > 0) ? size : 1);

In both cases, wouldn't it be simpler to write qMax(1, size) instead? :)

> +    int newIndex;
> +    for (newIndex = 0 ; newIndex < options->Count() && !options->At(newIndex).IsSelected(); newIndex++) {}

Coding style, no space after the semicolon.

> +#if ENABLE(SYMBIAN_DIALOG_PROVIDERS)
> +    void showL();
> +#endif

Why call it showL()? Why not a more descriptive name, maybe such as showS60BrowserDialog() ?

[Chang Shu]

Created attachment 50514 [details]
fix patch

Thanks for the excellent review, Simon. The patch is updated based on the comments.

[WebKit Commit Bot]

Comment on attachment 50514 [details]
fix patch

Clearing flags on attachment: 50514

Committed r55981: <http://trac.webkit.org/changeset/55981>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Hausmann]

I can't backport this patch as it doesn't apply against qtwebkit-4.6 ;(

[Chang Shu]

(In reply to comment #6)
> I can't backport this patch as it doesn't apply against qtwebkit-4.6 ;(

Looks like I have to create another patch based on qt4.6 in git. :(