Bug 134613 – When showing the selection menu, include menu options for all selected phone numbers

• Component: WebKit2
• Status: RESOLVED FIXED

[Brady Eidson]

When showing the selection menu, include menu options for all selected phone numbers.

<rdar://problem/16983434> and <rdar://problem/16874568>

[Brady Eidson]

Created attachment 234374 [details]
Patch v1

[WebKit Commit Bot]

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]

Created attachment 234376 [details]
Patch v2 - Fixed style errors and rebased

[WebKit Commit Bot]

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 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]

Created attachment 234382 [details]
Patch v3

[Tim Horton]

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]

http://trac.webkit.org/changeset/170782

[Tim Horton]

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]

(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 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.