Bug 71163 – --webkit-visual-word enable it in cr-win by command line flag

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Xiaomei Ji]

To minimize the risk to LTR users, we are thinking to enable this feature first to chromium win by command line flag.

And we can also short-cut in the code that for LTR text in LTR context, fallback to logical word movement (which is the same order as visual order).

[Xiaomei Ji]

Created attachment 112948 [details]
wip

[Xiaomei Ji]

(In reply to comment #1)
> Created an attachment (id=112948) [details]
> wip

The corresponding chromium side change is at: 
http://codereview.chromium.org/8400078/

workflow:
1. add command line flag --use-visual-word-movement, pass it from browser to renderer, and set m_visualWordMovement (newly added data member in WebViewImpl) to true when the flag is passed-in;
2. in EditorClientImpl, when webview's m_visualWordMovement is true, reset the mapping of ctrl-arrow from "MoveWordLeft/Right" to "MoveWordLeft/RightVisually" which are 2 newly added editing commands in EditorCommand.cpp

[Xiaomei Ji]

This and the corresponding chromium side CL are temporary, and they should be removed after the feature is enabled without command line flag.

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Darin Fisher (:fishd, Google)]

Comment on attachment 112948 [details]
wip

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

> WebKit/chromium/public/WebWidget.h:182
> +    virtual void setVisualWordMovement() { }

this feels a lot like a per-Page setting.  shouldn't this be in WebSettings?

[Ryosuke Niwa]

Comment on attachment 112948 [details]
wip

This should just be a run-time flag and should be done by WebSettings as Darin pointed out.

[Xiaomei Ji]

Created attachment 113234 [details]
patch

Thanks for the suggestion!
It is a per-page setting, and I set it through WebKit::WebSettings (not in WebCore::Settings to minimize the change in WebCore).

[WebKit Review Bot]

Comment on attachment 113234 [details]
patch

Attachment 113234 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10257071

[Darin Fisher (:fishd, Google)]

Comment on attachment 113234 [details]
patch

I'm probably not the best reviewer for the EditorClientImpl.cpp changes, but the rest of the changes LGTM.

[Xiaomei Ji]

Created attachment 113251 [details]
patch

fix cr-linux ew.

[Ryosuke Niwa]

Comment on attachment 113251 [details]
patch

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

> Source/WebKit/chromium/src/EditorClientImpl.cpp:536
> +            const char* name = keyDownEntries[i].name;
> +            if (visualWordMovementEnabled) {
> +                if (!strcmp(name, "MoveWordLeft"))
> +                    name = "MoveWordLeftVisually";
> +                else if (!strcmp(name, "MoveWordRight"))
> +                    name = "MoveWordRightVisually";
> +            }

I don't think this is the right layer to fix this issue. We probably want to change the behavior based on preference inside Selection::modify.

[Xiaomei Ji]

(In reply to comment #11)
> (From update of attachment 113251 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113251&action=review
> 
> > Source/WebKit/chromium/src/EditorClientImpl.cpp:536
> > +            const char* name = keyDownEntries[i].name;
> > +            if (visualWordMovementEnabled) {
> > +                if (!strcmp(name, "MoveWordLeft"))
> > +                    name = "MoveWordLeftVisually";
> > +                else if (!strcmp(name, "MoveWordRight"))
> > +                    name = "MoveWordRightVisually";
> > +            }
> 
> I don't think this is the right layer to fix this issue. We probably want to change the behavior based on preference inside Selection::modify.

that probably needs to add this flag into WebCore::Settings.

[Xiaomei Ji]

Created attachment 113361 [details]
patch

updated per comment #11

[Ryosuke Niwa]

Comment on attachment 113361 [details]
patch

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

> Source/WebCore/editing/FrameSelection.cpp:638
> +        Settings* settings = m_frame ? m_frame->settings() : 0;
> +        if (settings && settings->visualWordMovementEnabled()) {

Please make this an inline helper function.

[Xiaomei Ji]

Committed r99255: <http://trac.webkit.org/changeset/99255>

[Joshua Bell]

Created attachment 113720 [details]
ignore this

[Joshua Bell]

Comment on attachment 113720 [details]
ignore this

Sorry, I screwed up the ChangeLog merge.