Use mojo typemaps to simplify arc::IntentFilter::IntentFilter()

components/arc/intent_helper/intent_filter.h

Index: components/arc/intent_helper/intent_filter.h
diff --git a/components/arc/intent_helper/intent_filter.h b/components/arc/intent_helper/intent_filter.h
index eb07523432deb312d8cfc597ec493e4ef28a67e7..29dde5be577d888ff436b8b56c174c9f84f26def 100644
--- a/components/arc/intent_helper/intent_filter.h
+++ b/components/arc/intent_helper/intent_filter.h
@@ -8,31 +8,31 @@
 #include <string>
 #include <vector>
 
-#include "components/arc/common/intent_helper.mojom.h"
-
 class GURL;
 
 namespace arc {
 
+namespace mojom {
+enum class PatternType;
+}

[Yusuke Sato, 2016/11/30 21:47:15]

  // namespace mojom

[yoshiki, 2016/12/02 19:34:27]

Done.

+
 // A chrome-side implementation of Android's IntentFilter class.  This is used
 // to approximate the intent filtering and determine whether a given URL is
 // likely to be handled by any android-side apps, prior to making expensive IPC
 // calls.
 class IntentFilter {
  public:
-  explicit IntentFilter(const mojom::IntentFilterPtr& mojo_intent_filter);
-  IntentFilter(const IntentFilter& other);
-  ~IntentFilter();
-
-  bool Match(const GURL& url) const;
-
- private:
   // A helper class for handling matching of the host part of the URL.
   class AuthorityEntry {

[Yusuke Sato, 2016/11/30 21:47:15]

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;

[Yusuke Sato, 2016/11/30 22:07:22]

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().

[yoshiki, 2016/12/02 19:34:27]

Move constructor and move assignment operator are necessary for other code, so
let me add them.

    public:
-    explicit AuthorityEntry(const mojom::AuthorityEntryPtr& entry);
+    AuthorityEntry();
+    AuthorityEntry(std::string host, int port);

[Luis Héctor Chávez, 2016/11/30 23:04:59]

Given that std::string is copiable, but you plan to std::move it into your
class, why not make it std::string&&?

[hidehiko, 2016/12/01 02:06:33]

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.

[Yusuke Sato, 2016/12/01 02:22:13]

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 :) 

[yoshiki, 2016/12/02 19:34:27]

I think the current way is ok since there is no good allowed way to restrict it
to rvalue reference.

dchen@, yusukes@, WDYT?

[Yusuke Sato, 2016/12/02 20:13:01]

I'm fine with either. Adding dcheng@ to the reviewer list.

+
     bool Match(const GURL& url) const;
 
+    std::string host() const { return host_; }

[Yusuke Sato, 2016/11/30 21:47:15]

const std::string& ?

[yoshiki, 2016/12/02 19:34:27]

Done.

+    int port() const { return port_; }
+
    private:
     std::string host_;
     bool wild_;
@@ -42,9 +42,14 @@ class IntentFilter {
   // A helper class for handling matching of various patterns in the URL.
   class PatternMatcher {

[Yusuke Sato, 2016/11/30 21:47:15]

same, DISALLOW_COPY_AND_ASSIGN, move ctor/operator with =default;.

[yoshiki, 2016/12/02 19:34:27]

Done.

    public:
-    explicit PatternMatcher(const mojom::PatternMatcherPtr& pattern);
+    PatternMatcher();
+    PatternMatcher(std::string pattern, mojom::PatternType match_type);

[Luis Héctor Chávez, 2016/11/30 23:04:59]

Same here, std::string&& pattern

+
     bool Match(const std::string& match) const;
 
+    std::string pattern() const { return pattern_; }

[Yusuke Sato, 2016/11/30 21:47:15]

const std::string&  ?

[yoshiki, 2016/12/02 19:34:27]

Done.

+    mojom::PatternType match_type() const { return match_type_; }
+
    private:
     bool MatchGlob(const std::string& match) const;
 
@@ -52,6 +57,24 @@ class IntentFilter {
     mojom::PatternType match_type_;
   };
 
+  IntentFilter();
+  IntentFilter(const IntentFilter& other);

[Yusuke Sato, 2016/11/30 21:47:15]

Can you remove this and add DISALLOW_COPY_AND_ASSIGN for IntentFilter instead?
Copying two vectors doesn't seem like a good idea.

[yoshiki, 2016/12/02 19:34:27]

Done.

+  IntentFilter(std::vector<AuthorityEntry> authorities,
+               std::vector<PatternMatcher> paths);
+

[Yusuke Sato, 2016/11/30 21:47:15]

..and then please add move ctor/operator.

[yoshiki, 2016/12/02 19:34:27]

Done.

+  ~IntentFilter();
+
+  bool Match(const GURL& url) const;
+
+  const std::vector<AuthorityEntry>& authorities() const {
+    return authorities_;
+  }
+  std::vector<AuthorityEntry>& authorities() { return authorities_; }

[Yusuke Sato, 2016/11/30 21:47:15]

This non-const version is only for intent_filter_unittest.cc, right? How about
renaming this to GetMutableAuthoritiesForTesting() or
AddAuthoritiesForTesting(....) ?

[Luis Héctor Chávez, 2016/11/30 23:04:59]

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).

[yoshiki, 2016/12/02 19:34:27]

Removed.

+
+  const std::vector<PatternMatcher>& paths() const { return paths_; }
+  std::vector<PatternMatcher>& paths() { return paths_; }

[Yusuke Sato, 2016/11/30 21:47:15]

same, please rename to ...ForTesting().

[yoshiki, 2016/12/02 19:34:27]

Also removed.

+
+ private:
   bool MatchDataAuthority(const GURL& url) const;
   bool HasDataPath(const GURL& url) const;