Bug 17366 – ContextMenu::itemAtIndex() should be static and declared for Windows port only

• Component: New Bugs
• Status: NEW

[Oleg Sukhodolsky]

after learning ContextMenu API I've came to conclusion that ContextMenu::itemAtIndex() should be static.  Also it looks like it is only used in Windows port, so it looks reasonable declare it only for this port.

[Oleg Sukhodolsky]

Created attachment 19127 [details]
the patch

[Darin Adler]

Comment on attachment 19127 [details]
the patch

I'd prefer to not have the #if in the header. There's no harm in declaring a function that isn't defined on all platforms, especially if we might want to define and use it on other platforms in the future. Is there something I'm overlooking here?

+        static ContextMenuItem* itemAtIndex(unsigned, const PlatformMenuDescription);

The "const" here, while not new, is not helpful. It doesn't have any effect on the type of the parameter.

Since there's no significant benefit to this patch, I'm going to set it review- just because of the minor comments above. If there was a clearer benefit I might just overlook these and say review+.

[Oleg Sukhodolsky]

(In reply to comment #2)
> (From update of attachment 19127 [details] [edit])
> I'd prefer to not have the #if in the header. There's no harm in declaring a
> function that isn't defined on all platforms, especially if we might want to
> define and use it on other platforms in the future. Is there something I'm
> overlooking here?

there are two changes in the patch:
- the method made static, since it here is no reason to have it non-static
- ifdef added to clarify the fact that this method is not used/needed by any other platform (at least for now)

Both changes have the same purpose - clarify the API of ContextMenu.
Since you do not have documentation there it is very hard to understand what 
every method is supposed to do and whether it is needed at all.  So, these suggested changes just add some infomration (you do not need this if you are 
not a Windows port and this method doesn need "this")

So this patch is about making code claerer, nothing else.

> +        static ContextMenuItem* itemAtIndex(unsigned, const
> PlatformMenuDescription);
> 
> The "const" here, while not new, is not helpful. It doesn't have any effect on
> the type of the parameter.

I have not added "const" I've just keeped it.  I can remove it if you want, btw 
there is at least one more places where "const PlatformMenuDescription" is used 
in this class.