When showing the selection menu, include menu options for all selected phone numbers. <rdar://problem/16983434> and <rdar://problem/16874568>
Created attachment 234374 [details] Patch v1
Attachment 234374 [details] did not pass style-queue: ERROR: Source/WebKit2/Platform/mac/StringUtilities.mm:50: The parameter name "allocator" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebKit2/Platform/mac/StringUtilities.mm:53: The parameter name "number" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebKit2/Platform/mac/StringUtilities.mm:56: The parameter name "number" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebKit2/Platform/mac/StringUtilities.mm:57: Missing space after , [whitespace/comma] [3] ERROR: Source/WebKit2/Platform/mac/MenuUtilities.mm:27: You should add a blank line after implementation file's own header. [build/include_order] [4] Total errors found: 5 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 234376 [details] Patch v2 - Fixed style errors and rebased
Attachment 234376 [details] did not pass style-queue: ERROR: Source/WebKit2/Platform/mac/StringUtilities.mm:57: Missing space after , [whitespace/comma] [3] Total errors found: 1 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 234376 [details] Patch v2 - Fixed style errors and rebased View in context: https://bugs.webkit.org/attachment.cgi?id=234376&action=review > Source/WebCore/English.lproj/Localizable.strings:122 > +"Call â%@â Using iPhone" = "Call â%@â Using iPhone"; I don't think this string belongs in WebCore (or WebKit at all). > Source/WebKit2/Platform/mac/MenuUtilities.mm:67 > + items= [NSMutableArray array]; non-centered = > Source/WebKit2/Platform/mac/MenuUtilities.mm:121 > + return (NSMenuItem *)processMenuForTelephoneNumber(telephoneNumber, ExpectedProcessMenuResult::DialItem); > +} > + > +NSArray *menuItemsForTelephoneNumber(const String& telephoneNumber) > +{ > + return (NSArray *)processMenuForTelephoneNumber(telephoneNumber, ExpectedProcessMenuResult::MenuArray); this seems like over-sharing of code.
Created attachment 234382 [details] Patch v3
Comment on attachment 234382 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=234382&action=review > Source/WebCore/English.lproj/Localizable.strings:122 > +"Call â%@â Using iPhone" = "Call â%@â Using iPhone"; Sam/Dan/Anders have vetoed my objection to this, so I guess it's ok :) > Source/WebKit2/UIProcess/mac/WebContextMenuProxyMac.mm:408 > + NSMenuItem *item = menuItemForTelephoneNumber(telephoneNumber); > + if (item) merge these lines
http://trac.webkit.org/changeset/170782
Comment on attachment 234382 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=234382&action=review > Source/WebKit2/Platform/mac/StringUtilities.mm:42 > +#if ENABLE(TELEPHONE_NUMBER_DETECTION) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101000 I missed how empty this file was before you added this stuff, and its name. Why is this here instead of next to the only caller?
(In reply to comment #9) > (From update of attachment 234382 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=234382&action=review > > > Source/WebKit2/Platform/mac/StringUtilities.mm:42 > > +#if ENABLE(TELEPHONE_NUMBER_DETECTION) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101000 > > I missed how empty this file was before you added this stuff, and its name. Why is this here instead of next to the only caller? No reason besides the fact that an earlier version of the patch had two callers in two different files.
Comment on attachment 234382 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=234382&action=review > Source/WebKit2/Platform/mac/StringUtilities.mm:51 > +// These functions are declared with __attribute__((visibility ("default"))) > +// We currently don't have a way to soft link such functions, so we forward declare them again here. > +CFPhoneNumberRef CFPhoneNumberCreate(CFAllocatorRef, CFStringRef, CFStringRef); > +SOFT_LINK(PhoneNumbers, CFPhoneNumberCreate, CFPhoneNumberRef, (CFAllocatorRef allocator, CFStringRef digits, CFStringRef countryCode), (allocator, digits, countryCode)); Could you please elaborate on why this is needed? The first thing SOFT_LINK does is declare the function.