Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2128913002:
Implement chrome-side intent filtering. (Closed)
|
Created:
4 years, 5 months ago by Ben Kwa Modified:
4 years, 5 months ago CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, chromium-reviews, darin (slow to review), elijahtaylor+arcwatch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yusukes+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement chrome-side intent filtering.
Add a basic IntentFilter implementation that approximates android's
intent filtering. Call this from the LocalActivityResolver, to
determine whether a given URL has any chance of resolving to an
Activity, before starting expensive IPC calls to get the app list from
android for intent disambiguation.
BUG=628788
Committed: https://crrev.com/88636a573855caa28498768bec178f5d316daa63
Cr-Commit-Position: refs/heads/master@{#404995}
Patch Set 1 #Patch Set 2 : Cleanup. Add missing dtors. #Patch Set 3 : Make the compiler happy. #
Total comments: 54
Patch Set 4 : Apply CL feedback. #Patch Set 5 : Apply CL feedback. #
Total comments: 30
Patch Set 6 : Apply CL feedback. #
Messages
Total messages: 23 (9 generated)
Description was changed from ========== Implement chrome-side intent filtering. Add a basic IntentFilter implementation that approximates android's intent filtering. Call this from the LocalActivityResolver, to determine whether a given URL has any chance of resolving to an Activity, before starting expensive IPC calls to get the app list from android for intent disambiguation. BUG=29248657 ========== to ========== Implement chrome-side intent filtering. Add a basic IntentFilter implementation that approximates android's intent filtering. Call this from the LocalActivityResolver, to determine whether a given URL has any chance of resolving to an Activity, before starting expensive IPC calls to get the app list from android for intent disambiguation. BUG=29248657 ==========
kenobi@chromium.org changed reviewers: + rickyz@chromium.org, yusukes@chromium.org
Will probably have more comments next week, sorry for the delay! https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... File components/arc/intent_helper/intent_filter.cc (right): https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:161: bool IntentFilter::PatternMatcher::matchGlob( This should have a comment saying that it was transcribed to C++ from Android's PatternMatcher. I'm afraid of copying/pasting code from Android into Chrome OS, especially given that this code is known to be pretty buggy/low quality, for example https://code.google.com/p/android/issues/detail?id=69080. I haven't looked through this carefully for other bugs or memory safety issues yet, but given the level of test coverage that this has in Android, I am far from optimistic: https://android.googlesource.com/platform/cts/+/master/tests/tests/os/src/and...
initial comments: https://codereview.chromium.org/2128913002/diff/40001/components/arc/BUILD.gn File components/arc/BUILD.gn (right): https://codereview.chromium.org/2128913002/diff/40001/components/arc/BUILD.gn... components/arc/BUILD.gn:1: # Copyright 2015 The Chromium Authors. All rights reserved. Please also modify components/arc.gypi. I might be wrong, but my understanding is that gyp builds haven't been deprecated yet. https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... File components/arc/intent_helper/intent_filter.cc (right): https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:4: <algorithm> for std::transform, <ctype.h> for tolower is missing? (see also the comment at line 110.) https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:5: #include "intent_filter.h" components/arc/intent_helper/intent_filter.h https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:13: { ... mojo_intent_filter) { https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:32: bool IntentFilter::match( Could you let me know a list of function you transcribed from Android (so that I can compare them side-by-side?) https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:110: std::transform(host_.begin(), host_.end(), host_.begin(), ::tolower); base/strings/string_util.cc has ToLowerASCII(). base/i18n/case_conversion.h also has ToLower(). We should probably use either of them. https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:117: std::transform(host.begin(), host.end(), host.begin(), ::tolower); same https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:169: char nextChar = pattern_[0]; What about explicitly checking all the string access to make the function as safe as the original one? Probably something like this: #define CHECK_BOUNDS(s, i) do { if (UNLIKELY((s).length() <= (i))) return false; } while(false) ... CHECK_BOUNDS(pattern_, 0); char nextChar = pattern_[0]; https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... File components/arc/intent_helper/intent_filter.h (right): https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.h:5: #ifndef COMPONENTS_ARC_INTENT_HELPER_INTENT_FILTER_H ..._FILTER_H_ https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.h:6: #define COMPONENTS_ARC_INTENT_HELPER_INTENT_FILTER_H same. the trailing underscore missing. https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.h:23: IntentFilter(const mojom::IntentFilterPtr& mojo_intent_filter); explicit https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.h:33: AuthorityEntry(const mojom::AuthorityEntryPtr& entry); explicit https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.h:45: PatternMatcher(const mojom::PatternMatcherPtr& pattern); explicit https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.h:66: #endif #endif // COMPONENTS_ARC_INTENT_HELPER_INTENT_FILTER_H_ https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... File components/arc/intent_helper/local_activity_resolver.cc (right): https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/local_activity_resolver.cc:21: for (const IntentFilter &filter: intent_filters_) { nit: I think we use '& filter' in arc/.
https://codereview.chromium.org/2128913002/diff/40001/components/arc/BUILD.gn File components/arc/BUILD.gn (right): https://codereview.chromium.org/2128913002/diff/40001/components/arc/BUILD.gn... components/arc/BUILD.gn:1: # Copyright 2015 The Chromium Authors. All rights reserved. On 2016/07/11 04:38:31, Yusuke Sato wrote: > Please also modify components/arc.gypi. I might be wrong, but my understanding > is that gyp builds haven't been deprecated yet. > Ah, thanks! I couldn't find that missing ref before. I think this may be the reason one of my trybots is failing. https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... File components/arc/intent_helper/intent_filter.cc (right): https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:4: On 2016/07/11 04:38:32, Yusuke Sato wrote: > <algorithm> for std::transform, <ctype.h> for tolower is missing? (see also the > comment at line 110.) Removed both after the switch to base::toLowerASCII. https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:5: #include "intent_filter.h" On 2016/07/11 04:38:31, Yusuke Sato wrote: > components/arc/intent_helper/intent_filter.h Done. https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:13: { On 2016/07/11 04:38:32, Yusuke Sato wrote: > ... mojo_intent_filter) { Done. https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:32: bool IntentFilter::match( On 2016/07/11 04:38:32, Yusuke Sato wrote: > Could you let me know a list of function you transcribed from Android (so that I > can compare them side-by-side?) Direct transcriptions: - IntentFilter::hasDataSchemeSpecificPart --> IntentFilter#hasDataSchemeSpecificPart [1] - IntentFilter::hasDataPath --> IntentFilter#hasDataPath [2] - IntentFilter::matchDataAuthority --> IntentFilter#matchDataAuthority [3] - AuthorityEntry::match --> IntentFilter.AuthorityEntry#match [4] - IntentFilter::match is a simplified transcription of android's IntentFilter#matchData [5]. Logically it really maps to IntentFilter#match, but the #matchAction and #matchCategories part of that code are redundant in this case. - PatternMatcher::match/matchBlob is a transcription/slight refactoring of PatternMatcher#matchPattern [6]. 1: https://cs.corp.google.com/android/frameworks/base/core/java/android/content/... 2: https://cs.corp.google.com/android/frameworks/base/core/java/android/content/... 3: https://cs.corp.google.com/android/frameworks/base/core/java/android/content/... 4: (https://cs.corp.google.com/android/frameworks/base/core/java/android/content/...) 5: https://cs.corp.google.com/android/frameworks/base/core/java/android/content/... 6: https://cs.corp.google.com/android/frameworks/base/core/java/android/os/Patte... https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:110: std::transform(host_.begin(), host_.end(), host_.begin(), ::tolower); On 2016/07/11 04:38:32, Yusuke Sato wrote: > base/strings/string_util.cc has ToLowerASCII(). base/i18n/case_conversion.h also > has ToLower(). We should probably use either of them. I'm going with base::ToLowerASCII for now. Probably not i18n-friendly, but I have a bit of research to do into IDN support on android and in chrome's GURL class before I know how to fix this properly. https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:117: std::transform(host.begin(), host.end(), host.begin(), ::tolower); On 2016/07/11 04:38:32, Yusuke Sato wrote: > same Done. https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:161: bool IntentFilter::PatternMatcher::matchGlob( On 2016/07/09 02:13:34, rickyz wrote: > This should have a comment saying that it was transcribed to C++ from Android's > PatternMatcher. > > I'm afraid of copying/pasting code from Android into Chrome OS, especially given > that this code is known to be pretty buggy/low quality, for example > https://code.google.com/p/android/issues/detail?id=69080. > > I haven't looked through this carefully for other bugs or memory safety issues > yet, but given the level of test coverage that this has in Android, I am far > from optimistic: > > https://android.googlesource.com/platform/cts/+/master/tests/tests/os/src/and... I've added a comments noting the origins of this and other code in the file. As for your concerns about cutting/pasting code.. I don't think any of us are super happy with the situation, but I'm not sure what other options are available. I'm happy to add more checks/tests/whatever you think might be necessary to make this safer. LMK if you want to chat more about this. https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:169: char nextChar = pattern_[0]; On 2016/07/11 04:38:32, Yusuke Sato wrote: > What about explicitly checking all the string access to make the function as > safe as the original one? Probably something like this: > > #define CHECK_BOUNDS(s, i) do { if (UNLIKELY((s).length() <= (i))) return > false; } while(false) > ... > CHECK_BOUNDS(pattern_, 0); > char nextChar = pattern_[0]; > Done. https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... File components/arc/intent_helper/intent_filter.h (right): https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.h:5: #ifndef COMPONENTS_ARC_INTENT_HELPER_INTENT_FILTER_H On 2016/07/11 04:38:32, Yusuke Sato wrote: > ..._FILTER_H_ Done. https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.h:6: #define COMPONENTS_ARC_INTENT_HELPER_INTENT_FILTER_H On 2016/07/11 04:38:32, Yusuke Sato wrote: > same. the trailing underscore missing. Done. https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.h:23: IntentFilter(const mojom::IntentFilterPtr& mojo_intent_filter); On 2016/07/11 04:38:32, Yusuke Sato wrote: > explicit Done. https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.h:33: AuthorityEntry(const mojom::AuthorityEntryPtr& entry); On 2016/07/11 04:38:32, Yusuke Sato wrote: > explicit Done. https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.h:45: PatternMatcher(const mojom::PatternMatcherPtr& pattern); On 2016/07/11 04:38:32, Yusuke Sato wrote: > explicit Done. https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.h:66: #endif On 2016/07/11 04:38:32, Yusuke Sato wrote: > #endif // COMPONENTS_ARC_INTENT_HELPER_INTENT_FILTER_H_ Done. https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... File components/arc/intent_helper/local_activity_resolver.cc (right): https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/local_activity_resolver.cc:21: for (const IntentFilter &filter: intent_filters_) { On 2016/07/11 04:38:32, Yusuke Sato wrote: > nit: I think we use '& filter' in arc/. Done.
https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... File components/arc/intent_helper/intent_filter.cc (right): https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:45: if (match == false) { nit: if (!match) Or better yet, how about just returning true above and getting rid of this if? https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:48: bool authMatch = matchDataAuthority(url); Could reduce some nesting with: match = matchDataAuthority(url) && (paths_.empty() || hasDataPath(url)); https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:50: if (paths_.empty()) { nit: would be nice to be consistent with .empty() vs. size() > 0 throughout https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:65: if (url.has_ref()) { Can we use GURL::ReplaceComponents instead to get rid of the ref? https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:110: std::transform(host_.begin(), host_.end(), host_.begin(), ::tolower); On 2016/07/11 at 04:38:32, Yusuke Sato wrote: > base/strings/string_util.cc has ToLowerASCII(). base/i18n/case_conversion.h also has ToLower(). We should probably use either of them. According to https://developer.android.com/reference/android/content/IntentFilter.Authorit... authority entry matching is case sensitive, so do we even need to lowercase here and below? Or is this due to Chrome potentially normalizing the URL and always making it lowercase, so we're explicitly allowing false positives here? If so, that probably deserves a comment. https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:118: const int port = url.IntPort(); Do you want EffectiveIntPort here? This returns PORT_UNSPECIFIED for https://domain/ or http://domain/. https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:124: host = host.substr(host.length() - host_.length()); Can we just use base::EndsWith from base/strings/string_util.h here? (Also wow, base:: is a such a polluted namespace...) https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:145: const std::string& match) const { I know Android's code uses match as a var name for some of these, but str might be a less confusing name. https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:151: return pattern_.compare(match) == 0; Perhaps pattern_ == match might read a little easier here and elsewhere? https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:153: return pattern_.compare(0, match.length(), match); Should this be == 0? Or better yet, base::StartsWith? We should probably also have some tests for this code.
Done - thanks! PTAL. https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... File components/arc/intent_helper/intent_filter.cc (right): https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:45: if (match == false) { On 2016/07/11 21:27:51, rickyz wrote: > nit: if (!match) > > Or better yet, how about just returning true above and getting rid of this if? Ack. I can't believe I wrote that. Done. https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:48: bool authMatch = matchDataAuthority(url); On 2016/07/11 21:27:51, rickyz wrote: > Could reduce some nesting with: > > match = matchDataAuthority(url) && (paths_.empty() || hasDataPath(url)); Done. https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:50: if (paths_.empty()) { On 2016/07/11 21:27:52, rickyz wrote: > nit: would be nice to be consistent with .empty() vs. size() > 0 throughout Done. https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:65: if (url.has_ref()) { On 2016/07/11 21:27:52, rickyz wrote: > Can we use GURL::ReplaceComponents instead to get rid of the ref? Done. https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:110: std::transform(host_.begin(), host_.end(), host_.begin(), ::tolower); On 2016/07/11 21:27:52, rickyz wrote: > On 2016/07/11 at 04:38:32, Yusuke Sato wrote: > > base/strings/string_util.cc has ToLowerASCII(). base/i18n/case_conversion.h > also has ToLower(). We should probably use either of them. > > According to > https://developer.android.com/reference/android/content/IntentFilter.Authorit... > authority entry matching is case sensitive, so do we even need to lowercase here > and below? Or is this due to Chrome potentially normalizing the URL and always > making it lowercase, so we're explicitly allowing false positives here? If so, > that probably deserves a comment. So, you're reading that note as prescriptive, but I think it's actually a warning to developers that URLs need to be case-normalized in order for authority comparison to work correctly. And.. uh, docs aside, the authority matching code is actually case insensitive. =P [1] 1: https://cs.corp.google.com/android/frameworks/base/core/java/android/content/... https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:118: const int port = url.IntPort(); On 2016/07/11 21:27:52, rickyz wrote: > Do you want EffectiveIntPort here? This returns PORT_UNSPECIFIED for > https://domain/ or http://domain/. A filter with an explicitly specified default port (e.g. 22) will *not* match a URL that does not specify an explicit port, so I think the answer is no here (i.e. we want IntPort). https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:124: host = host.substr(host.length() - host_.length()); On 2016/07/11 21:27:52, rickyz wrote: > Can we just use base::EndsWith from base/strings/string_util.h here? (Also wow, > base:: is a such a polluted namespace...) Done. https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:145: const std::string& match) const { On 2016/07/11 21:27:52, rickyz wrote: > I know Android's code uses match as a var name for some of these, but str might > be a less confusing name. Done. https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:151: return pattern_.compare(match) == 0; On 2016/07/11 21:27:52, rickyz wrote: > Perhaps pattern_ == match might read a little easier here and elsewhere? Done. https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:153: return pattern_.compare(0, match.length(), match); On 2016/07/11 21:27:52, rickyz wrote: > Should this be == 0? Or better yet, base::StartsWith? > > We should probably also have some tests for this code. Done.
lgtm - it's unfortunate that we don't have a great way to avoid duplicating Android logic here - though the current 10ms IPC latency sounds surprisingly high and might be worth investigating further. https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... File components/arc/intent_helper/intent_filter.cc (right): https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:153: return pattern_.compare(0, match.length(), match); On 2016/07/11 at 22:34:01, Ben Kwa wrote: > On 2016/07/11 21:27:52, rickyz wrote: > > Should this be == 0? Or better yet, base::StartsWith? > > > > We should probably also have some tests for this code. > > Done. I'd still like to see some Chrome side tests for this code, even if it's mostly a transcription from Android. https://codereview.chromium.org/2128913002/diff/80001/components/arc/intent_h... File components/arc/intent_helper/intent_filter.cc (right): https://codereview.chromium.org/2128913002/diff/80001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:40: if (!url.SchemeIsHTTPOrHTTPS()) { Does this mean the intent filters are already filtered out to avoid non-HTTP/HTTPS schemes, or do we still need to do this? https://codereview.chromium.org/2128913002/diff/80001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:61: if (url.has_ref()) { Hm, does has_ref return true for http://example.com/#? Might be safer to just ClearRef unconditionally. https://codereview.chromium.org/2128913002/diff/80001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:69: for (const PatternMatcher& pattern: scheme_specific_parts_) { nit: space before : here and elsewhere (might be good to run git cl format - I see some other minor whitespace it might change)
Sorry for the delay. LGTM with (mostly style) comments: https://codereview.chromium.org/2128913002/diff/80001/components/arc/intent_h... File components/arc/intent_helper/intent_filter.cc (right): https://codereview.chromium.org/2128913002/diff/80001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:6: IWYU: base/compiler_specific.h for UNLIKELY https://codereview.chromium.org/2128913002/diff/80001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:16: authorities_.push_back(AuthorityEntry(authorityptr)); nit: .emplace_back(authorityptr); ? https://codereview.chromium.org/2128913002/diff/80001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:20: paths_.push_back(PatternMatcher(pattern)); same https://codereview.chromium.org/2128913002/diff/80001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:24: scheme_specific_parts_.push_back(PatternMatcher(pattern)); same https://codereview.chromium.org/2128913002/diff/80001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:57: bool IntentFilter::hasDataSchemeSpecificPart( Would you mind adding a test or two for this method? As Ricky mentioned, this somewhat depends on GURL's behavior which Android doesn't use. Since most files in this directory already have _unittest.cc, you can follow the pattern. It's up to you but unittests for other methods are also nice to have, of course. https://codereview.chromium.org/2128913002/diff/80001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:106: wild_ = host_.length() > 0 && host_[0] == '*'; nit: !host_.empty() && would probably easier to read. https://codereview.chromium.org/2128913002/diff/80001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:112: host_ = base::ToLowerASCII(host_); nit: you can probably call base::ToLowerASCII() only at line 130 to make this slightly more efficient. Line 128 already uses CompareCase::INSENSITIVE. https://codereview.chromium.org/2128913002/diff/80001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:120: std::string host = base::ToLowerASCII(url.host_piece()); nit: same. you can probably move base::ToLowerASCII() to line 130. https://codereview.chromium.org/2128913002/diff/80001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:133: nit: remove this line https://codereview.chromium.org/2128913002/diff/80001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:144: if (str.length() == 0) { nit: same. if (str.empty()) https://codereview.chromium.org/2128913002/diff/80001/components/arc/intent_h... File components/arc/intent_helper/local_activity_resolver.cc (right): https://codereview.chromium.org/2128913002/diff/80001/components/arc/intent_h... components/arc/intent_helper/local_activity_resolver.cc:35: intent_filters_.push_back(IntentFilter(mojo_filter)); nit: you could use .emplace_back(mojo_filter); probably. https://codereview.chromium.org/2128913002/diff/80001/components/arc/intent_h... File components/arc/intent_helper/local_activity_resolver.h (right): https://codereview.chromium.org/2128913002/diff/80001/components/arc/intent_h... components/arc/intent_helper/local_activity_resolver.h:5: #ifndef COMPONENTS_ARC_INTENT_HELPER_LOCAL_ACTIVITY_RESOLVER_H_ Good catch. Could you also run 'git cl lint' to make detect other mistakes (if any)?
Thanks! I will add tests in a follow-up CL for two reasons: 1) this CL is holding up a bunch of android-side work and the intent filtering in general. Releasing this will enable us to start testing those new flows. 2) I'm still pretty noobish w.r.t. chrome c++ code, so I'm uncomfortable tacking a bunch of new code into an already-LG'd CL. https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... File components/arc/intent_helper/intent_filter.cc (right): https://codereview.chromium.org/2128913002/diff/40001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:153: return pattern_.compare(0, match.length(), match); On 2016/07/11 23:26:44, rickyz wrote: > On 2016/07/11 at 22:34:01, Ben Kwa wrote: > > On 2016/07/11 21:27:52, rickyz wrote: > > > Should this be == 0? Or better yet, base::StartsWith? > > > > > > We should probably also have some tests for this code. > > > > Done. > > I'd still like to see some Chrome side tests for this code, even if it's mostly > a transcription from Android. Oops, sorry. Will add tests in a follow-up. https://codereview.chromium.org/2128913002/diff/80001/components/arc/intent_h... File components/arc/intent_helper/intent_filter.cc (right): https://codereview.chromium.org/2128913002/diff/80001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:6: On 2016/07/12 10:52:58, Yusuke Sato wrote: > IWYU: base/compiler_specific.h for UNLIKELY Done. https://codereview.chromium.org/2128913002/diff/80001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:16: authorities_.push_back(AuthorityEntry(authorityptr)); On 2016/07/12 10:52:57, Yusuke Sato wrote: > nit: .emplace_back(authorityptr); ? Done. https://codereview.chromium.org/2128913002/diff/80001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:20: paths_.push_back(PatternMatcher(pattern)); On 2016/07/12 10:52:57, Yusuke Sato wrote: > same Done. https://codereview.chromium.org/2128913002/diff/80001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:24: scheme_specific_parts_.push_back(PatternMatcher(pattern)); On 2016/07/12 10:52:57, Yusuke Sato wrote: > same Done. https://codereview.chromium.org/2128913002/diff/80001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:40: if (!url.SchemeIsHTTPOrHTTPS()) { On 2016/07/11 23:26:44, rickyz wrote: > Does this mean the intent filters are already filtered out to avoid > non-HTTP/HTTPS schemes, or do we still need to do this? The android side should already filter the filters so we only get http/https. I guess I was just being paranoid here. https://codereview.chromium.org/2128913002/diff/80001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:57: bool IntentFilter::hasDataSchemeSpecificPart( On 2016/07/12 10:52:57, Yusuke Sato wrote: > Would you mind adding a test or two for this method? As Ricky mentioned, this > somewhat depends on GURL's behavior which Android doesn't use. Since most files > in this directory already have _unittest.cc, you can follow the pattern. > > It's up to you but unittests for other methods are also nice to have, of course. Definitely. I'll add the tests as a follow-up CL. https://codereview.chromium.org/2128913002/diff/80001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:61: if (url.has_ref()) { On 2016/07/11 23:26:44, rickyz wrote: > Hm, does has_ref return true for http://example.com/#? > > Might be safer to just ClearRef unconditionally. Done. https://codereview.chromium.org/2128913002/diff/80001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:69: for (const PatternMatcher& pattern: scheme_specific_parts_) { On 2016/07/11 23:26:44, rickyz wrote: > nit: space before : here and elsewhere (might be good to run git cl format - I > see some other minor whitespace it might change) Done. https://codereview.chromium.org/2128913002/diff/80001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:106: wild_ = host_.length() > 0 && host_[0] == '*'; On 2016/07/12 10:52:58, Yusuke Sato wrote: > nit: > > !host_.empty() && > > would probably easier to read. Done. https://codereview.chromium.org/2128913002/diff/80001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:112: host_ = base::ToLowerASCII(host_); On 2016/07/12 10:52:57, Yusuke Sato wrote: > nit: you can probably call base::ToLowerASCII() only at line 130 to make this > slightly more efficient. Line 128 already uses CompareCase::INSENSITIVE. I'm actually going to keep this here in order to avoid repeating the conversion every time we attempt a match. I'll move the lowercasing of the candidate hostname. Thanks! https://codereview.chromium.org/2128913002/diff/80001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:120: std::string host = base::ToLowerASCII(url.host_piece()); On 2016/07/12 10:52:57, Yusuke Sato wrote: > nit: same. you can probably move base::ToLowerASCII() to line 130. Done. https://codereview.chromium.org/2128913002/diff/80001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:133: On 2016/07/12 10:52:58, Yusuke Sato wrote: > nit: remove this line Done. https://codereview.chromium.org/2128913002/diff/80001/components/arc/intent_h... components/arc/intent_helper/intent_filter.cc:144: if (str.length() == 0) { On 2016/07/12 10:52:57, Yusuke Sato wrote: > nit: same. if (str.empty()) Done. https://codereview.chromium.org/2128913002/diff/80001/components/arc/intent_h... File components/arc/intent_helper/local_activity_resolver.cc (right): https://codereview.chromium.org/2128913002/diff/80001/components/arc/intent_h... components/arc/intent_helper/local_activity_resolver.cc:35: intent_filters_.push_back(IntentFilter(mojo_filter)); On 2016/07/12 10:52:58, Yusuke Sato wrote: > nit: you could use .emplace_back(mojo_filter); probably. Done. https://codereview.chromium.org/2128913002/diff/80001/components/arc/intent_h... File components/arc/intent_helper/local_activity_resolver.h (right): https://codereview.chromium.org/2128913002/diff/80001/components/arc/intent_h... components/arc/intent_helper/local_activity_resolver.h:5: #ifndef COMPONENTS_ARC_INTENT_HELPER_LOCAL_ACTIVITY_RESOLVER_H_ On 2016/07/12 10:52:58, Yusuke Sato wrote: > Good catch. Could you also run 'git cl lint' to make detect other mistakes (if > any)? Done.
The CQ bit was checked by kenobi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rickyz@chromium.org, yusukes@chromium.org Link to the patchset: https://codereview.chromium.org/2128913002/#ps100001 (title: "Apply CL feedback.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yusukes@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Implement chrome-side intent filtering. Add a basic IntentFilter implementation that approximates android's intent filtering. Call this from the LocalActivityResolver, to determine whether a given URL has any chance of resolving to an Activity, before starting expensive IPC calls to get the app list from android for intent disambiguation. BUG=29248657 ========== to ========== Implement chrome-side intent filtering. Add a basic IntentFilter implementation that approximates android's intent filtering. Call this from the LocalActivityResolver, to determine whether a given URL has any chance of resolving to an Activity, before starting expensive IPC calls to get the app list from android for intent disambiguation. BUG=29248657 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Implement chrome-side intent filtering. Add a basic IntentFilter implementation that approximates android's intent filtering. Call this from the LocalActivityResolver, to determine whether a given URL has any chance of resolving to an Activity, before starting expensive IPC calls to get the app list from android for intent disambiguation. BUG=29248657 ========== to ========== Implement chrome-side intent filtering. Add a basic IntentFilter implementation that approximates android's intent filtering. Call this from the LocalActivityResolver, to determine whether a given URL has any chance of resolving to an Activity, before starting expensive IPC calls to get the app list from android for intent disambiguation. BUG=29248657 Committed: https://crrev.com/88636a573855caa28498768bec178f5d316daa63 Cr-Commit-Position: refs/heads/master@{#404995} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/88636a573855caa28498768bec178f5d316daa63 Cr-Commit-Position: refs/heads/master@{#404995}
Message was sent while issue was closed.
Description was changed from ========== Implement chrome-side intent filtering. Add a basic IntentFilter implementation that approximates android's intent filtering. Call this from the LocalActivityResolver, to determine whether a given URL has any chance of resolving to an Activity, before starting expensive IPC calls to get the app list from android for intent disambiguation. BUG=29248657 Committed: https://crrev.com/88636a573855caa28498768bec178f5d316daa63 Cr-Commit-Position: refs/heads/master@{#404995} ========== to ========== Implement chrome-side intent filtering. Add a basic IntentFilter implementation that approximates android's intent filtering. Call this from the LocalActivityResolver, to determine whether a given URL has any chance of resolving to an Activity, before starting expensive IPC calls to get the app list from android for intent disambiguation. BUG=628788 Committed: https://crrev.com/88636a573855caa28498768bec178f5d316daa63 Cr-Commit-Position: refs/heads/master@{#404995} ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
