Fix assertion failure in range.mm while examining system dictionary popup.

Fix assertion failure in range.mm while examining system dictionary popup.

Convert WTF::notFound to NSNotFound.

BUG=83158
TEST=chrome/browser/renderer_host/text_input_client_mac_unittest.mm

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

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

[bashi, 2011-05-19 01:20:06]

Hi rsesek,

Could you take a look?

[Robert Sesek, 2011-05-19 15:16:56]

Thanks for finding and fixing this! I've seen this assertion before but hadn't
tracked it down.

http://codereview.chromium.org/7039054/diff/1/chrome/browser/renderer_host/te...
File chrome/browser/renderer_host/text_input_client_mac.mm (right):

http://codereview.chromium.org/7039054/diff/1/chrome/browser/renderer_host/te...
chrome/browser/renderer_host/text_input_client_mac.mm:89: if (index ==
static_cast<NSUInteger>(-1)) {
I think a better place to do this would be in
text_input_client_message_filter.mm. WTF::notFound is actually
static_cast<size_t>(-1) and I think it's better if this class doesn't have to
know about WTF. The message filter is also where the validation of other values
happens.

The downside with that is there's no test for that message filter. Perhaps you
could change this new test to create a TextInputClientMessageFilter and PostTask
to TextInputClientMessageFilter::OnGotCharacterIndexForPoint instead of
SetCharacterIndexAndSignal.

[bashi, 2011-05-20 04:43:15]

Thank you for review.  I've addressed your comment.

http://codereview.chromium.org/7039054/diff/1/chrome/browser/renderer_host/te...
File chrome/browser/renderer_host/text_input_client_mac.mm (right):

http://codereview.chromium.org/7039054/diff/1/chrome/browser/renderer_host/te...
chrome/browser/renderer_host/text_input_client_mac.mm:89: if (index ==
static_cast<NSUInteger>(-1)) {
On 2011/05/19 15:16:56, rsesek wrote:
> I think a better place to do this would be in
> text_input_client_message_filter.mm. WTF::notFound is actually
> static_cast<size_t>(-1) and I think it's better if this class doesn't have to
> know about WTF.

Moved it to text_input_client_message_filter.mm.

> Perhaps you
> could change this new test to create a TextInputClientMessageFilter and
PostTask
> to TextInputClientMessageFilter::OnGotCharacterIndexForPoint instead of
> SetCharacterIndexAndSignal.

Since OnGotCharacterIndexForPoint() is private so I use OnMessageReceived()
instead.

[Robert Sesek, 2011-05-20 12:50:15]

Very nice; LGTM.

[bashi, 2011-05-23 04:34:13]

Hi rsesek,

The test failed at bot. 
http://build.chromium.org/p/chromium/builders/Mac10.5%20Tests%20%282%29/build...

I've revised the CL.  Could you take another look?  Sorry for bothering you..

[Robert Sesek, 2011-05-23 12:31:39]

LGTM

http://codereview.chromium.org/7039054/diff/8001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/text_input_client_mac_unittest.mm (right):

http://codereview.chromium.org/7039054/diff/8001/chrome/browser/renderer_host...
chrome/browser/renderer_host/text_input_client_mac_unittest.mm:130: kTaskDelayMs
+ kTaskDelayMs);
Use *2?

http://codereview.chromium.org/7039054/diff/8001/chrome/browser/renderer_host...
chrome/browser/renderer_host/text_input_client_mac_unittest.mm:139: for
(unsigned i = 0; i < ipc_sink().message_count(); ++i) {
Chromium prefers size_t to unsigned.

[bashi, 2011-05-24 00:44:17]

Submitted. Thanks!

http://codereview.chromium.org/7039054/diff/8001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/text_input_client_mac_unittest.mm (right):

http://codereview.chromium.org/7039054/diff/8001/chrome/browser/renderer_host...
chrome/browser/renderer_host/text_input_client_mac_unittest.mm:130: kTaskDelayMs
+ kTaskDelayMs);
On 2011/05/23 12:31:39, rsesek wrote:
> Use *2?

Done.

http://codereview.chromium.org/7039054/diff/8001/chrome/browser/renderer_host...
chrome/browser/renderer_host/text_input_client_mac_unittest.mm:139: for
(unsigned i = 0; i < ipc_sink().message_count(); ++i) {
On 2011/05/23 12:31:39, rsesek wrote:
> Chromium prefers size_t to unsigned.

Done.