Bug 71163 - --webkit-visual-word enable it in cr-win by command line flag
Summary: --webkit-visual-word enable it in cr-win by command line flag
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joshua Bell
URL:
Keywords:
Depends on:
Blocks: 25298
  Show dependency treegraph
 
Reported: 2011-10-28 17:27 PDT by Xiaomei Ji
Modified: 2011-11-04 16:10 PDT (History)
9 users (show)

See Also:


Attachments
wip (4.98 KB, patch)
2011-10-28 17:28 PDT, Xiaomei Ji
no flags Details | Formatted Diff | Diff
patch (9.88 KB, patch)
2011-11-01 15:10 PDT, Xiaomei Ji
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
patch (9.57 KB, patch)
2011-11-01 16:27 PDT, Xiaomei Ji
no flags Details | Formatted Diff | Diff
patch (8.21 KB, patch)
2011-11-02 13:20 PDT, Xiaomei Ji
no flags Details | Formatted Diff | Diff
ignore this (126.57 KB, text/plain)
2011-11-04 15:39 PDT, Joshua Bell
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Xiaomei Ji 2011-10-28 17:27:41 PDT
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).
Comment 1 Xiaomei Ji 2011-10-28 17:28:29 PDT
Created attachment 112948 [details]
wip
Comment 2 Xiaomei Ji 2011-10-28 17:35:24 PDT
(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
Comment 3 Xiaomei Ji 2011-10-28 17:39:55 PDT
This and the corresponding chromium side CL are temporary, and they should be removed after the feature is enabled without command line flag.
Comment 4 WebKit Review Bot 2011-10-28 17:55:15 PDT
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 5 Darin Fisher (:fishd, Google) 2011-10-31 09:45:04 PDT
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?
Comment 6 Ryosuke Niwa 2011-10-31 09:50:24 PDT
Comment on attachment 112948 [details]
wip

This should just be a run-time flag and should be done by WebSettings as Darin pointed out.
Comment 7 Xiaomei Ji 2011-11-01 15:10:31 PDT
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).
Comment 8 WebKit Review Bot 2011-11-01 15:41:01 PDT
Comment on attachment 113234 [details]
patch

Attachment 113234 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10257071
Comment 9 Darin Fisher (:fishd, Google) 2011-11-01 16:27:14 PDT
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.
Comment 10 Xiaomei Ji 2011-11-01 16:27:45 PDT
Created attachment 113251 [details]
patch

fix cr-linux ew.
Comment 11 Ryosuke Niwa 2011-11-02 11:25:22 PDT
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.
Comment 12 Xiaomei Ji 2011-11-02 11:30:32 PDT
(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.
Comment 13 Xiaomei Ji 2011-11-02 13:20:58 PDT
Created attachment 113361 [details]
patch

updated per comment #11
Comment 14 Ryosuke Niwa 2011-11-02 13:45:21 PDT
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.
Comment 15 Xiaomei Ji 2011-11-03 18:18:41 PDT
Committed r99255: <http://trac.webkit.org/changeset/99255>
Comment 16 Joshua Bell 2011-11-04 15:39:01 PDT
Created attachment 113720 [details]
ignore this
Comment 17 Joshua Bell 2011-11-04 16:10:14 PDT
Comment on attachment 113720 [details]
ignore this

Sorry, I screwed up the ChangeLog merge.