Send Feedback Experiment

chrome/browser/ui/send_feedback_experiment.cc

+// Copyright (c) 2013 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.
+
+#include "chrome/browser/ui/send_feedback_experiment.h"
+
+#include "base/metrics/field_trial.h"
+#include "grit/generated_resources.h"
+
+namespace chrome {

[SteveT, 2013/03/19 14:38:58]

nit: I believe there should be an extra line break here.

[Harry McCleave, 2013/03/19 22:23:29]

Done.

+namespace send_feedback_experiment {
+
+// Constants for Send Feedback Link Location Experiment
+const char kSendFeedbackLinkExperimentName[] =
+    "SendFeedbackLinkLocation";
+const char kSendFeedbackLinkExperimentAltText[] = "alt-text";
+const char kSendFeedbackLinkExperimentAltLocation[] = "alt-location";
+const char kSendFeedbackLinkExperimentAltTextAndLocation[] =
+    "alt-text-and-location";
+
+bool UseAlternateText() {
+  return base::FieldTrialList::TrialExists(kSendFeedbackLinkExperimentName) &&
+      (base::FieldTrialList::FindFullName(kSendFeedbackLinkExperimentName) ==
+          kSendFeedbackLinkExperimentAltText ||
+      base::FieldTrialList::FindFullName(kSendFeedbackLinkExperimentName) ==
+          kSendFeedbackLinkExperimentAltTextAndLocation);

[SteveT, 2013/03/19 14:38:58]

You don't need to call TrialExists. FindFullName will just return an empty
string if the trial does not exist.

I recommend you call
base::FieldTrialList::FindFullName(kSendFeedbackLinkExperimentName) once and
store the result in a "const std::string kSelectedGroupName;" variable, then
compare it to consts twice.

(The reasoning is that it's a tiny bit easier to read, and FindFullName hits a
lock, which we should do less whenever possible)

[Harry McCleave, 2013/03/19 22:23:29]

Done.

+}
+
+bool UseAlternateLocation() {
+  return base::FieldTrialList::TrialExists(kSendFeedbackLinkExperimentName) &&
+      (base::FieldTrialList::FindFullName(kSendFeedbackLinkExperimentName) ==
+          kSendFeedbackLinkExperimentAltLocation ||
+      base::FieldTrialList::FindFullName(kSendFeedbackLinkExperimentName) ==
+          kSendFeedbackLinkExperimentAltTextAndLocation);

[SteveT, 2013/03/19 14:38:58]

Some requests as above :)

[Harry McCleave, 2013/03/19 22:23:29]

Done.

+}
+
+int GetMenuLabelID() {
+  return UseAlternateText() ? IDS_FEEDBACK_ALT : IDS_FEEDBACK;

[SteveT, 2013/03/19 14:38:58]

Question: Is UseAlternateText called anywhere but here?

[Harry McCleave, 2013/03/19 22:23:29]

Yes, GetMenuLabelID is used for menu items, the AltText is used in determining
the help screen button text (which uses different ids (no menu shortcut &)

+}
+
+}  // send_feedback_experiment

[SteveT, 2013/03/19 14:38:58]

nit: I believe there should be an extra line break here.

nit: "// namespace send_feedback_experiment"

[Harry McCleave, 2013/03/19 22:23:29]

Done.

+}  // chrome

[SteveT, 2013/03/19 14:38:58]

nit: "// namespace chrome"

[Harry McCleave, 2013/03/19 22:23:29]

Done.