Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2382003002:
Fix dictionary look-up for highlighted text in Mac. (Closed)
DescriptionFix dictionary look-up for highlighted text in Mac.
Currently, TextInputClientMac only taks to the main frames RenderWidgetHost in order to
obtian the string from specific range. This CL fixes this issue by routing the request
to the focused RenderWidgetHost.
BUG=651519
Committed: https://crrev.com/3f7daa9733a4d5d3c06359b9f92c3bde20c7dd5d
Cr-Commit-Position: refs/heads/master@{#426030}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Rebased #Patch Set 3 : Added a test #
Total comments: 12
Patch Set 4 : Addressing comments #Patch Set 5 : Rebased #
Total comments: 1
Messages
Total messages: 29 (13 generated)
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ekaramad@chromium.org changed reviewers: + creis@chromium.org, erikchen@chromium.org
This was mostly fixed in the previous CL on Mac dictionary for OOPIF. I just did not route the IPC to the right widget. Please take a look. https://codereview.chromium.org/2382003002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/2382003002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_mac.mm:2376: gfx::Rect root_box = renderWidgetHostView_->GetViewBounds(); The logic is almost identical to showLookupDictionaryOverlayAtPoint except that we already know the widget to talk to is the focused one (as opposed to the one at the specific cursor position).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
On 2016/09/29 20:37:40, erikchen wrote: > lgtm Thanks erikchen@ for the review. creis@ (regarding adding a test if possible): We already have tests which verify that requests for strings within a given range (or at some position) are correctly handled. We did NOT have a test to verify that when we right click and click on Lookup on a word, we get the definition at some point. But to add a test that would have caught this..I think we had to have a UI test that would: 1) click on lookup and search for the word in range, 2) verify that dictionary was shown. 1) might not be too hard (hopefully, there is something within context menus realm). But I am not sure how to make 2 happen. Maybe I will be able to intercept the IPCs on their way back and verify the result. I will see if I can add a test.
Ok, thanks. RS LGTM for owners, but a test would be good to include if it's possible. https://codereview.chromium.org/2382003002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/2382003002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_mac.mm:2376: gfx::Rect root_box = renderWidgetHostView_->GetViewBounds(); On 2016/09/29 18:20:35, EhsanK wrote: > The logic is almost identical to showLookupDictionaryOverlayAtPoint except that > we already know the widget to > talk to is the focused one (as opposed to the one at the specific cursor > position). Will we be able to resolve these TODOs soon? I'm not thrilled to have this much workaround code duplicated (with a possible risk of needing it in more places).
Thanks Charlie. I added a test which needed some changes. PTAL. https://codereview.chromium.org/2382003002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/2382003002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_mac.mm:2376: gfx::Rect root_box = renderWidgetHostView_->GetViewBounds(); On 2016/09/30 23:00:55, Charlie Reis wrote: > On 2016/09/29 18:20:35, EhsanK wrote: > > The logic is almost identical to showLookupDictionaryOverlayAtPoint except > that > > we already know the widget to > > talk to is the focused one (as opposed to the one at the specific cursor > > position). > > Will we be able to resolve these TODOs soon? I'm not thrilled to have this much > workaround code duplicated (with a possible risk of needing it in more places). This one unfortunately might need a major reworking of TextInputClientMac; which is a good thing to do IMO. I will try to do this in very near future.
creis@chromium.org changed reviewers: + avi@chromium.org
Apologies for the long delay, and I'm not sure if I'm going to have time to look at the test soon enough. Avi, could you review since this is dealing with Mac features? Thanks! https://codereview.chromium.org/2382003002/diff/40001/chrome/browser/renderer... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2382003002/diff/40001/chrome/browser/renderer... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:976: #if defined(OS_MACOSX) We added a site_per_process_mac_browsertest.mm recently. Would this fit better there, or does it depend on enough things in this file that we need it here? (If the latter, please mention so in a comment above this line.) https://codereview.chromium.org/2382003002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/2382003002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:2349: return; nit: Add blank line after.
https://codereview.chromium.org/2382003002/diff/40001/content/public/test/tex... File content/public/test/text_input_test_utils.h (right): https://codereview.chromium.org/2382003002/diff/40001/content/public/test/tex... content/public/test/text_input_test_utils.h:228: const gfx::Range& range); Typo in the function name: "Dictionary" https://codereview.chromium.org/2382003002/diff/40001/content/public/test/tex... File content/public/test/text_input_test_utils_mac.mm (right): https://codereview.chromium.org/2382003002/diff/40001/content/public/test/tex... content/public/test/text_input_test_utils_mac.mm:75: return [[[NSApplication sharedApplication] windows] count]; return [[NSApp windows] count]; https://codereview.chromium.org/2382003002/diff/40001/content/test/BUILD.gn File content/test/BUILD.gn (right): https://codereview.chromium.org/2382003002/diff/40001/content/test/BUILD.gn#n... content/test/BUILD.gn:367: sources += [ "../public/test/text_input_test_utils_mac.mm" ] I think you can put this into the normal sources section, and it will automatically be filtered out if we're not on the Mac.
Thanks for the reviews. PTAL. https://codereview.chromium.org/2382003002/diff/40001/chrome/browser/renderer... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2382003002/diff/40001/chrome/browser/renderer... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:355: // inside |rfh|. Apologies. This method should have not been in the previous patch since it is unused. https://codereview.chromium.org/2382003002/diff/40001/chrome/browser/renderer... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:976: #if defined(OS_MACOSX) On 2016/10/07 19:07:21, Charlie Reis (Away till 10-14) wrote: > We added a site_per_process_mac_browsertest.mm recently. Would this fit better > there, or does it depend on enough things in this file that we need it here? > (If the latter, please mention so in a comment above this line.) Yes, ideally, we should stay within that file. Specifically given that I do not use any interactive features from chrome/ and making things happen with javascript. I use ContentBrowserClient to setup filters here. For that, the TestBrowserClient inherits the default ContentBrowserClient type used in the tests (to make sure we will initialize the WebContents/RenderProcessHost properly). Ax explained before this is to give myself a chance to add the test filter in time. My knowledge is limited here but I think we have to inherit from the test type or otherwise, implement a lot of methods in ContentBrowserClient with duplicated code. Having said that, for shell, the type would be ShellContentBrowserClient. But there is a static variable in which is set when the first instance of ShellContentBrowserClient is created. That prohibits further instantiations of any test classes. Now to solve this what I think we could do is: 1- Use ChromeContentBrowserClient which does not have the troublesome static. 2- Add a method to ShellContentBrowserClient to reset the static and perhaps set it again when test is about to finish. 3- Implement an all new ContentBrowserClient for testing which wraps the content one. 4- Remove the static from ShellContentBrowserClient. Out of them all, 1) was the easiest in terms of changing pre-existing code. I think 4) might be a good thing to do unless there is a compelling reason to make ShellContentBrowserClient singleton even for tests. Specifically, given that existence of this static variable and DCHECK obviates what content::SetBrowserClientForTesting has to offer. WDYT? https://codereview.chromium.org/2382003002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/2382003002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:2349: return; On 2016/10/07 19:07:21, Charlie Reis (Away till 10-14) wrote: > nit: Add blank line after. Done. https://codereview.chromium.org/2382003002/diff/40001/content/public/test/tex... File content/public/test/text_input_test_utils.h (right): https://codereview.chromium.org/2382003002/diff/40001/content/public/test/tex... content/public/test/text_input_test_utils.h:228: const gfx::Range& range); On 2016/10/07 21:11:26, Avi wrote: > Typo in the function name: "Dictionary" Thanks! Done. https://codereview.chromium.org/2382003002/diff/40001/content/public/test/tex... File content/public/test/text_input_test_utils_mac.mm (right): https://codereview.chromium.org/2382003002/diff/40001/content/public/test/tex... content/public/test/text_input_test_utils_mac.mm:75: return [[[NSApplication sharedApplication] windows] count]; On 2016/10/07 21:11:26, Avi wrote: > return [[NSApp windows] count]; Done. https://codereview.chromium.org/2382003002/diff/40001/content/test/BUILD.gn File content/test/BUILD.gn (right): https://codereview.chromium.org/2382003002/diff/40001/content/test/BUILD.gn#n... content/test/BUILD.gn:367: sources += [ "../public/test/text_input_test_utils_mac.mm" ] On 2016/10/07 21:11:26, Avi wrote: > I think you can put this into the normal sources section, and it will > automatically be filtered out if we're not on the Mac. Acknowledged.
lgtm
Thanks for the reviews. creis@: Could you please take another look at this regarding the comment moving test to content/?
On 2016/10/17 15:55:39, EhsanK wrote: > Thanks for the reviews. > > creis@: Could you please take another look at this regarding the comment moving > test to content/? Sorry for the delay. Answer below. https://codereview.chromium.org/2382003002/diff/40001/chrome/browser/renderer... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2382003002/diff/40001/chrome/browser/renderer... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:976: #if defined(OS_MACOSX) On 2016/10/11 19:31:56, EhsanK wrote: > On 2016/10/07 19:07:21, Charlie Reis (Away till 10-14) wrote: > > We added a site_per_process_mac_browsertest.mm recently. Would this fit > better > > there, or does it depend on enough things in this file that we need it here? > > (If the latter, please mention so in a comment above this line.) > Yes, ideally, we should stay within that file. Specifically given that I do not > use any interactive features from chrome/ and making things happen with > javascript. > > I use ContentBrowserClient to setup filters here. For that, the > TestBrowserClient inherits the default ContentBrowserClient type used in the > tests (to make sure we will initialize the WebContents/RenderProcessHost > properly). Ax explained before this is to give myself a chance to add the test > filter in time. My knowledge is limited here but I think we have to inherit from > the test type or otherwise, implement a lot of methods in ContentBrowserClient > with duplicated code. > > Having said that, for shell, the type would be ShellContentBrowserClient. But > there is a static variable in which is set when the first instance of > ShellContentBrowserClient is created. That prohibits further instantiations of > any test classes. > > Now to solve this what I think we could do is: > 1- Use ChromeContentBrowserClient which does not have the troublesome static. > 2- Add a method to ShellContentBrowserClient to reset the static and perhaps set > it again when test is about to finish. > 3- Implement an all new ContentBrowserClient for testing which wraps the content > one. > 4- Remove the static from ShellContentBrowserClient. > > Out of them all, 1) was the easiest in terms of changing pre-existing code. I > think 4) might be a good thing to do unless there is a compelling reason to make > ShellContentBrowserClient singleton even for tests. Specifically, given that > existence of this static variable and DCHECK obviates what > content::SetBrowserClientForTesting has to offer. WDYT? Ok, sounds like ShellContentBrowserClient was non-trivial. Let's just put a comment explaining that the dependency on setting a ContentBrowserClient is why this is in this file.
Thanks Charlie! I think I already added a comment in the more recent patches. Please take a look and let me know if it needs further revisions. Thanks! https://codereview.chromium.org/2382003002/diff/80001/chrome/browser/renderer... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2382003002/diff/80001/chrome/browser/renderer... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:963: // which does not have the same singleton constraint. This comment explains why we are adding the test here.
Yes, that comment is fine, thanks.
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ekaramad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org, erikchen@chromium.org, avi@chromium.org Link to the patchset: https://codereview.chromium.org/2382003002/#ps80001 (title: "Rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Fix dictionary look-up for highlighted text in Mac. Currently, TextInputClientMac only taks to the main frames RenderWidgetHost in order to obtian the string from specific range. This CL fixes this issue by routing the request to the focused RenderWidgetHost. BUG=651519 ========== to ========== Fix dictionary look-up for highlighted text in Mac. Currently, TextInputClientMac only taks to the main frames RenderWidgetHost in order to obtian the string from specific range. This CL fixes this issue by routing the request to the focused RenderWidgetHost. BUG=651519 Committed: https://crrev.com/3f7daa9733a4d5d3c06359b9f92c3bde20c7dd5d Cr-Commit-Position: refs/heads/master@{#426030} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/3f7daa9733a4d5d3c06359b9f92c3bde20c7dd5d Cr-Commit-Position: refs/heads/master@{#426030} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
