Summary: | [Qt] [Symbian] Use native popup menu in Symbian | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chang Shu <cshu> | ||||||
Component: | WebKit Qt | Assignee: | Chang Shu <cshu> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | commit-queue, hausmann, laszlo.gombos | ||||||
Priority: | P2 | Keywords: | PlatformOnly, Qt | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | S60 Hardware | ||||||||
OS: | Other | ||||||||
Attachments: |
|
Description
Chang Shu
2010-03-09 07:06:51 PST
Created attachment 50328 [details]
fix patch
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() ? Created attachment 50514 [details]
fix patch
Thanks for the excellent review, Simon. The patch is updated based on the comments.
Comment on attachment 50514 [details] fix patch Clearing flags on attachment: 50514 Committed r55981: <http://trac.webkit.org/changeset/55981> All reviewed patches have been landed. Closing bug. I can't backport this patch as it doesn't apply against qtwebkit-4.6 ;( (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. :( |