Bug 59836 – [Mac] Need to truncate the string sent to "Look Up … " menu item, if it's too long.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jia Pu]

This behavior is consistent with that in AppKit.
<rdar://problem/9350317>

[Jia Pu]

Created attachment 91746 [details]
Patch (v1)

[Jia Pu]

<rdar://problem/9275983>

[Jia Pu]

Created attachment 91747 [details]
Patch (v2)

[Mark Rowe (bdash)]

Comment on attachment 91747 [details]
Patch (v2)

View in context: https://bugs.webkit.org/attachment.cgi?id=91747&action=review

> Source/WebCore/platform/DefaultLocalizationStrategy.cpp:76
> +    const UChar ellipsis = 0x2026;

A predefined name exists for this in WTF::Unicode: horizontalEllipsis.

[Jia Pu]

Created attachment 91760 [details]
Patch (v3)

[Alexey Proskuryakov]

Comment on attachment 91760 [details]
Patch (v3)

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)?

[Jia Pu]

Created attachment 91926 [details]
Patch (v3)

Updated patch according to comment #5

[Jia Pu]

(In reply to comment #6)
> (From update of attachment 91760 [details])
> 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?

I don't think anything special need to be done here. AppKit would take care of it when draw the menu. That said, if the text doesn't match current system locale, the behavior isn't very well defined on system level.

> 
> 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)?

Added early return in truncatedStringForLookupMenuItem() if the input string() is empty.

[Maciej Stachowiak]

Comment on attachment 91926 [details]
Patch (v3)

r=me

[WebKit Commit Bot]

Comment on attachment 91926 [details]
Patch (v3)

Rejecting attachment 91926 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'build'..." exit_code: 2

Last 500 characters of output:
/build-webkit', '--debug']" exit_code: 1
/mnt/git/webkit-commit-queue/WebKitBuild/WebCore.build/Debug/WebCore.build/Script-5DF50887116F3077005202AB.sh

** BUILD FAILED **


The following build commands failed:
WebCore:
	CompileC /mnt/git/webkit-commit-queue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/DefaultLocalizationStrategy.o /mnt/git/webkit-commit-queue/Source/WebCore/platform/DefaultLocalizationStrategy.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2
(1 failure)


Full output: http://queues.webkit.org/results/8534104

[Jia Pu]

Created attachment 91971 [details]
Patch (v5)

Resolved build failure on SnowLeopard.

[WebKit Commit Bot]

Comment on attachment 91971 [details]
Patch (v5)

Clearing flags on attachment: 91971

Committed r85551: <http://trac.webkit.org/changeset/85551>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.