Bug 35919 - [Qt] [Symbian] Use native popup menu in Symbian
Summary: [Qt] [Symbian] Use native popup menu in Symbian
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: S60 Hardware Other
: P2 Normal
Assignee: Chang Shu
URL:
Keywords: PlatformOnly, Qt
Depends on:
Blocks:
 
Reported: 2010-03-09 07:06 PST by Chang Shu
Modified: 2010-03-15 09:51 PDT (History)
3 users (show)

See Also:


Attachments
fix patch (5.30 KB, patch)
2010-03-09 10:49 PST, Chang Shu
hausmann: review-
Details | Formatted Diff | Diff
fix patch (5.49 KB, patch)
2010-03-11 10:14 PST, Chang Shu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chang Shu 2010-03-09 07:06:51 PST
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.
Comment 1 Chang Shu 2010-03-09 10:49:09 PST
Created attachment 50328 [details]
fix patch
Comment 2 Simon Hausmann 2010-03-10 13:43:31 PST
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() ?
Comment 3 Chang Shu 2010-03-11 10:14:04 PST
Created attachment 50514 [details]
fix patch

Thanks for the excellent review, Simon. The patch is updated based on the comments.
Comment 4 WebKit Commit Bot 2010-03-14 18:28:08 PDT
Comment on attachment 50514 [details]
fix patch

Clearing flags on attachment: 50514

Committed r55981: <http://trac.webkit.org/changeset/55981>
Comment 5 WebKit Commit Bot 2010-03-14 18:28:12 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Simon Hausmann 2010-03-15 09:05:39 PDT
I can't backport this patch as it doesn't apply against qtwebkit-4.6 ;(
Comment 7 Chang Shu 2010-03-15 09:51:56 PDT
(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. :(