Bug 134613

Summary: When showing the selection menu, include menu options for all selected phone numbers
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebKit2Assignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: bunhere, cdumez, commit-queue, esprehn+autocc, gyuyoung.kim, kangil.han, sergio, simon.fraser
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: All   
Attachments:
Description Flags
Patch v1
none
Patch v2 - Fixed style errors and rebased
none
Patch v3 thorton: review+

Brady Eidson
Reported 2014-07-03 15:23:43 PDT
When showing the selection menu, include menu options for all selected phone numbers. <rdar://problem/16983434> and <rdar://problem/16874568>
Attachments
Patch v1 (29.99 KB, patch)
2014-07-03 15:32 PDT, Brady Eidson
no flags
Patch v2 - Fixed style errors and rebased (29.91 KB, patch)
2014-07-03 15:55 PDT, Brady Eidson
no flags
Patch v3 (29.91 KB, patch)
2014-07-03 17:14 PDT, Brady Eidson
thorton: review+
Brady Eidson
Comment 1 2014-07-03 15:32:52 PDT
Created attachment 234374 [details] Patch v1
WebKit Commit Bot
Comment 2 2014-07-03 15:34:35 PDT
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.
Brady Eidson
Comment 3 2014-07-03 15:55:23 PDT
Created attachment 234376 [details] Patch v2 - Fixed style errors and rebased
WebKit Commit Bot
Comment 4 2014-07-03 15:56:05 PDT
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.
Tim Horton
Comment 5 2014-07-03 16:14:19 PDT
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.
Brady Eidson
Comment 6 2014-07-03 17:14:44 PDT
Created attachment 234382 [details] Patch v3
Tim Horton
Comment 7 2014-07-03 17:35:27 PDT
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
Brady Eidson
Comment 8 2014-07-03 17:40:19 PDT
Tim Horton
Comment 9 2014-07-03 18:10:46 PDT
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?
Brady Eidson
Comment 10 2014-07-03 20:39:22 PDT
(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.
Alexey Proskuryakov
Comment 11 2015-02-14 23:41:19 PST
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.
Note You need to log in before you can comment on or make changes to this bug.