Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1313553006:
Implement "Look Up In Dictionary" context menu item asynchronously. (Closed)
|
Created:
5 years, 3 months ago by Shu Chen Modified:
5 years, 3 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, shuchen+watch_chromium.org, jam, nona+watch_chromium.org, darin-cc_chromium.org, James Su Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement "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).
BUG=121917
TEST=Verified "Look Up in Dictionary" by context menu feature works well.
Committed: https://crrev.com/cabb6815f85f0e6297a55eb313a8e11373e2da9e
Cr-Commit-Position: refs/heads/master@{#347306}
Patch Set 1 #Patch Set 2 : #
Total comments: 7
Patch Set 3 : separated cl. #Patch Set 4 : #
Total comments: 6
Patch Set 5 : addressed asvitkine@'s comments. #
Total comments: 2
Patch Set 6 : #
Messages
Total messages: 20 (4 generated)
shuchen@chromium.org changed reviewers: + asvitkine@chromium.org, palmer@chromium.org, sievers@chromium.org
asvitkine@ for the overall Mac related changes. palmer@ for owners of text_input_client_messages.h. sievers@ for owners of content/renderer/... Thanks!
I'm wondering it worths to write up a test for attributedSubstringForProposedRange & ShowLookUpDictionary methods, and how to implement them. Do you guys have some ideas?
https://codereview.chromium.org/1313553006/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.h (right): https://codereview.chromium.org/1313553006/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.h:211: - (void) ShowLookUpDictionary:(NSPoint)point; Nit: Use objc naming convention please. e.g. - (void)showLookUpDictionaryOverlayAtPoint:(NSPoint)point; https://codereview.chromium.org/1313553006/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/1313553006/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:2386: - (void)quickLookWithEvent:(NSEvent*)event { Please separate the dictionary logic change into its own CL, that way your previous CL to change -attributedSubstringForProposedRange can be kept very close to what you landed before.
https://codereview.chromium.org/1313553006/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/1313553006/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:2927: expected_range = gfx::Range(offset, offset + expected_text->size()); How do you know this range is valid? Can offset + expected_text->size() go outside the available space? https://codereview.chromium.org/1313553006/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:2936: &(*expected_text)[requested_range.start() - expected_range.start()]; I don't understand this code. What is going on? What guarantees that the expression "requested_range.start() - expected_range.start()" describes a valid range of memory? https://codereview.chromium.org/1313553006/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:2937: NSUInteger bytes_len = requested_range.length() * sizeof(base::char16); Is NSUInteger guaranteed to have the same range as a size_t? If not, there could be integer truncation here. You might want to make use of base/numerics/safe_conversions.h.
https://codereview.chromium.org/1313553006/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.h (right): https://codereview.chromium.org/1313553006/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.h:211: - (void) ShowLookUpDictionary:(NSPoint)point; On 2015/09/01 16:27:03, Alexei Svitkine wrote: > Nit: Use objc naming convention please. > > e.g. > > - (void)showLookUpDictionaryOverlayAtPoint:(NSPoint)point; Done. https://codereview.chromium.org/1313553006/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/1313553006/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:2386: - (void)quickLookWithEvent:(NSEvent*)event { On 2015/09/01 16:27:03, Alexei Svitkine wrote: > Please separate the dictionary logic change into its own CL, that way your > previous CL to change -attributedSubstringForProposedRange can be kept very > close to what you landed before. Done.
According to Alexei's comment, I've separated this cl into 3 smaller CLs. - This cl will only use asynchronous way to support "Look Up in Dictionary" by context menu. - https://codereview.chromium.org/1327743002 will remove the use of sync IPC for firstRectForCharacterRange/attributedSubstringForProposedRange. - https://codereview.chromium.org/1326763003 will clean up the useless code related to sync IPCs. I will address palmer@'s comments in the cl https://codereview.chromium.org/1327743002. Thanks!
https://codereview.chromium.org/1313553006/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac_dictionary_helper.mm (left): https://codereview.chromium.org/1313553006/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac_dictionary_helper.mm:33: // http://crbug.com/152438 Nit: I guess triple-finger tap also doesn't work PDFs. Maybe update the bug and add a comment referencing it to -showLookUpDictionaryOverlayAtPoint: https://codereview.chromium.org/1313553006/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac_dictionary_helper.mm:50: rect.origin.y += NSHeight(rect) - [font ascender]; Looks like you're no longer subtracting the font's ascender value for the point. Does the overlay still show up in the right place? https://codereview.chromium.org/1313553006/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac_dictionary_helper.mm (right): https://codereview.chromium.org/1313553006/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac_dictionary_helper.mm:28: rect.origin.y += NSHeight(rect); Nit: Combine this with line 30.
LGTM. You could clean up the CL description a bit. Try something like this, perhaps: === 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). BUG=121917 TEST=Verified "Look Up in Dictionary" by context menu feature works well. ===
On 2015/09/02 20:06:43, palmer wrote: > LGTM. > > You could clean up the CL description a bit. Try something like this, perhaps: > > === > 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). > > BUG=121917 > TEST=Verified "Look Up in Dictionary" by context menu feature works well. > === Done. Thanks for your suggestion.
https://codereview.chromium.org/1313553006/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac_dictionary_helper.mm (left): https://codereview.chromium.org/1313553006/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac_dictionary_helper.mm:33: // http://crbug.com/152438 On 2015/09/02 15:10:25, Alexei Svitkine wrote: > Nit: I guess triple-finger tap also doesn't work PDFs. Maybe update the bug and > add a comment referencing it to -showLookUpDictionaryOverlayAtPoint: Done. https://codereview.chromium.org/1313553006/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac_dictionary_helper.mm:50: rect.origin.y += NSHeight(rect) - [font ascender]; On 2015/09/02 15:10:25, Alexei Svitkine wrote: > Looks like you're no longer subtracting the font's ascender value for the point. > Does the overlay still show up in the right place? Yes, it still shows up in the right place. Because originally it needs to precisely get the base point of the bubble, but with this cl, it only needs to get a point within the rect, the IPC TextInputClientReplyMsg_GotStringAtPoint will come with the precise base point. https://codereview.chromium.org/1313553006/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac_dictionary_helper.mm (right): https://codereview.chromium.org/1313553006/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac_dictionary_helper.mm:28: rect.origin.y += NSHeight(rect); On 2015/09/02 15:10:25, Alexei Svitkine wrote: > Nit: Combine this with line 30. Done.
shuchen@chromium.org changed reviewers: - sievers@chromium.org
lgtm https://codereview.chromium.org/1313553006/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/1313553006/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:2359: // The PDF plugin does not support getting the attributed string at point. Nit: Make this a TODO().
https://codereview.chromium.org/1313553006/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/1313553006/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:2359: // The PDF plugin does not support getting the attributed string at point. On 2015/09/03 16:28:11, Alexei Svitkine wrote: > Nit: Make this a TODO(). Done.
The CQ bit was checked by shuchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1313553006/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1313553006/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1313553006/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/cabb6815f85f0e6297a55eb313a8e11373e2da9e Cr-Commit-Position: refs/heads/master@{#347306}
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/1306713007/ by shuchen@chromium.org. The reason for reverting is: Caused regression crbug.com/528929.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
