Bug 107737 - Editor::m_compositionNode not updated on HTMLInputElement::setValue()
Summary: Editor::m_compositionNode not updated on HTMLInputElement::setValue()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 108543 108564
Blocks: 128323
  Show dependency treegraph
 
Reported: 2013-01-23 14:24 PST by Aurimas Liutikas
Modified: 2014-02-06 12:12 PST (History)
20 users (show)

See Also:


Attachments
Screenshot of www.google.com (102.63 KB, image/png)
2013-01-23 14:24 PST, Aurimas Liutikas
no flags Details
Patch (1.53 KB, patch)
2013-01-25 15:54 PST, Aurimas Liutikas
no flags Details | Formatted Diff | Diff
Patch (3.94 KB, patch)
2013-01-30 18:20 PST, Aurimas Liutikas
no flags Details | Formatted Diff | Diff
Patch (6.31 KB, patch)
2013-01-30 20:05 PST, Aurimas Liutikas
no flags Details | Formatted Diff | Diff
Patch (10.30 KB, patch)
2013-01-30 21:31 PST, Aurimas Liutikas
no flags Details | Formatted Diff | Diff
Patch (11.17 KB, patch)
2013-01-31 09:09 PST, Aurimas Liutikas
no flags Details | Formatted Diff | Diff
Patch (12.87 KB, patch)
2013-01-31 11:13 PST, Aurimas Liutikas
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aurimas Liutikas 2013-01-23 14:24:02 PST
Created attachment 184314 [details]
Screenshot of www.google.com

m_compositionNode is not being updated on JavaScript changes to the focused input text field. It has to be explicitly updated using Editor::confirmCompositon(). In Chrome most of the platforms send confirmComposition on mouse clicks which does not really a good idea and it should probably get called automatically on HTMLInputElement::setValue() calls.

On Chrome for Android we were not calling confirmComposition and then cause the Editor::setComposition() do behave weirdly as it was not able to change the node value anymore returning false. It can be checked using Chrome Beta M25 on mobile version of google.com when clicking on the autocomplete suggestion with a little arrow (see screenshot) when there still is a non-empty composition.
Comment 1 Aurimas Liutikas 2013-01-23 14:36:47 PST
The user case:
1. User goes to google.com on their mobile phone.
2. User starts typing a word in an input field, for example "Hello"
3. User selects a suggestion from the page (for example "Hello Kitty") by tapping on a little arrow that uses JavaScript to replace the input field value.
4. User tries to type some more words (composing).

Expectation:
User can continue typing at step 4.

What happens:
User cannot input any new text at step 4.
Comment 2 Aurimas Liutikas 2013-01-23 14:54:32 PST
What I observed is that if Editor::confirmComposition() is called before the JavaScript value change, then the user can continue typing after the value has changed. I am guessing confirmComposition resets the Editor state correctly.

Should the WebWidget implementor be responsible for calling confirmComposition before JavaScript changes to the input field values?
Comment 3 Aurimas Liutikas 2013-01-23 15:31:36 PST
I found a way to reproduce this bug (I think the root cause is the same that the editor does not get updated on javascript changes) on Chrome GTK and ChromeOS (might be affecting other platforms too but I do not have a device to test).

User steps:
1. Navigate to http://jsfiddle.net/eEpQz/2/
2. Within 10 seconds type in something in the input field using IME (I used ibus for Linux and built-in IME on ChromeOS). You must have a composition (underline in Chrome). 
3. Wait for the JavaScript timer to expire.

Observation:
The composition (the underline) will be set to the number of characters the you typed in the field.
For example, if I typed '啊' before the replacement, it will become 'Nonononono' with the first letter 'N' as the composition.

Expectation:
The composition should get reset when JS changes the value of the focused field.
Comment 4 Ryosuke Niwa 2013-01-23 15:40:34 PST
I don't observe the said behavior on Mac port. I'm pretty certain this is a problem in Chromium's IME code.
Comment 5 Ryosuke Niwa 2013-01-23 15:57:10 PST
I can reproduce this problem on Chrome (Mac).
Comment 6 Aurimas Liutikas 2013-01-23 16:00:50 PST
Does Mac port call commitComposition or cancelComposition before JavaScript changes?
Comment 7 Levi Weintraub 2013-01-25 13:01:53 PST
Here's a backtrace where the JS change blows away the composition on Mac.

