Add a triggered profile reset mechanism.

chrome/browser/profile_resetter/triggered_profile_resetter.h

+// Copyright 2015 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.
+
+#ifndef CHROME_BROWSER_PROFILE_RESETTER_TRIGGERED_PROFILE_RESETTER_H_
+#define CHROME_BROWSER_PROFILE_RESETTER_TRIGGERED_PROFILE_RESETTER_H_
+
+#include <string>

[grt (UTC plus 2), 2015/09/14 14:35:34]

unused

[robertshield, 2015/09/21 04:16:22]

Done.

+
+#include "base/basictypes.h"

[grt (UTC plus 2), 2015/09/14 14:35:34]

use macros.h instead of this for DISALLOW_COPY_AND_ASSIGN

[robertshield, 2015/09/21 04:16:22]

Done.

+#include "base/logging.h"
+#include "base/memory/ref_counted.h"

[grt (UTC plus 2), 2015/09/14 14:35:34]

unused

[robertshield, 2015/09/21 04:16:22]

Done.

+#include "base/memory/weak_ptr.h"

[grt (UTC plus 2), 2015/09/14 14:35:34]

unused

[robertshield, 2015/09/21 04:16:22]

Done.

+#include "base/strings/string16.h"
+#include "components/keyed_service/core/keyed_service.h"
+
+class Profile;
+
+// This service is responsible for evaluating whether a profile reset trigger
+// has been set and not yet consumed by |profile_|. If it has, the profile is
+// eligible for reset and a profile reset UI will be show to the user. The

[engedy, 2015/09/14 23:28:55]

nit: shown

[robertshield, 2015/09/21 04:16:22]

Done.

+// intended use case for this is to provide a sanctioned profile reset API for
+// third party tools (AVs or cleaner tools) that wish to reset users' profiles

[engedy, 2015/09/14 16:08:29]

nit: s/AVs/anti-virus/, if I understand correctly. I'd say it's a borderline
case in terms of how well-known an abbreviation it is.

[robertshield, 2015/09/21 04:16:22]

Done.

+// as part of their cleanup flow.
+//
+// To use this mechanism from a third party tool, perform the following steps:
+//   1) Create (or open) the registry key
+//      HCKU\Software\$PRODUCT_STRING_PATH\TriggeredReset where

[engedy, 2015/09/14 16:08:29]

nit: HKCU.

[robertshield, 2015/09/21 04:16:22]

Done.

+//      $PRODUCT_NAME is one of the values in chrome\common\chrome_constants.h,

[engedy, 2015/09/14 16:08:28]

I think this should be $PRODUCT_STRING_PATH.

[robertshield, 2015/09/21 04:16:22]

Done.

+//      currently either "Google\\Chrome" or "Chromium"

[engedy, 2015/09/14 23:28:55]

super nit: Period at the end.

[robertshield, 2015/09/21 04:16:22]

Done.

+//   2) Set a REG_SZ value called "ToolName" to the localized name of the tool.
+//      This string (truncated to kMaxToolNameLength) will be displayed in a
+//      notification UI.
+//   3) Set a REG_QWORD value with the timestamp of the reset. This value should

[engedy, 2015/09/14 16:08:29]

nit: Clarify what the "timestamp of the reset" means. Is this the time when the
reset is requested? 

[robertshield, 2015/09/21 04:16:22]

Yes, added clarifying text.

+//      be obtained from a call to ::GetSystemTimeAsFileTime(). This value
+//      will be persisted in a reset profile and will be used to avoid

[engedy, 2015/09/14 16:08:29]

nit: in the profile when it is reset

[robertshield, 2015/09/21 04:16:22]

Done.

+//      multiple resets.
+//
+// Some considerations:
+//
+// * Chrome supports multiple profiles. When the above steps are followed,
+//   each profile will enter the reset flow as it is opened.
+// * New profiles created while a timestamp is present will not get the reset

[engedy, 2015/09/14 16:08:29]

nit: While any timestamp is present, or after the current time is past the
timestamp?

[robertshield, 2015/09/21 04:16:22]

Any (updated wording).

+//   flow.
+class TriggeredProfileResetter : public KeyedService {
+ public:
+  enum : int { kMaxToolNameLength = 100 };

[engedy, 2015/09/14 23:28:56]

nit: Is there a particular reason to use the enum trick here? Would a simple
static const member do?

[grt (UTC plus 2), 2015/09/15 14:13:51]

I suggested this since storage isn't needed, and the recent chromium-dev thread
made it sound like "static const int kFoo = 100;" is an accident waiting to
happen and should be avoided either by narrowing the scope of the constant, or
by using an enum
(https://groups.google.com/a/chromium.org/d/msg/chromium-dev/hDqJ4KBVqog/jfNPH...).

[engedy, 2015/09/15 14:48:43]

Thanks for pointing me to this discussion! Is there a performance and/or memory
usage benefit of using the enum?

If so, then I am happy with using an enum, even though it's not used
conventionally in this directory. Still, please change the underlying type to
size_t.

(The alternative being declaring/defining it as a static const *with* storage.)

[robertshield, 2015/09/21 04:16:22]

Done.

[robertshield, 2015/09/21 04:16:22]

Acknowledged.

[robertshield, 2015/09/21 04:16:22]

I was told in a review comment further up that the static const member is no
longer the Way Things Are Done and that the enum trick is the appropriate way
henceforth.

+
+  explicit TriggeredProfileResetter(Profile* profile);
+  ~TriggeredProfileResetter() override;
+
+  // Causes the TriggeredProfileResetter to look for the presence of a trigger
+  // and perform a reset, subsequent calls to HasResetTrigger will return

[engedy, 2015/09/14 23:28:55]

Phrasing nit: s/and perform a reset, s/. If a trigger is found, it is disarmed
so that future instances of the service will no longer trigger a reset. S/.

[robertshield, 2015/09/21 04:16:22]

Done.

+  // whether |profile_| is subject to a reset.
+  virtual void Activate();
+
+  // Returns true iff the given profile should have a reset reset according to

[engedy, 2015/09/14 23:28:55]

nit: -reset

[robertshield, 2015/09/21 04:16:22]

Done, also rephrased a bit.

+  // the description in the class comment. Must call Activate() first.
+  virtual bool HasResetTrigger();
+
+  // Returns the name of the tool that performed the reset. This string will be
+  // truncated to a length of |kMaxToolNameLength|.
+  virtual base::string16 GetResetToolName();
+
+ private:
+  Profile* profile_;
+
+  bool has_reset_trigger_ = false;
+
+#if DCHECK_IS_ON()

[Alexei Svitkine (slow), 2015/09/14 17:05:43]

Nit: I don't think it's worth guarding having this member with DCHECK_IS_ON()
nor the other places that touch it. Just adds extra noise to the code for
basically no benefit (saving a slot for a boolean and setting that boolean -
both of which are basically free).

[engedy, 2015/09/14 23:28:55]

Agreed.

[robertshield, 2015/09/21 04:16:22]

Ack, removed.

[robertshield, 2015/09/21 04:16:22]

Acknowledged.

+  bool activate_called_ = false;
+#endif
+
+  base::string16 tool_name_;
+
+  DISALLOW_COPY_AND_ASSIGN(TriggeredProfileResetter);
+};
+
+#endif  // CHROME_BROWSER_PROFILE_RESETTER_TRIGGERED_PROFILE_RESETTER_H_