Bug 59836 - [Mac] Need to truncate the string sent to "Look Up … " menu item, if it's too long.
Summary: [Mac] Need to truncate the string sent to "Look Up … " menu item, if it's too...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.6
: P2 Normal
Assignee: Jia Pu
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-04-29 15:04 PDT by Jia Pu
Modified: 2011-05-02 16:43 PDT (History)
3 users (show)

See Also:


Attachments
Patch (v1) (3.00 KB, patch)
2011-04-29 15:08 PDT, Jia Pu
no flags Details | Formatted Diff | Diff
Patch (v2) (3.02 KB, patch)
2011-04-29 15:18 PDT, Jia Pu
no flags Details | Formatted Diff | Diff
Patch (v3) (3.18 KB, patch)
2011-04-29 16:06 PDT, Jia Pu
no flags Details | Formatted Diff | Diff
Patch (v3) (3.08 KB, patch)
2011-05-02 09:19 PDT, Jia Pu
no flags Details | Formatted Diff | Diff
Patch (v5) (3.15 KB, patch)
2011-05-02 13:44 PDT, Jia Pu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.