This behavior is consistent with that in AppKit. <rdar://problem/9350317>
Created attachment 91746 [details] Patch (v1)
<rdar://problem/9275983>
Created attachment 91747 [details] Patch (v2)
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.
Created attachment 91760 [details] Patch (v3)
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)?
Created attachment 91926 [details] Patch (v3) Updated patch according to comment #5
(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.
Comment on attachment 91926 [details] Patch (v3) r=me
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
Created attachment 91971 [details] Patch (v5) Resolved build failure on SnowLeopard.
Comment on attachment 91971 [details] Patch (v5) Clearing flags on attachment: 91971 Committed r85551: <http://trac.webkit.org/changeset/85551>
All reviewed patches have been landed. Closing bug.