Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
166903005:
mac: Add support for asynchronous dictionary lookup (Closed)
|
Created:
6 years, 10 months ago by Andre Modified:
6 years, 9 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, yukishiino+watch_chromium.org, jam, penghuang+watch_chromium.org, joi+watch-content_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, James Su, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Descriptionmac: Add support for asynchronous dictionary lookup.
Override -[NSResponder quickLookWithEvent:] to perform dictionary lookup
asynchronously. The "Lookup in Dictionary" context menu item still goes
through the synchronous code path for now.
BUG=121917
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=257682
Patch Set 1 #
Total comments: 2
Patch Set 2 : #Patch Set 3 : Use ScopedBlock. #
Total comments: 15
Patch Set 4 : Fixes for avi. #
Total comments: 4
Patch Set 5 : Fix 80 columns formatting. #
Total comments: 2
Patch Set 6 : Fix for dcheng and rebase. #Messages
Total messages: 23 (0 generated)
https://codereview.chromium.org/166903005/diff/1/content/renderer/text_input_... File content/renderer/text_input_client_observer.cc (right): https://codereview.chromium.org/166903005/diff/1/content/renderer/text_input_... content/renderer/text_input_client_observer.cc:48: void TextInputClientObserver::OnStringAtPoint(gfx::Point point) { Given a point in view coordinates, find the closest word as attributed string and also its containing rectangle. I'm trying it by adding the following code in blink's WebFrameImpl.cpp, but it's probably completely wrong. The intent is to show the dictionary definition popup for a word when three-finger tapping, but without doing synchronous IPC to the renderer. With this code it kind of works on a simple .txt file, other than the rect being a little too low vertically. But it doesn't work on a simple page like chromium.org for example. Eric, I would appreciate some pointers on how to do this properly, thanks! WebRange WebFrameImpl::rangeForWordAtPoint(const WebPoint& webPoint) const { IntPoint point = frame()->view()->windowToContents(webPoint); HitTestResult result = frame()->eventHandler().hitTestResultAtPoint(point, HitTestRequest::ReadOnly | HitTestRequest::Active | HitTestRequest::ConfusingAndOftenMisusedDisallowShadowContent); RefPtr<Range> range = frame()->rangeForPoint(result.roundedPointInInnerNodeFrame()); VisibleSelection selection = VisibleSelection(range.get()); selection.expandUsingGranularity(WordGranularity); return selection.toNormalizedRange(); }
This is the kind of thing where you're going to want to test on 10.6 to make sure it works all the way back. https://codereview.chromium.org/166903005/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/text_input_client_mac.h (right): https://codereview.chromium.org/166903005/diff/1/content/browser/renderer_hos... content/browser/renderer_host/text_input_client_mac.h:90: void (^replyHandler_)(NSAttributedString*, NSRect); Don't use raw pointers if possible. ScopedBlock?
On 2014/02/18 21:42:24, Avi wrote: > This is the kind of thing where you're going to want to test on 10.6 to make > sure it works all the way back. > > https://codereview.chromium.org/166903005/diff/1/content/browser/renderer_hos... > File content/browser/renderer_host/text_input_client_mac.h (right): > > https://codereview.chromium.org/166903005/diff/1/content/browser/renderer_hos... > content/browser/renderer_host/text_input_client_mac.h:90: void > (^replyHandler_)(NSAttributedString*, NSRect); > Don't use raw pointers if possible. ScopedBlock? Done, now using ScopedBlock.
Chromium style is so crazy. :) https://codereview.chromium.org/166903005/diff/80001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/166903005/diff/80001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_mac.mm:2807: ^(NSAttributedString* string, NSPoint baselinePoint) { Woh, crazy. Maybe things have changed, but I recalled one of the projects I worked on banning block-functions similar to these because they required your executable pages to be loaded as writeable (since they captured locals) which was a security concern. Maybe that wasn't Chromium, or maybe that wasn't this particular block syntax. Maybe it was probably SyncServices back at Apple... It's been a long time since I've written Obj-C professionally. Then again, I'm probably remembering some ancient, retired block syntax which is no longer used. :)
https://codereview.chromium.org/166903005/diff/80001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/166903005/diff/80001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_mac.mm:2802: - (void)quickLookWithEvent:(NSEvent*)event { Can you note that this only will be invoked for >= 10.8? https://codereview.chromium.org/166903005/diff/80001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_mac.mm:2808: if (string && [string length] > 0) { Technically speaking you don't need to check string. If string were to be nil, sending a message to it would result in a 0 return, disqualifying it on the second condition here. https://codereview.chromium.org/166903005/diff/80001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_mac.mm:2809: dispatch_async(dispatch_get_main_queue(), ^{ Why the dispatch on the main thread? Are we really not guaranteed to get called back already on the main thread? I commented in the api asking for explicit documentation about that. https://codereview.chromium.org/166903005/diff/80001/content/browser/renderer... File content/browser/renderer_host/text_input_client_mac.h (right): https://codereview.chromium.org/166903005/diff/80001/content/browser/renderer... content/browser/renderer_host/text_input_client_mac.h:43: void (^replyHandler)(NSAttributedString*, NSPoint)); Two questions: 1. Is this async, given the callback? If so, then fix the class comment above about how all these methods are synchronous. 2. What are the thread guarantees here about the reply handler? This function needs to be called on the UI thread, of course, but you should document what thread guarantees are given for the callback. https://codereview.chromium.org/166903005/diff/80001/content/browser/renderer... File content/browser/renderer_host/text_input_client_mac.mm (right): https://codereview.chromium.org/166903005/diff/80001/content/browser/renderer... content/browser/renderer_host/text_input_client_mac.mm:40: replyHandler_.reset(replyHandler, base::scoped_policy::RETAIN); Out of paranoia, DCHECK if the replyHandler_ isn't NULL before this point. https://codereview.chromium.org/166903005/diff/80001/content/browser/renderer... content/browser/renderer_host/text_input_client_mac.mm:46: NSPoint point) { Isn't this guaranteed to be on the main thread?
https://codereview.chromium.org/166903005/diff/80001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/166903005/diff/80001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_mac.mm:2807: ^(NSAttributedString* string, NSPoint baselinePoint) { On 2014/03/12 04:48:36, eseidel wrote: > Then again, I'm probably remembering some ancient, retired block syntax which is > no longer used. :) I will be kind and attribute this to your weird historical experiences rather than some particularly potent stuff. Blocks most certainly play well with W^X, and we use them all over the place.
According to Wikipedia, I must be thinking of nested functions: http://en.wikipedia.org/wiki/Blocks_(C_language_extension)
On 2014/03/12 04:55:39, Avi wrote: > https://codereview.chromium.org/166903005/diff/80001/content/browser/renderer... > File content/browser/renderer_host/render_widget_host_view_mac.mm (right): > > https://codereview.chromium.org/166903005/diff/80001/content/browser/renderer... > content/browser/renderer_host/render_widget_host_view_mac.mm:2802: - > (void)quickLookWithEvent:(NSEvent*)event { > Can you note that this only will be invoked for >= 10.8? > > https://codereview.chromium.org/166903005/diff/80001/content/browser/renderer... > content/browser/renderer_host/render_widget_host_view_mac.mm:2808: if (string && > [string length] > 0) { > Technically speaking you don't need to check string. If string were to be nil, > sending a message to it would result in a 0 return, disqualifying it on the > second condition here. > > https://codereview.chromium.org/166903005/diff/80001/content/browser/renderer... > content/browser/renderer_host/render_widget_host_view_mac.mm:2809: > dispatch_async(dispatch_get_main_queue(), ^{ > Why the dispatch on the main thread? Are we really not guaranteed to get called > back already on the main thread? I commented in the api asking for explicit > documentation about that. > > https://codereview.chromium.org/166903005/diff/80001/content/browser/renderer... > File content/browser/renderer_host/text_input_client_mac.h (right): > > https://codereview.chromium.org/166903005/diff/80001/content/browser/renderer... > content/browser/renderer_host/text_input_client_mac.h:43: void > (^replyHandler)(NSAttributedString*, NSPoint)); > Two questions: > > 1. Is this async, given the callback? If so, then fix the class comment above > about how all these methods are synchronous. > > 2. What are the thread guarantees here about the reply handler? This function > needs to be called on the UI thread, of course, but you should document what > thread guarantees are given for the callback. > > https://codereview.chromium.org/166903005/diff/80001/content/browser/renderer... > File content/browser/renderer_host/text_input_client_mac.mm (right): > > https://codereview.chromium.org/166903005/diff/80001/content/browser/renderer... > content/browser/renderer_host/text_input_client_mac.mm:40: > replyHandler_.reset(replyHandler, base::scoped_policy::RETAIN); > Out of paranoia, DCHECK if the replyHandler_ isn't NULL before this point. > > https://codereview.chromium.org/166903005/diff/80001/content/browser/renderer... > content/browser/renderer_host/text_input_client_mac.mm:46: NSPoint point) { > Isn't this guaranteed to be on the main thread? Done. I just realized that the "Look Up in Dictionary" context menu does not go through this code path even on >=10.8, only 3-fingers tap does. To fix that, we may need to completely replace that system menu item with our own custom one that will invoke dictionary lookup asynchronously. We probably should work on that as a follow-up patch separate from this one.
https://codereview.chromium.org/166903005/diff/80001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/166903005/diff/80001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_mac.mm:2802: - (void)quickLookWithEvent:(NSEvent*)event { On 2014/03/12 04:55:40, Avi wrote: > Can you note that this only will be invoked for >= 10.8? Done. https://codereview.chromium.org/166903005/diff/80001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_mac.mm:2808: if (string && [string length] > 0) { On 2014/03/12 04:55:40, Avi wrote: > Technically speaking you don't need to check string. If string were to be nil, > sending a message to it would result in a 0 return, disqualifying it on the > second condition here. True, but my thinking was the extra check makes it clear to the reader that string can be nil. Reduces the potential of wrong assumptions in future code changes. Also, I'm less comfortable relying on the return value of messaging nil when it's not an object type, such as comparing it to another integer. But I don't mind changing it if you want. https://codereview.chromium.org/166903005/diff/80001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_mac.mm:2809: dispatch_async(dispatch_get_main_queue(), ^{ On 2014/03/12 04:55:40, Avi wrote: > Why the dispatch on the main thread? Are we really not guaranteed to get called > back already on the main thread? I commented in the api asking for explicit > documentation about that. The callback happens on the IO/IPC thread. I will document the api accordingly. https://codereview.chromium.org/166903005/diff/80001/content/browser/renderer... File content/browser/renderer_host/text_input_client_mac.h (right): https://codereview.chromium.org/166903005/diff/80001/content/browser/renderer... content/browser/renderer_host/text_input_client_mac.h:43: void (^replyHandler)(NSAttributedString*, NSPoint)); On 2014/03/12 04:55:40, Avi wrote: > Two questions: > > 1. Is this async, given the callback? If so, then fix the class comment above > about how all these methods are synchronous. > > 2. What are the thread guarantees here about the reply handler? This function > needs to be called on the UI thread, of course, but you should document what > thread guarantees are given for the callback. 1. Yes, this is async. Updated comments accordingly. 2. Reply callback happens on the IO thread. Added documentation. https://codereview.chromium.org/166903005/diff/80001/content/browser/renderer... File content/browser/renderer_host/text_input_client_mac.mm (right): https://codereview.chromium.org/166903005/diff/80001/content/browser/renderer... content/browser/renderer_host/text_input_client_mac.mm:40: replyHandler_.reset(replyHandler, base::scoped_policy::RETAIN); On 2014/03/12 04:55:40, Avi wrote: > Out of paranoia, DCHECK if the replyHandler_ isn't NULL before this point. Done. https://codereview.chromium.org/166903005/diff/80001/content/browser/renderer... content/browser/renderer_host/text_input_client_mac.mm:46: NSPoint point) { On 2014/03/12 04:55:40, Avi wrote: > Isn't this guaranteed to be on the main thread? No, this happens on the IO thread.
LGTM if you fix the formatting issues. https://codereview.chromium.org/166903005/diff/80001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/166903005/diff/80001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_mac.mm:2808: if (string && [string length] > 0) { Please leave it alone. I don't mind redundancy in this way. https://codereview.chromium.org/166903005/diff/100001/content/browser/rendere... File content/browser/renderer_host/text_input_client_mac.h (right): https://codereview.chromium.org/166903005/diff/100001/content/browser/rendere... content/browser/renderer_host/text_input_client_mac.h:40: // But currently the "Look Up in Dictionary" context menu item still goes through 80 col https://codereview.chromium.org/166903005/diff/100001/content/browser/rendere... content/browser/renderer_host/text_input_client_mac.h:71: // This async method is invoked from -quickLookWithEvent: (tap with 3 fingers). 80 col
https://codereview.chromium.org/166903005/diff/100001/content/browser/rendere... File content/browser/renderer_host/text_input_client_mac.h (right): https://codereview.chromium.org/166903005/diff/100001/content/browser/rendere... content/browser/renderer_host/text_input_client_mac.h:40: // But currently the "Look Up in Dictionary" context menu item still goes through On 2014/03/12 17:10:54, Avi wrote: > 80 col Done. https://codereview.chromium.org/166903005/diff/100001/content/browser/rendere... content/browser/renderer_host/text_input_client_mac.h:71: // This async method is invoked from -quickLookWithEvent: (tap with 3 fingers). On 2014/03/12 17:10:54, Avi wrote: > 80 col Done.
On 2014/03/12 17:17:51, Andre wrote: > https://codereview.chromium.org/166903005/diff/100001/content/browser/rendere... > File content/browser/renderer_host/text_input_client_mac.h (right): > > https://codereview.chromium.org/166903005/diff/100001/content/browser/rendere... > content/browser/renderer_host/text_input_client_mac.h:40: // But currently the > "Look Up in Dictionary" context menu item still goes through > On 2014/03/12 17:10:54, Avi wrote: > > 80 col > > Done. > > https://codereview.chromium.org/166903005/diff/100001/content/browser/rendere... > content/browser/renderer_host/text_input_client_mac.h:71: // This async method > is invoked from -quickLookWithEvent: (tap with 3 fingers). > On 2014/03/12 17:10:54, Avi wrote: > > 80 col > > Done. Adding cdn@ for the required security review of changes to IPC messages.
On 2014/03/12 17:21:00, Andre wrote: > On 2014/03/12 17:17:51, Andre wrote: > > > https://codereview.chromium.org/166903005/diff/100001/content/browser/rendere... > > File content/browser/renderer_host/text_input_client_mac.h (right): > > > > > https://codereview.chromium.org/166903005/diff/100001/content/browser/rendere... > > content/browser/renderer_host/text_input_client_mac.h:40: // But currently the > > "Look Up in Dictionary" context menu item still goes through > > On 2014/03/12 17:10:54, Avi wrote: > > > 80 col > > > > Done. > > > > > https://codereview.chromium.org/166903005/diff/100001/content/browser/rendere... > > content/browser/renderer_host/text_input_client_mac.h:71: // This async method > > is invoked from -quickLookWithEvent: (tap with 3 fingers). > > On 2014/03/12 17:10:54, Avi wrote: > > > 80 col > > > > Done. > > Adding cdn@ for the required security review of changes to IPC messages. Adding cevans or dcheng for security review. According to the OWNERS file: Changes to IPC messages require a security review to avoid introducing new sandbox escapes.
On 2014/03/14 23:01:16, Andre wrote: > On 2014/03/12 17:21:00, Andre wrote: > > On 2014/03/12 17:17:51, Andre wrote: > > > > > > https://codereview.chromium.org/166903005/diff/100001/content/browser/rendere... > > > File content/browser/renderer_host/text_input_client_mac.h (right): > > > > > > > > > https://codereview.chromium.org/166903005/diff/100001/content/browser/rendere... > > > content/browser/renderer_host/text_input_client_mac.h:40: // But currently > the > > > "Look Up in Dictionary" context menu item still goes through > > > On 2014/03/12 17:10:54, Avi wrote: > > > > 80 col > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/166903005/diff/100001/content/browser/rendere... > > > content/browser/renderer_host/text_input_client_mac.h:71: // This async > method > > > is invoked from -quickLookWithEvent: (tap with 3 fingers). > > > On 2014/03/12 17:10:54, Avi wrote: > > > > 80 col > > > > > > Done. > > > > Adding cdn@ for the required security review of changes to IPC messages. > > Adding cevans or dcheng for security review. > According to the OWNERS file: Changes to IPC messages require a security review > to avoid introducing > new sandbox escapes. Still no security review yet, adding more reviewers.
LGTM with some nits. If you add an IPC reviewer, I'd try pinging them on chat first next time rather than adding more IPC reviewers, since it's quite likely that an IPC reviewer assumes that someone else is already looking at it (and I count 5 IPC reviewers on this CL =). https://codereview.chromium.org/166903005/diff/120001/content/common/text_inp... File content/common/text_input_client_messages.h (right): https://codereview.chromium.org/166903005/diff/120001/content/common/text_inp... content/common/text_input_client_messages.h:44: mac::AttributedStringCoder::EncodedString, Does this compile on non-Mac platforms? I'm surprised it does without this being protected by an #ifdef. https://codereview.chromium.org/166903005/diff/120001/content/renderer/text_i... File content/renderer/text_input_client_observer.h (right): https://codereview.chromium.org/166903005/diff/120001/content/renderer/text_i... content/renderer/text_input_client_observer.h:38: void OnStringAtPoint(gfx::Point point); Is there any guidance on whether this should be passed by value (ala base::Time types) or by reference? I see quite a few other places in the code pass by const ref, but I'm not sure why.
On 2014/03/17 17:45:04, dcheng wrote: > LGTM with some nits. > > If you add an IPC reviewer, I'd try pinging them on chat first next time rather > than adding more IPC reviewers, since it's quite likely that an IPC reviewer > assumes that someone else is already looking at it (and I count 5 IPC reviewers > on this CL =). > > https://codereview.chromium.org/166903005/diff/120001/content/common/text_inp... > File content/common/text_input_client_messages.h (right): > > https://codereview.chromium.org/166903005/diff/120001/content/common/text_inp... > content/common/text_input_client_messages.h:44: > mac::AttributedStringCoder::EncodedString, > Does this compile on non-Mac platforms? I'm surprised it does without this being > protected by an #ifdef. You're right, fix coming. > > https://codereview.chromium.org/166903005/diff/120001/content/renderer/text_i... > File content/renderer/text_input_client_observer.h (right): > > https://codereview.chromium.org/166903005/diff/120001/content/renderer/text_i... > content/renderer/text_input_client_observer.h:38: void > OnStringAtPoint(gfx::Point point); > Is there any guidance on whether this should be passed by value (ala base::Time > types) or by reference? I see quite a few other places in the code pass by const > ref, but I'm not sure why. Thanks! Sorry about that, I didn't know the protocol so I just kept adding security reviewers until I got a response =) Point is a tiny struct so should be safe to pass by value. The code around it are like this, so not worth being different I thought.
The CQ bit was checked by andresantoso@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/andresantoso@chromium.org/166903005/19...
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by andresantoso@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/andresantoso@chromium.org/166903005/19...
Message was sent while issue was closed.
Change committed as 257682 |
