[Mac] Implement the system dictionary popup by implementing NSTextInput methods.

[Mac] Implement the system dictionary popup by implementing NSTextInput methods.

This is a two-sided patch; the Chromium side is plumbing to marshall data from
the renderer to the system APIs.

Note that just hitting Cmd+Ctrl+D usually does not bring up the popup. I think
this may be an Apple bug, but I have not yet found a work-around.

BUG=17951, 37715, 47141
TEST=Hold Cmd+Ctrl+D on a web page and mouse around. The dictionary popup should follow the mouse and show the definition of the current word.
TEST=In a text area, the dictionary popup should work only if the text area has focus.
R=avi,suzhe,jam

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

[Robert Sesek, 2011-01-19 16:38:42]

+pinkerton: general thoughts
+darin: plumbing & IPC; and if you wouldn't mind, take a look at the WebKit part
to make sure I'm not doing something stupid before I try upstreaming this.
+suzhe: NSTextInput changes
+CC/thakis & jeremy: FYI; feel free to comment

[Avi (use Gerrit), 2011-01-19 17:26:04]

lookup_in_dictionary.[h|mm] seems to be an integral part of RWHVM's text input
handling capabilities. It seems to me to be misnamed and I don't understand why
it lives in ui/cocoa/ rather than alongside RWHVM (if not within that file).

[Robert Sesek, 2011-01-19 17:52:03]

On 2011/01/19 17:26:04, Avi wrote:
> lookup_in_dictionary.[h|mm] seems to be an integral part of RWHVM's text input
> handling capabilities. It seems to me to be misnamed and I don't understand
why
> it lives in ui/cocoa/ rather than alongside RWHVM (if not within that file).

RWHVM is already huge, so I don't want to merge it with that. But I could see
moving it to chrome/browser/renderer_host/.

[James Su, 2011-01-19 18:57:19]

Can we generalize the code of lookup_in_dictionary.* and WebTextHelper.*, so
that other platforms can use it as well? We need similar functionalities to fix
issues like 45605 and 55130.

[Robert Sesek, 2011-01-19 19:52:24]

On 2011/01/19 18:57:19, James Su wrote:
> Can we generalize the code of lookup_in_dictionary.* and WebTextHelper.*, so
> that other platforms can use it as well? We need similar functionalities to
fix
> issues like 45605 and 55130.

The code in WebTextHelper is likely reusable for other platforms (minus
WebTextHelper::substringInRange which uses a Cocoa string). I just put it in the
mac/ subdir for proper scoping. It could be split up and moved in the future,
though.

LookupInDictionary could be made more generalized to support waiting on any data
type, I think it would be unwise to do so because it could encourage this sort
of synchronous browser->renderer messaging if LookupInDictionary turned into a
generic sync pipeline. This is a very specific case that required approval and
discussion to do, and I think it's best left in the concrete form. I'm not
familiar with Japanese text reconversion, but it could also use this same
pattern if necessary. A better option (that was considered for this) is to
handle the action/key event in the renderer and just send an async message to
the browser with data and a message to show whatever UI is necessary. The design
doc for the dictionary popup explains why I didn't do that for this, though it
would be a better option if it could be made to work right. This also has the
benefit of getting NSTextInput more thoroughly implemented, though.

[James Su, 2011-01-19 20:29:05]

I'll give you detailed feedback after reviewing the CL thoroughly. Following are
two quick points:
1. You probably need to implement -selectedRange and -markedRange correctly,
otherwise it may cause problem with some input methods.
2. I'd prefer to keep LookupInDictionary for text service only, but maybe we can
get a little more generalized name for it.

http://codereview.chromium.org/6289009/diff/1/chrome/browser/renderer_host/re...
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/6289009/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:2357:
service->AfterRequest();
I'm wondering if it's possible to move this piece of code into
LookupInDictionary. Same as other similar changes.

[Robert Sesek, 2011-01-20 15:06:32]

On 2011/01/19 20:29:05, James Su wrote:
> I'll give you detailed feedback after reviewing the CL thoroughly. Following
are
> two quick points:
> 1. You probably need to implement -selectedRange and -markedRange correctly,
> otherwise it may cause problem with some input methods.
> 2. I'd prefer to keep LookupInDictionary for text service only, but maybe we
can
> get a little more generalized name for it.

1. Have I regressed the behavior with this change? I'm not familiar with how
marked and selected text play together within the context of
|-firstRectForCharacterRange:|.

If I did regress the behavior, I'd prefer to not add any more synchronous
messages if possible, so I'll see if I can instead send an async selection
changed message from the renderer. If the behavior hasn't regressed, would you
mind me pushing out that change to a separate CL? Is there a good way for me to
test input method composition (does inserting the ü character exercise the right
code path, for example)?

2. I didn't understand this at first, but now I get it, especially in the
context of (1). I've rename LookupInDictionary to TextInputClientMac and put it
in chrome/renderer_host/.

http://codereview.chromium.org/6289009/diff/1/chrome/browser/renderer_host/re...
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/6289009/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:2357:
service->AfterRequest();
On 2011/01/19 20:29:06, James Su wrote:
> I'm wondering if it's possible to move this piece of code into
> LookupInDictionary. Same as other similar changes.

