Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2511883003:
Use mojo typemaps to simplify arc::IntentFilter::IntentFilter() (Closed)
DescriptionUse mojo typemaps to simplify arc::IntentFilter::IntentFilter()
BUG=665719
Committed: https://crrev.com/6d90e3c056d091756ae21d32a85c843350310878
Cr-Commit-Position: refs/heads/master@{#436545}
Patch Set 1 #
Total comments: 57
Patch Set 2 : Addressed comments #
Total comments: 19
Patch Set 3 : Addressed comments #Messages
Total messages: 75 (54 generated)
The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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 yoshiki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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 yoshiki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:1) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== . BUG= ========== to ========== Use mojo typemaps to simplify GpuArcVideoService::BindDmabuf() BUG=665723 ==========
Description was changed from ========== Use mojo typemaps to simplify GpuArcVideoService::BindDmabuf() BUG=665723 ========== to ========== Use mojo typemaps to simplify arc::IntentFilter::IntentFilter() BUG=665719 ==========
yoshiki@chromium.org changed reviewers: + yusukes@chromium.org
yoshiki@chromium.org changed reviewers: - yusukes@chromium.org
The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run
Patchset #1 (id:80001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:100001) has been deleted
yoshiki@chromium.org changed reviewers: + yusukes@chromium.org
Yusuke-san, PTAL
yusukes@chromium.org changed reviewers: + djacobo@chromium.org, lhchavez@chromium.org
+Luis and David FYI. https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... File components/arc/intent_helper/arc_intent_helper_bridge.h (right): https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/arc_intent_helper_bridge.h:64: std::vector<arc::IntentFilter> intent_filters) override; Is arc:: necessary? https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... File components/arc/intent_helper/intent_filter.cc (right): https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter.cc:20: : authorities_(std::move(authorities)), paths_(std::move(paths)) {} Please merge https://codereview.chromium.org/2533983004/diff/60001/components/arc/intent_h... . I think you need to clear() paths_ when authorities_ is empty(). https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... File components/arc/intent_helper/intent_filter.h (right): https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter.h:17: } // namespace mojom https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter.h:26: class AuthorityEntry { This is not really your fault, but classes in this file are copyable but aren't movable, which I feel is somewhat unusual. What about adding DISALLOW_COPY_AND_ASSIGN(AuthorityEntry); at the end of the class and add move constructor and move assignment operator as public: because you're defining the typemap as moveonly? Both the constructor and operator can be just '= default;' I guess. AuthorityEntry(AuthorityEntry&& other) = default; AuthorityEntry& operator=(AuthorityEntry&& other) = default; https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter.h:33: std::string host() const { return host_; } const std::string& ? https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter.h:43: class PatternMatcher { same, DISALLOW_COPY_AND_ASSIGN, move ctor/operator with =default;. https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter.h:50: std::string pattern() const { return pattern_; } const std::string& ? https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter.h:61: IntentFilter(const IntentFilter& other); Can you remove this and add DISALLOW_COPY_AND_ASSIGN for IntentFilter instead? Copying two vectors doesn't seem like a good idea. https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter.h:64: ..and then please add move ctor/operator. https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter.h:72: std::vector<AuthorityEntry>& authorities() { return authorities_; } This non-const version is only for intent_filter_unittest.cc, right? How about renaming this to GetMutableAuthoritiesForTesting() or AddAuthoritiesForTesting(....) ? https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter.h:75: std::vector<PatternMatcher>& paths() { return paths_; } same, please rename to ...ForTesting(). https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... File components/arc/intent_helper/intent_filter_struct_traits.h (right): https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter_struct_traits.h:29: static const std::vector<arc::IntentFilter::AuthorityEntry> data_authorities( Do we really have to return a copy of the vector here? Can't this be const vector<>& ? https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter_struct_traits.h:33: static const std::vector<arc::IntentFilter::PatternMatcher> data_paths( same, const reference? https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter_struct_traits.h:50: static const std::string host(const arc::IntentFilter::AuthorityEntry& r) { same https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter_struct_traits.h:64: static const std::string pattern(const arc::IntentFilter::PatternMatcher& r) { same https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... File components/arc/intent_helper/intent_filter_unittest.cc (right): https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter_unittest.cc:23: IntentFilterBuilder() {} = default; https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter_unittest.cc:42: operator IntentFilter() const { Not sure const is still relevant. Please remove if needed. (also see my comment below) https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter_unittest.cc:43: return std::move(IntentFilter(filter_spec_)); I think this can be std::move(filter_spec_); now. https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... File components/arc/intent_helper/local_activity_resolver.h (right): https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/local_activity_resolver.h:19: class IntentFilter; seems unnecessary (see L13 & L42)
Oh and thanks a lot for working on this, Yoshiki. Accidentally pressed publish before writing this :D
https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... File components/arc/intent_helper/intent_filter.h (right): https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter.h:26: class AuthorityEntry { Chatted with Luis about this. For our code, defining a move constructor might be unnecessary. Up to you, I'm fine as long as we have DISALLOW_COPY_AND_ASSIGN().
https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... File components/arc/intent_helper/intent_filter.h (right): https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter.h:29: AuthorityEntry(std::string host, int port); Given that std::string is copiable, but you plan to std::move it into your class, why not make it std::string&&? https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter.h:46: PatternMatcher(std::string pattern, mojom::PatternType match_type); Same here, std::string&& pattern https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter.h:72: std::vector<AuthorityEntry>& authorities() { return authorities_; } On 2016/11/30 21:47:15, Yusuke Sato wrote: > This non-const version is only for intent_filter_unittest.cc, right? How about > renaming this to GetMutableAuthoritiesForTesting() or > AddAuthoritiesForTesting(....) ? Actually you don't need these :D You can pass the pre-constructed std::vector to the constructor of IntentFilter (see my comment in the unittest). https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... File components/arc/intent_helper/intent_filter_struct_traits.cc (right): https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter_struct_traits.cc:23: *out = std::move(intent_filter); You don't need any of the *out = std::move(lvalue). *out = arc::IntentFilter(...) should suffice. https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... File components/arc/intent_helper/intent_filter_unittest.cc (right): https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter_unittest.cc:31: filter_spec_.authorities().emplace_back(std::move(ae)); nit: filter_spec_.authorities().emplace_back(host, port); https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter_unittest.cc:38: filter_spec_.paths().emplace_back(std::move(p)); nit: filter_spec_.paths().emplace_back(path, type); https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... File components/arc/intent_helper/local_activity_resolver_unittest.cc (right): https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/local_activity_resolver_unittest.cc:21: filter.authorities().emplace_back(std::move(authority_entry)); std::vector<IntentFilter::AuthorityEntry> authorities; authorities.emplace_back(host, -1); return IntentFilter(std::move(authorities), std::vector<IntentFilter::PatternMatcher>{});
Thanks for working on this Yoshiki! https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... File components/arc/intent_helper/intent_filter.cc (right): https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter.cc:20: : authorities_(std::move(authorities)), paths_(std::move(paths)) {} On 2016/11/30 21:47:15, Yusuke Sato wrote: > Please merge > https://codereview.chromium.org/2533983004/diff/60001/components/arc/intent_h... > . I think you need to clear() paths_ when authorities_ is empty(). The IntentFilter::Match() method by itself should ignore the paths_ when no authorities_ are described, but please clear as Yusuke suggests to be consistent with the matching rules :)
hidehiko@chromium.org changed reviewers: + hidehiko@chromium.org
Just drive-by and FYI: https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... File components/arc/intent_helper/intent_filter.h (right): https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter.h:29: AuthorityEntry(std::string host, int port); On 2016/11/30 23:04:59, Luis Héctor Chávez wrote: > Given that std::string is copiable, but you plan to std::move it into your > class, why not make it std::string&&? std::string is preferred IIUC, considering; "A(std::string s);" supports both A a("foo"); std::string s = "foo"; A a(s); On the other hand, A(std::string&& s) only supports former, and latter causes an compile error. https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... File components/arc/intent_helper/intent_filter_unittest.cc (right): https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter_unittest.cc:47: IntentFilter filter_spec_; DISALLOW_COPY_AND_ASSIGN?
https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... File components/arc/intent_helper/intent_filter.h (right): https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter.h:29: AuthorityEntry(std::string host, int port); On 2016/12/01 02:06:33, hidehiko wrote: > On 2016/11/30 23:04:59, Luis Héctor Chávez wrote: > > Given that std::string is copiable, but you plan to std::move it into your > > class, why not make it std::string&&? > > std::string is preferred IIUC, considering; > > "A(std::string s);" supports both > > A a("foo"); > > std::string s = "foo"; > A a(s); > > On the other hand, A(std::string&& s) only supports former, and latter causes an > compile error. > I guess the compiler error on copy would probably be the behavior Luis wanted to have here? Having that said, using rvalue reference here may violate the guideline at https://chromium-cpp.appspot.com/. I think dcheng@, our security reviewer, would know the definitive answer :)
The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run
Patchset #2 (id:140001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thank you for comments. PTAL https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... File components/arc/intent_helper/arc_intent_helper_bridge.h (right): https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/arc_intent_helper_bridge.h:64: std::vector<arc::IntentFilter> intent_filters) override; On 2016/11/30 21:47:15, Yusuke Sato wrote: > Is arc:: necessary? Done. https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... File components/arc/intent_helper/intent_filter.cc (right): https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter.cc:20: : authorities_(std::move(authorities)), paths_(std::move(paths)) {} On 2016/11/30 23:46:04, djacobo wrote: > On 2016/11/30 21:47:15, Yusuke Sato wrote: > > Please merge > > > https://codereview.chromium.org/2533983004/diff/60001/components/arc/intent_h... > > . I think you need to clear() paths_ when authorities_ is empty(). > > The IntentFilter::Match() method by itself should ignore the paths_ when no > authorities_ are described, but please clear as Yusuke suggests to be consistent > with the matching rules :) Done. https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... File components/arc/intent_helper/intent_filter.h (right): https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter.h:17: } On 2016/11/30 21:47:15, Yusuke Sato wrote: > // namespace mojom Done. https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter.h:26: class AuthorityEntry { On 2016/11/30 22:07:22, Yusuke Sato wrote: > Chatted with Luis about this. For our code, defining a move constructor might be > unnecessary. Up to you, I'm fine as long as we have DISALLOW_COPY_AND_ASSIGN(). Move constructor and move assignment operator are necessary for other code, so let me add them. https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter.h:29: AuthorityEntry(std::string host, int port); On 2016/12/01 02:22:13, Yusuke Sato wrote: > On 2016/12/01 02:06:33, hidehiko wrote: > > On 2016/11/30 23:04:59, Luis Héctor Chávez wrote: > > > Given that std::string is copiable, but you plan to std::move it into your > > > class, why not make it std::string&&? > > > > std::string is preferred IIUC, considering; > > > > "A(std::string s);" supports both > > > > A a("foo"); > > > > std::string s = "foo"; > > A a(s); > > > > On the other hand, A(std::string&& s) only supports former, and latter causes > an > > compile error. > > > > I guess the compiler error on copy would probably be the behavior Luis wanted to > have here? Having that said, using rvalue reference here may violate the > guideline at https://chromium-cpp.appspot.com/. I think dcheng@, our security > reviewer, would know the definitive answer :) I think the current way is ok since there is no good allowed way to restrict it to rvalue reference. dchen@, yusukes@, WDYT? https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter.h:33: std::string host() const { return host_; } On 2016/11/30 21:47:15, Yusuke Sato wrote: > const std::string& ? Done. https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter.h:43: class PatternMatcher { On 2016/11/30 21:47:15, Yusuke Sato wrote: > same, DISALLOW_COPY_AND_ASSIGN, move ctor/operator with =default;. Done. https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter.h:50: std::string pattern() const { return pattern_; } On 2016/11/30 21:47:15, Yusuke Sato wrote: > const std::string& ? Done. https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter.h:61: IntentFilter(const IntentFilter& other); On 2016/11/30 21:47:15, Yusuke Sato wrote: > Can you remove this and add DISALLOW_COPY_AND_ASSIGN for IntentFilter instead? > Copying two vectors doesn't seem like a good idea. Done. https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter.h:64: On 2016/11/30 21:47:15, Yusuke Sato wrote: > ..and then please add move ctor/operator. Done. https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter.h:72: std::vector<AuthorityEntry>& authorities() { return authorities_; } On 2016/11/30 23:04:59, Luis Héctor Chávez wrote: > On 2016/11/30 21:47:15, Yusuke Sato wrote: > > This non-const version is only for intent_filter_unittest.cc, right? How about > > renaming this to GetMutableAuthoritiesForTesting() or > > AddAuthoritiesForTesting(....) ? > > Actually you don't need these :D You can pass the pre-constructed std::vector to > the constructor of IntentFilter (see my comment in the unittest). Removed. https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter.h:75: std::vector<PatternMatcher>& paths() { return paths_; } On 2016/11/30 21:47:15, Yusuke Sato wrote: > same, please rename to ...ForTesting(). Also removed. https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... File components/arc/intent_helper/intent_filter_struct_traits.cc (right): https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter_struct_traits.cc:23: *out = std::move(intent_filter); On 2016/11/30 23:05:00, Luis Héctor Chávez wrote: > You don't need any of the *out = std::move(lvalue). *out = > arc::IntentFilter(...) should suffice. Done. https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... File components/arc/intent_helper/intent_filter_struct_traits.h (right): https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter_struct_traits.h:29: static const std::vector<arc::IntentFilter::AuthorityEntry> data_authorities( On 2016/11/30 21:47:15, Yusuke Sato wrote: > Do we really have to return a copy of the vector here? Can't this be const > vector<>& ? Done. https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter_struct_traits.h:33: static const std::vector<arc::IntentFilter::PatternMatcher> data_paths( On 2016/11/30 21:47:15, Yusuke Sato wrote: > same, const reference? Done. https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter_struct_traits.h:50: static const std::string host(const arc::IntentFilter::AuthorityEntry& r) { On 2016/11/30 21:47:15, Yusuke Sato wrote: > same Done. https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter_struct_traits.h:64: static const std::string pattern(const arc::IntentFilter::PatternMatcher& r) { On 2016/11/30 21:47:15, Yusuke Sato wrote: > same Done. https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... File components/arc/intent_helper/intent_filter_unittest.cc (right): https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter_unittest.cc:23: IntentFilterBuilder() {} On 2016/11/30 21:47:16, Yusuke Sato wrote: > = default; Done. https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter_unittest.cc:31: filter_spec_.authorities().emplace_back(std::move(ae)); On 2016/11/30 23:05:00, Luis Héctor Chávez wrote: > nit: filter_spec_.authorities().emplace_back(host, port); Done. https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter_unittest.cc:38: filter_spec_.paths().emplace_back(std::move(p)); On 2016/11/30 23:05:00, Luis Héctor Chávez wrote: > nit: filter_spec_.paths().emplace_back(path, type); Done. https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter_unittest.cc:42: operator IntentFilter() const { On 2016/11/30 21:47:16, Yusuke Sato wrote: > Not sure const is still relevant. Please remove if needed. (also see my comment > below) Removed. https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter_unittest.cc:43: return std::move(IntentFilter(filter_spec_)); On 2016/11/30 21:47:16, Yusuke Sato wrote: > I think this can be std::move(filter_spec_); now. I modified the code around here a bit. https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter_unittest.cc:47: IntentFilter filter_spec_; On 2016/12/01 02:06:33, hidehiko wrote: > DISALLOW_COPY_AND_ASSIGN? Done. https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... File components/arc/intent_helper/local_activity_resolver.h (right): https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/local_activity_resolver.h:19: class IntentFilter; On 2016/11/30 21:47:16, Yusuke Sato wrote: > seems unnecessary (see L13 & L42) Done. https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... File components/arc/intent_helper/local_activity_resolver_unittest.cc (right): https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/local_activity_resolver_unittest.cc:21: filter.authorities().emplace_back(std::move(authority_entry)); On 2016/11/30 23:05:00, Luis Héctor Chávez wrote: > std::vector<IntentFilter::AuthorityEntry> authorities; > authorities.emplace_back(host, -1); > > return IntentFilter(std::move(authorities), > std::vector<IntentFilter::PatternMatcher>{}); Done.
https://codereview.chromium.org/2511883003/diff/160001/components/arc/intent_... File components/arc/intent_helper/intent_filter.cc (right): https://codereview.chromium.org/2511883003/diff/160001/components/arc/intent_... components/arc/intent_helper/intent_filter.cc:16: // IntentFilter& IntentFilter::operator=(IntentFilter&& other) = default; remove https://codereview.chromium.org/2511883003/diff/160001/components/arc/intent_... components/arc/intent_helper/intent_filter.cc:22: if (!authorities_.empty()) Please copy the comment at L21 on the left. // In order to register a path we need to have at least one authority.
yusukes@chromium.org changed reviewers: + dcheng@chromium.org
lgtm https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... File components/arc/intent_helper/intent_filter.h (right): https://codereview.chromium.org/2511883003/diff/120001/components/arc/intent_... components/arc/intent_helper/intent_filter.h:29: AuthorityEntry(std::string host, int port); On 2016/12/02 19:34:27, yoshiki wrote: > On 2016/12/01 02:22:13, Yusuke Sato wrote: > > On 2016/12/01 02:06:33, hidehiko wrote: > > > On 2016/11/30 23:04:59, Luis Héctor Chávez wrote: > > > > Given that std::string is copiable, but you plan to std::move it into your > > > > class, why not make it std::string&&? > > > > > > std::string is preferred IIUC, considering; > > > > > > "A(std::string s);" supports both > > > > > > A a("foo"); > > > > > > std::string s = "foo"; > > > A a(s); > > > > > > On the other hand, A(std::string&& s) only supports former, and latter > causes > > an > > > compile error. > > > > > > > I guess the compiler error on copy would probably be the behavior Luis wanted > to > > have here? Having that said, using rvalue reference here may violate the > > guideline at https://chromium-cpp.appspot.com/. I think dcheng@, our security > > reviewer, would know the definitive answer :) > > I think the current way is ok since there is no good allowed way to restrict it > to rvalue reference. > > dchen@, yusukes@, WDYT? I'm fine with either. Adding dcheng@ to the reviewer list.
mojo lgtm I assume the things that currently return empty arrays are because the functionality is not yet implemented? https://codereview.chromium.org/2511883003/diff/160001/components/arc/intent_... File components/arc/intent_helper/intent_filter.h (right): https://codereview.chromium.org/2511883003/diff/160001/components/arc/intent_... components/arc/intent_helper/intent_filter.h:32: AuthorityEntry(std::string host, int port); Nit: I think it's still much more typical to pass strings by const ref and copy where needed rather than move-by-value, unless there's a big gain to be made by moving. (Move-by-value has its own costs; a std::string is bigger than a pointer, this emits more code to destroy strings, etc) https://codereview.chromium.org/2511883003/diff/160001/components/arc/intent_... File components/arc/intent_helper/local_activity_resolver_unittest.cc (right): https://codereview.chromium.org/2511883003/diff/160001/components/arc/intent_... components/arc/intent_helper/local_activity_resolver_unittest.cc:23: std::vector<IntentFilter::PatternMatcher>{}); It's legal to just write this as {} (without the type)
lgtm with one nit. https://codereview.chromium.org/2511883003/diff/160001/components/arc/intent_... File components/arc/intent_helper/intent_filter.h (right): https://codereview.chromium.org/2511883003/diff/160001/components/arc/intent_... components/arc/intent_helper/intent_filter.h:34: AuthorityEntry& operator=(AuthorityEntry&& other) = default; nit: since these classes are not templated, the definition can be safely moved to the .cc file. That'll avoid the comment there.
(still lgtm) https://codereview.chromium.org/2511883003/diff/160001/components/arc/intent_... File components/arc/intent_helper/intent_filter.cc (right): https://codereview.chromium.org/2511883003/diff/160001/components/arc/intent_... components/arc/intent_helper/intent_filter.cc:75: : host_(std::move(host)), port_(port) { remove std::move() https://codereview.chromium.org/2511883003/diff/160001/components/arc/intent_... components/arc/intent_helper/intent_filter.cc:124: : pattern_(std::move(pattern)), match_type_(match_type) {} same https://codereview.chromium.org/2511883003/diff/160001/components/arc/intent_... File components/arc/intent_helper/intent_filter.h (right): https://codereview.chromium.org/2511883003/diff/160001/components/arc/intent_... components/arc/intent_helper/intent_filter.h:32: AuthorityEntry(std::string host, int port); yoshiki@, let's switch to const std::string& then. https://codereview.chromium.org/2511883003/diff/160001/components/arc/intent_... components/arc/intent_helper/intent_filter.h:54: PatternMatcher(std::string pattern, mojom::PatternType match_type); same https://codereview.chromium.org/2511883003/diff/160001/components/arc/intent_... components/arc/intent_helper/intent_filter.h:74: IntentFilter(std::vector<AuthorityEntry> authorities, this one seems fine as-is.
The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! https://codereview.chromium.org/2511883003/diff/160001/components/arc/intent_... File components/arc/intent_helper/intent_filter.cc (right): https://codereview.chromium.org/2511883003/diff/160001/components/arc/intent_... components/arc/intent_helper/intent_filter.cc:16: // IntentFilter& IntentFilter::operator=(IntentFilter&& other) = default; On 2016/12/02 20:10:34, Yusuke Sato wrote: > remove Done. https://codereview.chromium.org/2511883003/diff/160001/components/arc/intent_... components/arc/intent_helper/intent_filter.cc:22: if (!authorities_.empty()) On 2016/12/02 20:10:34, Yusuke Sato wrote: > Please copy the comment at L21 on the left. > > // In order to register a path we need to have at least one authority. Done. https://codereview.chromium.org/2511883003/diff/160001/components/arc/intent_... components/arc/intent_helper/intent_filter.cc:75: : host_(std::move(host)), port_(port) { On 2016/12/03 02:05:22, Yusuke Sato wrote: > remove std::move() Done. https://codereview.chromium.org/2511883003/diff/160001/components/arc/intent_... components/arc/intent_helper/intent_filter.cc:124: : pattern_(std::move(pattern)), match_type_(match_type) {} On 2016/12/03 02:05:22, Yusuke Sato wrote: > same Done. https://codereview.chromium.org/2511883003/diff/160001/components/arc/intent_... File components/arc/intent_helper/intent_filter.h (right): https://codereview.chromium.org/2511883003/diff/160001/components/arc/intent_... components/arc/intent_helper/intent_filter.h:32: AuthorityEntry(std::string host, int port); On 2016/12/03 02:05:22, Yusuke Sato wrote: > yoshiki@, let's switch to const std::string& then. dcheng, yusuke, thank you. Done. https://codereview.chromium.org/2511883003/diff/160001/components/arc/intent_... components/arc/intent_helper/intent_filter.h:34: AuthorityEntry& operator=(AuthorityEntry&& other) = default; On 2016/12/03 01:13:45, Luis Héctor Chávez wrote: > nit: since these classes are not templated, the definition can be safely moved > to the .cc file. That'll avoid the comment there. Done. https://codereview.chromium.org/2511883003/diff/160001/components/arc/intent_... components/arc/intent_helper/intent_filter.h:54: PatternMatcher(std::string pattern, mojom::PatternType match_type); On 2016/12/03 02:05:22, Yusuke Sato wrote: > same Done. https://codereview.chromium.org/2511883003/diff/160001/components/arc/intent_... components/arc/intent_helper/intent_filter.h:74: IntentFilter(std::vector<AuthorityEntry> authorities, On 2016/12/03 02:05:22, Yusuke Sato wrote: > this one seems fine as-is. Acknowledged. https://codereview.chromium.org/2511883003/diff/160001/components/arc/intent_... File components/arc/intent_helper/local_activity_resolver_unittest.cc (right): https://codereview.chromium.org/2511883003/diff/160001/components/arc/intent_... components/arc/intent_helper/local_activity_resolver_unittest.cc:23: std::vector<IntentFilter::PatternMatcher>{}); On 2016/12/03 01:09:05, dcheng wrote: > It's legal to just write this as {} (without the type) Done.
> I assume the things that currently return empty arrays are because the > functionality is not yet implemented? These fields are not used in the current code. So I return empty arrays for them. I believe these are left in the code for compatibility of mojo version between the host and the container.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yoshiki@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org, yusukes@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2511883003/#ps180001 (title: "Addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1481012968595120,
"parent_rev": "84f2294838499dcd2848b4292aaf5d09c8b027ab", "commit_rev":
"defe84be2d011328cc109fe339d6e359ddc1696e"}
Message was sent while issue was closed.
Description was changed from ========== Use mojo typemaps to simplify arc::IntentFilter::IntentFilter() BUG=665719 ========== to ========== Use mojo typemaps to simplify arc::IntentFilter::IntentFilter() BUG=665719 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Use mojo typemaps to simplify arc::IntentFilter::IntentFilter() BUG=665719 ========== to ========== Use mojo typemaps to simplify arc::IntentFilter::IntentFilter() BUG=665719 Committed: https://crrev.com/6d90e3c056d091756ae21d32a85c843350310878 Cr-Commit-Position: refs/heads/master@{#436545} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/6d90e3c056d091756ae21d32a85c843350310878 Cr-Commit-Position: refs/heads/master@{#436545}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:180001) has been created in https://codereview.chromium.org/2557703002/ by tkent@chromium.org. The reason for reverting is: Build failure on Linux ChromiumOS Builder (dbg). https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%... .
Message was sent while issue was closed.
FYI: Findit identified this CL at revision 436545 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb... |
