[webkit-reviews] review denied: [Bug 35919] [Qt] [Symbian] Use native popup menu in Symbian : [Attachment 50328] fix patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Mar 10 13:43:31 PST 2010


Simon Hausmann <hausmann at webkit.org> has denied Chang Shu
<Chang.Shu at nokia.com>'s request for review:
Bug 35919: [Qt] [Symbian] Use native popup menu in Symbian
https://bugs.webkit.org/show_bug.cgi?id=35919

Attachment 50328: fix patch
https://bugs.webkit.org/attachment.cgi?id=50328&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
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() ?


More information about the webkit-reviews mailing list