Bug 59836

Summary: [Mac] Need to truncate the string sent to "Look Up … " menu item, if it's too long.
Product: WebKit Reporter: Jia Pu <jiapu.mail>
Component: New BugsAssignee: Jia Pu <jiapu.mail>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, commit-queue, ddavidso
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.6   
Attachments:
Description Flags
Patch (v1)
none
Patch (v2)
none
Patch (v3)
none
Patch (v3)
none
Patch (v5) none

Description Jia Pu 2011-04-29 15:04:31 PDT
This behavior is consistent with that in AppKit.
<rdar://problem/9350317>
Comment 1 Jia Pu 2011-04-29 15:08:48 PDT
Created attachment 91746 [details]
Patch (v1)
Comment 2 Jia Pu 2011-04-29 15:11:22 PDT
<rdar://problem/9275983>
Comment 3 Jia Pu 2011-04-29 15:18:58 PDT
Created attachment 91747 [details]
Patch (v2)
Comment 4 Mark Rowe (bdash) 2011-04-29 15:30:41 PDT
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.
Comment 5 Jia Pu 2011-04-29 16:06:32 PDT
Created attachment 91760 [details]
Patch (v3)
Comment 6 Alexey Proskuryakov 2011-04-29 22:54:34 PDT
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)?
Comment 7 Jia Pu 2011-05-02 09:19:47 PDT
Created attachment 91926 [details]
Patch (v3)

Updated patch according to comment #5
Comment 8 Jia Pu 2011-05-02 09:24:45 PDT
(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 9 Maciej Stachowiak 2011-05-02 11:25:17 PDT
Comment on attachment 91926 [details]
Patch (v3)

r=me
Comment 10 WebKit Commit Bot 2011-05-02 12:12:28 PDT
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
Comment 11 Jia Pu 2011-05-02 13:44:45 PDT
Created attachment 91971 [details]
Patch (v5)

Resolved build failure on SnowLeopard.
Comment 12 WebKit Commit Bot 2011-05-02 16:43:51 PDT
Comment on attachment 91971 [details]
Patch (v5)

Clearing flags on attachment: 91971

Committed r85551: <http://trac.webkit.org/changeset/85551>
Comment 13 WebKit Commit Bot 2011-05-02 16:43:57 PDT
All reviewed patches have been landed.  Closing bug.