Android: Remove ContentViewCore::RequestTextSurroundingSelection() |
|||||
Issue descriptionThe overall goal here is to simply get rid of that method in ContentViewCore in order to be able to remove this public interface. As far as what the code does is concerned: ContentViewCoreImpl::RequestTextSurroundingSelection() gets the focused frame from the WebContents then calls into it to send a FrameMsg. And there is a separate API to set a callback for the reply (and that API is in a different class, uh). The reply is also routed through a single 'current' callback which is buggy too since there is it doesn't match replies with the corresponding requests. I suggest something simple like this: - Merge the request and reply API and put it into a single class. It could just be in RenderFrameHost and then the context search call site would call into the focused frame intentionally (those are all public APIs). - Handle more requests while one is still in flight (it could just return an empty text for successive requests if one is still pending).
,
Jul 8 2016
,
Jul 15 2016
@sievers I can take up this task if none started looking into this.
,
Jul 15 2016
We have an upcoming major refactoring that probably takes precedence over this work, so if you can work on it that would be great!
,
Jul 15 2016
@donnd Thanks for the update. I will check this problem soon :)
,
Jul 15 2016
@sievers/donnd - Can we maintain a Callback list with an auto generated id inside RenderFrameHost to identify correct callback when the response comes from renderer ? This id will be passed to renderer and will come back to browser when the response message is delivered, at that time we remove this callback from this list by searching the id. WDYT ? Or as you said - Handle more requests while one is still in flight (it could just return an empty text for successive requests if one is still pending). - This can be handled by checking the callback object status of RenderFrameHost. If it's not NULL, then we skip the current request from processing. Pass request to renderer only when this callback is empty and set this incoming callback for delivering the response. Could you pls comment on your thoughts on this ?
,
Jul 15 2016
Sounds good to keep it simple and support only one in flight as it does now. It would be nice though if consecutive attempts would at least complete (and signal an empty callback with no text).
,
Jul 15 2016
It would be nice though if consecutive attempts would at least complete (and signal an empty callback with no text). - This part I didn't got it correctly. If we support multiple requests at same time, why do we need to send an empty text ? Could you please elaborate bit more ?
,
Jul 15 2016
I meant that if you call RequestTextSurroundingSelection(Callback cb) then cb should be triggered for every invocation. Otherwise it leads to flaky code in the caller if the contract is 'the callback *might* be called or not'. If we cannot handle the request (because there is already one pending or other error condition), then it can just call |cb| with null arguments.
,
Jul 20 2016
Thanks sievers for the clear explanations :)
,
Jul 21 2016
,
Jul 21 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/deb84849f66aa9db12a4c4bd8eb0bb3e18fd441d commit deb84849f66aa9db12a4c4bd8eb0bb3e18fd441d Author: ajith.v <ajith.v@chromium.org> Date: Thu Jul 21 10:23:49 2016 Remove ContentViewCore::RequestTextSurroundingSelection This change takes care of scaling down the scope of CVC by removing RequestTextSurroundingSelection() method. Logic has been moved to RenderFrameHost. Additionally added the support of responding with empty text when a new request comes during processing of a pending request. BUG= 626770 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2157793002 Cr-Commit-Position: refs/heads/master@{#406812} [modify] https://crrev.com/deb84849f66aa9db12a4c4bd8eb0bb3e18fd441d/chrome/browser/android/contextualsearch/contextual_search_delegate.cc [modify] https://crrev.com/deb84849f66aa9db12a4c4bd8eb0bb3e18fd441d/content/browser/android/content_view_core_impl.cc [modify] https://crrev.com/deb84849f66aa9db12a4c4bd8eb0bb3e18fd441d/content/browser/android/content_view_core_impl.h [modify] https://crrev.com/deb84849f66aa9db12a4c4bd8eb0bb3e18fd441d/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/deb84849f66aa9db12a4c4bd8eb0bb3e18fd441d/content/browser/frame_host/render_frame_host_impl.h [modify] https://crrev.com/deb84849f66aa9db12a4c4bd8eb0bb3e18fd441d/content/browser/renderer_host/render_view_host_impl.cc [modify] https://crrev.com/deb84849f66aa9db12a4c4bd8eb0bb3e18fd441d/content/browser/renderer_host/render_view_host_impl.h [modify] https://crrev.com/deb84849f66aa9db12a4c4bd8eb0bb3e18fd441d/content/browser/renderer_host/render_widget_host_view_android.cc [modify] https://crrev.com/deb84849f66aa9db12a4c4bd8eb0bb3e18fd441d/content/browser/renderer_host/render_widget_host_view_android.h [modify] https://crrev.com/deb84849f66aa9db12a4c4bd8eb0bb3e18fd441d/content/browser/renderer_host/render_widget_host_view_base.cc [modify] https://crrev.com/deb84849f66aa9db12a4c4bd8eb0bb3e18fd441d/content/browser/renderer_host/render_widget_host_view_base.h [modify] https://crrev.com/deb84849f66aa9db12a4c4bd8eb0bb3e18fd441d/content/public/browser/android/content_view_core.h [modify] https://crrev.com/deb84849f66aa9db12a4c4bd8eb0bb3e18fd441d/content/public/browser/render_frame_host.h
,
Sep 28 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e4fcb595ed939ec2f912d6fdd211ce70c73ee0ce commit e4fcb595ed939ec2f912d6fdd211ce70c73ee0ce Author: donnd <donnd@chromium.org> Date: Tue Sep 27 18:38:29 2016 [TTS] Cleanup a potential issue introduced in CL 2157793002 Refactoring done in that CL looks like it could introduce potential problems because now there's a path through the affected method that does not reset the context. This CL refactors the affected method to always reset the context while preserving the refactoring done in 2157793002. BUG= 626770 Review-Url: https://codereview.chromium.org/2300753003 Cr-Commit-Position: refs/heads/master@{#421279} [modify] https://crrev.com/e4fcb595ed939ec2f912d6fdd211ce70c73ee0ce/chrome/browser/android/contextualsearch/contextual_search_delegate.cc |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by siev...@chromium.org
, Jul 8 2016