I was wondering the same thing. I could add a IPC::ChannelProxy::MessageFilter
just for these 3 messages to centralize the logic. I was hoping Darin could
weigh in on that.

[James Su, 2011-01-20 17:50:13]

On 2011/01/20 15:06:32, rsesek wrote:
> On 2011/01/19 20:29:05, James Su wrote:
> > I'll give you detailed feedback after reviewing the CL thoroughly. Following
> are
> > two quick points:
> > 1. You probably need to implement -selectedRange and -markedRange correctly,
> > otherwise it may cause problem with some input methods.
> > 2. I'd prefer to keep LookupInDictionary for text service only, but maybe we
> can
> > get a little more generalized name for it.
> 
> 1. Have I regressed the behavior with this change? I'm not familiar with how
> marked and selected text play together within the context of
> |-firstRectForCharacterRange:|.
I haven't test your CL yet. But according to the definition of NSTextInput
interface, this CL may break some input methods. For example, an input method
may call -selectedRange to get the current selected range and then call
-firstRectForCharacterRange: with the that range to get its position on the
screen. But because the range returned by -selectedRange is not correct, this
feature may be broken.
But we actually don't need to use sync call for selectedRange and markedRange.
Instead, the renderer may send a notification message to the browser whenever
the range is changed, so that we can cache the result in the browser.

> 
> If I did regress the behavior, I'd prefer to not add any more synchronous
> messages if possible, so I'll see if I can instead send an async selection
> changed message from the renderer. If the behavior hasn't regressed, would you
> mind me pushing out that change to a separate CL? Is there a good way for me
to
> test input method composition (does inserting the ü character exercise the
right
> code path, for example)?
> 
> 2. I didn't understand this at first, but now I get it, especially in the
> context of (1). I've rename LookupInDictionary to TextInputClientMac and put
it
> in chrome/renderer_host/.
Good. And it would be better if it can be generalized to support other
platforms. But I'm ok if you'd prefer to do it later.

> 
>
http://codereview.chromium.org/6289009/diff/1/chrome/browser/renderer_host/re...
> File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):
> 
>
http://codereview.chromium.org/6289009/diff/1/chrome/browser/renderer_host/re...
> chrome/browser/renderer_host/render_widget_host_view_mac.mm:2357:
> service->AfterRequest();
> On 2011/01/19 20:29:06, James Su wrote:
> > I'm wondering if it's possible to move this piece of code into
> > LookupInDictionary. Same as other similar changes.
> 
> I was wondering the same thing. I could add a IPC::ChannelProxy::MessageFilter
> just for these 3 messages to centralize the logic. I was hoping Darin could
> weigh in on that.

[Robert Sesek, 2011-01-20 19:17:40]

I've properly implemented |-selectedRange| by plumbing that info down with
ViewHostMsg_SelectionChanged, so now it's async and cached in RWHVM.

On 2011/01/20 17:50:13, James Su wrote:
> Good. And it would be better if it can be generalized to support other
> platforms. But I'm ok if you'd prefer to do it later.

I'm not familiar enough with the needs of other platforms in this area, so I
think leaving this Mac-only for now is for the best.

[James Su, 2011-01-20 23:46:05]

I tried to test this CL on my Mac, but the renderer crashed as soon as I tried
to input something with an input method. I'll try to figure out the problem and
get back to you later.
But I believe a correct implementation of -markedRange is also required for
supporting input methods.

And though according to Apple's documentation, -selectedRange should return
{NSNotFound, 0} if there is no selection, it actually returns the caret position
when selection length is zero. -markedRange has the same behavior.

http://codereview.chromium.org/6289009/diff/1/chrome/browser/renderer_host/re...
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/6289009/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:2357:
service->AfterRequest();
On 2011/01/20 15:06:33, rsesek wrote:
> On 2011/01/19 20:29:06, James Su wrote:
> > I'm wondering if it's possible to move this piece of code into
> > LookupInDictionary. Same as other similar changes.
> 
> I was wondering the same thing. I could add a IPC::ChannelProxy::MessageFilter
> just for these 3 messages to centralize the logic. I was hoping Darin could
> weigh in on that.
I mean is it possible to simplify this part to something like:
return service->GetCharacterIndex(renderWidgetHostView_, thePoint.x,
thePoint.y);

http://codereview.chromium.org/6289009/diff/40001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:2384: return
hasMarkedText_ ? selectedRange_ : NSMakeRange(NSNotFound, 0);
Ｗe probably can do an optimization here:
Usually when the user is using an input method, this method will be called
frequently by the input method to get the position of the selected or marked
range. So if we can send the rects of selected and marked range from the
renderer along with the selected and marked range and cache them in this object,
then we usually can just return the cached value here rather than doing a sync
all again.

http://codereview.chromium.org/6289009/diff/40001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:2389: // a text
being composed when hasMarkedText call returns true.
How about to use @synthesize for this property?

http://codereview.chromium.org/6289009/diff/40001/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/6289009/diff/40001/chrome/renderer/render_view...
chrome/renderer/render_view.cc:2262: last_selection_, 0, 0));
As caretRect_ in RWHVM is not used anymore. We need to return the caret position
here, which is just like a zero length selection. And we need to make sure it'll
be updated correctly whenever the caret moves.

[James Su, 2011-01-21 00:02:46]

