Implement "Look Up In Dictionary" context menu item asynchronously. (OS X)

Implement "Look Up In Dictionary" context menu item asynchronously. (OS X)

Reduces the number of callers of the synchronous IPC in
TextInputClientMac::GetAttributedSubstringFromRange() (so that
eventually we can remove it).

Note: This is the second attempt to commit cl
https://codereview.chromium.org/1313553006, which caused regression
crbug.com/528929, because the point getten from selection range may not
be accurate to get the string through GetStringAtPoint.
This cl fixes that issue by leveraging
TextInputClientMsg_StringForRange IPCs instead of
TextInputClientMsg_StringAtPoint.

The change is cross browser & blink so it is 2-way patches.
This cl is based on the blink cl https://codereview.chromium.org/1329103002.
TextInputClientObserver::OnStringForRange() will send the IPC
TextInputClientReplyMsg_GotStringForRange to carry the basepiont info,
which is got by the new change in blink.

So this cl should be landed after https://codereview.chromium.org/1329103002
is landed.

TBR=sievers@chromium.org
BUG=121917
TEST=crbug.com/528929 not repro; Select multiple words and lookup
dictionary by context menu can work well; Select non-English words (e.g.
Japanese/Chinese) and lookup dictionary by context menu can work well.

Committed: https://crrev.com/3146b3a43b240dcc6d64d5dc6986a311505104e8
Cr-Commit-Position: refs/heads/master@{#350164}

[Shu Chen, 2015-09-08 05:33:56]

shuchen@chromium.org changed reviewers:
+ asvitkine@chromium.org, palmer@chromium.org

[Shu Chen, 2015-09-08 05:33:57]

Please review the cl, thanks.

[Shu Chen, 2015-09-08 05:47:51]

This cl should be combined with another cl in blink:
https://codereview.chromium.org/1329103002.

How to combine the 2 CLs together? Thanks.

[Alexei Svitkine (slow), 2015-09-08 15:56:45]

On 2015/09/08 05:47:51, Shu Chen wrote:
> This cl should be combined with another cl in blink:
> https://codereview.chromium.org/1329103002.
> 
> How to combine the 2 CLs together? Thanks.

It's not possible to combine CLs between Chromium and Blink.

You have to land them independently - in a way that doesn't break things in the
meanwhile when the other one is landed.

Sometimes this means that you have to duplicate an API temporarily before
migrating to the new one or something similar along those lines.

[palmer, 2015-09-09 23:24:17]

https://codereview.chromium.org/1318483007/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/1318483007/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:2448: // TODO:
support GetStringAtPoint() for PDF. See crbug.com/152438.
Why don't you include the old code that launches Dictionary.app?

[Shu Chen, 2015-09-10 01:26:03]

https://codereview.chromium.org/1318483007/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/1318483007/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:2448: // TODO:
support GetStringAtPoint() for PDF. See crbug.com/152438.
On 2015/09/09 23:24:17, palmer wrote:
> Why don't you include the old code that launches Dictionary.app?

Sorry, that was accidentally removed. And thanks for catching it.

I've added it back.

[Alexei Svitkine (slow), 2015-09-10 04:27:46]

Can you expand the TEST= section with more concrete steps to test the various
edge cases that the previous versions of the CL failed but this one passes?

[Shu Chen, 2015-09-11 02:41:54]

On 2015/09/10 04:27:46, Alexei Svitkine wrote:
> Can you expand the TEST= section with more concrete steps to test the various
> edge cases that the previous versions of the CL failed but this one passes?

Done.

[palmer, 2015-09-11 21:34:36]

lgtm

[Alexei Svitkine (slow), 2015-09-14 14:56:31]

Can you summarize the approach you'll use for the 2-way patch between
Chromium and Blink? i.e. sequence of patches that you plan to land? (Was
waiting for something like that from you before continuing the review.)

On Fri, Sep 11, 2015 at 5:34 PM, <palmer@chromium.org> wrote:

> lgtm
>
>
>
> https://codereview.chromium.org/1318483007/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Shu Chen, 2015-09-15 01:40:04]

On 2015/09/14 14:56:31, Alexei Svitkine wrote:
> Can you summarize the approach you'll use for the 2-way patch between
> Chromium and Blink? i.e. sequence of patches that you plan to land? (Was
> waiting for something like that from you before continuing the review.)

Done. I've added the summary in the CL description, please take a look. Thanks.

[Shu Chen, 2015-09-18 02:40:56]

Pinging. Alexei, mind taking another look? Thanks

[Alexei Svitkine (slow), 2015-09-18 16:48:43]

https://codereview.chromium.org/1318483007/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/1318483007/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:2435:
^(NSAttributedString* string, NSPoint baselinePoint) {
Is there a reason this block is different than the one in the next function?

If they should be the same, suggest refactoring out the code into a helper
function so it can be shared.

https://codereview.chromium.org/1318483007/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:2451: if (!string
|| [string length] == 0) {
Nit: [string length] == 0 check is sufficient, because if |string| is nil, it
will result in 0.

https://codereview.chromium.org/1318483007/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:2455: // TODO:
support GetStringAtPoint() for PDF. See crbug.com/152438.
Nit: TODO()'s should have a name inside them. Since you're touching this last,
you can put yourself. Or you can keep it as-is with my name there. (The name is
a contact person for the TODO, not necessarily the person who implements the
fix.)

Also, make the text after the todo a full sentence - i.e. capitalize Support.

https://codereview.chromium.org/1318483007/diff/40001/content/browser/rendere...
File content/browser/renderer_host/text_input_client_mac.h (right):

https://codereview.chromium.org/1318483007/diff/40001/content/browser/rendere...
content/browser/renderer_host/text_input_client_mac.h:92: void
(^replyHandler)(NSAttributedString*, NSPoint));
replyHandler -> reply_handler

https://codereview.chromium.org/1318483007/diff/40001/content/browser/rendere...
content/browser/renderer_host/text_input_client_mac.h:93: // This is called on
the IO thread when we get the renderer's reply for
Add an empty line above this.

https://codereview.chromium.org/1318483007/diff/40001/content/browser/rendere...
content/browser/renderer_host/text_input_client_mac.h:95: void
GetStringFromRangeReply(NSAttributedString*, NSPoint);
Name the params.

https://codereview.chromium.org/1318483007/diff/40001/content/browser/rendere...
content/browser/renderer_host/text_input_client_mac.h:118:
base::mac::ScopedBlock<void(^)(NSAttributedString*, NSPoint)>
Add comments.

[Shu Chen, 2015-09-21 02:47:40]

https://codereview.chromium.org/1318483007/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/1318483007/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:2435:
^(NSAttributedString* string, NSPoint baselinePoint) {
On 2015/09/18 16:48:43, Alexei Svitkine (slow) wrote:
> Is there a reason this block is different than the one in the next function?
> 
> If they should be the same, suggest refactoring out the code into a helper
> function so it can be shared.

Done.

https://codereview.chromium.org/1318483007/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:2451: if (!string
|| [string length] == 0) {
On 2015/09/18 16:48:43, Alexei Svitkine (slow) wrote:
> Nit: [string length] == 0 check is sufficient, because if |string| is nil, it
> will result in 0.

Done.

https://codereview.chromium.org/1318483007/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:2455: // TODO:
support GetStringAtPoint() for PDF. See crbug.com/152438.
On 2015/09/18 16:48:43, Alexei Svitkine (slow) wrote:
> Nit: TODO()'s should have a name inside them. Since you're touching this last,
> you can put yourself. Or you can keep it as-is with my name there. (The name
is
> a contact person for the TODO, not necessarily the person who implements the
> fix.)
> 
> Also, make the text after the todo a full sentence - i.e. capitalize Support.

Done.

https://codereview.chromium.org/1318483007/diff/40001/content/browser/rendere...
File content/browser/renderer_host/text_input_client_mac.h (right):

https://codereview.chromium.org/1318483007/diff/40001/content/browser/rendere...
content/browser/renderer_host/text_input_client_mac.h:92: void
(^replyHandler)(NSAttributedString*, NSPoint));
On 2015/09/18 16:48:43, Alexei Svitkine (slow) wrote:
> replyHandler -> reply_handler

Done.

https://codereview.chromium.org/1318483007/diff/40001/content/browser/rendere...
content/browser/renderer_host/text_input_client_mac.h:93: // This is called on
the IO thread when we get the renderer's reply for
On 2015/09/18 16:48:43, Alexei Svitkine (slow) wrote:
> Add an empty line above this.

Done.

https://codereview.chromium.org/1318483007/diff/40001/content/browser/rendere...
content/browser/renderer_host/text_input_client_mac.h:95: void
GetStringFromRangeReply(NSAttributedString*, NSPoint);
On 2015/09/18 16:48:43, Alexei Svitkine (slow) wrote:
> Name the params.

Done.

https://codereview.chromium.org/1318483007/diff/40001/content/browser/rendere...
content/browser/renderer_host/text_input_client_mac.h:118:
base::mac::ScopedBlock<void(^)(NSAttributedString*, NSPoint)>
On 2015/09/18 16:48:43, Alexei Svitkine (slow) wrote:
> Add comments.

Done.

[Alexei Svitkine (slow), 2015-09-21 15:47:56]

https://codereview.chromium.org/1318483007/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/1318483007/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:2441: // at point.
Until it does, use NSPerformService(), which opens
Nit: Some of the words can fit on the previous line. Please re-wrap.

https://codereview.chromium.org/1318483007/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:2443: //
TODO(shuchen): Support GetStringAtPoint() for PDF. See crbug.com/152438.
Nit: GetStringAndPoint() and GetStringFromRange()

https://codereview.chromium.org/1318483007/diff/60001/content/browser/rendere...
File content/browser/renderer_host/text_input_client_mac.h (right):

https://codereview.chromium.org/1318483007/diff/60001/content/browser/rendere...
content/browser/renderer_host/text_input_client_mac.h:83: // This is called on
the IO thread when we get the renderer's reply for
Nit: Add an empty line before this for consistency with the below too.

https://codereview.chromium.org/1318483007/diff/60001/content/browser/rendere...
content/browser/renderer_host/text_input_client_mac.h:85: void
GetStringAtPointReply(NSAttributedString*, NSPoint);
Nit: Can you name these params too?

https://codereview.chromium.org/1318483007/diff/60001/content/browser/rendere...
content/browser/renderer_host/text_input_client_mac.h:88: //
ShowDefinitionForSelection().
This comment is referencing code outside content/ - which is a layering
violation. Can you rephrase it as to the purpose (and can allude to it's example
use, but not actual class names, etc).

https://codereview.chromium.org/1318483007/diff/60001/content/browser/rendere...
content/browser/renderer_host/text_input_client_mac.h:89: // The reply callback
will be invoked from the IO thread, the caller is
Nit: , -> ;

Also make the same change for the comment above.

https://codereview.chromium.org/1318483007/diff/60001/content/browser/rendere...
File content/browser/renderer_host/text_input_client_message_filter.mm (right):

https://codereview.chromium.org/1318483007/diff/60001/content/browser/rendere...
content/browser/renderer_host/text_input_client_message_filter.mm:75:
service->SetSubstringAndSignal(string);
Can this call be merged into GetStringFromRangeReply(), since this is the only
caller?

https://codereview.chromium.org/1318483007/diff/60001/content/browser/rendere...
content/browser/renderer_host/text_input_client_message_filter.mm:77: string,
NSPointFromCGPoint(point.ToCGPoint()));
Does it make sense to not do this when string is empty or nil (since there's an
early return above)? OnGotStringAtPoint() doesn't have the same logic. It seems
to me that it would make sense for them to be consistent - but perhaps I don't
know the implications here. What are they?