#0  WebCore::Editor::cancelComposition (this=0x115096770) at /Volumes/Workspace/webkit/Source/WebCore/editing/Editor.cpp:1333
#1  0x0000000101926834 in -[WebHTMLView(WebNSTextInputSupport) _updateSelectionForInputManager] (self=0x120ebc0a0, _cmd=0x7fff897b3d8c) at /Volumes/Workspace/webkit/Source/WebKit/mac/WebView/WebHTMLView.mm:6051
#2  0x000000010192192b in -[WebHTMLView(WebInternal) _selectionChanged] (self=0x120ebc0a0, _cmd=0x7fff897ad14a) at /Volumes/Workspace/webkit/Source/WebKit/mac/WebView/WebHTMLView.mm:5050
#3  0x00000001018b7aa7 in WebEditorClient::respondToChangedSelection (this=0x109f7f990, frame=0x115096200) at /Volumes/Workspace/webkit/Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm:295
#4  0x000000010395c247 in WebCore::Editor::notifyComponentsOnChangedSelection (this=0x115096770, oldSelection=@0x7fff5fbfd388, options=6) at /Volumes/Workspace/webkit/Source/WebCore/editing/Editor.cpp:485
#5  0x0000000103969a05 in WebCore::Editor::respondToChangedSelection (this=0x115096770, oldSelection=@0x7fff5fbfd388, options=6) at /Volumes/Workspace/webkit/Source/WebCore/editing/Editor.cpp:2879
#6  0x0000000103ab0ffd in WebCore::FrameSelection::setSelection (this=0x115096838, newSelection=@0x7fff5fbfd5a0, options=6, align=WebCore::FrameSelection::AlignCursorOnScrollIfNeeded, granularity=WebCore::CharacterGranularity) at /Volumes/Workspace/webkit/Source/WebCore/editing/FrameSelection.cpp:313
#7  0x0000000103d0dcd0 in WebCore::HTMLTextFormControlElement::setSelectionRange (this=0x120ef0580, start=14, end=14, direction=WebCore::SelectionHasNoDirection) at /Volumes/Workspace/webkit/Source/WebCore/html/HTMLTextFormControlElement.cpp:344
#8  0x0000000104d4e08d in WebCore::TextFieldInputType::setValue (this=0x11d0009f0, sanitizedValue=@0x7fff5fbfd850, valueChanged=true, eventBehavior=WebCore::DispatchNoEvent) at /Volumes/Workspace/webkit/Source/WebCore/html/TextFieldInputType.cpp:111
#9  0x0000000103ca1d5d in WebCore::HTMLInputElement::setValue (this=0x120ef0580, value=@0x7fff5fbfd8e8, eventBehavior=WebCore::DispatchNoEvent) at /Volumes/Workspace/webkit/Source/WebCore/html/HTMLInputElement.cpp:1014
#10 0x0000000103ca2604 in WebCore::HTMLInputElement::setValue (this=0x120ef0580, value=@0x7fff5fbfd8e8, ec=@0x7fff5fbfd8f4, eventBehavior=WebCore::DispatchNoEvent) at /Volumes/Workspace/webkit/Source/WebCore/html/HTMLInputElement.cpp:998
#11 0x0000000104138722 in WebCore::setJSHTMLInputElementValue (exec=0x1167fc058, thisObject=0x118a8fd40, value={u = {asInt64 = 4636448704, ptr = 0x1145a97c0, asBits = {payload = 341481408, tag = 1}}}) at /Volumes/Workspace/WebKit/WebKitBuild/Debug/DerivedSources/WebCore/JSHTMLInputElement.cpp:1034
#12 0x000000010413b079 in JSC::lookupPut<WebCore::JSHTMLInputElement> (exec=0x1167fc058, propertyName={m_impl = 0x10e14d310, static NotAnIndex = 4294967295}, value={u = {asInt64 = 4636448704, ptr = 0x1145a97c0, asBits = {payload = 341481408, tag = 1}}}, table=0x105d6d840, thisObj=0x118a8fd40, shouldThrow=false) at Lookup.h:373
#13 0x000000010413aab8 in JSC::lookupPut<WebCore::JSHTMLInputElement, WebCore::JSHTMLElement> (exec=0x1167fc058, propertyName={m_impl = 0x10e14d310, static NotAnIndex = 4294967295}, value={u = {asInt64 = 4636448704, ptr = 0x1145a97c0, asBits = {payload = 341481408, tag = 1}}}, table=0x105d6d840, thisObj=0x118a8fd40, slot=@0x7fff5fbfdbe0) at Lookup.h:389
#14 0x0000000104135e37 in WebCore::JSHTMLInputElement::put (cell=0x118a8fd40, exec=0x1167fc058, propertyName={m_impl = 0x10e14d310, static NotAnIndex = 4294967295}, value={u = {asInt64 = 4636448704, ptr = 0x1145a97c0, asBits = {payload = 341481408, tag = 1}}}, slot=@0x7fff5fbfdbe0) at /Volumes/Workspace/WebKit/WebKitBuild/Debug/DerivedSources/WebCore/JSHTMLInputElement.cpp:751
#15 0x00000001009d50f9 in JSC::JSValue::put (this=0x7fff5fbfdbf8, exec=0x1167fc058, propertyName={m_impl = 0x10e14d310, static NotAnIndex = 4294967295}, value={u = {asInt64 = 4636448704, ptr = 0x1145a97c0, asBits = {payload = 341481408, tag = 1}}}, slot=@0x7fff5fbfdbe0) at JSValueInlines.h:678
Comment 8 Aurimas Liutikas 2013-01-25 15:54:39 PST
Created attachment 184829 [details]
Patch
Comment 9 Aurimas Liutikas 2013-01-25 15:56:00 PST
rniwa, do this this is a reasonable way of solving this?
Comment 10 Levi Weintraub 2013-01-25 15:56:50 PST
Comment on attachment 184829 [details]
Patch

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

