Bug 191854

Summary: Remove the need for LocalizedStringsWPE.cpp
Product: WebKit Reporter: Christopher Reid <chris.reid>
Component: WebKit Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bfulgham, cgarcia, commit-queue, don.olmstead, mcatanzaro, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
patch
none
rebased patch
none
patch
none
Patch for landing none

Description Christopher Reid 2018-11-20 00:02:49 PST
These functions can be shared between ports.
Comment 1 Christopher Reid 2018-11-20 00:52:09 PST
Created attachment 355319 [details]
patch
Comment 2 Christopher Reid 2018-11-20 00:56:03 PST
Created attachment 355320 [details]
patch
Comment 3 Christopher Reid 2018-11-20 01:18:52 PST
Created attachment 355324 [details]
rebased patch
Comment 4 Christopher Reid 2018-11-20 01:59:13 PST
Created attachment 355326 [details]
patch
Comment 5 Christopher Reid 2018-11-20 02:01:53 PST
I wanted to check with Apple Win if there's any objection to replacing "Search with Google" to a more generic "Search the Web".
Comment 6 Don Olmstead 2018-11-20 10:23:24 PST
Comment on attachment 355326 [details]
patch

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

I don't think it matters for AppleWin since its just used for embedding in apps.

> Source/WebCore/en.lproj/Localizable.strings:602
> -/* Search with Google context menu item */
> -"Search with Google" = "Search with Google";
> +/* Search the Web context menu item */
> +"Search the Web" = "Search the Web";

I have a feeling we might want to keep the original and add the new one. I'm not sure how this will affect XCode builds.
Comment 7 Christopher Reid 2018-11-20 11:16:23 PST
Comment on attachment 355326 [details]
patch

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

> Source/WebCore/ChangeLog:13
> +        of String::fromUTF8. Move that to LocalizedString.cpp as the default implement

Just noticed this typo, will change implement to implementation before landing

>> Source/WebCore/en.lproj/Localizable.strings:602
>> +"Search the Web" = "Search the Web";
> 
> I have a feeling we might want to keep the original and add the new one. I'm not sure how this will affect XCode builds.

That string seems to only be used in windows CF ports. COCOA is using the "Search with %@" string. I'm not certain about the NSPerformService(@"Search With Google", pasteboard); call in WebKitLegacy's WebView.mm but it looks like it's only used for invoking a context menu action and not for the context menu text so it doesn't seem to be doing localization. I'm not familiar enough with NSPerformService to confirm whether that's true or not. If it is the case, I don't see much value in keeping the original string around.
Comment 8 Don Olmstead 2018-11-20 12:09:32 PST
(In reply to Christopher Reid from comment #7)
> Comment on attachment 355326 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=355326&action=review
> 
> > Source/WebCore/ChangeLog:13
> > +        of String::fromUTF8. Move that to LocalizedString.cpp as the default implement
> 
> Just noticed this typo, will change implement to implementation before
> landing
> 
> >> Source/WebCore/en.lproj/Localizable.strings:602
> >> +"Search the Web" = "Search the Web";
> > 
> > I have a feeling we might want to keep the original and add the new one. I'm not sure how this will affect XCode builds.
> 
> That string seems to only be used in windows CF ports. COCOA is using the
> "Search with %@" string. I'm not certain about the NSPerformService(@"Search
> With Google", pasteboard); call in WebKitLegacy's WebView.mm but it looks
> like it's only used for invoking a context menu action and not for the
> context menu text so it doesn't seem to be doing localization. I'm not
> familiar enough with NSPerformService to confirm whether that's true or not.
> If it is the case, I don't see much value in keeping the original string
> around.

Ok then I guess this is good to go after you fix your typo.
Comment 9 Christopher Reid 2018-11-20 13:42:47 PST
Created attachment 355362 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2018-11-20 14:09:30 PST
Comment on attachment 355362 [details]
Patch for landing

Clearing flags on attachment: 355362

Committed r238406: <https://trac.webkit.org/changeset/238406>
Comment 11 WebKit Commit Bot 2018-11-20 14:09:32 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2018-11-20 14:10:27 PST
<rdar://problem/46190457>