Moving ResolvedSearchTerm and ContextualSearchContext to components. [chromium/src : main]
Josh Simmons (Gerrit)
Attention is currently required from: Donn Denman.
Patch set 4:Commit-Queue +1
1 comment:
Patchset:
Hi Donn! Here's a start on the process of making Contextual Search available on Desktop. Please take a look when you have a chance and let me know if you have any questions or concerns :)
To view, visit change 3732755. To unsubscribe, or for help writing mail filters, visit settings.
Donn Denman (Gerrit)
Attention is currently required from: Josh Simmons.
2 comments:
Patchset:
Josh, I'm really excited to see this progress!
You should add Gang as a reviewer since he's the new owner of this code.
I would also add bttk@ as an FYI reviewer.I have to run but wanted to send some quick feedback - I've not looked at the refactoring of the CSContext yet.
File chrome/browser/android/contextualsearch/native_contextual_search_context.h:
Patch Set #4, Line 1:
// Copyright 2015 The Chromium Authors. All rights reserved.I'm unsure whether this should be updated to 2022.
Is this a pure move or are there edits?
To view, visit change 3732755. To unsubscribe, or for help writing mail filters, visit settings.
Josh Simmons (Gerrit)
Attention is currently required from: Donn Denman.
2 comments:
Patchset:
Thank you for taking a look, Donn! I'll add Gang and bttk@ as well 👍
File chrome/browser/android/contextualsearch/native_contextual_search_context.h:
Patch Set #4, Line 1:
// Copyright 2015 The Chromium Authors. All rights reserved.
I'm unsure whether this should be updated to 2022. […]
The file has basically been split into two now (contextual_search_context.h and native_*.h), so updating to 2022 seems fine to me 👍
To view, visit change 3732755. To unsubscribe, or for help writing mail filters, visit settings.
Josh Simmons (Gerrit)
Attention is currently required from: Donn Denman, Gang Wu.
1 comment:
Patchset:
Hi Gang! Would you mind taking a look at this refactor when you have a moment? Happy to answer any questions you may have about the direction of these CLs 👍
Cheers,
Josh
To view, visit change 3732755. To unsubscribe, or for help writing mail filters, visit settings.
Gang Wu (Gerrit)
Attention is currently required from: Donn Denman, Josh Simmons.
Patch set 10:Code-Review +1
Josh Simmons (Gerrit)
Attention is currently required from: David Trainor, Donn Denman.
1 comment:
Patchset:
Thank you for taking a look, Gang!
David, would you mind taking a look at the chrome/BUILD.gn and chrome/test/DEPS changes when you have a minute?
To view, visit change 3732755. To unsubscribe, or for help writing mail filters, visit settings.
David Trainor (Gerrit)
Attention is currently required from: Donn Denman, Josh Simmons.
Patch set 10:Code-Review +1
To view, visit change 3732755. To unsubscribe, or for help writing mail filters, visit settings.
David Trainor (Gerrit)
Attention is currently required from: Donn Denman, Josh Simmons.
2 comments:
Patchset:
lgtm % suggestion
File chrome/BUILD.gn:
java_cpp_enum("quick_action_category_enum_javagen") {
sources =
[ "//components/contextual_search/core/browser/resolved_search_term.h" ]
}Actually should this target exist in //components/contextual_search/core/browser/BUILD.gn? Feels weird having it here.
To view, visit change 3732755. To unsubscribe, or for help writing mail filters, visit settings.
Josh Simmons (Gerrit)
Attention is currently required from: Donn Denman.
2 comments:
Patchset:
Thank you both for your help!!
File chrome/BUILD.gn:
java_cpp_enum("quick_action_category_enum_javagen") {
sources =
[ "//components/contextual_search/core/browser/resolved_search_term.h" ]
}
Actually should this target exist in //components/contextual_search/core/browser/BUILD. […]
Good point! Moved :)
To view, visit change 3732755. To unsubscribe, or for help writing mail filters, visit settings.
Josh Simmons (Gerrit)
Attention is currently required from: Donn Denman.
Patch set 12:Commit-Queue +2
Josh Simmons (Gerrit)
Attention is currently required from: Daniel Cheng, Donn Denman.
1 comment:
Patchset:
Hi Daniel! Would you mind taking a look at the addition of +url to components/contextual_search/DEPS when you have a chance? Please let me know if this isn't the correct way to take that dependency :)
To view, visit change 3732755. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Cheng (Gerrit)
Attention is currently required from: Donn Denman, Josh Simmons.
3 comments:
File components/contextual_search/core/browser/contextual_search_context.cc:
Patch Set #13, Line 34:
DCHECK(start_offset_ + start_adjust >= 0);Several things:
1. C-style casts are not allowed, except for casting to void.
2. I read through the code fairly extensively and as far as I can tell, these adjustment indices actually originate from the renderer process (after all, the renderer is the process that knows the selection). Therefore, these indices are untrusted, and we cannot be DCHECKing that these conditions are correct. [1]It would be better if we could check that these indices are valid, much closer to where we handle the reply from the renderer, but maybe that's not easy.
[1] As far as I can tell, the chain is:
https://source.chromium.org/chromium/chromium/src/+/main:chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchContext.java;l=261;drc=5dcf43bf5bb5e68c54a5a3fe2d77e6217ac69110 - call into here over JNI
https://source.chromium.org/chromium/chromium/src/+/main:chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java;l=1407;drc=843911a994428b40537550f48b52d0d58cf08dd3 - one possible call path through Java that looks quite plausible
https://source.chromium.org/chromium/chromium/src/+/main:content/public/android/java/src/org/chromium/content/browser/selection/SelectionPopupControllerImpl.java;l=1511;drc=843911a994428b40537550f48b52d0d58cf08dd3 - one possible call path through Java that looks quite plausible as well
https://source.chromium.org/chromium/chromium/src/+/main:content/browser/android/selection/selection_popup_controller.cc;l=207;drc=aa96e66bd3c5dbaad5a3a8ace54647b1e17ffa73 - back in C++
https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_widget_host_view_android.cc;l=766;drc=843911a994428b40537550f48b52d0d58cf08dd3 - call via RWHVA
https://source.chromium.org/chromium/chromium/src/+/main:content/browser/web_contents/web_contents_android.cc;l=494;drc=843911a994428b40537550f48b52d0d58cf08dd3 - call from WebContentsAndroid
https://source.chromium.org/chromium/chromium/src/+/main:content/browser/web_contents/web_contents_android.cc;l=512;drc=843911a994428b40537550f48b52d0d58cf08dd3 - bound as the reply callback of a Mojo call to the rendererPatch Set #13, Line 53:
return base_page_url_;Optional, but for "simple" getters and setters (such as for base page url, base page encoding, et cetera), they are usually inlined into the header, and they just return a const ref to avoid forcing a copy.
Patch Set #13, Line 84:
const std::u16string& surrounding_text) {Unlike above, it seems like we do try to sanitize it:
https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/android/contextualsearch/contextual_search_delegate.cc;l=373;drc=caf2747fb86c3c4d62a8d434e6d775bda9316e56Though there are still questions like:
- can start_offset be > end_offset?
- what happens if start_offset and/or end_offset are >= std::numeric_limits<int>::max()
et cetera
To view, visit change 3732755. To unsubscribe, or for help writing mail filters, visit settings.
Josh Simmons (Gerrit)
Attention is currently required from: Daniel Cheng, Donn Denman.
Patch set 14:Commit-Queue +1
4 comments:
Patchset:
Thank you for your help, Daniel! Opened crbug/1343955 to track the sanitization you'd mentioned.
File components/contextual_search/core/browser/contextual_search_context.cc:
Patch Set #13, Line 34:
DCHECK(start_offset_ + start_adjust >= 0);
Several things: […]
Thank you for calling this out! Since this is just a refactor I've filed crbug/1343955 to track the sanitization work and will send out a separate CL for it. I've made the cast improvements here, however 👍
Patch Set #13, Line 53:
return base_page_url_;
Optional, but for "simple" getters and setters (such as for base page url, base page encoding, et ce […]
Makes sense! Moved all of the trivial getters/setters into the header and updated the complex types to use const refs 👍
Patch Set #13, Line 84:
const std::u16string& surrounding_text) {
Unlike above, it seems like we do try to sanitize it: […]
Similar to the above comment, added a TODO to track this behavior change in a followup CL.
To view, visit change 3732755. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Cheng (Gerrit)
Attention is currently required from: Donn Denman, Josh Simmons.
Patch set 14:Code-Review +1
1 comment:
Patchset:
LGTM
To view, visit change 3732755. To unsubscribe, or for help writing mail filters, visit settings.
Donn Denman (Gerrit)
Attention is currently required from: Josh Simmons.
To view, visit change 3732755. To unsubscribe, or for help writing mail filters, visit settings.
Josh Simmons (Gerrit)
Patch set 14:Commit-Queue +2
1 comment:
Patchset:
Thank you all for your reviews!!
To view, visit change 3732755. To unsubscribe, or for help writing mail filters, visit settings.
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
Moving ResolvedSearchTerm and ContextualSearchContext to components.
These will also be used by Desktop Partial Search, so moving them out of
android/. Next step will be to move ContextualSearchDelegate to components/, as well, which will involve making it depend on the components/ version of ContextualSearchContext rather than the Native (jni-enabled) version.
Bug: 1340303
Change-Id: I853f122113bfbab2fe2622a006d3c27eb4093c0a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3732755
Commit-Queue: Josh Simmons <j...@google.com>
Reviewed-by: Gang Wu <gan...@chromium.org>
Reviewed-by: Donn Denman <do...@chromium.org>
Reviewed-by: David Trainor <dtra...@chromium.org>
Reviewed-by: Daniel Cheng <dch...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1023784}
---
M chrome/BUILD.gn
M chrome/android/BUILD.gn
M chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchContext.java
M chrome/browser/BUILD.gn
M chrome/browser/android/bottombar/overlay_panel_content.h
D chrome/browser/android/contextualsearch/contextual_search_context.cc
D chrome/browser/android/contextualsearch/contextual_search_context.h
M chrome/browser/android/contextualsearch/contextual_search_delegate.cc
M chrome/browser/android/contextualsearch/contextual_search_delegate.h
M chrome/browser/android/contextualsearch/contextual_search_delegate_unittest.cc
M chrome/browser/android/contextualsearch/contextual_search_manager.cc
M chrome/browser/android/contextualsearch/contextual_search_manager.h
A chrome/browser/android/contextualsearch/native_contextual_search_context.cc
A chrome/browser/android/contextualsearch/native_contextual_search_context.h
M chrome/test/BUILD.gn
M chrome/test/DEPS
D components/contextual_search/core/BUILD.gn
A components/contextual_search/core/DEPS
A components/contextual_search/core/browser/BUILD.gn
A components/contextual_search/core/browser/contextual_search_context.cc
A components/contextual_search/core/browser/contextual_search_context.h
R components/contextual_search/core/browser/resolved_search_term.cc
R components/contextual_search/core/browser/resolved_search_term.h
23 files changed, 587 insertions(+), 497 deletions(-)