Summary: | --webkit-visual-word enable it in cr-win by command line flag | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Xiaomei Ji <xji> | ||||||||||||
Component: | HTML Editing | Assignee: | Joshua Bell <jsbell> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | abarth, dglazkov, fishd, japhet, jsbell, playmobil, rniwa, tkent, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 25298 | ||||||||||||||
Attachments: |
|
Description
Xiaomei Ji
2011-10-28 17:27:41 PDT
Created attachment 112948 [details]
wip
(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 This and the corresponding chromium side CL are temporary, and they should be removed after the feature is enabled without command line flag. Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API. 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 on attachment 112948 [details]
wip
This should just be a run-time flag and should be done by WebSettings as Darin pointed out.
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 on attachment 113234 [details] patch Attachment 113234 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10257071 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.
Created attachment 113251 [details]
patch
fix cr-linux ew.
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. (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. Created attachment 113361 [details] patch updated per comment #11 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. Committed r99255: <http://trac.webkit.org/changeset/99255> Created attachment 113720 [details]
ignore this
Comment on attachment 113720 [details]
ignore this
Sorry, I screwed up the ChangeLog merge.
|