Bug 56975 - WebKit2:Services menu item to convert selected Simplified/Traditional Chinese Text is not working
Summary: WebKit2:Services menu item to convert selected Simplified/Traditional Chinese...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Enrica Casucci
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-03-23 16:35 PDT by Enrica Casucci
Modified: 2011-04-11 13:42 PDT (History)
2 users (show)

See Also:


Attachments
Patch (10.51 KB, patch)
2011-03-24 13:42 PDT, Enrica Casucci
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Enrica Casucci 2011-03-23 16:35:55 PDT
Repro steps:
1. Launch Safari and navigate to http://www.mozilla.org/editor/midasdemo/
2. Type "简体".
3. Control-click on the first character to bring up the context menu.

Expected

There should be an entry at the bottom of the menu "Convert Selected Simplified Chinese Text"

Actual

The entry is not there.
Comment 1 Enrica Casucci 2011-03-24 13:42:53 PDT
Created attachment 86822 [details]
Patch

This patch adds the support for the services from the Services menu. The context menu part is still missing.
Comment 2 Alexey Proskuryakov 2011-03-24 14:01:07 PDT
Comment on attachment 86822 [details]
Patch

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

> Source/WebCore/editing/mac/EditorMac.mm:219
> +void Editor::readSelectionFromPasteboard(const String& pasteboardName)

Should this be share with WebKit1?

> Source/WebKit2/UIProcess/API/mac/WKView.mm:503
> +// The following three methods are needed to support Mac OS X services.

I would have put a "This method is needed to support Mac OS X Services" before each of the two (?) functions that are required. It's easy to overlook a comment that's position like this.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:540
> +    /*
> +    Frame* coreFrame = core([self _frame]); 
> +    if (!coreFrame) 
> +        return NO; 
> +    if (coreFrame->selection()->isContentRichlyEditable()) 
> +        [self _pasteWithPasteboard:pasteboard allowPlainText:YES]; 
> +    else 
> +        [self _pasteAsPlainTextWithPasteboard:pasteboard]; 
> +     */

Commented out code.

> Source/WebKit2/UIProcess/mac/WebPageProxyMac.mm:167
> +    const double MessageTimeout = 20;

Why capital M? 

I assume that this is in seconds. Seems appropriate to me, but it's strange to see the constant in the function, not at the top of the file.

> LayoutTests/ChangeLog:5
> +        Repeated copy and paste-in-place operation results in increasingly verbose HTML.

This is from another patch.
Comment 3 Enrica Casucci 2011-03-24 14:17:49 PDT
(In reply to comment #2)
> (From update of attachment 86822 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86822&action=review
> 
> > Source/WebCore/editing/mac/EditorMac.mm:219
> > +void Editor::readSelectionFromPasteboard(const String& pasteboardName)
> 
> Should this be share with WebKit1?
> 
> > Source/WebKit2/UIProcess/API/mac/WKView.mm:503
> > +// The following three methods are needed to support Mac OS X services.
> 
> I would have put a "This method is needed to support Mac OS X Services" before each of the two (?) functions that are required. It's easy to overlook a comment that's position like this.
> 
Done.
> > Source/WebKit2/UIProcess/API/mac/WKView.mm:540
> > +    /*
> > +    Frame* coreFrame = core([self _frame]); 
> > +    if (!coreFrame) 
> > +        return NO; 
> > +    if (coreFrame->selection()->isContentRichlyEditable()) 
> > +        [self _pasteWithPasteboard:pasteboard allowPlainText:YES]; 
> > +    else 
> > +        [self _pasteAsPlainTextWithPasteboard:pasteboard]; 
> > +     */
> 
> Commented out code.
>
My bad.
> > Source/WebKit2/UIProcess/mac/WebPageProxyMac.mm:167
> > +    const double MessageTimeout = 20;
> 
> Why capital M? 
> 
> I assume that this is in seconds. Seems appropriate to me, but it's strange to see the constant in the function, not at the top of the file.
> 
I followed how it was done in other places.
> > LayoutTests/ChangeLog:5
> > +        Repeated copy and paste-in-place operation results in increasingly verbose HTML.
> 
> This is from another patch.
Mistake.
Comment 4 Enrica Casucci 2011-03-24 14:23:18 PDT
http://trac.webkit.org/changeset/81898.
Leaving the bug open since the context menu specific portion is still not finished.
Comment 5 Brady Eidson 2011-04-11 13:42:43 PDT
http://trac.webkit.org/changeset/82471 resolved the context menu piece of this.