Merge lp:~vanvugt/mir/appid into lp:~mir-team/mir/trunk

[Daniel van Vugt]

Allow the client app_name to optionally represent a universally unique URN.
If so, this can be used by the server/shell to look up application metadata.

URNs follow the specification laid out in RFC2141:
  http://www.ietf.org/rfc/rfc2141.txt
URNs can be used to represent universally unique IDs such as RFC4122:
  http://www.ietf.org/rfc/rfc4122.txt
but I'm not committing to UUIDs being the only supported syntax right now.

This is based on a vague recollection of what we discussed for:
"[vanvugt] Unique key for applications: INPROGRESS"
(https://blueprints.launchpad.net/ubuntu/+spec/client-1303-mir-phone-iteration-0)

If no one is too sure we will ever need it, then I'm happy to drop it.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:528
http://jenkins.qa.ubuntu.com/job/mir-ci/157/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/72
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/158
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/158/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/157/rebuild

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

I don't see that overloading name for this purpose is the right approach.

Why shouldn't a session have both a urn and a name? (Admittedly adding an optional urn is a slightly bigger code change, but it seems more natural to me.)

review: Needs Information

[Robert Ancell (robert-ancell)]

I don't see what this is adding over the name. The contents of name are currently undefined and could contain a URN if that's what we require. I think we need Thomas to work out how applications identify themselves and how authenticate that a Mir connection is the application we think.

My gut feel is the only way the name can really be useful is if it is passed to the application by the shell so the shell can confirm a connection came from an expected application.

review: Needs Information

[Daniel van Vugt (vanvugt)]

Alan,

Initially I was thinking the same. In fact my iterations went:
  1. Replace app_name with app_urn.
  2. Complement app_name with adjacent app_urn.
  3. Optionally use app_name as the URN (proposed here).

I did not go with #1 because it forced all developers to create a universally unique ID for their app, regardless of how trivial the app. Admittedly you just need to run "uuidgen" on Ubuntu. But that also makes reporting ugly. UUIDs (or whatever) are not very pleasant to read in a log.

I did not go with #2 because of the complexity as you say. But also because providing a universally unique ID as we discussed in London was a means for avoiding specifying a fixed app name in a single language. The intention is that you look at the URN and then look up the localized name of the app if you need it. Though the obvious argument against that is that it depends on a common config backend to work at all. And schemas to be defined etc. That's not always required by app developers so you should be allowed to just use a simple app_name alone.

So I went with #3. It retains simplicity by default. You are not forced to engage in the complexity of allocating URNs and providing a configuration/metadata backend if you don't want it. It's simple by default, but retains the power to do more if you want it.

---
Robert,

Yes agreed. The longer I spent thinking about the problem, the more I started to believe it should involve zero code change. At least for now, until we have a practical need for it. I would personally prefer we rejected the whole requirement/work item, for now.

[Daniel van Vugt (vanvugt)]

Also, a simple app_name already works fine for discriminating applications on Linux. Just look in:
   /usr/share/applications/

So I do recommend rejecting this proposal, but only if we can reject the requirement for it too:
   https://blueprints.launchpad.net/ubuntu/+spec/client-1303-mir-phone-iteration-0

[Daniel van Vugt (vanvugt)]

Work in progress and task postponed. Pending better requirements gathering.

Unmerged revisions

528. By Daniel van Vugt on 2013-03-21

Remember puctuation.

527. By Daniel van Vugt on 2013-03-21

Always use std::string::empty() where possible.

526. By Daniel van Vugt on 2013-03-21

Improve comments.

525. By Daniel van Vugt on 2013-03-21

Improve unit tests for ApplicationSession::urn()

524. By Daniel van Vugt on 2013-03-21

Don't link the the RFC for UUIDs. They're not a requirement.

523. By Daniel van Vugt on 2013-03-21

Make the URN accessible by ApplicationSession::urn()

522. By Daniel van Vugt on 2013-03-21

mir_client_library.h: Initial spec of how to pass app URNs

521. By Daniel van Vugt on 2013-03-21

Const'ify name() for all Session classes.

520. By Daniel van Vugt on 2013-03-21

mir_client_library.h: Tidy up client API docs.

Approved by Robert Ancell, Alan Griffiths, PS Jenkins bot.

519. By Alexandros Frantzis on 2013-03-20

examples: Change render-surfaces to use the DisplayServer class.

Approved by Alan Griffiths, PS Jenkins bot.

=== modified file 'include/client/mir_toolkit/mir_client_library.h'
--- include/client/mir_toolkit/mir_client_library.h	2013-03-20 10:26:48 +0000
+++ include/client/mir_toolkit/mir_client_library.h	2013-03-21 10:03:12 +0000
@@ -155,7 +155,14 @@
  * the connection is established, or fails. The returned wait handle remains
  * valid until the connection has been released.
  *   \param [in] server       A name identifying the server
- *   \param [in] app_name     A name referring to the application
+ *   \param [in] app_name     A name identifying the application. This may be
+ *                            either a simple name or a URN (e.g.
+ *                            "urn:uuid:52bb8448-98d2-404f-b354-b6122020766a").
+ *                            If the name begins with "urn:" then it is assumed
+ *                            to be a URN which can be used by the Mir server
+ *                            and shell to look up further information about
+ *                            the application. For more information, see:
+ *                            http://www.ietf.org/rfc/rfc2141.txt
  *   \param [in] callback     Callback function to be invoked when request
  *                            completes
  *   \param [in,out] context  User data passed to the callback function

=== modified file 'include/server/mir/shell/application_session.h'
--- include/server/mir/shell/application_session.h	2013-03-21 10:03:12 +0000
+++ include/server/mir/shell/application_session.h	2013-03-21 10:03:12 +0000
@@ -42,6 +42,7 @@
     std::shared_ptr<frontend::Surface> get_surface(frontend::SurfaceId surface) const;
 
     std::string name() const;
+    std::string urn() const;  // Unique application ID "urn:..." or empty.
 
     void shutdown();
 

=== modified file 'src/server/shell/application_session.cpp'
--- src/server/shell/application_session.cpp	2013-03-21 10:03:12 +0000
+++ src/server/shell/application_session.cpp	2013-03-21 10:03:12 +0000
@@ -96,6 +96,20 @@
     return session_name;
 }
 