> Source/WebCore/editing/Editor.cpp:2884
> +    // Cancel the composition if the composition selection cannot be retrieved.
> +    unsigned start;
> +    unsigned end;
> +    if (hasComposition() && !ignoreCompositionSelectionChange() && !getCompositionSelection(start, end))
> +        cancelComposition();
> +

The Mac port has embedding code that seems to handle this case. Is there a reason your fix goes in Editor instead? If this is always desired, is the Mac platform code no longer needed?
Comment 11 Levi Weintraub 2013-01-25 16:06:51 PST
For the record, I talked with Aurimas about needing a test, which was why I suggested he drop the R?. Niwa-san, it'd be good if you could verify this is a reasonable place to make this check, as I'm unfamiliar with our IME design.
Comment 12 Aurimas Liutikas 2013-01-25 16:07:40 PST
Comment on attachment 184829 [details]
Patch

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

>> Source/WebCore/editing/Editor.cpp:2884
>> +
> 
> The Mac port has embedding code that seems to handle this case. Is there a reason your fix goes in Editor instead? If this is always desired, is the Mac platform code no longer needed?

I think this code will make the Mac port code unnecessary. The only thing that I would need to add is a new notification call didCancelComposition to Editor client to be able to completely replace Mac port code.

I also need to add a test for this.
Comment 13 Ryosuke Niwa 2013-01-25 16:10:17 PST
Comment on attachment 184829 [details]
Patch

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

> Source/WebCore/editing/Editor.cpp:2883
> +    // Cancel the composition if the composition selection cannot be retrieved.
> +    unsigned start;
> +    unsigned end;
> +    if (hasComposition() && !ignoreCompositionSelectionChange() && !getCompositionSelection(start, end))
> +        cancelComposition();

I'd like to know why this bug only reproduces in Chromium first.
Comment 14 Aurimas Liutikas 2013-01-25 16:16:23 PST
Comment on attachment 184829 [details]
Patch

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

>> Source/WebCore/editing/Editor.cpp:2883
>> +        cancelComposition();
> 
> I'd like to know why this bug only reproduces in Chromium first.

This bug is fixed in Mac port because on WebEditorClient::respondToChangedSelection there is a call to WebHTMLView::_selectionChanged which in turn calls WebHTMLView::_updateSelectionForInputManager. In _updateSelectionForInputManager there is this exact check as in this patch that calls editor()->cancelComposition().

Chromium does not have a similar call path.
Comment 15 Ryosuke Niwa 2013-01-25 16:17:56 PST
Comment on attachment 184829 [details]
Patch

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

>>> Source/WebCore/editing/Editor.cpp:2883
>>> +        cancelComposition();
>> 
>> I'd like to know why this bug only reproduces in Chromium first.
> 
> This bug is fixed in Mac port because on WebEditorClient::respondToChangedSelection there is a call to WebHTMLView::_selectionChanged which in turn calls WebHTMLView::_updateSelectionForInputManager. In _updateSelectionForInputManager there is this exact check as in this patch that calls editor()->cancelComposition().
> 
> Chromium does not have a similar call path.

