Bug 67569 - Rename confirmCompositionWithoutDisturbingSelection to cancelComposition
Summary: Rename confirmCompositionWithoutDisturbingSelection to cancelComposition
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: Ryosuke Niwa
URL:
Keywords:
Depends on: 67377
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-03 16:37 PDT by Ryosuke Niwa
Modified: 2011-09-06 16:05 PDT (History)
8 users (show)

See Also:


Attachments
cleanup (16.61 KB, patch)
2011-09-06 12:49 PDT, Ryosuke Niwa
tonikitoo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2011-09-03 16:37:09 PDT
Per discussion on the bug 67377, we found that confirmCompositionWithoutDisturbingSelection is only used to cancel composition and the corresponding changeset http://trac.webkit.org/changeset/94497 made a change that enforces this notion.  We should rename this function to match the actual behavior now.
Comment 1 Ryosuke Niwa 2011-09-06 12:49:15 PDT
Created attachment 106469 [details]
cleanup
Comment 2 Kenneth Rohde Christiansen 2011-09-06 13:17:37 PDT
So what if I really want to confirm? We are depending a lot of this method in our slightly outdated branch as we commit the composition instead of cancelling it in many cases (ie. moving from one field to another etc)
Comment 3 Antonio Gomes 2011-09-06 13:20:21 PDT
(In reply to comment #2)
> So what if I really want to confirm? We are depending a lot of this method in our slightly outdated branch as we commit the composition instead of cancelling it in many cases (ie. moving from one field to another etc)

r+ upon kenneth's concern is not an issue.

I was about to ask the same: if one want to cancel selection without actually disturbing selection.
Comment 4 Kenneth Rohde Christiansen 2011-09-06 13:23:23 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > So what if I really want to confirm? We are depending a lot of this method in our slightly outdated branch as we commit the composition instead of cancelling it in many cases (ie. moving from one field to another etc)
> 
> r+ upon kenneth's concern is not an issue.
> 
> I was about to ask the same: if one want to cancel selection without actually disturbing selection.

We kind of need that. That is how our mobile platform works. We commit when loosing focus (could be just moving the cursor position with a touch in the same input field) and before injecting compositions into our virtual keyboard (used when the user touches a word and it becomes a composition)
Comment 5 Antonio Gomes 2011-09-06 13:25:43 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > So what if I really want to confirm? We are depending a lot of this method in our slightly outdated branch as we commit the composition instead of cancelling it in many cases (ie. moving from one field to another etc)
> > 
> > r+ upon kenneth's concern is not an issue.
> > 
> > I was about to ask the same: if one want to cancel selection without actually disturbing selection.
> 
> We kind of need that. That is how our mobile platform works. We commit when loosing focus (could be just moving the cursor position with a touch in the same input field) and before injecting compositions into our virtual keyboard (used when the user touches a word and it becomes a composition)

Although the patch is not chaning any actual behavior, but cleaning it up instead. So if it is/was broken for us with the patch, it will keep beeing.
Comment 6 Kenneth Rohde Christiansen 2011-09-06 13:28:28 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (In reply to comment #2)
> > > > So what if I really want to confirm? We are depending a lot of this method in our slightly outdated branch as we commit the composition instead of cancelling it in many cases (ie. moving from one field to another etc)
> > > 
> > > r+ upon kenneth's concern is not an issue.
> > > 
> > > I was about to ask the same: if one want to cancel selection without actually disturbing selection.
> > 
> > We kind of need that. That is how our mobile platform works. We commit when loosing focus (could be just moving the cursor position with a touch in the same input field) and before injecting compositions into our virtual keyboard (used when the user touches a word and it becomes a composition)
> 
> Although the patch is not chaning any actual behavior, but cleaning it up instead. So if it is/was broken for us with the patch, it will keep beeing.

Sure, but the behavior was changed without me being aware of it. Examples of use:

http://gitorious.org/+qtwebkit-webkit2-dev/webkit/qtwebkit-webkit2-dev/blobs/master/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.cpp

I think we are using it one other place, but I don't have the source handy right now.
Comment 7 Ryosuke Niwa 2011-09-06 14:31:33 PDT
(In reply to comment #6)
> Sure, but the behavior was changed without me being aware of it. Examples of use:
> 
> http://gitorious.org/+qtwebkit-webkit2-dev/webkit/qtwebkit-webkit2-dev/blobs/master/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.cpp
> 
> I think we are using it one other place, but I don't have the source handy right now.

Kenneth and I chatted over this and he's going to investigate further if he can work-around by using other API.

I'm going to land this patch for now because I don't want to leave the function name mismatching to its behavior. We can always add new API for Qt if it's necessary.
Comment 8 Ryosuke Niwa 2011-09-06 15:45:40 PDT
Comment on attachment 106469 [details]
cleanup

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

> Source/WebCore/editing/Editor.cpp:1494
> -    if (preserveSelection)
> +    if (mode == ConfirmComposition)

Oops, this obviously needs to be "mode == CancelComposition". Will fix before landing the patch.
Comment 9 Ryosuke Niwa 2011-09-06 16:05:30 PDT
Committed r94613: <http://trac.webkit.org/changeset/94613>