Bug 25639

Created attachment 55819 [details] Patch

(In reply to comment #1) > Created an attachment (id=55819) [details] > Patch This is not for review because it has no tests. But I wanted some suggestions on how to test. We can't use a pixel test because context menus aren't shown. I considered adding a method to eventSender, but there's no method in the WebKit api for getting the menu items. It's probably possible for me to crawl the NSMenu of a showing context menu, but is that too much of a hack? The other option is to just write a manual layout test. Suggestions welcome, otherwise I'll probably try to crawl the NSMenu tomorrow.

I wonder how other context menu items (cut/copy/paste/copy link address) are tested?

I don't think this is the right level to do this check; the check itself is OK but a bit too low level. If you look at the call site in ContextMenu::populate, it has some logic about menu items related to spelling that runs even if the function returns false. I think a better fix would be creating a function that combines the isInPasswordField check with the spellCheckingEnabledInFocusedNode check and calling that in ContextMenu::populate. That function could be local to the ContextMenu file and class or in the Editor or wherever seems most natural and clear.

Created attachment 55937 [details] Patch

(In reply to comment #3) > I wonder how other context menu items (cut/copy/paste/copy link address) are tested? As far as I can tell, people test the behavior (e.g., the result of a paste), not the menu itself. (In reply to comment #4) > I don't think this is the right level to do this check; the check itself is OK but a bit too low level. If you look at the call site in ContextMenu::populate, it has some logic about menu items related to spelling that runs even if the function returns false. I moved the check to ContextMenu. I wasn't sure what to do about the "Spelling and Grammar" menu. The current patch keeps it, but I think either behavior would be ok. I didn't merge with the isInPasswordField because it's used for other things like disabling transformations. This includes a layout test with an extension to eventSender. Since it gets the context menu items, I'll have to grab results for Tiger and Snow Leopard from the bots.

Comment on attachment 55937 [details] Patch Comments in Skipped files start with "#" rather than "//". You have a TODO to add a bug number. Please do that. > + if (subView) { > + NSMenu* menu = [subView menuForEvent:event]; > + if (shouldPrintMenuItems) { > + printf("ContextMenuItems: "); > + for (int i = 0; i < [menu numberOfItems]; ++i) { It doesn't make sense to set up the menu local variable before checking the shouldPrintMenuItems. > + if (i > 0) > + printf(", "); > + NSMenuItem* menuItem = [menu itemAtIndex:i]; > + printf("%s", [menuItem isSeparatorItem] ? "----" : [[menuItem title] UTF8String]); Since menu item titles are localized, it might be better to log something else, perhaps the selector, reformatted in to the style of a WebCore editor command name. SEL selector = [menuItem action]; char commandName[256]; strcpy(commandName, sel_getName(selector)); size_t commandName Length = strlen(commandName); if (commandNameLength >= 2 && commandName[commandNameLength - 1] == ':') { commandName[0] = toupper(commandName[0]); commandName[commandNameLength - 1] = '\0'; } Using printf with a constant string or a string of just "%s" is an anti-pattern. fputs(stdio, <string>) is better.

Comment on attachment 55937 [details] Patch review- so you can rework the tests a bit. The code change seems fine.

Created attachment 56044 [details] Patch

(In reply to comment #7) > (From update of attachment 55937 [details]) > Comments in Skipped files start with "#" rather than "//". Fixed. > You have a TODO to add a bug number. Please do that. Fixed. > > + if (subView) { > > + NSMenu* menu = [subView menuForEvent:event]; > > + if (shouldPrintMenuItems) { > > + printf("ContextMenuItems: "); > > + for (int i = 0; i < [menu numberOfItems]; ++i) { > > It doesn't make sense to set up the menu local variable before checking the shouldPrintMenuItems. We always want to run [subView menuForEvent:event], so I put it outside the shouldPrintMenuItems check. > > + if (i > 0) > > + printf(", "); > > + NSMenuItem* menuItem = [menu itemAtIndex:i]; > > + printf("%s", [menuItem isSeparatorItem] ? "----" : [[menuItem title] UTF8String]); > > Since menu item titles are localized, it might be better to log something else, perhaps the selector, reformatted in to the style of a WebCore editor command name. I see. I tried this using the code you provided, but I got a bunch of ForwardContextMenuAction and SubmenuAction. It looks like it keeps the value in tag and there's a big switch statement for determining the action. We could just write out the tag (int) or maybe have a switch to map enum values to strings (seems like it might be a pain to keep in sync). The current patch has tag number + title, but it's mostly just to demonstrate the output. Thoughts?

Comment on attachment 56044 [details] Patch I guess dumping the title is best. Probably not all that helpful to dump the tag. Sorry for sending you off on a wild goose chase before.

Attachment 56044 [details] was posted by a committer and has review+, assigning to Tony Chang for commit.

Comment on attachment 56044 [details] Patch This looks straightforward enough. Tossing it on the commit-queue for Tony.

Comment on attachment 56044 [details] Patch Rejecting patch 56044 from commit-queue. Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Darin Adler', u'--force']" exit_code: 1 Last 500 characters of output: opard/editing/spelling/context-menu-suggestions-expected.txt patching file LayoutTests/platform/qt/Skipped Hunk #1 succeeded at 5090 (offset -6 lines). patching file LayoutTests/platform/win/Skipped Hunk #1 succeeded at 876 (offset 4 lines). patching file WebCore/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file WebCore/platform/ContextMenu.cpp patching file WebKitTools/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file WebKitTools/DumpRenderTree/mac/EventSendingController.mm Full output: http://webkit-commit-queue.appspot.com/results/2273195

Committed r59585: <http://trac.webkit.org/changeset/59585>