Why don't you make the same call in WebEditorClient then? Alternatively, we can move that code in the mac port to WebCore but then we should be deleting the code in WebEditorClient instead.
Comment 16 Aurimas Liutikas 2013-01-25 16:30:21 PST
Comment on attachment 184829 [details]
Patch

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

>>>> Source/WebCore/editing/Editor.cpp:2883
>>>> +        cancelComposition();
>>> 
>>> I'd like to know why this bug only reproduces in Chromium first.
>> 
>> This bug is fixed in Mac port because on WebEditorClient::respondToChangedSelection there is a call to WebHTMLView::_selectionChanged which in turn calls WebHTMLView::_updateSelectionForInputManager. In _updateSelectionForInputManager there is this exact check as in this patch that calls editor()->cancelComposition().
>> 
>> Chromium does not have a similar call path.
> 
> Why don't you make the same call in WebEditorClient then? Alternatively, we can move that code in the mac port to WebCore but then we should be deleting the code in WebEditorClient instead.

I think it would be good to have shared code as this would benefit all the WebKit clients. Do yo think this is a good place to put it though?

Also, do you think we should pass a new argument to EditorClient::respondToChangedSelection() that would tell the client if the composition was cancelled? That way I could remove most of the updateSelectionForInputManager code.
Comment 17 Ryosuke Niwa 2013-01-25 16:53:43 PST
Comment on attachment 184829 [details]
Patch

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

>>>>> Source/WebCore/editing/Editor.cpp:2883
>>>>> +        cancelComposition();
>>>> 
>>>> I'd like to know why this bug only reproduces in Chromium first.
>>> 
>>> This bug is fixed in Mac port because on WebEditorClient::respondToChangedSelection there is a call to WebHTMLView::_selectionChanged which in turn calls WebHTMLView::_updateSelectionForInputManager. In _updateSelectionForInputManager there is this exact check as in this patch that calls editor()->cancelComposition().
>>> 
>>> Chromium does not have a similar call path.
>> 
>> Why don't you make the same call in WebEditorClient then? Alternatively, we can move that code in the mac port to WebCore but then we should be deleting the code in WebEditorClient instead.
> 
> I think it would be good to have shared code as this would benefit all the WebKit clients. Do yo think this is a good place to put it though?
> 
> Also, do you think we should pass a new argument to EditorClient::respondToChangedSelection() that would tell the client if the composition was cancelled? That way I could remove most of the updateSelectionForInputManager code.

We could do that. Alternatively, the client can keep track of when the composition was canceled since I think we already have a callback for when a composition is canceled.
Comment 18 Aurimas Liutikas 2013-01-25 18:33:40 PST
I just found that there was another WebKit bug open that is pretty similar to this one https://bugs.webkit.org/show_bug.cgi?id=55560 . The previous Chrome developer claimed that this should not be done in Chrome. Do you think his concerns are still valid? I tried to look him up, but he does not seem to be working at Google anymore.

