Convert utility process extension Unpacker IPC to mojo

extensions/common/manifest_location.mojom

Index: extensions/common/manifest_location.mojom
diff --git a/extensions/common/manifest_location.mojom b/extensions/common/manifest_location.mojom
new file mode 100644
index 0000000000000000000000000000000000000000..3c7f2e18ee903f32929c62704586f91ea6db9655
--- /dev/null
+++ b/extensions/common/manifest_location.mojom
@@ -0,0 +1,33 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+module extensions.mojom;
+
+// What an extension was loaded from.
+// NOTE: These values are stored as integers in the preferences and used
+// in histograms so don't remove or reorder existing items.  Just append
+// to the end.
+enum ManifestLocation {

[Devlin, 2017/03/08 16:22:24]

If it were me, I'd probably just opt for using [Native] enum ManifestLocation
and adding IPC_ENUM_TRAITS_MIN and IPC_ENUM_TRAITS_MAX for the location, so that
this is a lot less verbose (and probably a bit less error prone).  But I suppose
it's largely an IPC decision, so I'll defer to dcheng.

[Noel Gordon, 2017/03/08 17:56:09]

/me would have just stuck with the ints :)  They seemed fine to me, and they
have been working for you for more than 5.5 years, without problem, when I
looked back through git blame.

For a [Native] enum ManifestLocation, we'd need to define IPC_ENUM_TRAITS
somewhere (they don't already exist), somewhere outside of the -message.h file
(since my goal in a follow-up patch is to delete the -message.h file).
Agree about full mojo enum traits being verbose (I had to type it all out, I'd
not done one before so it was good practice for me, but yeah, verbose).  sammc@
has been thinking about how we (mojo) can make them better.
Hmmmm.  While I was writing them, I was thinking about you and your team, and
how y'all are going to manage them in future, do they add maintenance burden,
how easy are they change and update, and so on.  I'm don't feel it's largely an
IPC decision when I consider those other factors as well.

It's good we explore the issues: using ints is KISS to me, others have other
view-points.

[Devlin, 2017/03/08 18:05:17]

My vote would be to add IPC_ENUM_TRAITS and use native.  It's a lot simpler and
gives us the same benefit, and reduces the risk of introduces a mismatch between
the enum conversion and the enum values (and has no maintenance burden, since
invalid and num locations are always the first/last values).  Additionally, the
main benefit that having full mojo type traits is having language agnosticism,
but these enums are only ever used in C++.  Since sammc@ and rockot@ are
investigating how to make these cleaner in the future, I'd say we do the
IPC_ENUM_TRAITS + [Native] for now, and update to these once they figure it out.
:)

Re just sticking with ints, it's true that it has worked in the past, but it's
not the right thing to do - enums should be passed as enums, not as ints.  Also,
as you mentioned, the messages were originally added 5.5 years ago - but IPC
enum validation is only 3.75 years old. :P [1]  It's good practice to make sure
this is safe.

[1] https://chromiumcodereview.appspot.com/15841011

[Noel Gordon, 2017/03/09 15:40:11]

Sounds like a plan to me, and done.
Thanks for the link, enum validation is more recent thing.  I made the enums for
our case validating with the limits you suggested, and also brought back the two
CHECKs on the utility process side.

+  INVALID_LOCATION,
+  INTERNAL,           // A crx file from the internal Extensions directory.

[Devlin, 2017/03/08 16:22:24]

If we do stick with this approach, we shouldn't duplicate each comment. 
Instead, just say "See Manifest::Location for what these enums mean."  That way,
if/when we change any, we don't have to update comments in multiple locations.

[Noel Gordon, 2017/03/08 17:56:09]

Agree if we go this way.

Not sure we should go this way: I wrote the mojo enum and traits out so we could
see what they are like.  I'm looking for the simplest thing that could possibly
work.  I'm happy to do what works best for you.

+  EXTERNAL_PREF,      // A crx file from an external directory (via prefs).
+  EXTERNAL_REGISTRY,  // A crx file from an external directory (via eg the
+                      // registry on Windows).
+  UNPACKED,           // From loading an unpacked extension from the
+                      // extensions settings page.
+  COMPONENT,          // An integral component of Chrome itself, which
+                      // happens to be implemented as an extension. We don't
+                      // show these in the management UI.
+  EXTERNAL_PREF_DOWNLOAD,    // A crx file from an external directory (via
+                             // prefs), installed from an update URL.
+  EXTERNAL_POLICY_DOWNLOAD,  // A crx file from an external directory (via
+                             // admin policies), installed from an update URL.
+  COMMAND_LINE,              // --load-extension.
+  EXTERNAL_POLICY,     // A crx file from an external directory (via admin
+                       // policies), cached locally and installed from the
+                       // cache.
+  EXTERNAL_COMPONENT,  // Similar to COMPONENT in that it's considered an
+                       // internal implementation detail of chrome, but
+                       // installed from an update URL like the *DOWNLOAD ones.
+};