mac: Almost fix Lion accented character bubble input in web contents.

mac: Almost fix Lion accented character bubble input in web contents.

Once webkit change https://bugs.webkit.org/show_bug.cgi?id=69846 is in,
it'll be fixed completely.

BUG=91003
TEST=press and hold a, then hit 1. An accented a should appear, the
unaccented a should disappear.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=105040

[Avi (use Gerrit), 2011-10-11 16:16:44]

nice!

http://codereview.chromium.org/8227018/diff/3009/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8227018/diff/3009/content/renderer/render_view...
content/renderer/render_view_impl.cc:4211: // TODO(thakis): Honor
|replacement_range|for plugins?
nit: space after second |?

[Nico, 2011-10-11 16:19:48]

http://codereview.chromium.org/8227018/diff/3009/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8227018/diff/3009/content/renderer/render_view...
content/renderer/render_view_impl.cc:4211: // TODO(thakis): Honor
|replacement_range|for plugins?
On 2011/10/11 16:16:44, Avi wrote:
> nit: space after second |?

Done.

[Avi (use Gerrit), 2011-10-11 23:58:36]

http://codereview.chromium.org/8227018/diff/3010/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/8227018/diff/3010/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_mac.mm:2493: [self
insertText:string replacementRange:NSMakeRange(0, 0)];
Does an NSRange of (0,0) convert to an invalid range on line 2481 above? Now
that we're passing that parameter along we should verify it's correct.

[Avi (use Gerrit), 2011-10-12 00:06:01]

http://codereview.chromium.org/8227018/diff/3010/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/8227018/diff/3010/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_mac.mm:2493: [self
insertText:string replacementRange:NSMakeRange(0, 0)];
According to ui::Range, this should be NSMakeRange(NSNotFound, 0) to convert to
an invalid range.

[Nico, 2011-10-12 00:10:36]

http://codereview.chromium.org/8227018/diff/3010/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/8227018/diff/3010/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_mac.mm:2493: [self
insertText:string replacementRange:NSMakeRange(0, 0)];
On 2011/10/12 00:06:01, Avi wrote:
> According to ui::Range, this should be NSMakeRange(NSNotFound, 0) to convert
to
> an invalid range.

Great catch, thanks! Done.

[Avi (use Gerrit), 2011-10-12 00:22:09]

lgtm

http://codereview.chromium.org/8227018/diff/6002/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/8227018/diff/6002/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_mac.mm:2473: //
TODO(suzhe): It's hard for us to support replacementRange without accessing
Is this comment meaningful?

[Nico, 2011-10-12 00:23:18]

http://codereview.chromium.org/8227018/diff/6002/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/8227018/diff/6002/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_mac.mm:2473: //
TODO(suzhe): It's hard for us to support replacementRange without accessing
On 2011/10/12 00:22:09, Avi wrote:
> Is this comment meaningful?

I don't know what it means. Maybe it makes sense for suzhe though :-)

[commit-bot: I haz the power, 2011-10-12 00:25:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thakis@chromium.org/8227018/6002

[commit-bot: I haz the power, 2011-10-12 00:25:25]

Presubmit check for 8227018-6002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
content/renderer/render_widget.cc,content/renderer/render_view_browsertest.cc,content/renderer/render_widget.h,content/renderer/render_view_impl.h,content/renderer/render_view_impl.cc,content/common/view_messages.h

Presubmit checks took 2.3s to calculate.

[Nico, 2011-10-12 00:28:33]

jamesr, can I get content/renderer/OWNERS approval please?

[Avi (use Gerrit), 2011-10-12 00:29:27]

http://codereview.chromium.org/8227018/diff/6002/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/8227018/diff/6002/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_mac.mm:2473: //
TODO(suzhe): It's hard for us to support replacementRange without accessing
Well, this patch _does_ implement replacementRange without accessing full web
content, in direct contradiction to this comment.

[Nico, 2011-10-12 00:34:30]

http://codereview.chromium.org/8227018/diff/6002/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/8227018/diff/6002/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_mac.mm:2473: //
TODO(suzhe): It's hard for us to support replacementRange without accessing
On 2011/10/12 00:29:27, Avi wrote:
> Well, this patch _does_ implement replacementRange without accessing full web
> content, in direct contradiction to this comment.

Only in the IME case, though.

[jamesr, 2011-10-12 00:34:51]

content/renderer LGTM w/ nit

http://codereview.chromium.org/8227018/diff/6002/content/renderer/render_widg...
File content/renderer/render_widget.cc (right):

http://codereview.chromium.org/8227018/diff/6002/content/renderer/render_widg...
content/renderer/render_widget.cc:1133: //
https://bugs.webkit.org/show_bug.cgi?id=69846 has landed.
A better way to do this is to guard this code with something like

#if WEBWIDGET_HAS_SELECTION_RANGE_CHANGES

and then #define that in the WebWidget.h change.  The way it goes live as soon
as the WebKit change is rolled in and all you need to do is come back at some
point and remove the #ifs and #defines

although it looks like the change may actually be on WebFrame or some other
object instead.

[commit-bot: I haz the power, 2011-10-12 00:38:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thakis@chromium.org/8227018/6002

[commit-bot: I haz the power, 2011-10-12 00:38:46]

Presubmit check for 8227018-6002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for: content/common/view_messages.h

Presubmit checks took 2.7s to calculate.

[Nico, 2011-10-12 00:41:40]

*sigh*

jam, can you approve for content/common/OWNERS, please?

[jam, 2011-10-12 03:05:27]

lgtm

[commit-bot: I haz the power, 2011-10-12 06:06:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thakis@chromium.org/8227018/6002

[commit-bot: I haz the power, 2011-10-12 08:45:46]

Change committed as 105040