Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1851793002:
Implement a shell of FindRequestManager, and hook it up to WebContentsImpl. (Closed)
|
Created:
4 years, 8 months ago by paulmeyer Modified:
4 years, 8 months ago CC:
chromium-apps-reviews_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, extensions-reviews_chromium.org, jam, nasko+codewatch_chromium.org, site-isolation-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement a shell of FindRequestManager, and hook it up to WebContentsImpl.
This is an intermediate step towards the multi-process find-in-page implementation. At this stage, FindRequestManager does not add any extra functionality, but find requests are plumbed through it. Note that no additional testing is required at this stage.
Design doc for multi-process find-in-page: https://docs.google.com/a/google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zhodPxMOxa8Bg4Wg0/edit?usp=sharing
Summary of specific changes introduced in this patch:
- Find request IDs are now always unique for every find request (previously unique per find session in most cases).
- Very basic shell of FindRequestManager added.
- Find, FindReply, StopFinding, FindMatchRects, FindMatchRectsReply, and ActivateNearestFindResult are now plumbed through FindRequestManager (though it does not yet process or alter these requests).
- FindMatchRects and ActivateNearestFindResult can no longer be called directly via RenderFrameHost. This ensures that all calls must go through FindRequestManager (via WebContents).
- Small find bug addressed in WebViewFindHelper.
BUG=457440
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/c0b762b6f91dec4fb0f20535c1c1fe98cf8d57ca
Cr-Commit-Position: refs/heads/master@{#386966}
Patch Set 1 : #
Total comments: 5
Patch Set 2 : Addressed comments by lfg@, dcheng@. #Patch Set 3 : Small rename to clarify 'session' vs. 'request'. #
Total comments: 27
Patch Set 4 : Addressed comments by ncarter. #
Total comments: 2
Patch Set 5 : Addressed comments and introduced FindRequest struct. #
Total comments: 14
Patch Set 6 : Addressed comments by lfg@ and ncarter. #
Total comments: 2
Patch Set 7 : Small fix. #Patch Set 8 : Rebased and small fix. #Messages
Total messages: 61 (33 generated)
Description was changed from ========== BUG= ========== to ========== BUG= CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== BUG= CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Implement a shell of FindRequestManager, and hook it up to WebContentsImpl. This is an intermediate step towards the multi-process find-in-page implementation. At this stage, FindRequestManager does not add any extra functionality, but find requests are plumbed through it. Note that no additional testing is required at this stage. Design doc for multi-process find-in-page: https://docs.google.com/a/google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zh... BUG=457440 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Implement a shell of FindRequestManager, and hook it up to WebContentsImpl. This is an intermediate step towards the multi-process find-in-page implementation. At this stage, FindRequestManager does not add any extra functionality, but find requests are plumbed through it. Note that no additional testing is required at this stage. Design doc for multi-process find-in-page: https://docs.google.com/a/google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zh... BUG=457440 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Implement a shell of FindRequestManager, and hook it up to WebContentsImpl. This is an intermediate step towards the multi-process find-in-page implementation. At this stage, FindRequestManager does not add any extra functionality, but find requests are plumbed through it. Note that no additional testing is required at this stage. Summary of specific changes introduced in this patch: - Find request IDs are now unique for every find request (previously unique per find session). - Find, StopFinding, FindMatchRects, and ActivateNearestFindResult are now plumbed through FindRequestManager (though it does not process or later the requests). - FindMatchRects and ActivateNearestFindResult can no longer be called directly via RenderFrameHost. This ensures that all calls must go through FindRequestManager (via WebContents). - Design doc for multi-process find-in-page: https://docs.google.com/a/google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zh... BUG=457440 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by paulmeyer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1851793002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1851793002/1
Description was changed from ========== Implement a shell of FindRequestManager, and hook it up to WebContentsImpl. This is an intermediate step towards the multi-process find-in-page implementation. At this stage, FindRequestManager does not add any extra functionality, but find requests are plumbed through it. Note that no additional testing is required at this stage. Summary of specific changes introduced in this patch: - Find request IDs are now unique for every find request (previously unique per find session). - Find, StopFinding, FindMatchRects, and ActivateNearestFindResult are now plumbed through FindRequestManager (though it does not process or later the requests). - FindMatchRects and ActivateNearestFindResult can no longer be called directly via RenderFrameHost. This ensures that all calls must go through FindRequestManager (via WebContents). - Design doc for multi-process find-in-page: https://docs.google.com/a/google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zh... BUG=457440 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Implement a shell of FindRequestManager, and hook it up to WebContentsImpl. This is an intermediate step towards the multi-process find-in-page implementation. At this stage, FindRequestManager does not add any extra functionality, but find requests are plumbed through it. Note that no additional testing is required at this stage. Summary of specific changes introduced in this patch: - Find request IDs are now unique for every find request (previously unique per find session). - Find, StopFinding, FindMatchRects, and ActivateNearestFindResult are now plumbed through FindRequestManager (though it does not yet process or alter these requests). - FindMatchRects and ActivateNearestFindResult can no longer be called directly via RenderFrameHost. This ensures that all calls must go through FindRequestManager (via WebContents). - Design doc for multi-process find-in-page: https://docs.google.com/a/google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zh... BUG=457440 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Implement a shell of FindRequestManager, and hook it up to WebContentsImpl. This is an intermediate step towards the multi-process find-in-page implementation. At this stage, FindRequestManager does not add any extra functionality, but find requests are plumbed through it. Note that no additional testing is required at this stage. Summary of specific changes introduced in this patch: - Find request IDs are now unique for every find request (previously unique per find session). - Find, StopFinding, FindMatchRects, and ActivateNearestFindResult are now plumbed through FindRequestManager (though it does not yet process or alter these requests). - FindMatchRects and ActivateNearestFindResult can no longer be called directly via RenderFrameHost. This ensures that all calls must go through FindRequestManager (via WebContents). - Design doc for multi-process find-in-page: https://docs.google.com/a/google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zh... BUG=457440 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Implement a shell of FindRequestManager, and hook it up to WebContentsImpl. This is an intermediate step towards the multi-process find-in-page implementation. At this stage, FindRequestManager does not add any extra functionality, but find requests are plumbed through it. Note that no additional testing is required at this stage. Summary of specific changes introduced in this patch: - Find request IDs are now unique for every find request (previously unique per find session). - Find, FindReply, StopFinding, FindMatchRects, FindMatchRectsReply, and ActivateNearestFindResult are now plumbed through FindRequestManager (though it does not yet process or alter these requests). - FindMatchRects and ActivateNearestFindResult can no longer be called directly via RenderFrameHost. This ensures that all calls must go through FindRequestManager (via WebContents). - Small find bug addressed in WebViewFindHelper. Design doc for multi-process find-in-page: https://docs.google.com/a/google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zh... BUG=457440 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was unchecked by paulmeyer@chromium.org
Description was changed from ========== Implement a shell of FindRequestManager, and hook it up to WebContentsImpl. This is an intermediate step towards the multi-process find-in-page implementation. At this stage, FindRequestManager does not add any extra functionality, but find requests are plumbed through it. Note that no additional testing is required at this stage. Summary of specific changes introduced in this patch: - Find request IDs are now unique for every find request (previously unique per find session). - Find, FindReply, StopFinding, FindMatchRects, FindMatchRectsReply, and ActivateNearestFindResult are now plumbed through FindRequestManager (though it does not yet process or alter these requests). - FindMatchRects and ActivateNearestFindResult can no longer be called directly via RenderFrameHost. This ensures that all calls must go through FindRequestManager (via WebContents). - Small find bug addressed in WebViewFindHelper. Design doc for multi-process find-in-page: https://docs.google.com/a/google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zh... BUG=457440 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== WIP DO NOT REVIEW. Implement a shell of FindRequestManager, and hook it up to WebContentsImpl. This is an intermediate step towards the multi-process find-in-page implementation. At this stage, FindRequestManager does not add any extra functionality, but find requests are plumbed through it. Note that no additional testing is required at this stage. Summary of specific changes introduced in this patch: - Find request IDs are now unique for every find request (previously unique per find session). - Find, FindReply, StopFinding, FindMatchRects, FindMatchRectsReply, and ActivateNearestFindResult are now plumbed through FindRequestManager (though it does not yet process or alter these requests). - FindMatchRects and ActivateNearestFindResult can no longer be called directly via RenderFrameHost. This ensures that all calls must go through FindRequestManager (via WebContents). - Small find bug addressed in WebViewFindHelper. Design doc for multi-process find-in-page: https://docs.google.com/a/google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zh... BUG=457440 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by paulmeyer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1851793002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1851793002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by paulmeyer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1851793002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1851793002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== WIP DO NOT REVIEW. Implement a shell of FindRequestManager, and hook it up to WebContentsImpl. This is an intermediate step towards the multi-process find-in-page implementation. At this stage, FindRequestManager does not add any extra functionality, but find requests are plumbed through it. Note that no additional testing is required at this stage. Summary of specific changes introduced in this patch: - Find request IDs are now unique for every find request (previously unique per find session). - Find, FindReply, StopFinding, FindMatchRects, FindMatchRectsReply, and ActivateNearestFindResult are now plumbed through FindRequestManager (though it does not yet process or alter these requests). - FindMatchRects and ActivateNearestFindResult can no longer be called directly via RenderFrameHost. This ensures that all calls must go through FindRequestManager (via WebContents). - Small find bug addressed in WebViewFindHelper. Design doc for multi-process find-in-page: https://docs.google.com/a/google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zh... BUG=457440 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Implement a shell of FindRequestManager, and hook it up to WebContentsImpl. This is an intermediate step towards the multi-process find-in-page implementation. At this stage, FindRequestManager does not add any extra functionality, but find requests are plumbed through it. Note that no additional testing is required at this stage. Summary of specific changes introduced in this patch: - Find request IDs are now unique for every find request (previously unique per find session). - Find, FindReply, StopFinding, FindMatchRects, FindMatchRectsReply, and ActivateNearestFindResult are now plumbed through FindRequestManager (though it does not yet process or alter these requests). - FindMatchRects and ActivateNearestFindResult can no longer be called directly via RenderFrameHost. This ensures that all calls must go through FindRequestManager (via WebContents). - Small find bug addressed in WebViewFindHelper. Design doc for multi-process find-in-page: https://docs.google.com/a/google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zh... BUG=457440 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Implement a shell of FindRequestManager, and hook it up to WebContentsImpl. This is an intermediate step towards the multi-process find-in-page implementation. At this stage, FindRequestManager does not add any extra functionality, but find requests are plumbed through it. Note that no additional testing is required at this stage. Summary of specific changes introduced in this patch: - Find request IDs are now unique for every find request (previously unique per find session). - Find, FindReply, StopFinding, FindMatchRects, FindMatchRectsReply, and ActivateNearestFindResult are now plumbed through FindRequestManager (though it does not yet process or alter these requests). - FindMatchRects and ActivateNearestFindResult can no longer be called directly via RenderFrameHost. This ensures that all calls must go through FindRequestManager (via WebContents). - Small find bug addressed in WebViewFindHelper. Design doc for multi-process find-in-page: https://docs.google.com/a/google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zh... BUG=457440 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Implement a shell of FindRequestManager, and hook it up to WebContentsImpl. This is an intermediate step towards the multi-process find-in-page implementation. At this stage, FindRequestManager does not add any extra functionality, but find requests are plumbed through it. Note that no additional testing is required at this stage. Design doc for multi-process find-in-page: https://docs.google.com/a/google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zh... Summary of specific changes introduced in this patch: - Find request IDs are now unique for every find request (previously unique per find session). - Find, FindReply, StopFinding, FindMatchRects, FindMatchRectsReply, and ActivateNearestFindResult are now plumbed through FindRequestManager (though it does not yet process or alter these requests). - FindMatchRects and ActivateNearestFindResult can no longer be called directly via RenderFrameHost. This ensures that all calls must go through FindRequestManager (via WebContents). - Small find bug addressed in WebViewFindHelper. BUG=457440 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Implement a shell of FindRequestManager, and hook it up to WebContentsImpl. This is an intermediate step towards the multi-process find-in-page implementation. At this stage, FindRequestManager does not add any extra functionality, but find requests are plumbed through it. Note that no additional testing is required at this stage. Design doc for multi-process find-in-page: https://docs.google.com/a/google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zh... Summary of specific changes introduced in this patch: - Find request IDs are now unique for every find request (previously unique per find session). - Find, FindReply, StopFinding, FindMatchRects, FindMatchRectsReply, and ActivateNearestFindResult are now plumbed through FindRequestManager (though it does not yet process or alter these requests). - FindMatchRects and ActivateNearestFindResult can no longer be called directly via RenderFrameHost. This ensures that all calls must go through FindRequestManager (via WebContents). - Small find bug addressed in WebViewFindHelper. BUG=457440 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Implement a shell of FindRequestManager, and hook it up to WebContentsImpl. This is an intermediate step towards the multi-process find-in-page implementation. At this stage, FindRequestManager does not add any extra functionality, but find requests are plumbed through it. Note that no additional testing is required at this stage. Design doc for multi-process find-in-page: https://docs.google.com/a/google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zh... Summary of specific changes introduced in this patch: - Find request IDs are now always unique for every find request (previously unique per find session in most cases). - Very Basis shell of FindRequestManager added. - Find, FindReply, StopFinding, FindMatchRects, FindMatchRectsReply, and ActivateNearestFindResult are now plumbed through FindRequestManager (though it does not yet process or alter these requests). - FindMatchRects and ActivateNearestFindResult can no longer be called directly via RenderFrameHost. This ensures that all calls must go through FindRequestManager (via WebContents). - Small find bug addressed in WebViewFindHelper. BUG=457440 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Implement a shell of FindRequestManager, and hook it up to WebContentsImpl. This is an intermediate step towards the multi-process find-in-page implementation. At this stage, FindRequestManager does not add any extra functionality, but find requests are plumbed through it. Note that no additional testing is required at this stage. Design doc for multi-process find-in-page: https://docs.google.com/a/google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zh... Summary of specific changes introduced in this patch: - Find request IDs are now always unique for every find request (previously unique per find session in most cases). - Very Basis shell of FindRequestManager added. - Find, FindReply, StopFinding, FindMatchRects, FindMatchRectsReply, and ActivateNearestFindResult are now plumbed through FindRequestManager (though it does not yet process or alter these requests). - FindMatchRects and ActivateNearestFindResult can no longer be called directly via RenderFrameHost. This ensures that all calls must go through FindRequestManager (via WebContents). - Small find bug addressed in WebViewFindHelper. BUG=457440 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Implement a shell of FindRequestManager, and hook it up to WebContentsImpl. This is an intermediate step towards the multi-process find-in-page implementation. At this stage, FindRequestManager does not add any extra functionality, but find requests are plumbed through it. Note that no additional testing is required at this stage. Design doc for multi-process find-in-page: https://docs.google.com/a/google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zh... Summary of specific changes introduced in this patch: - Find request IDs are now always unique for every find request (previously unique per find session in most cases). - Very basic shell of FindRequestManager added. - Find, FindReply, StopFinding, FindMatchRects, FindMatchRectsReply, and ActivateNearestFindResult are now plumbed through FindRequestManager (though it does not yet process or alter these requests). - FindMatchRects and ActivateNearestFindResult can no longer be called directly via RenderFrameHost. This ensures that all calls must go through FindRequestManager (via WebContents). - Small find bug addressed in WebViewFindHelper. BUG=457440 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
paulmeyer@chromium.org changed reviewers: + lfg@chromium.org
+lfg@ for initial review.
lgtm with nit. Add ncarter (nick@chromium.org) for content/ review. https://codereview.chromium.org/1851793002/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/1851793002/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:753: friend class FindRequestManager; Those functions can be public in WebContentsImpl instead of this.
drive-by https://codereview.chromium.org/1851793002/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1851793002/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4820: if (!find_request_manager_.get()) Nit: no .get() https://codereview.chromium.org/1851793002/diff/60001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_find_helper.cc (right): https://codereview.chromium.org/1851793002/diff/60001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_find_helper.cc:165: if (find_update_event_.get()) Nit: no .get()
paulmeyer@chromium.org changed reviewers: + nick@chromium.org
+nick@ for content/ review. https://codereview.chromium.org/1851793002/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1851793002/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4820: if (!find_request_manager_.get()) On 2016/04/04 18:59:52, dcheng wrote: > Nit: no .get() Done. Thanks for catching this! I didn't realize that the check worked properly without the '.get()'. https://codereview.chromium.org/1851793002/diff/60001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_find_helper.cc (right): https://codereview.chromium.org/1851793002/diff/60001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_find_helper.cc:165: if (find_update_event_.get()) On 2016/04/04 18:59:52, dcheng wrote: > Nit: no .get() Done.
The following feedback is mostly naming and comment suggestions. Feel free to push back on any of these, if I've missed something. Thanks! https://codereview.chromium.org/1851793002/diff/100001/content/browser/find_r... File content/browser/find_request_manager.cc (right): https://codereview.chromium.org/1851793002/diff/100001/content/browser/find_r... content/browser/find_request_manager.cc:30: void FindRequestManager::Find(int request_id, if request_id is comparable to current_session_id, should one of them be renamed to match the other? The multiplicity of id flavors is a little hard to follow. https://codereview.chromium.org/1851793002/diff/100001/content/browser/find_r... content/browser/find_request_manager.cc:33: DCHECK(request_id > current_session_id_); DCHECK_GT. Also could you add a comment (or << "log statement") explaining why this is an invariant? https://codereview.chromium.org/1851793002/diff/100001/content/browser/find_r... content/browser/find_request_manager.cc:109: void FindRequestManager::SendFindReply(int request_id, I propose we rename both FindRequestManager::SendFindReply and WebContentsImpl::FindReply to have the same name, like "NotifyFindReply". My reasoning: this method is called Send, but it doesn't seem to send an IPC message, so the name is inconsistent with how Send is used elsewhere in the class. "Notify" is probably a better verb for this -- there is precedent for that in WebContentsImpl, e.g. WebContentsImpl::NotifyNavigationStateChanged. https://codereview.chromium.org/1851793002/diff/100001/content/browser/find_r... File content/browser/find_request_manager.h (right): https://codereview.chromium.org/1851793002/diff/100001/content/browser/find_r... content/browser/find_request_manager.h:24: class FindRequestManager { Could you add a class comment here? You can probably just paraphrase the first couple paragraphs of your excellent design doc. https://codereview.chromium.org/1851793002/diff/100001/content/browser/find_r... content/browser/find_request_manager.h:42: void FindReply(RenderFrameHost* rfh, What would you think about naming this method (and FindMatchRectsReply) to have an "On" prefix? That makes it a little clearer that these events are triggered by an IPC. https://codereview.chromium.org/1851793002/diff/100001/content/browser/find_r... content/browser/find_request_manager.h:87: int fmr_request_version_; fmr -> former ( https://google.github.io/styleguide/cppguide.html#General_Naming_Rules ) https://codereview.chromium.org/1851793002/diff/100001/content/browser/find_r... content/browser/find_request_manager.h:97: int current_session_id_; We could consider making find-in-page code use typed IDs, a la SavePackageId. Seems like a good fit here, to help distinguish request_version vs session_id vs request_id. (some background: Currently IdType32 lives in gpu, but it's okay to use in content. If it picks up sufficiently many users, we hope to promote it to base. lukasza is the expert on this and would be happy to answer questions) If you want to do that, a follow-on CL would be totally fine. https://codereview.chromium.org/1851793002/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1851793002/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4826: FindRequestManager* WebContentsImpl::GetOrCreateFindRequestManager() { I am okay with this name (it matches what we did for the accessibility manager). I would also be okay with calling this GetFindRequestManager(), and letting the lazy-allocation be an implementation detail. Your call, just throwing it out there. https://codereview.chromium.org/1851793002/diff/100001/content/public/browser... File content/public/browser/render_frame_host.h (right): https://codereview.chromium.org/1851793002/diff/100001/content/public/browser... content/public/browser/render_frame_host.h:165: virtual void ActivateFindInPageResultForAccessibility(int request_id) = 0; Is this method on your road map? Should it move to web_contents.h as well, to match ActivateNearestFindResult()? https://codereview.chromium.org/1851793002/diff/100001/content/public/browser... File content/public/browser/web_contents.h (right): https://codereview.chromium.org/1851793002/diff/100001/content/public/browser... content/public/browser/web_contents.h:712: // find-in-page coordinates. Could you document what find-in-page coordinates are? https://codereview.chromium.org/1851793002/diff/100001/content/public/browser... content/public/browser/web_contents.h:715: // Requests the rects of the current find matches from the renderers. Could you document what |current_version| means, who generates them (do they come from content or the embedder?) and what the semantics are?
Patchset #4 (id:120001) has been deleted
PTAL. https://codereview.chromium.org/1851793002/diff/100001/content/browser/find_r... File content/browser/find_request_manager.cc (right): https://codereview.chromium.org/1851793002/diff/100001/content/browser/find_r... content/browser/find_request_manager.cc:30: void FindRequestManager::Find(int request_id, On 2016/04/06 19:08:03, ncarter wrote: > if request_id is comparable to current_session_id, should one of them be renamed > to match the other? > > The multiplicity of id flavors is a little hard to follow. Yeah, they can be a bit confusing. Ironically, |current_session_id_| was called |current_request_id_| until very recently when I changed it to "session" it in order to make it more clear. :) Basically, the session ID is a find request ID, but it is specifically the request ID of the initial find request in a single find session (a group of consecutive find requests for the same search string, consisting of an initial find request to find all of the matches, followed by any number of "find next" requests that just advance the active match). Because this variable actually tracks the initial find request ID, which does not change throughout a find session, it really identifies the whole session, not just one find request, so I changed the name to reflect that. This rename was also necessary because I recently had the need to additionally keep track of the find request ID of the latest individual request sent, so I called THAT variable |current_request_id_| instead. I think I'll add that into this CL and hopefully the set of comments will make the difference clear. Some context: previously, for some find helpers (like FindTabHelper), all find requests in the same session were given the same request IDs (because there was no need to differentiate their replies), while for some others (like WebViewFindHelper), every request was given a unique ID because their replies needed to be matched up with specific callbacks. This led to request IDs sometimes identifying specific find requests, and sometimes just identifying which find session the requests belong to. In both cases, they were just called "request IDs", which was somewhat confusing. As part of the multi-process find-in-page implementation, I was forced to make it so that all requests always get unique IDs, because FindRequestManager will (in an upcoming CL) need to differentiate all requests from each other. That being said, it also needs to keep track of the ID for the first request in the session, so that it can easily disregard replies with lower IDs that must have come from an older session (which don't matter anymore). I introduced the term "session ID" so that it is more clear what an ID is actually identifying. In summary, the session ID is similar to a request ID, but only updates once per find session instead of for every request. They always match up so that when a new find session begins with an initial request, the request ID is updated and then the session ID is updated to match the initial request ID. It is then easy to tell when replies come in with request_id < current_session_id_ that they must be from a previous session and should be ignored. https://codereview.chromium.org/1851793002/diff/100001/content/browser/find_r... content/browser/find_request_manager.cc:33: DCHECK(request_id > current_session_id_); On 2016/04/06 19:08:03, ncarter wrote: > DCHECK_GT. > > Also could you add a comment (or << "log statement") explaining why this is an > invariant? Done. https://codereview.chromium.org/1851793002/diff/100001/content/browser/find_r... content/browser/find_request_manager.cc:109: void FindRequestManager::SendFindReply(int request_id, On 2016/04/06 19:08:03, ncarter wrote: > I propose we rename both FindRequestManager::SendFindReply and > WebContentsImpl::FindReply to have the same name, like "NotifyFindReply". > > My reasoning: this method is called Send, but it doesn't seem to send an IPC > message, so the name is inconsistent with how Send is used elsewhere in the > class. "Notify" is probably a better verb for this -- there is precedent for > that in WebContentsImpl, e.g. WebContentsImpl::NotifyNavigationStateChanged. You're right, "Send" doesn't fit. "Notify" seems more reasonable to me too. Done. https://codereview.chromium.org/1851793002/diff/100001/content/browser/find_r... File content/browser/find_request_manager.h (right): https://codereview.chromium.org/1851793002/diff/100001/content/browser/find_r... content/browser/find_request_manager.h:24: class FindRequestManager { On 2016/04/06 19:08:03, ncarter wrote: > Could you add a class comment here? > > You can probably just paraphrase the first couple paragraphs of your excellent > design doc. Done. https://codereview.chromium.org/1851793002/diff/100001/content/browser/find_r... content/browser/find_request_manager.h:42: void FindReply(RenderFrameHost* rfh, On 2016/04/06 19:08:03, ncarter wrote: > What would you think about naming this method (and FindMatchRectsReply) to have > an "On" prefix? That makes it a little clearer that these events are triggered > by an IPC. That sounds reasonable. Done. https://codereview.chromium.org/1851793002/diff/100001/content/browser/find_r... content/browser/find_request_manager.h:87: int fmr_request_version_; On 2016/04/06 19:08:03, ncarter wrote: > fmr -> former ( > https://google.github.io/styleguide/cppguide.html#General_Naming_Rules ) Actually, "fmr" here means "Find Match Rects". In upcoming CLs, I have even more variables prefixed with "fmr", and some others prefixed with "nfr" (meaning "Nearest Find Results", which will be explained in comments). I just didn't want all of these variables to be too long, so I shortened them like "rfh" often is. https://codereview.chromium.org/1851793002/diff/100001/content/browser/find_r... content/browser/find_request_manager.h:97: int current_session_id_; On 2016/04/06 19:08:03, ncarter wrote: > We could consider making find-in-page code use typed IDs, a la SavePackageId. > Seems like a good fit here, to help distinguish request_version vs session_id vs > request_id. > > (some background: Currently IdType32 lives in gpu, but it's okay to use in > content. If it picks up sufficiently many users, we hope to promote it to base. > lukasza is the expert on this and would be happy to answer questions) > > If you want to do that, a follow-on CL would be totally fine. I would prefer to keep these as ints. Session IDs and request IDs are actually the same type of ID, and request versions are different but are only used in one android-specific case. I have updated some comments to hopefully make these IDs more clear. https://codereview.chromium.org/1851793002/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1851793002/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4826: FindRequestManager* WebContentsImpl::GetOrCreateFindRequestManager() { On 2016/04/06 19:08:03, ncarter wrote: > I am okay with this name (it matches what we did for the accessibility manager). > > I would also be okay with calling this GetFindRequestManager(), and letting the > lazy-allocation be an implementation detail. Your call, just throwing it out > there. I'll keep it this way. I think it does have some value when looking at the callsites. You can tell that the thing may be created there, and that the method won't return nullptr. https://codereview.chromium.org/1851793002/diff/100001/content/public/browser... File content/public/browser/render_frame_host.h (right): https://codereview.chromium.org/1851793002/diff/100001/content/public/browser... content/public/browser/render_frame_host.h:165: virtual void ActivateFindInPageResultForAccessibility(int request_id) = 0; On 2016/04/06 19:08:03, ncarter wrote: > Is this method on your road map? Should it move to web_contents.h as well, to > match ActivateNearestFindResult()? As far as I know, no. ActivateNearestFindResult() needs coordination by FindRequestManager (it can't know which process/frame to send the IPC to without some work from FindRequestManager), so I moved the it out of RenderFrameHost so that it can't be called without going through FindRequestManager. This doesn't seem to be the case for ActivateFindInPageResultForAccessibility (I'm not aware of changes needed to the way it works for multi-process find-in-page), though I'm not as familiar with how it works. https://codereview.chromium.org/1851793002/diff/100001/content/public/browser... File content/public/browser/web_contents.h (right): https://codereview.chromium.org/1851793002/diff/100001/content/public/browser... content/public/browser/web_contents.h:712: // find-in-page coordinates. On 2016/04/06 19:08:03, ncarter wrote: > Could you document what find-in-page coordinates are? Find-in-page coordinates are explained in FindInPageCoordinates.h (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...). I just copied this comment from the existing FindTabHelper::ActivateNearestFindResult() method. You will notice if you search for references (https://code.google.com/p/chromium/codesearch#search/&q=%22find-in-page%20coo...) that these coordinates are referred to without further explanation in all of the places they are used. I'm not sure if it is worth changing the comment here. https://codereview.chromium.org/1851793002/diff/100001/content/public/browser... content/public/browser/web_contents.h:715: // Requests the rects of the current find matches from the renderers. On 2016/04/06 19:08:03, ncarter wrote: > Could you document what |current_version| means, who generates them (do they > come from content or the embedder?) and what the semantics are? Done.
https://codereview.chromium.org/1851793002/diff/100001/content/browser/find_r... File content/browser/find_request_manager.cc (right): https://codereview.chromium.org/1851793002/diff/100001/content/browser/find_r... content/browser/find_request_manager.cc:30: void FindRequestManager::Find(int request_id, On 2016/04/07 17:36:14, Paul Meyer wrote: > On 2016/04/06 19:08:03, ncarter wrote: > > if request_id is comparable to current_session_id, should one of them be > renamed > > to match the other? > > > > The multiplicity of id flavors is a little hard to follow. > > Yeah, they can be a bit confusing. Ironically, |current_session_id_| was called > |current_request_id_| until very recently when I changed it to "session" it in > order to make it more clear. :) > > Basically, the session ID is a find request ID, but it is specifically the > request ID of the initial find request in a single find session (a group of > consecutive find requests for the same search string, consisting of an initial > find request to find all of the matches, followed by any number of "find next" > requests that just advance the active match). Because this variable actually > tracks the initial find request ID, which does not change throughout a find > session, it really identifies the whole session, not just one find request, so I > changed the name to reflect that. > > This rename was also necessary because I recently had the need to additionally > keep track of the find request ID of the latest individual request sent, so I > called THAT variable |current_request_id_| instead. I think I'll add that into > this CL and hopefully the set of comments will make the difference clear. > > Some context: previously, for some find helpers (like FindTabHelper), all find > requests in the same session were given the same request IDs (because there was > no need to differentiate their replies), while for some others (like > WebViewFindHelper), every request was given a unique ID because their replies > needed to be matched up with specific callbacks. This led to request IDs > sometimes identifying specific find requests, and sometimes just identifying > which find session the requests belong to. In both cases, they were just called > "request IDs", which was somewhat confusing. As part of the multi-process > find-in-page implementation, I was forced to make it so that all requests always > get unique IDs, because FindRequestManager will (in an upcoming CL) need to > differentiate all requests from each other. That being said, it also needs to > keep track of the ID for the first request in the session, so that it can easily > disregard replies with lower IDs that must have come from an older session > (which don't matter anymore). I introduced the term "session ID" so that it is > more clear what an ID is actually identifying. > > In summary, the session ID is similar to a request ID, but only updates once per > find session instead of for every request. They always match up so that when a > new find session begins with an initial request, the request ID is updated and > then the session ID is updated to match the initial request ID. It is then easy > to tell when replies come in with request_id < current_session_id_ that they > must be from a previous session and should be ignored. Oh, everything makes a lot more sense knowing this. The new comment on current_session_id_ definitely helps clarify. https://codereview.chromium.org/1851793002/diff/100001/content/browser/find_r... File content/browser/find_request_manager.h (right): https://codereview.chromium.org/1851793002/diff/100001/content/browser/find_r... content/browser/find_request_manager.h:87: int fmr_request_version_; On 2016/04/07 17:36:15, Paul Meyer wrote: > On 2016/04/06 19:08:03, ncarter wrote: > > fmr -> former ( > > https://google.github.io/styleguide/cppguide.html#General_Naming_Rules ) > > Actually, "fmr" here means "Find Match Rects". In upcoming CLs, I have even more > variables prefixed with "fmr", and some others prefixed with "nfr" (meaning > "Nearest Find Results", which will be explained in comments). I just didn't want > all of these variables to be too long, so I shortened them like "rfh" often is. We almost always avoid abbreviations in the names of data members. My misintepretation of 'fmr' as 'former' here is a good real-life example of why we have that rule, which reads as follows: """Names should be descriptive; eschew abbreviation. Give as descriptive a name as possible, within reason. Do not worry about saving horizontal space as it is far more important to make your code immediately understandable by a new reader. Do not use abbreviations that are ambiguous or unfamiliar to readers outside your project, and do not abbreviate by deleting letters within a word.""" Because this class is Find-focused, we can probably omit 'Find' from a bunch of member names. So fmr_request_version_ could become: match_rects_request_version_ Which doesn't look too long to me. You can also use sub-structs/classes for grouping, though the boilerplate required (e.g. initialization can get harder to grok) isn't always worth it: struct FindMatchRectsState { request_version; }; FindMatchRectsState match_rects_; An upside to such grouping is that you can define e.g. a Reset() operation in terms of "match_rects_ = FindMatchRectsState();" https://codereview.chromium.org/1851793002/diff/100001/content/browser/find_r... content/browser/find_request_manager.h:97: int current_session_id_; On 2016/04/07 17:36:15, Paul Meyer wrote: > On 2016/04/06 19:08:03, ncarter wrote: > > We could consider making find-in-page code use typed IDs, a la SavePackageId. > > Seems like a good fit here, to help distinguish request_version vs session_id > vs > > request_id. > > > > (some background: Currently IdType32 lives in gpu, but it's okay to use in > > content. If it picks up sufficiently many users, we hope to promote it to > base. > > lukasza is the expert on this and would be happy to answer questions) > > > > If you want to do that, a follow-on CL would be totally fine. > > I would prefer to keep these as ints. Session IDs and request IDs are actually > the same type of ID, and request versions are different but are only used in one > android-specific case. I have updated some comments to hopefully make these IDs > more clear. OK. https://codereview.chromium.org/1851793002/diff/100001/content/public/browser... File content/public/browser/render_frame_host.h (right): https://codereview.chromium.org/1851793002/diff/100001/content/public/browser... content/public/browser/render_frame_host.h:165: virtual void ActivateFindInPageResultForAccessibility(int request_id) = 0; On 2016/04/07 17:36:15, Paul Meyer wrote: > On 2016/04/06 19:08:03, ncarter wrote: > > Is this method on your road map? Should it move to web_contents.h as well, to > > match ActivateNearestFindResult()? > > As far as I know, no. ActivateNearestFindResult() needs coordination by > FindRequestManager (it can't know which process/frame to send the IPC to without > some work from FindRequestManager), so I moved the it out of RenderFrameHost so > that it can't be called without going through FindRequestManager. This doesn't > seem to be the case for ActivateFindInPageResultForAccessibility (I'm not aware > of changes needed to the way it works for multi-process find-in-page), though > I'm not as familiar with how it works. Acknowledged. https://codereview.chromium.org/1851793002/diff/140001/content/browser/find_r... File content/browser/find_request_manager.h (right): https://codereview.chromium.org/1851793002/diff/140001/content/browser/find_r... content/browser/find_request_manager.h:64: void FindMatchRectsReply(RenderFrameHost* rfh, Rename to OnFindMatchRectsReply, to match OnFindReply.
ncarter: PTAL https://codereview.chromium.org/1851793002/diff/100001/content/browser/find_r... File content/browser/find_request_manager.h (right): https://codereview.chromium.org/1851793002/diff/100001/content/browser/find_r... content/browser/find_request_manager.h:87: int fmr_request_version_; On 2016/04/07 18:19:14, ncarter wrote: > On 2016/04/07 17:36:15, Paul Meyer wrote: > > On 2016/04/06 19:08:03, ncarter wrote: > > > fmr -> former ( > > > https://google.github.io/styleguide/cppguide.html#General_Naming_Rules ) > > > > Actually, "fmr" here means "Find Match Rects". In upcoming CLs, I have even > more > > variables prefixed with "fmr", and some others prefixed with "nfr" (meaning > > "Nearest Find Results", which will be explained in comments). I just didn't > want > > all of these variables to be too long, so I shortened them like "rfh" often > is. > > We almost always avoid abbreviations in the names of data members. My > misintepretation of 'fmr' as 'former' here is a good real-life example of why we > have that rule, which reads as follows: > > """Names should be descriptive; eschew abbreviation. Give as descriptive a name > as possible, within reason. Do not worry about saving horizontal space as it is > far more important to make your code immediately understandable by a new reader. > Do not use abbreviations that are ambiguous or unfamiliar to readers outside > your project, and do not abbreviate by deleting letters within a word.""" > > Because this class is Find-focused, we can probably omit 'Find' from a bunch of > member names. So fmr_request_version_ could become: > > match_rects_request_version_ > > Which doesn't look too long to me. > > You can also use sub-structs/classes for grouping, though the boilerplate > required (e.g. initialization can get harder to grok) isn't always worth it: > > struct FindMatchRectsState { > request_version; > }; > FindMatchRectsState match_rects_; > > An upside to such grouping is that you can define e.g. a Reset() operation in > terms of "match_rects_ = FindMatchRectsState();" I like the idea of grouping. I've decided to also make a struct for FindRequest to encompass a single request's ID, search text, and options. https://codereview.chromium.org/1851793002/diff/140001/content/browser/find_r... File content/browser/find_request_manager.h (right): https://codereview.chromium.org/1851793002/diff/140001/content/browser/find_r... content/browser/find_request_manager.h:64: void FindMatchRectsReply(RenderFrameHost* rfh, On 2016/04/07 18:19:14, ncarter wrote: > Rename to OnFindMatchRectsReply, to match OnFindReply. Sorry, missed that. Done. I've also added Notify to WebContentsImpl::FindMatchRectsReply().
https://codereview.chromium.org/1851793002/diff/160001/content/browser/find_r... File content/browser/find_request_manager.cc (right): https://codereview.chromium.org/1851793002/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:44: FindRequest request{request_id, search_text, options}; Compared to what you had before, this will cause an extra unnecessary heap memory allocation because of the string. https://codereview.chromium.org/1851793002/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:120: void FindRequestManager::SendFindIPC(FindRequest request, Pass by const reference.
https://codereview.chromium.org/1851793002/diff/160001/content/browser/find_r... File content/browser/find_request_manager.cc (right): https://codereview.chromium.org/1851793002/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:25: match_rects_{kInvalidId}, This is C++11 uniform initialization syntax, which is banned according to https://chromium-cpp.appspot.com/. It may be allowed very soon ( https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/GF96FshwHLw ). But even if it were allowed, I think you're better off with just making it so that the MatchRects struct's default constructor does this initialization, since you rely on that later in the file. (you can use the C++11 "Non-Static Class Member Initializers" to do this in the header) https://codereview.chromium.org/1851793002/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:44: FindRequest request{request_id, search_text, options}; Use assignment syntax here, per the "BarStruct" draft rules on https://groups.google.com/a/chromium.org/d/msg/chromium-dev/GF96FshwHLw/9HytI... FindRequest request = {request_id, search_text, options}; https://codereview.chromium.org/1851793002/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:116: match_rects_ = FindMatchRectsState(); The default ctor isn't declared, so as far as I can tell this will initialize match_rects_ to random memory values. https://codereview.chromium.org/1851793002/diff/160001/content/browser/find_r... File content/browser/find_request_manager.h (right): https://codereview.chromium.org/1851793002/diff/160001/content/browser/find_r... content/browser/find_request_manager.h:85: void SendFindIPC(FindRequest request, RenderFrameHost* rfh); Usually a struct like this would be passed by const ref.
https://codereview.chromium.org/1851793002/diff/160001/content/browser/find_r... File content/browser/find_request_manager.cc (right): https://codereview.chromium.org/1851793002/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:116: match_rects_ = FindMatchRectsState(); On 2016/04/11 21:27:35, ncarter wrote: > The default ctor isn't declared, so as far as I can tell this will initialize > match_rects_ to random memory values. We had this discussion offline (I also didn't know about this), but when you add the brackets () the class is zero-initialized. That said, I would also prefer to see the constructor. For reference: http://en.cppreference.com/w/cpp/language/value_initialization "if T is a class type with a default constructor that is neither user-provided nor deleted (that is, it may be a class with an implicitly-defined or defaulted default constructor), the object is zero-initialized and then it is default-initialized if it has a non-trivial default constructor;"
PTAL https://codereview.chromium.org/1851793002/diff/160001/content/browser/find_r... File content/browser/find_request_manager.cc (right): https://codereview.chromium.org/1851793002/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:25: match_rects_{kInvalidId}, On 2016/04/11 21:27:35, ncarter wrote: > This is C++11 uniform initialization syntax, which is banned according to > https://chromium-cpp.appspot.com/. > > It may be allowed very soon ( > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/GF96FshwHLw > ). > > But even if it were allowed, I think you're better off with just making it so > that the MatchRects struct's default constructor does this initialization, since > you rely on that later in the file. (you can use the C++11 "Non-Static Class > Member Initializers" to do this in the header) Done. https://codereview.chromium.org/1851793002/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:44: FindRequest request{request_id, search_text, options}; On 2016/04/11 20:10:53, lfg wrote: > Compared to what you had before, this will cause an extra unnecessary heap > memory allocation because of the string. lfg: That won't be the case anymore after subsequent CL, but I'd like to keep it set up this way for now. https://codereview.chromium.org/1851793002/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:44: FindRequest request{request_id, search_text, options}; On 2016/04/11 21:27:35, ncarter wrote: > Use assignment syntax here, per the "BarStruct" draft rules on > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/GF96FshwHLw/9HytI... > > FindRequest request = {request_id, search_text, options}; ncarter: Done. https://codereview.chromium.org/1851793002/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:116: match_rects_ = FindMatchRectsState(); On 2016/04/11 21:46:42, lfg wrote: > On 2016/04/11 21:27:35, ncarter wrote: > > The default ctor isn't declared, so as far as I can tell this will initialize > > match_rects_ to random memory values. > > We had this discussion offline (I also didn't know about this), but when you add > the brackets () the class is zero-initialized. That said, I would also prefer to > see the constructor. > > For reference: http://en.cppreference.com/w/cpp/language/value_initialization > > "if T is a class type with a default constructor that is neither user-provided > nor deleted (that is, it may be a class with an implicitly-defined or defaulted > default constructor), the object is zero-initialized and then it is > default-initialized if it has a non-trivial default constructor;" I made the construction explicit now. https://codereview.chromium.org/1851793002/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:120: void FindRequestManager::SendFindIPC(FindRequest request, On 2016/04/11 20:10:53, lfg wrote: > Pass by const reference. Done. https://codereview.chromium.org/1851793002/diff/160001/content/browser/find_r... File content/browser/find_request_manager.h (right): https://codereview.chromium.org/1851793002/diff/160001/content/browser/find_r... content/browser/find_request_manager.h:85: void SendFindIPC(FindRequest request, RenderFrameHost* rfh); On 2016/04/11 21:27:35, ncarter wrote: > Usually a struct like this would be passed by const ref. Right, I didn't do that because this function will eventually only be called with Bind(), but now thinking about it, it's okay to use it as const ref anyways because it will always be used synchronously. Done.
lgtm with one fix https://codereview.chromium.org/1851793002/diff/160001/content/browser/find_r... File content/browser/find_request_manager.cc (right): https://codereview.chromium.org/1851793002/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:116: match_rects_ = FindMatchRectsState(); On 2016/04/12 17:54:25, Paul Meyer wrote: > On 2016/04/11 21:46:42, lfg wrote: > > On 2016/04/11 21:27:35, ncarter wrote: > > > The default ctor isn't declared, so as far as I can tell this will > initialize > > > match_rects_ to random memory values. > > > > We had this discussion offline (I also didn't know about this), but when you > add > > the brackets () the class is zero-initialized. That said, I would also prefer > to > > see the constructor. > > > > For reference: http://en.cppreference.com/w/cpp/language/value_initialization > > > > "if T is a class type with a default constructor that is neither user-provided > > nor deleted (that is, it may be a class with an implicitly-defined or > defaulted > > default constructor), the object is zero-initialized and then it is > > default-initialized if it has a non-trivial default constructor;" > > I made the construction explicit now. Thanks for explaining this (the zero initialization is new with c++11?). Seems like we still needed a default ctor, since we wanted the member to be initialized to -1 and not zero. https://codereview.chromium.org/1851793002/diff/180001/content/browser/find_r... File content/browser/find_request_manager.h (right): https://codereview.chromium.org/1851793002/diff/180001/content/browser/find_r... content/browser/find_request_manager.h:21: namespace { https://google.github.io/styleguide/cppguide.html#Namespaces """Do not use unnamed namespaces in .h files.""" Fix this by moving the constant to the private: section of the class.
paulmeyer@chromium.org changed reviewers: + sgurun@chromium.org, sky@chromium.org
+sky@ for OWNER review of find_tab_helper.cc +sgurun@ for OWNER review of android_webview/ https://codereview.chromium.org/1851793002/diff/180001/content/browser/find_r... File content/browser/find_request_manager.h (right): https://codereview.chromium.org/1851793002/diff/180001/content/browser/find_r... content/browser/find_request_manager.h:21: namespace { On 2016/04/12 18:16:35, ncarter wrote: > https://google.github.io/styleguide/cppguide.html#Namespaces > > """Do not use unnamed namespaces in .h files.""" > > Fix this by moving the constant to the private: section of the class. Done.
Patchset #7 (id:200001) has been deleted
LGTM
On 2016/04/12 21:05:47, sky wrote: > LGTM lgtm
The CQ bit was checked by paulmeyer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lfg@chromium.org, nick@chromium.org Link to the patchset: https://codereview.chromium.org/1851793002/#ps220001 (title: "Small fix.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1851793002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1851793002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by paulmeyer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sgurun@chromium.org, nick@chromium.org, sky@chromium.org, lfg@chromium.org Link to the patchset: https://codereview.chromium.org/1851793002/#ps240001 (title: "Rebased and small fix.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1851793002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1851793002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by paulmeyer@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1851793002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1851793002/240001
Message was sent while issue was closed.
Description was changed from ========== Implement a shell of FindRequestManager, and hook it up to WebContentsImpl. This is an intermediate step towards the multi-process find-in-page implementation. At this stage, FindRequestManager does not add any extra functionality, but find requests are plumbed through it. Note that no additional testing is required at this stage. Design doc for multi-process find-in-page: https://docs.google.com/a/google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zh... Summary of specific changes introduced in this patch: - Find request IDs are now always unique for every find request (previously unique per find session in most cases). - Very basic shell of FindRequestManager added. - Find, FindReply, StopFinding, FindMatchRects, FindMatchRectsReply, and ActivateNearestFindResult are now plumbed through FindRequestManager (though it does not yet process or alter these requests). - FindMatchRects and ActivateNearestFindResult can no longer be called directly via RenderFrameHost. This ensures that all calls must go through FindRequestManager (via WebContents). - Small find bug addressed in WebViewFindHelper. BUG=457440 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Implement a shell of FindRequestManager, and hook it up to WebContentsImpl. This is an intermediate step towards the multi-process find-in-page implementation. At this stage, FindRequestManager does not add any extra functionality, but find requests are plumbed through it. Note that no additional testing is required at this stage. Design doc for multi-process find-in-page: https://docs.google.com/a/google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zh... Summary of specific changes introduced in this patch: - Find request IDs are now always unique for every find request (previously unique per find session in most cases). - Very basic shell of FindRequestManager added. - Find, FindReply, StopFinding, FindMatchRects, FindMatchRectsReply, and ActivateNearestFindResult are now plumbed through FindRequestManager (though it does not yet process or alter these requests). - FindMatchRects and ActivateNearestFindResult can no longer be called directly via RenderFrameHost. This ensures that all calls must go through FindRequestManager (via WebContents). - Small find bug addressed in WebViewFindHelper. BUG=457440 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #8 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Implement a shell of FindRequestManager, and hook it up to WebContentsImpl. This is an intermediate step towards the multi-process find-in-page implementation. At this stage, FindRequestManager does not add any extra functionality, but find requests are plumbed through it. Note that no additional testing is required at this stage. Design doc for multi-process find-in-page: https://docs.google.com/a/google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zh... Summary of specific changes introduced in this patch: - Find request IDs are now always unique for every find request (previously unique per find session in most cases). - Very basic shell of FindRequestManager added. - Find, FindReply, StopFinding, FindMatchRects, FindMatchRectsReply, and ActivateNearestFindResult are now plumbed through FindRequestManager (though it does not yet process or alter these requests). - FindMatchRects and ActivateNearestFindResult can no longer be called directly via RenderFrameHost. This ensures that all calls must go through FindRequestManager (via WebContents). - Small find bug addressed in WebViewFindHelper. BUG=457440 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Implement a shell of FindRequestManager, and hook it up to WebContentsImpl. This is an intermediate step towards the multi-process find-in-page implementation. At this stage, FindRequestManager does not add any extra functionality, but find requests are plumbed through it. Note that no additional testing is required at this stage. Design doc for multi-process find-in-page: https://docs.google.com/a/google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zh... Summary of specific changes introduced in this patch: - Find request IDs are now always unique for every find request (previously unique per find session in most cases). - Very basic shell of FindRequestManager added. - Find, FindReply, StopFinding, FindMatchRects, FindMatchRectsReply, and ActivateNearestFindResult are now plumbed through FindRequestManager (though it does not yet process or alter these requests). - FindMatchRects and ActivateNearestFindResult can no longer be called directly via RenderFrameHost. This ensures that all calls must go through FindRequestManager (via WebContents). - Small find bug addressed in WebViewFindHelper. BUG=457440 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/c0b762b6f91dec4fb0f20535c1c1fe98cf8d57ca Cr-Commit-Position: refs/heads/master@{#386966} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/c0b762b6f91dec4fb0f20535c1c1fe98cf8d57ca Cr-Commit-Position: refs/heads/master@{#386966} |
