Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2817673003:
Making Mac Dictionary better for OOPIFs and <webview> (Closed)
|
Created:
3 years, 8 months ago by EhsanK Modified:
3 years, 8 months ago CC:
chromium-reviews, creis+watch_chromium.org, yusukes+watch_chromium.org, shuchen+watch_chromium.org, nasko+codewatch_chromium.org, jam, nona+watch_chromium.org, darin-cc_chromium.org, mac-reviews_chromium.org, James Su, site-isolation-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMaking Mac Dictionary better for OOPIFs and <webview>
There is two ways to invoke Mac dictionary for Chromium: choosing look-up word
in context menu for highlighted text, or pressing Ctrl + Command + D. Both methods
might involve sending an IPC through TextInputClientMac to the RenderWidget
corresponding to the string (i.e., focused RenderWidget or the RenderWidget at
the position of mouse cursor). In both cases the IPC comes back with a string
and a position with respect to the local roots view port coordinates. The result
however is handled on IO thread which leads to complications such as a DCHECK
firing (at times) due to accessing the RenderWidgetHostViews weak pointer when
getting the focused RenderWidgetHost.
Furthermore, word lookup through context menu is broken for <webview>s; both
OOPIF-based and BrowserPlugin-based. The former type of <webview>s don't
support this feature because RenderWidgetHostViewChildFrame does not implement
ShowDefinitionForSelection() method. The latter have regressed after implementing
Mac dictionary support for OOPIFs () where the IPC for getting the string from
range is only sent to a RenderWidgetHost which is part of the frame tree (which
will not be that of a guest represented by a BrowserPlugin).
This CL will:
1) Handle dictionary related IPCs on the UI thread to remove the threading issue
and make transforming the reported points to root coordinates based on compositor
frames and the right OOPIF way (calling RWHV::TransformPointToRooTCoordSpace which
can only be done on the UI thread).
2) Implement RenderWidgetHostViewChildFrame::ShowDefinitionForSelection().
3) Make sure that for BrowserPlugin-based guest the IPC is sent to the
RenderWidget of the guest renderer.
The CL also adds a test to avoid regressions in word lookup inside guests.
TBR=shuchen@chromium.org
BUG=643233, 702348, 708183
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2817673003
Cr-Commit-Position: refs/heads/master@{#467149}
Committed: https://chromium.googlesource.com/chromium/src/+/67a361af4485aee793aa2ad8dacddd249d19fc1c
Patch Set 1 #Patch Set 2 : Added the interactive test which now passes both on OOPIF and non-OOPIF webview. #
Total comments: 2
Patch Set 3 : Fixed a comment #
Total comments: 12
Patch Set 4 : Addressing lazyboy@'s comments #
Total comments: 1
Patch Set 5 : Remove ScopedBrowserClient #
Messages
Total messages: 42 (28 generated)
Description was changed from ========== Making Mac Dictionary better for OOPIFs and <webview> There is two ways to invoke Mac dictionary for Chromium: choosing look-up word in context menu for highlighted text, or pressing Ctrl + Command + D. Both methods might involve sending an IPC through TextInputClientMac to the RenderWidget corresponding to the string (i.e., focused RenderWidget or the RenderWidget at the position of mouse cursor). In both cases the IPC comes back with a string and a position with respect to the local roots view port coordinates. The result however is handled on IO thread which leads to complications such as a DCHECK firing (at times) due to accessing the RenderWidgetHostViews weak pointer when getting the focused RenderWidgetHost. Furthermore, word lookup through context menu is broken for <webview>s; both OOPIF-based and BrowserPlugin-based. The former type of <webview>s don't support this feature because RenderWidgetHostViewChildFrame does not implement ShowDefinitionForSelection() method. The latter have regressed after implementing Mac dictionary support for OOPIFs () where the IPC for getting the string from range is only sent to a RenderWidgetHost which is part of the frame tree (which will not be that of a guest represented by a BrowserPlugin). This CL will: 1) Handle dictionary related IPCs on the UI thread to remove the threading issue and make transforming the reported points to root coordinates based on compositor frames and the right OOPIF way (calling RWHV::TransformPointToRooTCoordSpace which can only be done on the UI thread). 2) Implement RenderWidgetHostViewChildFrame::ShowDefinitionForSelection(). 3) Make sure that for BrowserPlugin-based guest the IPC is sent to the RenderWidget of the guest renderer. The CL also adds a test to avoid regressions in word lookup inside guests. BUG=643233, 702348, 708183 ========== to ========== Making Mac Dictionary better for OOPIFs and <webview> There is two ways to invoke Mac dictionary for Chromium: choosing look-up word in context menu for highlighted text, or pressing Ctrl + Command + D. Both methods might involve sending an IPC through TextInputClientMac to the RenderWidget corresponding to the string (i.e., focused RenderWidget or the RenderWidget at the position of mouse cursor). In both cases the IPC comes back with a string and a position with respect to the local roots view port coordinates. The result however is handled on IO thread which leads to complications such as a DCHECK firing (at times) due to accessing the RenderWidgetHostViews weak pointer when getting the focused RenderWidgetHost. Furthermore, word lookup through context menu is broken for <webview>s; both OOPIF-based and BrowserPlugin-based. The former type of <webview>s don't support this feature because RenderWidgetHostViewChildFrame does not implement ShowDefinitionForSelection() method. The latter have regressed after implementing Mac dictionary support for OOPIFs () where the IPC for getting the string from range is only sent to a RenderWidgetHost which is part of the frame tree (which will not be that of a guest represented by a BrowserPlugin). This CL will: 1) Handle dictionary related IPCs on the UI thread to remove the threading issue and make transforming the reported points to root coordinates based on compositor frames and the right OOPIF way (calling RWHV::TransformPointToRooTCoordSpace which can only be done on the UI thread). 2) Implement RenderWidgetHostViewChildFrame::ShowDefinitionForSelection(). 3) Make sure that for BrowserPlugin-based guest the IPC is sent to the RenderWidget of the guest renderer. The CL also adds a test to avoid regressions in word lookup inside guests. BUG=643233, 702348, 708183 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
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: + avi@chromium.org
Hello Avi, Can you please take a look? Thanks!
Description was changed from ========== Making Mac Dictionary better for OOPIFs and <webview> There is two ways to invoke Mac dictionary for Chromium: choosing look-up word in context menu for highlighted text, or pressing Ctrl + Command + D. Both methods might involve sending an IPC through TextInputClientMac to the RenderWidget corresponding to the string (i.e., focused RenderWidget or the RenderWidget at the position of mouse cursor). In both cases the IPC comes back with a string and a position with respect to the local roots view port coordinates. The result however is handled on IO thread which leads to complications such as a DCHECK firing (at times) due to accessing the RenderWidgetHostViews weak pointer when getting the focused RenderWidgetHost. Furthermore, word lookup through context menu is broken for <webview>s; both OOPIF-based and BrowserPlugin-based. The former type of <webview>s don't support this feature because RenderWidgetHostViewChildFrame does not implement ShowDefinitionForSelection() method. The latter have regressed after implementing Mac dictionary support for OOPIFs () where the IPC for getting the string from range is only sent to a RenderWidgetHost which is part of the frame tree (which will not be that of a guest represented by a BrowserPlugin). This CL will: 1) Handle dictionary related IPCs on the UI thread to remove the threading issue and make transforming the reported points to root coordinates based on compositor frames and the right OOPIF way (calling RWHV::TransformPointToRooTCoordSpace which can only be done on the UI thread). 2) Implement RenderWidgetHostViewChildFrame::ShowDefinitionForSelection(). 3) Make sure that for BrowserPlugin-based guest the IPC is sent to the RenderWidget of the guest renderer. The CL also adds a test to avoid regressions in word lookup inside guests. BUG=643233, 702348, 708183 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Making Mac Dictionary better for OOPIFs and <webview> There is two ways to invoke Mac dictionary for Chromium: choosing look-up word in context menu for highlighted text, or pressing Ctrl + Command + D. Both methods might involve sending an IPC through TextInputClientMac to the RenderWidget corresponding to the string (i.e., focused RenderWidget or the RenderWidget at the position of mouse cursor). In both cases the IPC comes back with a string and a position with respect to the local roots view port coordinates. The result however is handled on IO thread which leads to complications such as a DCHECK firing (at times) due to accessing the RenderWidgetHostViews weak pointer when getting the focused RenderWidgetHost. Furthermore, word lookup through context menu is broken for <webview>s; both OOPIF-based and BrowserPlugin-based. The former type of <webview>s don't support this feature because RenderWidgetHostViewChildFrame does not implement ShowDefinitionForSelection() method. The latter have regressed after implementing Mac dictionary support for OOPIFs () where the IPC for getting the string from range is only sent to a RenderWidgetHost which is part of the frame tree (which will not be that of a guest represented by a BrowserPlugin). This CL will: 1) Handle dictionary related IPCs on the UI thread to remove the threading issue and make transforming the reported points to root coordinates based on compositor frames and the right OOPIF way (calling RWHV::TransformPointToRooTCoordSpace which can only be done on the UI thread). 2) Implement RenderWidgetHostViewChildFrame::ShowDefinitionForSelection(). 3) Make sure that for BrowserPlugin-based guest the IPC is sent to the RenderWidget of the guest renderer. The CL also adds a test to avoid regressions in word lookup inside guests. BUG=643233, 702348, 708183 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
FYI, re "There is two ways to invoke Mac dictionary for Chromium: choosing look-up word in context menu for highlighted text, or pressing Ctrl + Command + D." there's a third way. You can also force-press on a word on hardware that supports force-press. Can you confirm you handle that case as well?
So, to my not-particularly-studied eye, this looks reasonable. But I was looking at the removal of the async dispatches, and I ran across their origin in https://codereview.chromium.org/1318483007 where synchronous calls to TextInputClientMac::GetAttributedSubstringFromRange were to be reduced. I can't find that function any more. Is that no longer a concern of ours? https://codereview.chromium.org/2817673003/diff/20001/chrome/browser/apps/gue... File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right): https://codereview.chromium.org/2817673003/diff/20001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1408: // TestBrowserClient needs to replace the ChromeContenBrowserClient after most typo: ChromeContentBrowserClient
On 2017/04/13 14:39:08, Avi (OOO 10-12 April) wrote: > FYI, re > > "There is two ways to invoke Mac dictionary for Chromium: choosing look-up word > in context menu for highlighted text, or pressing Ctrl + Command + D." > > there's a third way. You can also force-press on a word on hardware that > supports force-press. Can you confirm you handle that case as well? I think force press lookup is the same as three finger tap. Either way it is implemented using quickLookWithEvent API which reuses showLookupDictionaryOverlayAtPoint which is the same as pressing ctrl + command + D. https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid... That being said I should update the description to reflect that.
On 2017/04/13 14:48:53, Avi (OOO 10-12 April) wrote: > So, to my not-particularly-studied eye, this looks reasonable. But I was looking > at the removal of the async dispatches, and I ran across their origin in > https://codereview.chromium.org/1318483007 where synchronous calls to > TextInputClientMac::GetAttributedSubstringFromRange were to be reduced. I can't > find that function any more. Is that no longer a concern of ours? > > https://codereview.chromium.org/2817673003/diff/20001/chrome/browser/apps/gue... > File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right): > > https://codereview.chromium.org/2817673003/diff/20001/chrome/browser/apps/gue... > chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1408: // > TestBrowserClient needs to replace the ChromeContenBrowserClient after most > typo: ChromeContentBrowserClient I believe it is not our concern any longer. The method was removed from the head file here: https://codereview.chromium.org/1442853003/ and the logic in render_widget_host_view_mac.mm was liberated from it in https://codereview.chromium.org/1327743002/.
ekaramad@chromium.org changed reviewers: + lazyboy@chromium.org, shuchen@chromium.org
Thanks Avi, Following the comment I am also cc-ing shuchen@ since he worked on removing the sync IPC calls. Also adding lazyboy@ as a reviewer since the test added is similar to 'TextSelection' test which he added. PTAL. https://codereview.chromium.org/2817673003/diff/20001/chrome/browser/apps/gue... File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right): https://codereview.chromium.org/2817673003/diff/20001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1408: // TestBrowserClient needs to replace the ChromeContenBrowserClient after most On 2017/04/13 14:48:53, Avi wrote: > typo: ChromeContentBrowserClient Thanks. Deleted most of the comment since we do not create tabs here (this was taken from site_per_process_text_input_browsertest.cc which deals with chrome and tabs and lookup string).
On 2017/04/13 15:13:07, EhsanK wrote: > Thanks Avi, > > Following the comment I am also cc-ing shuchen@ since he worked on removing the > sync IPC calls. > > Also adding lazyboy@ as a reviewer since the test added is similar to > 'TextSelection' test which he added. > > PTAL. > > https://codereview.chromium.org/2817673003/diff/20001/chrome/browser/apps/gue... > File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right): > > https://codereview.chromium.org/2817673003/diff/20001/chrome/browser/apps/gue... > chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1408: // > TestBrowserClient needs to replace the ChromeContenBrowserClient after most > On 2017/04/13 14:48:53, Avi wrote: > > typo: ChromeContentBrowserClient > > Thanks. Deleted most of the comment since we do not create tabs here (this was > taken from site_per_process_text_input_browsertest.cc which deals with chrome > and tabs and lookup string). So this lgtm, but getting the input of shuchen@ and lazyboy@ will be great.
Sending comments for the added test... https://codereview.chromium.org/2817673003/diff/40001/chrome/browser/apps/gue... File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right): https://codereview.chromium.org/2817673003/diff/40001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1372: class TestBrowserClient : public ChromeContentBrowserClient { nit: rename class too less generic and more specific for text input client. Also can we put this around the top of the file where other classes are? https://codereview.chromium.org/2817673003/diff/40001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1406: // dictionary popup window. I couldn't figure out which part is checking the popup window. Can you explain a bit or update the description? I could only find that the test waits for IPC in line 1429 below. https://codereview.chromium.org/2817673003/diff/40001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1407: IN_PROC_BROWSER_TEST_P(WebViewInteractiveTest, WordLookUp) { nit: WordLookup (small u) or LookUpWord https://codereview.chromium.org/2817673003/diff/40001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1422: EXPECT_TRUE(guest_message_filter); Since we need this to be non-null in line 1429, we should ASSERT_TRUE https://codereview.chromium.org/2817673003/diff/40001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1426: SimulateRWHMouseClick(guest_web_contents()->GetRenderViewHost()->GetWidget(), Can you add a quick note about what the significance of (20,20) is? (Of course that's my fault for not putting the note in the original test). https://codereview.chromium.org/2817673003/diff/40001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1434: content::SetBrowserClientForTesting(old_browser_client); It's better to use a separate class that sets the new client in its constructor and reset it back in the destructor. That will ensure we always reset the client regardless of ASSERTs above: ScopedBrowserClient { ScopedBrowserClient(browser_client) { old_ = SetBrowserClientForTesting(browser_client); } ~ScopedBrowserClient() { SetBrowserClientForTesting(old_); } };
Thanks Istiaque! I will wait for reviews and LGTM from shuchen@ as well. PTAL. https://codereview.chromium.org/2817673003/diff/40001/chrome/browser/apps/gue... File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right): https://codereview.chromium.org/2817673003/diff/40001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1372: class TestBrowserClient : public ChromeContentBrowserClient { On 2017/04/13 16:40:45, lazyboy wrote: > nit: rename class too less generic and more specific for text input client. > Also can we put this around the top of the file where other classes are? Done. https://codereview.chromium.org/2817673003/diff/40001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1406: // dictionary popup window. On 2017/04/13 16:40:45, lazyboy wrote: > I couldn't figure out which part is checking the popup window. Can you explain a > bit or update the description? I could only find that the test waits for IPC in > line 1429 below. Yes sorry about the bad comment. That is what the test does. Getting the IPC means RWHVCocoa will call showDefinitionForAttributedString which is supposed to popup the dict. https://codereview.chromium.org/2817673003/diff/40001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1407: IN_PROC_BROWSER_TEST_P(WebViewInteractiveTest, WordLookUp) { On 2017/04/13 16:40:45, lazyboy wrote: > nit: WordLookup (small u) or LookUpWord Done. https://codereview.chromium.org/2817673003/diff/40001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1422: EXPECT_TRUE(guest_message_filter); On 2017/04/13 16:40:45, lazyboy wrote: > Since we need this to be non-null in line 1429, we should ASSERT_TRUE Done. Thanks! https://codereview.chromium.org/2817673003/diff/40001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1426: SimulateRWHMouseClick(guest_web_contents()->GetRenderViewHost()->GetWidget(), On 2017/04/13 16:40:45, lazyboy wrote: > Can you add a quick note about what the significance of (20,20) is? (Of course > that's my fault for not putting the note in the original test). Sure. I guess that is where the text is (to be highlighted). https://codereview.chromium.org/2817673003/diff/40001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1434: content::SetBrowserClientForTesting(old_browser_client); On 2017/04/13 16:40:45, lazyboy wrote: > It's better to use a separate class that sets the new client in its constructor > and reset it back in the destructor. That will ensure we always reset the client > regardless of ASSERTs above: > ScopedBrowserClient { > ScopedBrowserClient(browser_client) { old_ = > SetBrowserClientForTesting(browser_client); } > ~ScopedBrowserClient() { SetBrowserClientForTesting(old_); } > }; Thanks for the suggestion. I implemented something similar.
lgtm https://codereview.chromium.org/2817673003/diff/60001/chrome/browser/apps/gue... File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right): https://codereview.chromium.org/2817673003/diff/60001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1427: BrowserClientForTextInputClientMac browser_client; optional: You could actually put the scoping logic inside this class's ctor/dtor and remove ScopedBrowserClient.
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...
On 2017/04/13 18:08:18, lazyboy wrote: > lgtm > > https://codereview.chromium.org/2817673003/diff/60001/chrome/browser/apps/gue... > File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right): > > https://codereview.chromium.org/2817673003/diff/60001/chrome/browser/apps/gue... > chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1427: > BrowserClientForTextInputClientMac browser_client; > optional: You could actually put the scoping logic inside this class's ctor/dtor > and remove ScopedBrowserClient. I did. Thanks again for the suggestions.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Making Mac Dictionary better for OOPIFs and <webview> There is two ways to invoke Mac dictionary for Chromium: choosing look-up word in context menu for highlighted text, or pressing Ctrl + Command + D. Both methods might involve sending an IPC through TextInputClientMac to the RenderWidget corresponding to the string (i.e., focused RenderWidget or the RenderWidget at the position of mouse cursor). In both cases the IPC comes back with a string and a position with respect to the local roots view port coordinates. The result however is handled on IO thread which leads to complications such as a DCHECK firing (at times) due to accessing the RenderWidgetHostViews weak pointer when getting the focused RenderWidgetHost. Furthermore, word lookup through context menu is broken for <webview>s; both OOPIF-based and BrowserPlugin-based. The former type of <webview>s don't support this feature because RenderWidgetHostViewChildFrame does not implement ShowDefinitionForSelection() method. The latter have regressed after implementing Mac dictionary support for OOPIFs () where the IPC for getting the string from range is only sent to a RenderWidgetHost which is part of the frame tree (which will not be that of a guest represented by a BrowserPlugin). This CL will: 1) Handle dictionary related IPCs on the UI thread to remove the threading issue and make transforming the reported points to root coordinates based on compositor frames and the right OOPIF way (calling RWHV::TransformPointToRooTCoordSpace which can only be done on the UI thread). 2) Implement RenderWidgetHostViewChildFrame::ShowDefinitionForSelection(). 3) Make sure that for BrowserPlugin-based guest the IPC is sent to the RenderWidget of the guest renderer. The CL also adds a test to avoid regressions in word lookup inside guests. BUG=643233, 702348, 708183 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Making Mac Dictionary better for OOPIFs and <webview> There is two ways to invoke Mac dictionary for Chromium: choosing look-up word in context menu for highlighted text, or pressing Ctrl + Command + D. Both methods might involve sending an IPC through TextInputClientMac to the RenderWidget corresponding to the string (i.e., focused RenderWidget or the RenderWidget at the position of mouse cursor). In both cases the IPC comes back with a string and a position with respect to the local roots view port coordinates. The result however is handled on IO thread which leads to complications such as a DCHECK firing (at times) due to accessing the RenderWidgetHostViews weak pointer when getting the focused RenderWidgetHost. Furthermore, word lookup through context menu is broken for <webview>s; both OOPIF-based and BrowserPlugin-based. The former type of <webview>s don't support this feature because RenderWidgetHostViewChildFrame does not implement ShowDefinitionForSelection() method. The latter have regressed after implementing Mac dictionary support for OOPIFs () where the IPC for getting the string from range is only sent to a RenderWidgetHost which is part of the frame tree (which will not be that of a guest represented by a BrowserPlugin). This CL will: 1) Handle dictionary related IPCs on the UI thread to remove the threading issue and make transforming the reported points to root coordinates based on compositor frames and the right OOPIF way (calling RWHV::TransformPointToRooTCoordSpace which can only be done on the UI thread). 2) Implement RenderWidgetHostViewChildFrame::ShowDefinitionForSelection(). 3) Make sure that for BrowserPlugin-based guest the IPC is sent to the RenderWidget of the guest renderer. The CL also adds a test to avoid regressions in word lookup inside guests. TBR=shuchen@chromium.org BUG=643233, 702348, 708183 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
ekaramad@chromium.org changed reviewers: - shuchen@chromium.org
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...
Description was changed from ========== Making Mac Dictionary better for OOPIFs and <webview> There is two ways to invoke Mac dictionary for Chromium: choosing look-up word in context menu for highlighted text, or pressing Ctrl + Command + D. Both methods might involve sending an IPC through TextInputClientMac to the RenderWidget corresponding to the string (i.e., focused RenderWidget or the RenderWidget at the position of mouse cursor). In both cases the IPC comes back with a string and a position with respect to the local roots view port coordinates. The result however is handled on IO thread which leads to complications such as a DCHECK firing (at times) due to accessing the RenderWidgetHostViews weak pointer when getting the focused RenderWidgetHost. Furthermore, word lookup through context menu is broken for <webview>s; both OOPIF-based and BrowserPlugin-based. The former type of <webview>s don't support this feature because RenderWidgetHostViewChildFrame does not implement ShowDefinitionForSelection() method. The latter have regressed after implementing Mac dictionary support for OOPIFs () where the IPC for getting the string from range is only sent to a RenderWidgetHost which is part of the frame tree (which will not be that of a guest represented by a BrowserPlugin). This CL will: 1) Handle dictionary related IPCs on the UI thread to remove the threading issue and make transforming the reported points to root coordinates based on compositor frames and the right OOPIF way (calling RWHV::TransformPointToRooTCoordSpace which can only be done on the UI thread). 2) Implement RenderWidgetHostViewChildFrame::ShowDefinitionForSelection(). 3) Make sure that for BrowserPlugin-based guest the IPC is sent to the RenderWidget of the guest renderer. The CL also adds a test to avoid regressions in word lookup inside guests. TBR=shuchen@chromium.org BUG=643233, 702348, 708183 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Making Mac Dictionary better for OOPIFs and <webview> There is two ways to invoke Mac dictionary for Chromium: choosing look-up word in context menu for highlighted text, or pressing Ctrl + Command + D. Both methods might involve sending an IPC through TextInputClientMac to the RenderWidget corresponding to the string (i.e., focused RenderWidget or the RenderWidget at the position of mouse cursor). In both cases the IPC comes back with a string and a position with respect to the local roots view port coordinates. The result however is handled on IO thread which leads to complications such as a DCHECK firing (at times) due to accessing the RenderWidgetHostViews weak pointer when getting the focused RenderWidgetHost. Furthermore, word lookup through context menu is broken for <webview>s; both OOPIF-based and BrowserPlugin-based. The former type of <webview>s don't support this feature because RenderWidgetHostViewChildFrame does not implement ShowDefinitionForSelection() method. The latter have regressed after implementing Mac dictionary support for OOPIFs () where the IPC for getting the string from range is only sent to a RenderWidgetHost which is part of the frame tree (which will not be that of a guest represented by a BrowserPlugin). This CL will: 1) Handle dictionary related IPCs on the UI thread to remove the threading issue and make transforming the reported points to root coordinates based on compositor frames and the right OOPIF way (calling RWHV::TransformPointToRooTCoordSpace which can only be done on the UI thread). 2) Implement RenderWidgetHostViewChildFrame::ShowDefinitionForSelection(). 3) Make sure that for BrowserPlugin-based guest the IPC is sent to the RenderWidget of the guest renderer. The CL also adds a test to avoid regressions in word lookup inside guests. BUG=643233, 702348, 708183 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
ekaramad@chromium.org changed reviewers: + shuchen@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
Description was changed from ========== Making Mac Dictionary better for OOPIFs and <webview> There is two ways to invoke Mac dictionary for Chromium: choosing look-up word in context menu for highlighted text, or pressing Ctrl + Command + D. Both methods might involve sending an IPC through TextInputClientMac to the RenderWidget corresponding to the string (i.e., focused RenderWidget or the RenderWidget at the position of mouse cursor). In both cases the IPC comes back with a string and a position with respect to the local roots view port coordinates. The result however is handled on IO thread which leads to complications such as a DCHECK firing (at times) due to accessing the RenderWidgetHostViews weak pointer when getting the focused RenderWidgetHost. Furthermore, word lookup through context menu is broken for <webview>s; both OOPIF-based and BrowserPlugin-based. The former type of <webview>s don't support this feature because RenderWidgetHostViewChildFrame does not implement ShowDefinitionForSelection() method. The latter have regressed after implementing Mac dictionary support for OOPIFs () where the IPC for getting the string from range is only sent to a RenderWidgetHost which is part of the frame tree (which will not be that of a guest represented by a BrowserPlugin). This CL will: 1) Handle dictionary related IPCs on the UI thread to remove the threading issue and make transforming the reported points to root coordinates based on compositor frames and the right OOPIF way (calling RWHV::TransformPointToRooTCoordSpace which can only be done on the UI thread). 2) Implement RenderWidgetHostViewChildFrame::ShowDefinitionForSelection(). 3) Make sure that for BrowserPlugin-based guest the IPC is sent to the RenderWidget of the guest renderer. The CL also adds a test to avoid regressions in word lookup inside guests. BUG=643233, 702348, 708183 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Making Mac Dictionary better for OOPIFs and <webview> There is two ways to invoke Mac dictionary for Chromium: choosing look-up word in context menu for highlighted text, or pressing Ctrl + Command + D. Both methods might involve sending an IPC through TextInputClientMac to the RenderWidget corresponding to the string (i.e., focused RenderWidget or the RenderWidget at the position of mouse cursor). In both cases the IPC comes back with a string and a position with respect to the local roots view port coordinates. The result however is handled on IO thread which leads to complications such as a DCHECK firing (at times) due to accessing the RenderWidgetHostViews weak pointer when getting the focused RenderWidgetHost. Furthermore, word lookup through context menu is broken for <webview>s; both OOPIF-based and BrowserPlugin-based. The former type of <webview>s don't support this feature because RenderWidgetHostViewChildFrame does not implement ShowDefinitionForSelection() method. The latter have regressed after implementing Mac dictionary support for OOPIFs () where the IPC for getting the string from range is only sent to a RenderWidgetHost which is part of the frame tree (which will not be that of a guest represented by a BrowserPlugin). This CL will: 1) Handle dictionary related IPCs on the UI thread to remove the threading issue and make transforming the reported points to root coordinates based on compositor frames and the right OOPIF way (calling RWHV::TransformPointToRooTCoordSpace which can only be done on the UI thread). 2) Implement RenderWidgetHostViewChildFrame::ShowDefinitionForSelection(). 3) Make sure that for BrowserPlugin-based guest the IPC is sent to the RenderWidget of the guest renderer. The CL also adds a test to avoid regressions in word lookup inside guests. TBR=shuchen@chromium.org BUG=643233, 702348, 708183 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
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 ekaramad@chromium.org
The CQ bit was checked by ekaramad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org, lazyboy@chromium.org Link to the patchset: https://codereview.chromium.org/2817673003/#ps80001 (title: "Remove ScopedBrowserClient")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493158746744560,
"parent_rev": "055ff3daf2d59a887a683da267bcd5137b5b84d1", "commit_rev":
"67a361af4485aee793aa2ad8dacddd249d19fc1c"}
Message was sent while issue was closed.
Description was changed from ========== Making Mac Dictionary better for OOPIFs and <webview> There is two ways to invoke Mac dictionary for Chromium: choosing look-up word in context menu for highlighted text, or pressing Ctrl + Command + D. Both methods might involve sending an IPC through TextInputClientMac to the RenderWidget corresponding to the string (i.e., focused RenderWidget or the RenderWidget at the position of mouse cursor). In both cases the IPC comes back with a string and a position with respect to the local roots view port coordinates. The result however is handled on IO thread which leads to complications such as a DCHECK firing (at times) due to accessing the RenderWidgetHostViews weak pointer when getting the focused RenderWidgetHost. Furthermore, word lookup through context menu is broken for <webview>s; both OOPIF-based and BrowserPlugin-based. The former type of <webview>s don't support this feature because RenderWidgetHostViewChildFrame does not implement ShowDefinitionForSelection() method. The latter have regressed after implementing Mac dictionary support for OOPIFs () where the IPC for getting the string from range is only sent to a RenderWidgetHost which is part of the frame tree (which will not be that of a guest represented by a BrowserPlugin). This CL will: 1) Handle dictionary related IPCs on the UI thread to remove the threading issue and make transforming the reported points to root coordinates based on compositor frames and the right OOPIF way (calling RWHV::TransformPointToRooTCoordSpace which can only be done on the UI thread). 2) Implement RenderWidgetHostViewChildFrame::ShowDefinitionForSelection(). 3) Make sure that for BrowserPlugin-based guest the IPC is sent to the RenderWidget of the guest renderer. The CL also adds a test to avoid regressions in word lookup inside guests. TBR=shuchen@chromium.org BUG=643233, 702348, 708183 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Making Mac Dictionary better for OOPIFs and <webview> There is two ways to invoke Mac dictionary for Chromium: choosing look-up word in context menu for highlighted text, or pressing Ctrl + Command + D. Both methods might involve sending an IPC through TextInputClientMac to the RenderWidget corresponding to the string (i.e., focused RenderWidget or the RenderWidget at the position of mouse cursor). In both cases the IPC comes back with a string and a position with respect to the local roots view port coordinates. The result however is handled on IO thread which leads to complications such as a DCHECK firing (at times) due to accessing the RenderWidgetHostViews weak pointer when getting the focused RenderWidgetHost. Furthermore, word lookup through context menu is broken for <webview>s; both OOPIF-based and BrowserPlugin-based. The former type of <webview>s don't support this feature because RenderWidgetHostViewChildFrame does not implement ShowDefinitionForSelection() method. The latter have regressed after implementing Mac dictionary support for OOPIFs () where the IPC for getting the string from range is only sent to a RenderWidgetHost which is part of the frame tree (which will not be that of a guest represented by a BrowserPlugin). This CL will: 1) Handle dictionary related IPCs on the UI thread to remove the threading issue and make transforming the reported points to root coordinates based on compositor frames and the right OOPIF way (calling RWHV::TransformPointToRooTCoordSpace which can only be done on the UI thread). 2) Implement RenderWidgetHostViewChildFrame::ShowDefinitionForSelection(). 3) Make sure that for BrowserPlugin-based guest the IPC is sent to the RenderWidget of the guest renderer. The CL also adds a test to avoid regressions in word lookup inside guests. TBR=shuchen@chromium.org BUG=643233, 702348, 708183 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2817673003 Cr-Commit-Position: refs/heads/master@{#467149} Committed: https://chromium.googlesource.com/chromium/src/+/67a361af4485aee793aa2ad8dacd... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/67a361af4485aee793aa2ad8dacd...
Message was sent while issue was closed.
lgtm |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
