ChromePrompt Mojo IPC interface

chrome_cleaner/mojom/chrome_prompt.mojom

Index: chrome_cleaner/mojom/chrome_prompt.mojom
diff --git a/chrome_cleaner/mojom/chrome_prompt.mojom b/chrome_cleaner/mojom/chrome_prompt.mojom
new file mode 100644
index 0000000000000000000000000000000000000000..56bb7def4ae9b30fef2f4c95867b6054a2b21111
--- /dev/null
+++ b/chrome_cleaner/mojom/chrome_prompt.mojom
@@ -0,0 +1,62 @@
+// 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 chrome_cleaner.mojom;
+
+[Extensible]
+enum ObservedBehaviour {
+  UNSPECIFIED = 0,
+  ADS_INJECTOR = 1,
+  SETTINGS_HIJACKER = 2,
+  BROWSER_HIJACKER = 3,
+};
+
+// Information about removable Unwanted Software matched by the Software
+// Reporter to be shown in the Chrome prompt.
+struct UwS {
+  // The id of this unwanted software.
+  int32 id;
+
+  // The name of this unwanted software.
+  string name;
+
+  // List of behaviors to be presented to the user.
+  array<ObservedBehaviour> observed_behaviours;

[grt (UTC plus 2), 2017/03/22 13:48:14]

is it easier for consumers of this api for this to be a bitfield rather than an
array? (not sure if this can be accomplished in mojom or not, so pardon if this
is a silly question.)

[ftirelo, 2017/03/22 14:25:36]

It's not silly at all.

In the compiled code, a Mojo enum are represented by an enum class, so
converting to and from the enum will require casts. However, I think it's easy
enough to wrap the conversions in auxiliary functions, so I changed this to int
instead.

Another alternative is to define ObservedBehaviour's enumerators as uint64
constants, with the expense of not having the enum's scoping protection. It may
not be a big deal though, since we are already in a separate namespace (mojom).

WDYT?

[joenotcharles, 2017/03/22 14:45:03]

I'd prefer to keep the mojom interface as straightforward as possible, which
would be "array<ObservedBehaviour>". (Actually it should be
set<ObservedBehaviour>" but mojom doesn't have a set type.) I think that will be
easier for the caller too - we're going to be using these values by iterating
through all "observed behaviours" and formatting them for display. It's more
straightforward to iterate through an array than a bitfield. The only issue that
a bitfield avoids is dealing with duplicates (and it's implicitly sorted, but
we'll want to sort the output explicitly anyway rather than relying on that.)

[ftirelo, 2017/03/22 17:37:37]

I'm taking up on Ken's suggestion of using a struct of bools since it's simple
and easy to extend.

+
+  // List of fully-qualified paths of the files that will be deleted by the
+  // Chrome Cleanup Tool for this unwanted software.
+  array<string> files_to_delete;
+};
+
+[Extensible]
+enum PromptAcceptance {
+  UNSPECIFIED = 0,
+  // The Chrome prompt was not shown to the user (for example, due to an
+  // experiment run that shouldn't prompt the user or because the user has
+  // been prompted recently).
+  NOT_SHOWN = 1,
+  // The user explicitly accepted the Chrome prompt.
+  ACCEPTED = 2,
+  // The user explicitly denied the Chrome prompt.
+  DENIED = 3,
+  // The user didn't interact with the Chrome prompt after a while.
+  IGNORED = 4,
+};
+
+// Service provided by Chrome to prompt the user to run the Chrome Cleanup Tool
+// if unwanted software is detected in the system. This service is used by the

[grt (UTC plus 2), 2017/03/22 13:48:14]

nit: "...on the system..." seems more natural to me than "in" it.

[ftirelo, 2017/03/22 14:25:36]

Prepositions... Thanks for the correction :-)

+// Software Reporter Tool so that all user interaction is provided by Chrome.
+interface ChromePrompt {
+  // Params:
+  //  - removable_uws_found: the list of UwS detected by the reporter;
+  //  - elevation_required: if the cleaner will need to run in elevated mode.
+  // Returns:
+  //  - prompt_acceptance: indicates if the user accepted the prompt;
+  //  - logs_uploading_enabled: if the user accepted the prompt, indicates if

[grt (UTC plus 2), 2017/03/22 13:48:14]

does it make sense for this to be removed and for ACCEPTED to be split into
ACCEPTED and ACCEPTED_WITH_LOGS? as it is, this value is only relevant for one
single acceptance code, correct? will this change down the road?

[ftirelo, 2017/03/22 14:25:36]

It doesn't make sense to have logs uploading with other option than ACCEPT, so I
think it makes sense to merge both.

Given that we want the permissions model to be as simple as possible to the
user, I think it's very unlikely that we change logs permissions to allow, for
example, different detail levels - only matched files, detailed system report.
If that happens, we would need to change this anyway, so I'd rather not
over-engineer now.

+  //                            they also opted into or opted out of uploading
+  //                            logs.
+  PromptUser(array<UwS> removable_uws_found, bool elevation_required)
+      => (PromptAcceptance prompt_acceptance,
+          bool logs_uploading_enabled);
+};