17366 2008-02-14 14:31:03 -0800 ContextMenu::itemAtIndex() should be static and declared for Windows port only 2010-06-10 15:28:07 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) PC Windows XP NEW P4 Normal --- 1 oleg.sukhodolsky webkit-unassigned oldest_to_newest 70699 0 oleg.sukhodolsky 2008-02-14 14:31:03 -0800 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. 70701 1 19127 oleg.sukhodolsky 2008-02-14 14:36:41 -0800 Created attachment 19127 the patch 71260 2 19127 darin 2008-02-20 09:30:29 -0800 Comment on attachment 19127 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+. 71269 3 oleg.sukhodolsky 2008-02-20 11:30:20 -0800 (In reply to comment #2) > (From update of attachment 19127 [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. 19127 2008-02-14 14:36:41 -0800 2010-06-10 15:28:07 -0700 the patch context-menu.txt text/plain 2440 oleg.sukhodolsky SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiAzMDIzMykKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsOSBAQAorMjAwOC0wMi0xNCAgT2xlZyBTdWtob2RvbHNreSAgPG9sZWcuc3VraG9k b2xza3lAc3VuLmNvbT4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKwor ICAgICAgICAqIHBsYXRmb3JtL0NvbnRleHRNZW51Lmg6IGl0ZW1BdEluZGV4KCkgaXMgc3RhdGlj IGFuZCBhbmQgZGVjbGFyZWQgb25seSBmb3IgV2luZG93cyBwb3J0LgorCiAyMDA4LTAyLTE0ICBU aW1vdGh5IEhhdGNoZXIgIDx0aW1vdGh5QGFwcGxlLmNvbT4KIAogICAgICAgICBSZXZpZXdlZCBi eSBEYXJpbiBBZGxlci4KSW5kZXg6IFdlYkNvcmUvcGxhdGZvcm0vQ29udGV4dE1lbnUuaAo9PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09Ci0tLSBXZWJDb3JlL3BsYXRmb3JtL0NvbnRleHRNZW51LmgJKHJldmlzaW9uIDMwMjMy KQorKysgV2ViQ29yZS9wbGF0Zm9ybS9Db250ZXh0TWVudS5oCSh3b3JraW5nIGNvcHkpCkBAIC01 Niw5ICs1NiwxMiBAQCBjbGFzcyBNZW51RXZlbnRQcm94eTsKIAogICAgICAgICB2b2lkIGluc2Vy dEl0ZW0odW5zaWduZWQgcG9zaXRpb24sIENvbnRleHRNZW51SXRlbSYpOwogICAgICAgICB2b2lk IGFwcGVuZEl0ZW0oQ29udGV4dE1lbnVJdGVtJik7Ci0gICAgICAgIAorCiAgICAgICAgIENvbnRl eHRNZW51SXRlbSogaXRlbVdpdGhBY3Rpb24odW5zaWduZWQpOwotICAgICAgICBDb250ZXh0TWVu dUl0ZW0qIGl0ZW1BdEluZGV4KHVuc2lnbmVkLCBjb25zdCBQbGF0Zm9ybU1lbnVEZXNjcmlwdGlv bik7CisKKyNpZiBQTEFURk9STShXSU4pCisgICAgICAgIHN0YXRpYyBDb250ZXh0TWVudUl0ZW0q IGl0ZW1BdEluZGV4KHVuc2lnbmVkLCBjb25zdCBQbGF0Zm9ybU1lbnVEZXNjcmlwdGlvbik7Cisj ZW5kaWYKIAogICAgICAgICB1bnNpZ25lZCBpdGVtQ291bnQoKSBjb25zdDsKIApJbmRleDogV2Vi S2l0L3dpbi9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2ViS2l0L3dpbi9DaGFuZ2VMb2cJKHJl dmlzaW9uIDMwMjMzKQorKysgV2ViS2l0L3dpbi9DaGFuZ2VMb2cJKHdvcmtpbmcgY29weSkKQEAg LTEsMyArMSwxMCBAQAorMjAwOC0wMi0xNCAgT2xlZyBTdWtob2RvbHNreSAgPG9sZWcuc3VraG9k b2xza3lAc3VuLmNvbT4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKwor ICAgICAgICAqIFdlYlZpZXcuY3BwOgorICAgICAgICAoV2ViVmlldzo6cGVyZm9ybUNvbnRleHRN ZW51QWN0aW9uKTogQ29udGV4dE1lbnU6Oml0ZW1BdEluZGV4KCkgaXMgc3RhdGljIG5vdy4KKwog MjAwOC0wMi0xNCAgQWxleGV5IFByb3NrdXJ5YWtvdiAgPGFwQHdlYmtpdC5vcmc+CiAKICAgICAg ICAgKiBXZWJDaHJvbWVDbGllbnQuY3BwOiAoV2ViQ2hyb21lQ2xpZW50OjpleGNlZWRlZERhdGFi YXNlUXVvdGEpOiBGb3Jnb3QgdG8gcmUtYXBwbHkgcmV2aWV3IGNvbW1lbnRzIHRvCkluZGV4OiBX ZWJLaXQvd2luL1dlYlZpZXcuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYktpdC93aW4vV2ViVmlldy5j cHAJKHJldmlzaW9uIDMwMjMyKQorKysgV2ViS2l0L3dpbi9XZWJWaWV3LmNwcAkod29ya2luZyBj b3B5KQpAQCAtMTE0NSw3ICsxMTQ1LDcgQEAgdm9pZCBXZWJWaWV3OjpwZXJmb3JtQ29udGV4dE1l bnVBY3Rpb24oVwogICAgIENvbnRleHRNZW51KiBtZW51ID0gbV9wYWdlLT5jb250ZXh0TWVudUNv bnRyb2xsZXIoKS0+Y29udGV4dE1lbnUoKTsKICAgICBBU1NFUlQobWVudSk7CiAKLSAgICBDb250 ZXh0TWVudUl0ZW0qIGl0ZW0gPSBieVBvc2l0aW9uID8gbWVudS0+aXRlbUF0SW5kZXgoKHVuc2ln bmVkKXdQYXJhbSwgKEhNRU5VKWxQYXJhbSkgOiBtZW51LT5pdGVtV2l0aEFjdGlvbigoQ29udGV4 dE1lbnVBY3Rpb24pd1BhcmFtKTsKKyAgICBDb250ZXh0TWVudUl0ZW0qIGl0ZW0gPSBieVBvc2l0 aW9uID8gQ29udGV4dE1lbnU6Oml0ZW1BdEluZGV4KCh1bnNpZ25lZCl3UGFyYW0sIChITUVOVSls UGFyYW0pIDogbWVudS0+aXRlbVdpdGhBY3Rpb24oKENvbnRleHRNZW51QWN0aW9uKXdQYXJhbSk7 CiAgICAgaWYgKCFpdGVtKQogICAgICAgICByZXR1cm47CiAgICAgbV9wYWdlLT5jb250ZXh0TWVu dUNvbnRyb2xsZXIoKS0+Y29udGV4dE1lbnVJdGVtU2VsZWN0ZWQoaXRlbSk7Cg==