[Shu Chen, 2015-09-22 05:46:09]

https://codereview.chromium.org/1318483007/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/1318483007/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:2441: // at point.
Until it does, use NSPerformService(), which opens
On 2015/09/21 15:47:55, Alexei Svitkine (slow) wrote:
> Nit: Some of the words can fit on the previous line. Please re-wrap.

Done.

https://codereview.chromium.org/1318483007/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:2443: //
TODO(shuchen): Support GetStringAtPoint() for PDF. See crbug.com/152438.
On 2015/09/21 15:47:55, Alexei Svitkine (slow) wrote:
> Nit: GetStringAndPoint() and GetStringFromRange()

Done.

https://codereview.chromium.org/1318483007/diff/60001/content/browser/rendere...
File content/browser/renderer_host/text_input_client_mac.h (right):

https://codereview.chromium.org/1318483007/diff/60001/content/browser/rendere...
content/browser/renderer_host/text_input_client_mac.h:83: // This is called on
the IO thread when we get the renderer's reply for
On 2015/09/21 15:47:55, Alexei Svitkine (slow) wrote:
> Nit: Add an empty line before this for consistency with the below too.

Done.

https://codereview.chromium.org/1318483007/diff/60001/content/browser/rendere...
content/browser/renderer_host/text_input_client_mac.h:85: void
GetStringAtPointReply(NSAttributedString*, NSPoint);
On 2015/09/21 15:47:55, Alexei Svitkine (slow) wrote:
> Nit: Can you name these params too?