Just figured out the reason of crash, and confirmed that we need to implement
-markedRange to support input methods.

http://codereview.chromium.org/6289009/diff/40001/third_party/WebKit/Source/W...
File third_party/WebKit/Source/WebKit/chromium/src/mac/WebTextHelper.mm (right):

http://codereview.chromium.org/6289009/diff/40001/third_party/WebKit/Source/W...
third_party/WebKit/Source/WebKit/chromium/src/mac/WebTextHelper.mm:189:
RefPtr<Range>
range(TextIterator::rangeFromLocationAndLength(m_private->rangeScope(),
location, length));
range could be NULL if the given location is out of the content range (eg. when
there is no content at all). But in this case, we still need to return the caret
rect.

http://codereview.chromium.org/6289009/diff/40001/third_party/WebKit/Source/W...
third_party/WebKit/Source/WebKit/chromium/src/mac/WebTextHelper.mm:205:
RefPtr<Range>
range(TextIterator::rangeFromLocationAndLength(m_private->rangeScope(),
location, length));
range could be NULL.

[jam, 2011-01-21 18:51:07]

http://codereview.chromium.org/6289009/diff/40001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_message_filter.h (right):

http://codereview.chromium.org/6289009/diff/40001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_message_filter.h:203: void
OnGotCharacterIndexForPoint(const uint index);
nit: for input parameters in IPC message dispatchers, when there are simple
types like ints the convention is to drop the "const"

http://codereview.chromium.org/6289009/diff/40001/chrome/common/render_messag...
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/6289009/diff/40001/chrome/common/render_messag...
chrome/common/render_messages_internal.h:1672: uint)
nit: convention is to have each parameter on a separate lines, and add in
comments the parameter_name

http://codereview.chromium.org/6289009/diff/40001/third_party/WebKit/Source/W...
File third_party/WebKit/Source/WebKit/chromium/public/mac/WebTextHelper.h
(right):