This bug has a test editing/input/setting-input-value-cancel-ime-composition.html that now passes with my patch.
Comment 19 Ryosuke Niwa 2013-01-25 22:18:56 PST
(In reply to comment #18)
> I just found that there was another WebKit bug open that is pretty similar to this one https://bugs.webkit.org/show_bug.cgi?id=55560 . The previous Chrome developer claimed that this should not be done in Chrome. Do you think his concerns are still valid? I tried to look him up, but he does not seem to be working at Google anymore.

I don't know.
Comment 20 Aurimas Liutikas 2013-01-30 18:20:10 PST
Created attachment 185648 [details]
Patch
Comment 21 WebKit Review Bot 2013-01-30 18:24:22 PST
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 22 Aurimas Liutikas 2013-01-30 20:05:56 PST
Created attachment 185663 [details]
Patch
Comment 23 Ryosuke Niwa 2013-01-30 20:07:38 PST
Comment on attachment 185663 [details]
Patch

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

> Source/WebKit/chromium/src/WebViewImpl.cpp:1365
> +        unsigned start, end;
> +        Editor* editor = frame->editor();
> +        if (editor->hasComposition() && !editor->ignoreCompositionSelectionChange() && !editor->getCompositionSelection(start, end)) {

Can we extract this as a member function of Editor? It seems like we're duplicating code in WebView.m here.
Comment 24 Aurimas Liutikas 2013-01-30 21:31:46 PST
Created attachment 185674 [details]
Patch
Comment 25 Aurimas Liutikas 2013-01-30 21:32:44 PST
rniwa: added the refactored call to Editor. Does this work better?
Comment 26 WebKit Review Bot 2013-01-30 22:00:35 PST
Comment on attachment 185674 [details]
Patch

Attachment 185674 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16217992
Comment 27 Build Bot 2013-01-30 22:11:13 PST
Comment on attachment 185674 [details]
Patch

Attachment 185674 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16252179
Comment 28 Ryosuke Niwa 2013-01-30 22:23:27 PST
(In reply to comment #25)
> rniwa: added the refactored call to Editor. Does this work better?

It seems like it doesn't build?
Comment 29 Aurimas Liutikas 2013-01-30 22:28:43 PST
rniwa: I was rushing to upload the patch before leaving the office. Will take a look at it first thing in the morning.
Comment 30 Peter Beverloo (cr-android ews) 2013-01-30 23:06:06 PST
Comment on attachment 185674 [details]
Patch

Attachment 185674 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/16251200
Comment 31 Build Bot 2013-01-31 01:22:41 PST
Comment on attachment 185674 [details]
Patch

Attachment 185674 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16282067
Comment 32 Build Bot 2013-01-31 02:00:05 PST
Comment on attachment 185674 [details]
Patch

Attachment 185674 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16251269
Comment 33 WebKit Review Bot 2013-01-31 03:34:34 PST
Comment on attachment 185674 [details]
Patch

Attachment 185674 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16270130
Comment 34 Ryosuke Niwa 2013-01-31 03:55:41 PST
Comment on attachment 185674 [details]
Patch

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

> Source/WebCore/editing/Editor.cpp:2403
> +bool Editor::cancelCompositionIfRequired()

IfRequired doesn't really tell us what that requirement is, and when we should be calling this function.
How about something like cancelCompositionIfSelectionIsInvalid, cancelCompositionIfSelectionIsOutdated, or cancelCompositionIfSelectionIsOrphaned?
(orphaned is a adjective we use to describe positions in a detached DOM node. Look for "orphaned" in FrameSelection/VisibleSelection/VisiblePosition).

> Source/WebCore/editing/Editor.cpp:2407
> +    if (hasComposition() && !ignoreCompositionSelectionChange() && !getCompositionSelection(start, end)) {

I would have negated this condition so that you can immediately return false instead.
Comment 35 Aurimas Liutikas 2013-01-31 09:09:34 PST
Created attachment 185795 [details]
Patch
Comment 36 Alexey Proskuryakov 2013-01-31 09:21:25 PST
This patch affects cross-platform code, so there should be no [chromium] in the title. The purpose of these prefixes is to signal that people working on other platforms can safely ignore the bug.
Comment 37 Build Bot 2013-01-31 09:30:47 PST
Comment on attachment 185795 [details]
Patch

Attachment 185795 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16282293
Comment 38 Alexey Proskuryakov 2013-01-31 09:42:43 PST
Comment on attachment 185795 [details]
Patch

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

Looks reasonable to me. I'm deferring to Ryosuke for final review, since he has been working with you on this for a while already.

> Source/WebCore/editing/Editor.cpp:1340
> +    unsigned start, end;

WebKit style is to define each variable on its own line.
Comment 39 Ryosuke Niwa 2013-01-31 09:44:32 PST
Comment on attachment 185795 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +

Please describe the nature of your change.

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

Please explain that an existing test, editing/input/setting-input-value-cancel-ime-composition.html, covers this change.

> Source/WebCore/ChangeLog:22
> +
> +        * WebCore.exp.in:
> +        * editing/Editor.cpp:
> +        (WebCore::Editor::cancelCompositionIfSelectionIsInvalid):
> +        (WebCore):
> +        * editing/Editor.h:
> +        (Editor):

Why is this repeated here?

> Source/WebKit/chromium/ChangeLog:12
> +        Adding a check whether composition is valid after the selection change.
> +        If there is no valid composition, then the composition should get cancelled
> +        and the WebViewClient should get notified about this cancellation.
> +
> +        This bug already had a test that had expectation set to fail, but now it will be passing.

This explanation should probably be done in Source/WebCore/ChangeLog instead.

> Source/WebKit/mac/WebView/WebHTMLView.mm:6048
>      if (coreFrame->editor()->getCompositionSelection(start, end))
>          [[NSInputManager currentInputManager] markedTextSelectionChanged:NSMakeRange(start, end - start) client:self];

Now this code is executed even if hasComposition() and coreFrame->editor()->ignoreCompositionSelectionChange() are both false.
r- because of this.

> LayoutTests/ChangeLog:10
> +        Adding a check whether composition is valid after the selection change.
> +        If there is no valid composition, then the composition should get cancelled
> +        and the WebViewClient should get notified about this cancellation.

This comment applies to Source/WebCore or Source/WebKit, not LayoutTests. Please remove.
Comment 40 Aurimas Liutikas 2013-01-31 11:13:22 PST
Created attachment 185821 [details]
Patch
Comment 41 Aurimas Liutikas 2013-01-31 11:17:10 PST
Comment on attachment 185795 [details]
Patch

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

>> Source/WebKit/chromium/ChangeLog:12
>> +        This bug already had a test that had expectation set to fail, but now it will be passing.
> 
> This explanation should probably be done in Source/WebCore/ChangeLog instead.

Done.

>> Source/WebCore/editing/Editor.cpp:1340
>> +    unsigned start, end;
> 
> WebKit style is to define each variable on its own line.

Done.

>> Source/WebKit/mac/WebView/WebHTMLView.mm:6048
>>          [[NSInputManager currentInputManager] markedTextSelectionChanged:NSMakeRange(start, end - start) client:self];
> 
> Now this code is executed even if hasComposition() and coreFrame->editor()->ignoreCompositionSelectionChange() are both false.
> r- because of this.

My refactoring does not really work for the Mac port (it does for all other ports) so I am leaving the code in this file as it was.

>> LayoutTests/ChangeLog:10
>> +        and the WebViewClient should get notified about this cancellation.
> 
> This comment applies to Source/WebCore or Source/WebKit, not LayoutTests. Please remove.

Done.
Comment 42 Eric Seidel (no email) 2013-01-31 11:49:35 PST
Your latest patch builds fine in Release on my Mac using build-webkit.  I assume that builds both wk1 and wk2.
Comment 43 Ryosuke Niwa 2013-01-31 12:02:08 PST
Comment on attachment 185821 [details]
Patch

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

> Source/WebKit/win/WebView.cpp:5486
> +    if (!targetFrame)
>          return;

I don't think focusedOrMainFrame can ever be null and it's obnoxious that it even checks this condition but that's a separate issue.
Comment 44 WebKit Review Bot 2013-01-31 13:51:51 PST
Comment on attachment 185821 [details]
Patch

Clearing flags on attachment: 185821

Committed r141479: <http://trac.webkit.org/changeset/141479>
Comment 45 WebKit Review Bot 2013-01-31 13:51:58 PST
All reviewed patches have been landed.  Closing bug.
Comment 46 Florin Malita 2013-01-31 15:04:27 PST
(In reply to comment #44)
> (From update of attachment 185821 [details])
> Clearing flags on attachment: 185821
> 
> Committed r141479: <http://trac.webkit.org/changeset/141479>

This is causing CR webkit_unit_tests failures:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=@ToT - chromium.org&testType=webkit_unit_tests&tests=WebViewTest.SetCompositionFromExistingText

http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux/builds/5060/steps/webkit_unit_tests/logs/SetCompositionFromExistingText
Comment 47 Hajime Morrita 2013-01-31 18:43:12 PST
It seems this also breaks content_browsertests 
http://build.chromium.org/p/chromium.webkit/builders/Win7%20%28dbg%29/builds/9181

Could you take a look or can I roll this out?
Comment 48 WebKit Review Bot 2013-01-31 18:58:27 PST
Re-opened since this is blocked by bug 108564
Comment 49 Aurimas Liutikas 2013-01-31 22:45:55 PST
It turns out that this patch exposed a bug in WebViewImpl::setCompositionFromExistingText(). Waiting for the fix to land and then I will re-land this patch.
Comment 50 Aurimas Liutikas 2013-01-31 23:08:36 PST
Comment on attachment 185821 [details]
Patch

The fix for WebViewImpl::SetCompositionFromExistingText() has landed. That fixes the issue that caused a revert for this patch. Can someone r+ and cq+ this again?

Thanks!
Comment 51 WebKit Review Bot 2013-01-31 23:44:46 PST
Comment on attachment 185821 [details]
Patch

Clearing flags on attachment: 185821

Committed r141545: <http://trac.webkit.org/changeset/141545>
Comment 52 WebKit Review Bot 2013-01-31 23:44:54 PST
All reviewed patches have been landed.  Closing bug.