+std::string msh::ApplicationSession::urn() const
+{
+    /*
+     * We could go through the pain of fully validating the URN according to
+     * RFC2141, but there's no real value in that right now.
+     * The only use-case we care about is whether the client is telling us
+     * that the app name is /intended/ as unique URN to look up metadata.
+     * So that's a yes (URN string), or no (blank).
+     */
+
+    return session_name.compare(0, 4, "urn:") == 0 ? session_name :
+           std::string();
+}
+
 void msh::ApplicationSession::shutdown()
 {
     std::unique_lock<std::mutex> lock(surfaces_mutex);

=== modified file 'tests/unit-tests/shell/test_application_session.cpp'
--- tests/unit-tests/shell/test_application_session.cpp	2013-03-20 10:56:07 +0000
+++ tests/unit-tests/shell/test_application_session.cpp	2013-03-21 10:03:12 +0000
@@ -114,3 +114,51 @@
             app_session.destroy_surface(invalid_surface_id);
     }, std::runtime_error);
 }
+
+TEST(Session, no_urn)
+{
+    static const char *invalid_urns[] =
+    {
+        "",
+        "u",
+        "ur",
+        "urn",
+        "Foo",
+        " urn:aa:bb",
+        "hello world",
+        "urn :apples:oranges",
+        "My Application version 3",
+        NULL
+    };
+
+    mtd::MockSurfaceFactory surface_factory;
+
+    for (const char **urn = invalid_urns; *urn; urn++)
+    {
+        msh::ApplicationSession app_session(mt::fake_shared(surface_factory),
+                                            *urn);
+        EXPECT_EQ(*urn, app_session.name());
+        EXPECT_TRUE(app_session.urn().empty());
+    }
+}
+
+TEST(Session, urns)
+{
+    static const char *valid_urns[] =
+    {
+        "urn:abc:def",
+        "urn:uuid:a7b7cceb-9182-4032-a1d5-a9c1f295efe7",
+        "urn:Well-Known-App:Super Widget Blaster 3.0",
+        NULL
+    };
+
+    mtd::MockSurfaceFactory surface_factory;
+
+    for (const char **urn = valid_urns; *urn; urn++)
+    {
+        msh::ApplicationSession app_session(mt::fake_shared(surface_factory),
+                                            *urn);
+        EXPECT_EQ(*urn, app_session.name());
+        EXPECT_EQ(*urn, app_session.urn());
+    }
+}