[webkit-reviews] review granted: [Bug 59836] [Mac] Need to truncate the string sent to "=?UTF-8?Q?Look=20Up=20=E2=80=A6=20?=" menu item, if it's too long. : [Attachment 91760] Patch (v3)

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Jia Pu <jpu at apple.com>'s
request for review:
Bug 59836: [Mac] Need to truncate the string sent to "Look Up … " menu item, if
it's too long.
https://bugs.webkit.org/show_bug.cgi?id=59836

Attachment 91760: Patch (v3)
https://bugs.webkit.org/attachment.cgi?id=91760&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=91760&action=review

Looks good. Am I right that this does work with RTL languages correctly?

> Source/WebCore/platform/DefaultLocalizationStrategy.cpp:45
> +using namespace WTF;
> +using namespace Unicode;

This should not be necessary. Like most WTF external names, horizontalEllipsis
is exported to global namespace with a using declaration already (see the
bottom of CharacterNames.h).

> Source/WebCore/platform/DefaultLocalizationStrategy.cpp:79
> +    const unsigned maxNumberOfGraphemeClustersInLookupMenuItem = 24;

As a matter of style (which is not my favorite rule, but anyway), we don't use
const with local variables, especially of POD types.

> Source/WebCore/platform/DefaultLocalizationStrategy.cpp:83
> +    unsigned numberOfCharacters = numCharactersInGraphemeClusters(trimmed,
maxNumberOfGraphemeClustersInLookupMenuItem);

As a comment not related to your patch, this function really needs a better
name.

> Source/WebCore/platform/DefaultLocalizationStrategy.cpp:323
> -    RetainPtr<CFStringRef> selectedCFString(AdoptCF,
selectedString.createCFString());
> +    RetainPtr<CFStringRef> selectedCFString(AdoptCF,
truncatedStringForLookupMenuItem(selectedString).createCFString());
>      return formatLocalizedString(WEB_UI_STRING("Look Up “%@”", "Look Up
context menu item with selected word"), selectedCFString.get());

It seems that the original worked with null strings, but new code doesn't. Will
this make UI process crash if the Web process crashed when asked for selected
string (so, null was returned)?