Done.

https://codereview.chromium.org/1318483007/diff/60001/content/browser/rendere...
content/browser/renderer_host/text_input_client_mac.h:88: //
ShowDefinitionForSelection().
On 2015/09/21 15:47:55, Alexei Svitkine (slow) wrote:
> This comment is referencing code outside content/ - which is a layering
> violation. Can you rephrase it as to the purpose (and can allude to it's
example
> use, but not actual class names, etc).

I don't understand. RenderWidgetHostViewMacDictionaryHelper class is within
content/.

But I've rephrased the comment anyway.

https://codereview.chromium.org/1318483007/diff/60001/content/browser/rendere...
content/browser/renderer_host/text_input_client_mac.h:89: // The reply callback
will be invoked from the IO thread, the caller is
On 2015/09/21 15:47:56, Alexei Svitkine (slow) wrote:
> Nit: , -> ;
> 
> Also make the same change for the comment above.

Done.

https://codereview.chromium.org/1318483007/diff/60001/content/browser/rendere...
File content/browser/renderer_host/text_input_client_message_filter.mm (right):

https://codereview.chromium.org/1318483007/diff/60001/content/browser/rendere...
content/browser/renderer_host/text_input_client_message_filter.mm:75:
service->SetSubstringAndSignal(string);
On 2015/09/21 15:47:56, Alexei Svitkine (slow) wrote:
> Can this call be merged into GetStringFromRangeReply(), since this is the only
> caller?