http://codereview.chromium.org/6289009/diff/40001/third_party/WebKit/Source/W...
third_party/WebKit/Source/WebKit/chromium/public/mac/WebTextHelper.h:1: /*
you need to add the new files to 
third_party/WebKit/Source/WebKit/chromium/WebKit.gyp

http://codereview.chromium.org/6289009/diff/40001/third_party/WebKit/Source/W...
third_party/WebKit/Source/WebKit/chromium/public/mac/WebTextHelper.h:47: //
class computes the required information to formulate the reply.
the webkit layer doesn't know anything about IPC, so we don't mention that in
comments here.  it can be used in a single process, i.e. testshell or drt

http://codereview.chromium.org/6289009/diff/40001/third_party/WebKit/Source/W...
File third_party/WebKit/Source/WebKit/chromium/src/mac/WebTextHelper.mm (right):

http://codereview.chromium.org/6289009/diff/40001/third_party/WebKit/Source/W...
third_party/WebKit/Source/WebKit/chromium/src/mac/WebTextHelper.mm:1: /*
nit: usually we have the interface in the public directory, like what you have. 
there are then two corresponding Impl.h/cpp/mm files in the src directory.

[jam, 2011-01-21 19:01:28]

also, perhaps these messages should go into their own X_messages.h files, so
that we don't keep expanding render_messages.h which is already very big.  the
message dispatching can also happen in a new class which derives from
RenderViewObserver, to avoid increasing RenderView (the goal is to make it
smaller now)

[James Su, 2011-01-21 21:28:11]

Some additional thoughts

1. Although we are currently using NSTextInput interface, we'll eventually
migrate to NSTextInputClient. So it'be better to support necessary functionality
in renderer/webkit. Specifically, the additional actualRange return value of
firstRectForCharacterRange and attributedSubstringForProposedRange methods. And
this additional return value is also useful for other platforms.

2. According to the API reference, this method will only return the rect of the
first line if the selection spans multiple lines. In this case, if the client
(eg. an input method) wants to know rects of all lines, it'll call this method
repeatedly for each lines. I'm not sure if it may cause any problem, as it's
sync call. But it would definitely be better if we can reduce the number of sync
calls by using some kind of cache mechanism.

3. I'm wondering if it's better to merge WebTextHelper into
chromium/public/WebWidget.h as platform independent APIs.

[Robert Sesek, 2011-01-21 23:40:06]

I've addressed all of John's feedback and some of James' -- thank you! I'm still
looking into the IME stuff, so that's not done yet.

On 2011/01/21 21:28:11, James Su wrote:
> Some additional thoughts
> 2. According to the API reference, this method will only return the rect of
the
> first line if the selection spans multiple lines. In this case, if the client
> (eg. an input method) wants to know rects of all lines, it'll call this method
> repeatedly for each lines. I'm not sure if it may cause any problem, as it's
> sync call. But it would definitely be better if we can reduce the number of
sync
> calls by using some kind of cache mechanism.

It won't cause a problem with this architecture. But sending rects that aren't
asked for may. I think we need to see how it performs (once the UMA data comes
in for the upper bound) to determine if we need to optimize this.

> 3. I'm wondering if it's better to merge WebTextHelper into
> chromium/public/WebWidget.h as platform independent APIs.

I think that this would be a good thing (as would be removing the old IME stuff
from WebFrame that we talked about on IRC). The only issue is the
substringInRange() which needs to return an NSAttributedString. I'm not sure how
we go about making that cross-platform.

http://codereview.chromium.org/6289009/diff/1/chrome/browser/renderer_host/re...
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/6289009/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:2357:
service->AfterRequest();
On 2011/01/20 23:46:05, James Su wrote:
> On 2011/01/20 15:06:33, rsesek wrote:
> > On 2011/01/19 20:29:06, James Su wrote:
> > > I'm wondering if it's possible to move this piece of code into
> > > LookupInDictionary. Same as other similar changes.
> > 
> > I was wondering the same thing. I could add a
IPC::ChannelProxy::MessageFilter
> > just for these 3 messages to centralize the logic. I was hoping Darin could
> > weigh in on that.
> I mean is it possible to simplify this part to something like:
> return service->GetCharacterIndex(renderWidgetHostView_, thePoint.x,
> thePoint.y);

Done.

http://codereview.chromium.org/6289009/diff/40001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_message_filter.h (right):

http://codereview.chromium.org/6289009/diff/40001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_message_filter.h:203: void
OnGotCharacterIndexForPoint(const uint index);
On 2011/01/21 18:51:07, John Abd-El-Malek wrote:
> nit: for input parameters in IPC message dispatchers, when there are simple
> types like ints the convention is to drop the "const"

Done.

http://codereview.chromium.org/6289009/diff/40001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/6289009/diff/40001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:2389: selectedRange_
= range;
On 2011/01/20 23:46:06, James Su wrote:
> How about to use @synthesize for this property?

Done.

http://codereview.chromium.org/6289009/diff/40001/chrome/common/render_messag...
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/6289009/diff/40001/chrome/common/render_messag...
chrome/common/render_messages_internal.h:1672: uint)
On 2011/01/21 18:51:07, John Abd-El-Malek wrote:
> nit: convention is to have each parameter on a separate lines, and add in
> comments the parameter_name

Done.

http://codereview.chromium.org/6289009/diff/40001/third_party/WebKit/Source/W...
File third_party/WebKit/Source/WebKit/chromium/public/mac/WebTextHelper.h
(right):

http://codereview.chromium.org/6289009/diff/40001/third_party/WebKit/Source/W...
third_party/WebKit/Source/WebKit/chromium/public/mac/WebTextHelper.h:1: /*
On 2011/01/21 18:51:07, John Abd-El-Malek wrote:
> you need to add the new files to 
> third_party/WebKit/Source/WebKit/chromium/WebKit.gyp

Yup, but I can't track that file under git, so I have it patched locally.

http://codereview.chromium.org/6289009/diff/40001/third_party/WebKit/Source/W...
third_party/WebKit/Source/WebKit/chromium/public/mac/WebTextHelper.h:47: //
class computes the required information to formulate the reply.
On 2011/01/21 18:51:07, John Abd-El-Malek wrote:
> the webkit layer doesn't know anything about IPC, so we don't mention that in
> comments here.  it can be used in a single process, i.e. testshell or drt

Done.

http://codereview.chromium.org/6289009/diff/40001/third_party/WebKit/Source/W...
File third_party/WebKit/Source/WebKit/chromium/src/mac/WebTextHelper.mm (right):

http://codereview.chromium.org/6289009/diff/40001/third_party/WebKit/Source/W...
third_party/WebKit/Source/WebKit/chromium/src/mac/WebTextHelper.mm:1: /*
On 2011/01/21 18:51:07, John Abd-El-Malek wrote:
> nit: usually we have the interface in the public directory, like what you
have. 
> there are then two corresponding Impl.h/cpp/mm files in the src directory.

Ok. I won't do that now because it's easier for this to be contained in one
place while developing, but I'll do that before upstreaming.

http://codereview.chromium.org/6289009/diff/40001/third_party/WebKit/Source/W...
third_party/WebKit/Source/WebKit/chromium/src/mac/WebTextHelper.mm:189:
RefPtr<Range>
range(TextIterator::rangeFromLocationAndLength(m_private->rangeScope(),
location, length));
On 2011/01/21 00:02:47, James Su wrote:
> range could be NULL if the given location is out of the content range (eg.
when
> there is no content at all). But in this case, we still need to return the
caret
> rect.

Taken care of the NULL dereference. But I don't think we should return an
arbitrary rect (the caret rect) if it's not found. We should probably handle
that in render_view instead.

http://codereview.chromium.org/6289009/diff/40001/third_party/WebKit/Source/W...
third_party/WebKit/Source/WebKit/chromium/src/mac/WebTextHelper.mm:205:
RefPtr<Range>
range(TextIterator::rangeFromLocationAndLength(m_private->rangeScope(),
location, length));
On 2011/01/21 00:02:47, James Su wrote:
> range could be NULL.

Done.

[James Su, 2011-02-11 03:33:06]

Hello,
  Sorry for late reply. I'm currently travelling in China. I'll review this CL
again ASAP.
  For the problems I mentioned before, it's probably ok for me to use this Mac
specific solution for now and migrate to a platform independent one later. But
at least we need to support markedRange properly to avoid breaking input
methods.

Regards
James Su

[Robert Sesek, 2011-02-16 21:31:00]

Sorry for the delay in getting back to this; I've been working on other things
for M11. In this latest patch set, I've made some strides at better IME support.
James: if you could please take a look at that. There's still some issue with
the firstRectForRange when editing, and I can't seem to pin it down.

Also, I've uploaded the rest of the WebKit side here:
http://codereview.chromium.org/6532004. Eventually I'll move WebTextHelper into
that CL for upstreaming, but I currently have a bunch of git history for it in
this CL, so I don't yet want to do that.

[James Su, 2011-02-17 02:03:00]

What issue did you encounter with input method?

http://codereview.chromium.org/6289009/diff/70001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/6289009/diff/70001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:836: [cocoa_view_
setMarkedRange:NSMakeRange(start, end - start)];
Do we need to care about invalid range like the code in SelectionChanged()
below? Please comment the reason if it's not necessary.

http://codereview.chromium.org/6289009/diff/70001/chrome/renderer/render_widg...
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/6289009/diff/70001/chrome/renderer/render_widg...
chrome/renderer/render_widget.cc:888: range.startOffset(), range.endOffset()));
We need this code in RenderWidget::resetInputMethod() as well.

[Robert Sesek, 2011-02-17 20:05:04]

On 2011/02/17 02:03:00, James Su wrote:
> What issue did you encounter with input method?

It seems that the rect returned by firstRectForRange() isn't always right with
IME. It sometimes draws the IME popup in the wrong spot because of this.

http://codereview.chromium.org/6289009/diff/70001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/6289009/diff/70001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:836: [cocoa_view_
setMarkedRange:NSMakeRange(start, end - start)];
On 2011/02/17 02:03:00, James Su wrote:
> Do we need to care about invalid range like the code in SelectionChanged()
> below? Please comment the reason if it's not necessary.

No; added a comment as to why.

http://codereview.chromium.org/6289009/diff/70001/chrome/renderer/render_widg...
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/6289009/diff/70001/chrome/renderer/render_widg...
chrome/renderer/render_widget.cc:888: range.startOffset(), range.endOffset()));
On 2011/02/17 02:03:00, James Su wrote:
> We need this code in RenderWidget::resetInputMethod() as well.

Done.

[James Su, 2011-02-21 02:03:14]

LGTM, as long as you are going to replace it with a platform independent
solution in a short term.

On 2011/02/17 20:05:04, rsesek wrote:
> On 2011/02/17 02:03:00, James Su wrote:
> > What issue did you encounter with input method?
> 
> It seems that the rect returned by firstRectForRange() isn't always right with
> IME. It sometimes draws the IME popup in the wrong spot because of this.
It'll be best if you could figure out the problem before checking in this CL.

> 
>
http://codereview.chromium.org/6289009/diff/70001/chrome/browser/renderer_hos...
> File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):
> 
>
http://codereview.chromium.org/6289009/diff/70001/chrome/browser/renderer_hos...
> chrome/browser/renderer_host/render_widget_host_view_mac.mm:836: [cocoa_view_
> setMarkedRange:NSMakeRange(start, end - start)];
> On 2011/02/17 02:03:00, James Su wrote:
> > Do we need to care about invalid range like the code in SelectionChanged()
> > below? Please comment the reason if it's not necessary.
> 
> No; added a comment as to why.
> 
>
http://codereview.chromium.org/6289009/diff/70001/chrome/renderer/render_widg...
> File chrome/renderer/render_widget.cc (right):
> 
>
http://codereview.chromium.org/6289009/diff/70001/chrome/renderer/render_widg...
> chrome/renderer/render_widget.cc:888: range.startOffset(),
range.endOffset()));
> On 2011/02/17 02:03:00, James Su wrote:
> > We need this code in RenderWidget::resetInputMethod() as well.
> 
> Done.

[jeremy, 2011-02-21 09:19:46]

http://codereview.chromium.org/6289009/diff/74001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/6289009/diff/74001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:835: void
RenderWidgetHostViewMac::ImeCompositionRangeChanged(int start, int end) {
Why are start & end signed?  The arguments to NSMakeRange() are unsigned.

Also, it might be a good idea to do some strict validation of these when they
come over IPC to make sure no funny business is happening on the renderer side.

http://codereview.chromium.org/6289009/diff/74001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:2467: NSLog(@"%s:%@
-> %d", _cmd, NSStringFromPoint(thePoint), point);
Remove NSLog? Also, below

http://codereview.chromium.org/6289009/diff/74001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/text_input_client_mac.h (right):

http://codereview.chromium.org/6289009/diff/74001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/text_input_client_mac.h:32: // and condition
(managed by this service).
Could you explain why it's OK to do this in this instance?

http://codereview.chromium.org/6289009/diff/74001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/text_input_client_mac_unittest.mm (right):

http://codereview.chromium.org/6289009/diff/74001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/text_input_client_mac_unittest.mm:159:
EXPECT_NSEQ(kSuccessValue, string);
Does this test that the string attributes are the same or just the string
contents?

http://codereview.chromium.org/6289009/diff/74001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/text_input_client_message_filter.mm (right):

http://codereview.chromium.org/6289009/diff/74001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/text_input_client_message_filter.mm:42: void
TextInputClientMessageFilter::OnGotCharacterIndexForPoint(uint index) {
Any point in asserting that these methods are called on the IO thread?

http://codereview.chromium.org/6289009/diff/74001/chrome/common/render_messag...
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/6289009/diff/74001/chrome/common/render_messag...
chrome/common/render_messages_internal.h:1642: int /* selection range start */,
Again, should these be unsigned?

