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+

Description Brady Eidson 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>
Comment 1 Brady Eidson 2014-07-03 15:32:52 PDT
Created attachment 234374 [details]
Patch v1
Comment 2 WebKit Commit Bot 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.
Comment 3 Brady Eidson 2014-07-03 15:55:23 PDT
Created attachment 234376 [details]
Patch v2 - Fixed style errors and rebased
Comment 4 WebKit Commit Bot 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.
Comment 5 Tim Horton 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.
Comment 6 Brady Eidson 2014-07-03 17:14:44 PDT
Created attachment 234382 [details]
Patch v3
Comment 7 Tim Horton 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
Comment 8 Brady Eidson 2014-07-03 17:40:19 PDT
http://trac.webkit.org/changeset/170782
Comment 9 Tim Horton 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?
Comment 10 Brady Eidson 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.
Comment 11 Alexey Proskuryakov 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.