Done.

https://codereview.chromium.org/1318483007/diff/60001/content/browser/rendere...
content/browser/renderer_host/text_input_client_message_filter.mm:77: string,
NSPointFromCGPoint(point.ToCGPoint()));
On 2015/09/21 15:47:56, Alexei Svitkine (slow) wrote:
> Does it make sense to not do this when string is empty or nil (since there's
an
> early return above)? OnGotStringAtPoint() doesn't have the same logic. It
seems
> to me that it would make sense for them to be consistent - but perhaps I don't
> know the implications here. What are they?

This call is necessary when string is empty or nil, because
RVHVCocoa::showLookUpDictionaryOverlayInternal() needs to do things differently
if |string| is empty.

I don't see an early return above. But I've removed the unnecessary code: "if
(![string length]) string = nil;" in case that caused confusion.

[Alexei Svitkine (slow), 2015-09-22 14:59:27]

lgtm

[Shu Chen, 2015-09-22 15:05:24]

The CQ bit was checked by shuchen@chromium.org

[Shu Chen, 2015-09-22 15:05:25]

The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org
Link to the patchset: https://codereview.chromium.org/1318483007/#ps80001
(title: "addressed review comments.")

[commit-bot: I haz the power, 2015-09-22 15:05:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1318483007/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1318483007/80001

[commit-bot: I haz the power, 2015-09-22 15:14:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-22 15:14:54]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Shu Chen, 2015-09-22 15:23:06]

TBR'ed sievers@ for rubber stamp on text_input_client_observer.cc.

[Shu Chen, 2015-09-22 15:23:12]

The CQ bit was checked by shuchen@chromium.org

[commit-bot: I haz the power, 2015-09-22 15:24:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1318483007/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1318483007/80001

[commit-bot: I haz the power, 2015-09-22 16:08:34]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-09-22 16:09:19]

Patchset 5 (id:??) landed as
https://crrev.com/3146b3a43b240dcc6d64d5dc6986a311505104e8
Cr-Commit-Position: refs/heads/master@{#350164}