http://codereview.chromium.org/6289009/diff/74001/third_party/WebKit/Source/W...
File third_party/WebKit/Source/WebKit/chromium/src/mac/WebTextHelper.mm (right):

http://codereview.chromium.org/6289009/diff/74001/third_party/WebKit/Source/W...
third_party/WebKit/Source/WebKit/chromium/src/mac/WebTextHelper.mm:250: NSData*
archivedData([NSArchiver archivedDataWithRootObject:string]);
If archiveData is nil , is this codepath safe?

[Robert Sesek, 2011-02-24 23:26:52]

I've now split WebTextHelper out of this patch set and am going to upstream this
CL tomorrow: http://codereview.chromium.org/6532004/. Do any of you have advice
on possible WebKit reviewers?

http://codereview.chromium.org/6289009/diff/74001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/6289009/diff/74001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:835: void
RenderWidgetHostViewMac::ImeCompositionRangeChanged(int start, int end) {
On 2011/02/21 09:19:47, jeremy wrote:
> Why are start & end signed?  The arguments to NSMakeRange() are unsigned.
> 
> Also, it might be a good idea to do some strict validation of these when they
> come over IPC to make sure no funny business is happening on the renderer
side.

WebRange uses int rather than uint, so that's why I transport them this way. Can
you advise on some sanity checks? Obviously >0 is a good one, but is <INT_MAX a
good upper bound, too?

http://codereview.chromium.org/6289009/diff/74001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:2467: NSLog(@"%s:%@
-> %d", _cmd, NSStringFromPoint(thePoint), point);
On 2011/02/21 09:19:47, jeremy wrote:
> Remove NSLog? Also, below

I'll do this for landing, but I'm still working on some things so the debug info
is helpful.

http://codereview.chromium.org/6289009/diff/74001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/text_input_client_mac.h (right):

http://codereview.chromium.org/6289009/diff/74001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/text_input_client_mac.h:32: // and condition
(managed by this service).
On 2011/02/21 09:19:47, jeremy wrote:
> Could you explain why it's OK to do this in this instance?

I think the first sentence explains this.

http://codereview.chromium.org/6289009/diff/74001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/text_input_client_mac_unittest.mm (right):

http://codereview.chromium.org/6289009/diff/74001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/text_input_client_mac_unittest.mm:159:
EXPECT_NSEQ(kSuccessValue, string);
On 2011/02/21 09:19:47, jeremy wrote:
> Does this test that the string attributes are the same or just the string
> contents?

This does -[NSObject isEqual:]

http://codereview.chromium.org/6289009/diff/74001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/text_input_client_message_filter.mm (right):

http://codereview.chromium.org/6289009/diff/74001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/text_input_client_message_filter.mm:42: void
TextInputClientMessageFilter::OnGotCharacterIndexForPoint(uint index) {
On 2011/02/21 09:19:47, jeremy wrote:
> Any point in asserting that these methods are called on the IO thread?

Not really. MessageFilters get dispatched on the IO thread pretty much by
design.

http://codereview.chromium.org/6289009/diff/74001/third_party/WebKit/Source/W...
File third_party/WebKit/Source/WebKit/chromium/src/mac/WebTextHelper.mm (right):

http://codereview.chromium.org/6289009/diff/74001/third_party/WebKit/Source/W...
third_party/WebKit/Source/WebKit/chromium/src/mac/WebTextHelper.mm:250: NSData*
archivedData([NSArchiver archivedDataWithRootObject:string]);
On 2011/02/21 09:19:47, jeremy wrote:
> If archiveData is nil , is this codepath safe?

Added a nil check.

[jam, 2011-02-25 18:08:06]

http://codereview.chromium.org/6289009/diff/78001/chrome/common/text_input_cl...
File chrome/common/text_input_client_messages_internal.h (right):

http://codereview.chromium.org/6289009/diff/78001/chrome/common/text_input_cl...
chrome/common/text_input_client_messages_internal.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
you don't need a .h and _internal.h anymore.  that was the old way.  see
file_utilities_messages.h as an example

http://codereview.chromium.org/6289009/diff/78001/chrome/common/text_input_cl...
chrome/common/text_input_client_messages_internal.h:7: #include "ui/gfx/point.h"
nit: if you include rect, no need to include point

[jam, 2011-02-25 18:15:28]

http://codereview.chromium.org/6289009/diff/78001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/text_input_client_message_filter.mm (right):

http://codereview.chromium.org/6289009/diff/78001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/text_input_client_message_filter.mm:71:
RenderViewHost* rvh =
RenderViewHost is not threadsafe.  Your message filter's dispatcher gets called
on the IO thread, so you can't do this.

you can always implement OverrideThreadForMessage and pass in UI for the thread,
that way your function dispatcher will get called on the UI thread for that
message.

[Robert Sesek, 2011-02-25 18:47:12]

Thanks for the feedback, John. All comments addressed.

http://codereview.chromium.org/6289009/diff/78001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/text_input_client_message_filter.mm (right):

http://codereview.chromium.org/6289009/diff/78001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/text_input_client_message_filter.mm:71:
RenderViewHost* rvh =
On 2011/02/25 18:15:28, John Abd-El-Malek wrote:
> RenderViewHost is not threadsafe.  Your message filter's dispatcher gets
called
> on the IO thread, so you can't do this.
> 
> you can always implement OverrideThreadForMessage and pass in UI for the
thread,
> that way your function dispatcher will get called on the UI thread for that
> message.

Clever. Done.

http://codereview.chromium.org/6289009/diff/78001/chrome/common/text_input_cl...
File chrome/common/text_input_client_messages_internal.h (right):

http://codereview.chromium.org/6289009/diff/78001/chrome/common/text_input_cl...
chrome/common/text_input_client_messages_internal.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2011/02/25 18:08:06, John Abd-El-Malek wrote:
> you don't need a .h and _internal.h anymore.  that was the old way.  see
> file_utilities_messages.h as an example

Done.

http://codereview.chromium.org/6289009/diff/78001/chrome/common/text_input_cl...
chrome/common/text_input_client_messages_internal.h:7: #include "ui/gfx/point.h"
On 2011/02/25 18:08:06, John Abd-El-Malek wrote:
> nit: if you include rect, no need to include point

Done.

[jam, 2011-02-25 19:15:15]

my nits lgtm with the one below

http://codereview.chromium.org/6289009/diff/85001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/text_input_client_message_filter.mm (right):

http://codereview.chromium.org/6289009/diff/85001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/text_input_client_message_filter.mm:51:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
please don't add these dchecks, they don't add much value.  we know that by
definition, filter functions are called on the IO thread unless a message's
thread is overriden.  adding dchecks here is like testing BrowserMessageFilter,
but the place to do that is not in derived classes.

[James Su, 2011-02-28 19:27:59]

http://codereview.chromium.org/6289009/diff/85001/chrome/renderer/text_input_...
File chrome/renderer/text_input_client_observer.cc (right):

http://codereview.chromium.org/6289009/diff/85001/chrome/renderer/text_input_...
chrome/renderer/text_input_client_observer.cc:44: uint index =
webview()->mainFrame()->characterIndexForPoint(web_point);
webview()->focusedFrame() is used in render_view.cc. Is it correct to use
mainFrame() in this file?

[James Su, 2011-02-28 19:44:04]

http://codereview.chromium.org/6289009/diff/85001/chrome/renderer/text_input_...
File chrome/renderer/text_input_client_observer.cc (right):

http://codereview.chromium.org/6289009/diff/85001/chrome/renderer/text_input_...
chrome/renderer/text_input_client_observer.cc:53:
frame->firstRectForCharacterRange(location, length, web_rect);
Do we need to call frame->view()->contentsToWindow(web_rect) to convert to
coordinates?

[James Su, 2011-03-03 04:10:31]

http://codereview.chromium.org/6289009/diff/85001/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/6289009/diff/85001/chrome/renderer/render_view...
chrome/renderer/render_view.cc:2248: WebRange range = frame->selectionRange();
IMHO we should use the same code path regardless of is_empty_selection's value.
And please refer to the comment I added in
http://codereview.chromium.org/6532004/.

[Robert Sesek, 2011-03-18 21:33:15]

I re-plumbed a lot of the methods to use ui::Range.

jam: I also added IPC param traits for ui::Range if you want to review that.

http://codereview.chromium.org/6289009/diff/85001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/text_input_client_message_filter.mm (right):

http://codereview.chromium.org/6289009/diff/85001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/text_input_client_message_filter.mm:51:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
On 2011/02/25 19:15:15, John Abd-El-Malek wrote:
> please don't add these dchecks, they don't add much value.  we know that by
> definition, filter functions are called on the IO thread unless a message's
> thread is overriden.  adding dchecks here is like testing
BrowserMessageFilter,
> but the place to do that is not in derived classes.

Removed all the DCHECKs.

http://codereview.chromium.org/6289009/diff/85001/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/6289009/diff/85001/chrome/renderer/render_view...
chrome/renderer/render_view.cc:2248: WebRange range = frame->selectionRange();
On 2011/03/03 04:10:31, James Su wrote:
> IMHO we should use the same code path regardless of is_empty_selection's
value.
> And please refer to the comment I added in
> http://codereview.chromium.org/6532004/.

Done.

http://codereview.chromium.org/6289009/diff/85001/chrome/renderer/text_input_...
File chrome/renderer/text_input_client_observer.cc (right):

http://codereview.chromium.org/6289009/diff/85001/chrome/renderer/text_input_...
chrome/renderer/text_input_client_observer.cc:44: uint index =
webview()->mainFrame()->characterIndexForPoint(web_point);
On 2011/02/28 19:28:00, James Su wrote:
> webview()->focusedFrame() is used in render_view.cc. Is it correct to use
> mainFrame() in this file?

Should be focusedFrame(). Done.

http://codereview.chromium.org/6289009/diff/85001/chrome/renderer/text_input_...
chrome/renderer/text_input_client_observer.cc:53:
frame->firstRectForCharacterRange(location, length, web_rect);
On 2011/02/28 19:44:04, James Su wrote:
> Do we need to call frame->view()->contentsToWindow(web_rect) to convert to
> coordinates?

No, it returns in window coordinates.

[Robert Sesek, 2011-04-11 20:15:26]

Please take another look Jeremy, Avi, and/or John. I've added a new class
AttributedStringCoder, which will serialize the data in an NSAttributedString
for sending across the process boundary.

[Avi (use Gerrit), 2011-04-11 20:22:16]

Attributed string LGTM

http://codereview.chromium.org/6289009/diff/101001/chrome/common/attributed_s...
File chrome/common/attributed_string_coder.h (right):

http://codereview.chromium.org/6289009/diff/101001/chrome/common/attributed_s...
chrome/common/attributed_string_coder.h:61: // A class that contains the
pertitent information from an NSAttributedString,
pertinent

[jeremy, 2011-04-12 09:44:06]

http://codereview.chromium.org/6289009/diff/101001/chrome/common/attributed_s...
File chrome/common/attributed_string_coder.mm (right):

http://codereview.chromium.org/6289009/diff/101001/chrome/common/attributed_s...
chrome/common/attributed_string_coder.mm:76: font_name_ =
base::SysNSStringToUTF8([font fontName]);
Could you use a FontDescriptor (content/common/font_descriptor_mac.mm) here,
since that class is supposed to encapsulate sending an NSFont over IPC.

http://codereview.chromium.org/6289009/diff/101001/chrome/common/attributed_s...
chrome/common/attributed_string_coder.mm:109: // IPC ParamTraits specialization
//////////////////////////////////////////////
When deserializing values here, could you add some paranoid sanity checks?  If
the renderer sends junk over, we don't want it getting to Cocoa...

esp. probably worth checking that the range is legal, font sizes, etc.

[Robert Sesek, 2011-04-25 20:58:18]

http://codereview.chromium.org/6289009/diff/101001/chrome/common/attributed_s...
File chrome/common/attributed_string_coder.h (right):

http://codereview.chromium.org/6289009/diff/101001/chrome/common/attributed_s...
chrome/common/attributed_string_coder.h:61: // A class that contains the
pertitent information from an NSAttributedString,
On 2011/04/11 20:22:16, Avi wrote:
> pertinent

Done.

http://codereview.chromium.org/6289009/diff/101001/chrome/common/attributed_s...
File chrome/common/attributed_string_coder.mm (right):

http://codereview.chromium.org/6289009/diff/101001/chrome/common/attributed_s...
chrome/common/attributed_string_coder.mm:76: font_name_ =
base::SysNSStringToUTF8([font fontName]);
On 2011/04/12 09:44:06, jeremy wrote:
> Could you use a FontDescriptor (content/common/font_descriptor_mac.mm) here,
> since that class is supposed to encapsulate sending an NSFont over IPC.

Done.

http://codereview.chromium.org/6289009/diff/101001/chrome/common/attributed_s...
chrome/common/attributed_string_coder.mm:109: // IPC ParamTraits specialization
//////////////////////////////////////////////
On 2011/04/12 09:44:06, jeremy wrote:
> When deserializing values here, could you add some paranoid sanity checks?  If
> the renderer sends junk over, we don't want it getting to Cocoa...
> 
> esp. probably worth checking that the range is legal, font sizes, etc.

ui::Range protects against this in its own ParamTraits<> deserialization and
initialization. Does FontDescriptor do the same?

[Robert Sesek, 2011-04-29 21:18:37]

This CL is *finally* ready to land. On Monday I'll land what's posted here. If
there's any further feedback, I'll happily incorporate it later (or if you post
it before I commit). Thanks, all :).

[jeremy, 2011-05-01 09:25:55]

I don't want to slow this down anymore, but I just want to double check with you
- are you satisfied that bogus data sent from a rogue renderer can't cause
issues in the browser process?

e.g. ranges that deviate from the length of the string, etc.

http://codereview.chromium.org/6289009/diff/101001/chrome/common/attributed_s...
File chrome/common/attributed_string_coder.mm (right):

http://codereview.chromium.org/6289009/diff/101001/chrome/common/attributed_s...
chrome/common/attributed_string_coder.mm:109: // IPC ParamTraits specialization
//////////////////////////////////////////////
On 2011/04/25 20:58:18, rsesek wrote:
> ui::Range protects against this in its own ParamTraits<> deserialization and
> initialization. Does FontDescriptor do the same?

FontDescriptor's code uses the standard macros and is defined in
content/common/view_messages.h, I'll add some more protection there.

Is there any that verifies that the ui:Range's passed around are within the
strings bounds?

[Robert Sesek, 2011-05-02 15:43:31]

I added a unit test to verify we don't decode out-of-range attributes (which
would cause a crash via an NSException). ui::Range is hardened against integer
overflows, and your change to FontDescriptor ensures that is protected, too.

I think now everything's clear for landing.

http://codereview.chromium.org/6289009/diff/101001/chrome/common/attributed_s...
File chrome/common/attributed_string_coder.mm (right):

http://codereview.chromium.org/6289009/diff/101001/chrome/common/attributed_s...
chrome/common/attributed_string_coder.mm:109: // IPC ParamTraits specialization
//////////////////////////////////////////////
On 2011/05/01 09:25:56, jeremy wrote:
> Is there any that verifies that the ui:Range's passed around are within the
> strings bounds?

Added a check that Range::GetMin() and GetMax() are both less than the string's
length.