Send Feedback Experiment

Send Feedback Experiment


Feedback Experiment to test response rate (usage of report an issue) when alternate text (Send Feedback) and or an alternate location (on windows/linux/mac the main Wrench Menu, on ChromeOS in the more tools option) is used. In the case of alternate text experiments the label for the report an issue button on the chrome://help page is also changed.


BUG=169339
TEST=Manual
Default behavior should not change, to confirm desired behavior run chrome with --force-fieldtrials=SendFeedbackLinkLocation/$GROUP_NAME/
Where GROUP_NAME is one of {alt-text,alt-location,alt-text-and-location} (note trailing '/' is required!)

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190049

[Harry McCleave, 2013-03-15 21:59:35]

Ready for review.

[sky, 2013-03-15 23:23:42]

You should get one of the guys that understands the field trial stuff to review
this.

https://codereview.chromium.org/12490012/diff/10001/chrome/browser/ui/send_fe...
File chrome/browser/ui/send_feedback_experiment.cc (right):

https://codereview.chromium.org/12490012/diff/10001/chrome/browser/ui/send_fe...
chrome/browser/ui/send_feedback_experiment.cc:7: #include "base/command_line.h"
Doesn't seem like you need this include.

https://codereview.chromium.org/12490012/diff/10001/chrome/browser/ui/send_fe...
chrome/browser/ui/send_feedback_experiment.cc:13: const char
kSendFeedbackLinkExperimentName[] =
nit: newline between 11/12.

https://codereview.chromium.org/12490012/diff/10001/chrome/browser/ui/send_fe...
File chrome/browser/ui/send_feedback_experiment.h (right):

https://codereview.chromium.org/12490012/diff/10001/chrome/browser/ui/send_fe...
chrome/browser/ui/send_feedback_experiment.h:14: bool UseAlternateText();
nit: newline between 13/14.

And add descriptions, these functions conveny nothing.

https://codereview.chromium.org/12490012/diff/10001/chrome/browser/ui/toolbar...
File chrome/browser/ui/toolbar/wrench_menu_model.cc (right):

https://codereview.chromium.org/12490012/diff/10001/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/wrench_menu_model.cc:210: IDS_FEEDBACK_ALT :
IDS_FEEDBACK);
indent 4 (same on 217)

https://codereview.chromium.org/12490012/diff/10001/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/wrench_menu_model.cc:647:
chrome::send_feedback_experiment::UseAlternateText() ?
You have this text four times, how about a GetAlternativeTextId() function or
something (in this file).

https://codereview.chromium.org/12490012/diff/10001/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/wrench_menu_model.cc:653: if
(browser_defaults::kShowExitMenuItem)
Doesn't this miss the logic on 625 old?

[Harry McCleave, 2013-03-19 03:11:31]

Added Steve (do you mind taking a look for the finch side of things).

https://codereview.chromium.org/12490012/diff/10001/chrome/browser/ui/send_fe...
File chrome/browser/ui/send_feedback_experiment.cc (right):

https://codereview.chromium.org/12490012/diff/10001/chrome/browser/ui/send_fe...
chrome/browser/ui/send_feedback_experiment.cc:7: #include "base/command_line.h"
On 2013/03/15 23:23:42, sky wrote:
> Doesn't seem like you need this include.

Done.

https://codereview.chromium.org/12490012/diff/10001/chrome/browser/ui/send_fe...
chrome/browser/ui/send_feedback_experiment.cc:13: const char
kSendFeedbackLinkExperimentName[] =
On 2013/03/15 23:23:42, sky wrote:
> nit: newline between 11/12.

Done.

https://codereview.chromium.org/12490012/diff/10001/chrome/browser/ui/send_fe...
File chrome/browser/ui/send_feedback_experiment.h (right):

https://codereview.chromium.org/12490012/diff/10001/chrome/browser/ui/send_fe...
chrome/browser/ui/send_feedback_experiment.h:14: bool UseAlternateText();
On 2013/03/15 23:23:42, sky wrote:
> nit: newline between 13/14.
> 
> And add descriptions, these functions conveny nothing.

Done.

https://codereview.chromium.org/12490012/diff/10001/chrome/browser/ui/toolbar...
File chrome/browser/ui/toolbar/wrench_menu_model.cc (right):

https://codereview.chromium.org/12490012/diff/10001/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/wrench_menu_model.cc:210: IDS_FEEDBACK_ALT :
IDS_FEEDBACK);
On 2013/03/15 23:23:42, sky wrote:
> indent 4 (same on 217)

Done.

https://codereview.chromium.org/12490012/diff/10001/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/wrench_menu_model.cc:647:
chrome::send_feedback_experiment::UseAlternateText() ?
On 2013/03/15 23:23:42, sky wrote:
> You have this text four times, how about a GetAlternativeTextId() function or
> something (in this file).

Done.

https://codereview.chromium.org/12490012/diff/10001/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/wrench_menu_model.cc:653: if
(browser_defaults::kShowExitMenuItem)
On 2013/03/15 23:23:42, sky wrote:
> Doesn't this miss the logic on 625 old?

Ah re-base my old nemesis (done).

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

Looking great! Can you add a TEST= line and a more verbose description
describing how your code works at a high level?

Speaking of testing... Do you know about --force-fieldtrial, by the way?

Some comments inline.

https://codereview.chromium.org/12490012/diff/19001/chrome/app/generated_reso...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/12490012/diff/19001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:1292: +        <message
name="IDS_FEEDBACK_ALT" desc="The alternate text label of the Report Bug menu
item, for finch experment">
Please avoid using the term "finch" in external code. You can just say "for the
feedback field trial." instead.

https://codereview.chromium.org/12490012/diff/19001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:1318: +        <message
name="IDS_FEEDBACK_ALT" desc="The alternate text label of the Report Bug menu
item, for finch experment">
ditto

https://codereview.chromium.org/12490012/diff/19001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:8429: +      <message
name="IDS_SEND_FEEDBACK" desc="Text of the button which allows the user to
report an issue with chrome, alternate text for finch experiment">
ditto

https://codereview.chromium.org/12490012/diff/19001/chrome/browser/ui/send_fe...
File chrome/browser/ui/send_feedback_experiment.cc (right):

https://codereview.chromium.org/12490012/diff/19001/chrome/browser/ui/send_fe...
chrome/browser/ui/send_feedback_experiment.cc:10: namespace chrome {
nit: I believe there should be an extra line break here.

https://codereview.chromium.org/12490012/diff/19001/chrome/browser/ui/send_fe...
chrome/browser/ui/send_feedback_experiment.cc:26:
kSendFeedbackLinkExperimentAltTextAndLocation);
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)

https://codereview.chromium.org/12490012/diff/19001/chrome/browser/ui/send_fe...
chrome/browser/ui/send_feedback_experiment.cc:34:
kSendFeedbackLinkExperimentAltTextAndLocation);
Some requests as above :)

https://codereview.chromium.org/12490012/diff/19001/chrome/browser/ui/send_fe...
chrome/browser/ui/send_feedback_experiment.cc:38: return UseAlternateText() ?
IDS_FEEDBACK_ALT : IDS_FEEDBACK;
Question: Is UseAlternateText called anywhere but here?

https://codereview.chromium.org/12490012/diff/19001/chrome/browser/ui/send_fe...
chrome/browser/ui/send_feedback_experiment.cc:41: }  // send_feedback_experiment
nit: I believe there should be an extra line break here.

nit: "// namespace send_feedback_experiment"

https://codereview.chromium.org/12490012/diff/19001/chrome/browser/ui/send_fe...
chrome/browser/ui/send_feedback_experiment.cc:42: }  // chrome
nit: "// namespace chrome"

https://codereview.chromium.org/12490012/diff/19001/chrome/browser/ui/send_fe...
File chrome/browser/ui/send_feedback_experiment.h (right):

https://codereview.chromium.org/12490012/diff/19001/chrome/browser/ui/send_fe...
chrome/browser/ui/send_feedback_experiment.h:11: // on the chrome://help page.
[crbug.com/169339.]
nit: Hmm, I think we can include more words on some of the lines above before
hitting the 80 char limit.

nit: "[crbug.com/169339.]" -> "(crbug.com/169339)"

https://codereview.chromium.org/12490012/diff/19001/chrome/browser/ui/send_fe...
chrome/browser/ui/send_feedback_experiment.h:12: namespace chrome {
Hmm, I *think* nested name spaces like this are allowed, but I think we might
want a space between these two lines.

https://codereview.chromium.org/12490012/diff/19001/chrome/browser/ui/send_fe...
chrome/browser/ui/send_feedback_experiment.h:27: }  // namespace
send_feedback_experiment
nit: I believe there should be a break between these two lines.

https://codereview.chromium.org/12490012/diff/19001/chrome/browser/ui/webui/h...
File chrome/browser/ui/webui/help/help_handler.cc (right):

https://codereview.chromium.org/12490012/diff/19001/chrome/browser/ui/webui/h...
chrome/browser/ui/webui/help/help_handler.cc:166: const std::string
sResourceReportIssue = "reportAnIssue";
nit: does the "s" stand for "string" in sResourceReportIssue? Not sure if this
fits our variable naming convention...

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Local_...

https://codereview.chromium.org/12490012/diff/19001/chrome/browser/ui/webui/h...
chrome/browser/ui/webui/help/help_handler.cc:167: // Finch experiment to
substitute "Report an Issue"
Please avoid using the term "Finch". You can just say "experiment" or "field
trial".

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

All 4 cases have been tested on my machine using --force-fieldtrials, and by
linking to a local variation server running the required .json files, I assume
we will need a start revision added to those files once this lands so as to
prevent data being poisoned by older version of chrome clients (local runs have
been both linux and chromeos on linux).

https://codereview.chromium.org/12490012/diff/19001/chrome/app/generated_reso...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/12490012/diff/19001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:1292: +        <message
name="IDS_FEEDBACK_ALT" desc="The alternate text label of the Report Bug menu
item, for finch experment">
On 2013/03/19 14:38:58, SteveT wrote:
> Please avoid using the term "finch" in external code. You can just say "for
the
> feedback field trial." instead.

Done.

https://codereview.chromium.org/12490012/diff/19001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:1318: +        <message
name="IDS_FEEDBACK_ALT" desc="The alternate text label of the Report Bug menu
item, for finch experment">
On 2013/03/19 14:38:58, SteveT wrote:
> ditto

Done.

https://codereview.chromium.org/12490012/diff/19001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:8429: +      <message
name="IDS_SEND_FEEDBACK" desc="Text of the button which allows the user to
report an issue with chrome, alternate text for finch experiment">
On 2013/03/19 14:38:58, SteveT wrote:
> ditto

Done.

https://codereview.chromium.org/12490012/diff/19001/chrome/browser/ui/send_fe...
File chrome/browser/ui/send_feedback_experiment.cc (right):

https://codereview.chromium.org/12490012/diff/19001/chrome/browser/ui/send_fe...
chrome/browser/ui/send_feedback_experiment.cc:10: namespace chrome {
On 2013/03/19 14:38:58, SteveT wrote:
> nit: I believe there should be an extra line break here.

Done.

https://codereview.chromium.org/12490012/diff/19001/chrome/browser/ui/send_fe...
chrome/browser/ui/send_feedback_experiment.cc:26:
kSendFeedbackLinkExperimentAltTextAndLocation);
On 2013/03/19 14:38:58, SteveT wrote:
> 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)

Done.

https://codereview.chromium.org/12490012/diff/19001/chrome/browser/ui/send_fe...
chrome/browser/ui/send_feedback_experiment.cc:34:
kSendFeedbackLinkExperimentAltTextAndLocation);
On 2013/03/19 14:38:58, SteveT wrote:
> Some requests as above :)

Done.

https://codereview.chromium.org/12490012/diff/19001/chrome/browser/ui/send_fe...
chrome/browser/ui/send_feedback_experiment.cc:38: return UseAlternateText() ?
IDS_FEEDBACK_ALT : IDS_FEEDBACK;
On 2013/03/19 14:38:58, SteveT wrote:
> Question: Is UseAlternateText called anywhere but here?
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 &)

https://codereview.chromium.org/12490012/diff/19001/chrome/browser/ui/send_fe...
chrome/browser/ui/send_feedback_experiment.cc:41: }  // send_feedback_experiment
On 2013/03/19 14:38:58, SteveT wrote:
> nit: I believe there should be an extra line break here.
> 
> nit: "// namespace send_feedback_experiment"

Done.

https://codereview.chromium.org/12490012/diff/19001/chrome/browser/ui/send_fe...
chrome/browser/ui/send_feedback_experiment.cc:42: }  // chrome
On 2013/03/19 14:38:58, SteveT wrote:
> nit: "// namespace chrome"

Done.

https://codereview.chromium.org/12490012/diff/19001/chrome/browser/ui/send_fe...
File chrome/browser/ui/send_feedback_experiment.h (right):

https://codereview.chromium.org/12490012/diff/19001/chrome/browser/ui/send_fe...
chrome/browser/ui/send_feedback_experiment.h:11: // on the chrome://help page.
[crbug.com/169339.]
On 2013/03/19 14:38:58, SteveT wrote:
> nit: Hmm, I think we can include more words on some of the lines above before
> hitting the 80 char limit.
> 
> nit: "[crbug.com/169339.]" -> "(crbug.com/169339)"

Done.

https://codereview.chromium.org/12490012/diff/19001/chrome/browser/ui/send_fe...
chrome/browser/ui/send_feedback_experiment.h:12: namespace chrome {
On 2013/03/19 14:38:58, SteveT wrote:
> Hmm, I *think* nested name spaces like this are allowed, but I think we might
> want a space between these two lines.

Done.

https://codereview.chromium.org/12490012/diff/19001/chrome/browser/ui/send_fe...
chrome/browser/ui/send_feedback_experiment.h:27: }  // namespace
send_feedback_experiment
On 2013/03/19 14:38:58, SteveT wrote:
> nit: I believe there should be a break between these two lines.

Done.

https://codereview.chromium.org/12490012/diff/19001/chrome/browser/ui/webui/h...
File chrome/browser/ui/webui/help/help_handler.cc (right):

https://codereview.chromium.org/12490012/diff/19001/chrome/browser/ui/webui/h...
chrome/browser/ui/webui/help/help_handler.cc:166: const std::string
sResourceReportIssue = "reportAnIssue";
On 2013/03/19 14:38:58, SteveT wrote:
> nit: does the "s" stand for "string" in sResourceReportIssue? Not sure if this
> fits our variable naming convention...
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Local_...

Done.

https://codereview.chromium.org/12490012/diff/19001/chrome/browser/ui/webui/h...
chrome/browser/ui/webui/help/help_handler.cc:167: // Finch experiment to
substitute "Report an Issue"
On 2013/03/19 14:38:58, SteveT wrote:
> Please avoid using the term "Finch". You can just say "experiment" or "field
> trial".

Done.

[SteveT, 2013-03-20 15:12:22]

On 2013/03/19 22:23:29, Harry McCleave wrote:
> All 4 cases have been tested on my machine using --force-fieldtrials, and by
> linking to a local variation server running the required .json files, I assume
> we will need a start revision added to those files once this lands so as to
> prevent data being poisoned by older version of chrome clients (local runs
have
> been both linux and chromeos on linux).

Awesome! Very thorough testing. Let's make sure we update the CL description
with an overview.

By start revision you mean min version, right? Because the server config can
control the latter but not the former. :)

Will finish the review presently...

> 
>
https://codereview.chromium.org/12490012/diff/19001/chrome/app/generated_reso...
> File chrome/app/generated_resources.grd (right):
> 
>
https://codereview.chromium.org/12490012/diff/19001/chrome/app/generated_reso...
> chrome/app/generated_resources.grd:1292: +        <message
> name="IDS_FEEDBACK_ALT" desc="The alternate text label of the Report Bug menu
> item, for finch experment">
> On 2013/03/19 14:38:58, SteveT wrote:
> > Please avoid using the term "finch" in external code. You can just say "for
> the
> > feedback field trial." instead.
> 
> Done.
> 
>
https://codereview.chromium.org/12490012/diff/19001/chrome/app/generated_reso...
> chrome/app/generated_resources.grd:1318: +        <message
> name="IDS_FEEDBACK_ALT" desc="The alternate text label of the Report Bug menu
> item, for finch experment">
> On 2013/03/19 14:38:58, SteveT wrote:
> > ditto
> 
> Done.
> 
>
https://codereview.chromium.org/12490012/diff/19001/chrome/app/generated_reso...
> chrome/app/generated_resources.grd:8429: +      <message
> name="IDS_SEND_FEEDBACK" desc="Text of the button which allows the user to
> report an issue with chrome, alternate text for finch experiment">
> On 2013/03/19 14:38:58, SteveT wrote:
> > ditto
> 
> Done.
> 
>
https://codereview.chromium.org/12490012/diff/19001/chrome/browser/ui/send_fe...
> File chrome/browser/ui/send_feedback_experiment.cc (right):
> 
>
https://codereview.chromium.org/12490012/diff/19001/chrome/browser/ui/send_fe...
> chrome/browser/ui/send_feedback_experiment.cc:10: namespace chrome {
> On 2013/03/19 14:38:58, SteveT wrote:
> > nit: I believe there should be an extra line break here.
> 
> Done.
> 
>
https://codereview.chromium.org/12490012/diff/19001/chrome/browser/ui/send_fe...
> chrome/browser/ui/send_feedback_experiment.cc:26:
> kSendFeedbackLinkExperimentAltTextAndLocation);
> On 2013/03/19 14:38:58, SteveT wrote:
> > 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)
> 
> Done.
> 
>
https://codereview.chromium.org/12490012/diff/19001/chrome/browser/ui/send_fe...
> chrome/browser/ui/send_feedback_experiment.cc:34:
> kSendFeedbackLinkExperimentAltTextAndLocation);
> On 2013/03/19 14:38:58, SteveT wrote:
> > Some requests as above :)
> 
> Done.
> 
>
https://codereview.chromium.org/12490012/diff/19001/chrome/browser/ui/send_fe...
> chrome/browser/ui/send_feedback_experiment.cc:38: return UseAlternateText() ?
> IDS_FEEDBACK_ALT : IDS_FEEDBACK;
> On 2013/03/19 14:38:58, SteveT wrote:
> > Question: Is UseAlternateText called anywhere but here?
> 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 &)
> 
>
https://codereview.chromium.org/12490012/diff/19001/chrome/browser/ui/send_fe...
> chrome/browser/ui/send_feedback_experiment.cc:41: }  //
send_feedback_experiment
> On 2013/03/19 14:38:58, SteveT wrote:
> > nit: I believe there should be an extra line break here.
> > 
> > nit: "// namespace send_feedback_experiment"
> 
> Done.
> 
>
https://codereview.chromium.org/12490012/diff/19001/chrome/browser/ui/send_fe...
> chrome/browser/ui/send_feedback_experiment.cc:42: }  // chrome
> On 2013/03/19 14:38:58, SteveT wrote:
> > nit: "// namespace chrome"
> 
> Done.
> 
>
https://codereview.chromium.org/12490012/diff/19001/chrome/browser/ui/send_fe...
> File chrome/browser/ui/send_feedback_experiment.h (right):
> 
>
https://codereview.chromium.org/12490012/diff/19001/chrome/browser/ui/send_fe...
> chrome/browser/ui/send_feedback_experiment.h:11: // on the chrome://help page.
> [crbug.com/169339.]
> On 2013/03/19 14:38:58, SteveT wrote:
> > nit: Hmm, I think we can include more words on some of the lines above
before
> > hitting the 80 char limit.
> > 
> > nit: "[crbug.com/169339.]" -> "(crbug.com/169339)"
> 
> Done.
> 
>
https://codereview.chromium.org/12490012/diff/19001/chrome/browser/ui/send_fe...
> chrome/browser/ui/send_feedback_experiment.h:12: namespace chrome {
> On 2013/03/19 14:38:58, SteveT wrote:
> > Hmm, I *think* nested name spaces like this are allowed, but I think we
might
> > want a space between these two lines.
> 
> Done.
> 
>
https://codereview.chromium.org/12490012/diff/19001/chrome/browser/ui/send_fe...
> chrome/browser/ui/send_feedback_experiment.h:27: }  // namespace
> send_feedback_experiment
> On 2013/03/19 14:38:58, SteveT wrote:
> > nit: I believe there should be a break between these two lines.
> 
> Done.
> 
>
https://codereview.chromium.org/12490012/diff/19001/chrome/browser/ui/webui/h...
> File chrome/browser/ui/webui/help/help_handler.cc (right):
> 
>
https://codereview.chromium.org/12490012/diff/19001/chrome/browser/ui/webui/h...
> chrome/browser/ui/webui/help/help_handler.cc:166: const std::string
> sResourceReportIssue = "reportAnIssue";
> On 2013/03/19 14:38:58, SteveT wrote:
> > nit: does the "s" stand for "string" in sResourceReportIssue? Not sure if
this
> > fits our variable naming convention...
> > 
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Local_...
> 
> Done.
> 
>
https://codereview.chromium.org/12490012/diff/19001/chrome/browser/ui/webui/h...
> chrome/browser/ui/webui/help/help_handler.cc:167: // Finch experiment to
> substitute "Report an Issue"
> On 2013/03/19 14:38:58, SteveT wrote:
> > Please avoid using the term "Finch". You can just say "experiment" or "field
> > trial".
> 
> Done.

[SteveT, 2013-03-20 15:19:44]

Question and a small nit for ya.

https://codereview.chromium.org/12490012/diff/25001/chrome/browser/ui/webui/h...
File chrome/browser/ui/webui/help/help_handler.cc (right):

https://codereview.chromium.org/12490012/diff/25001/chrome/browser/ui/webui/h...
chrome/browser/ui/webui/help/help_handler.cc:143: { "reportAnIssue",
IDS_REPORT_AN_ISSUE },
* (see below)

https://codereview.chromium.org/12490012/diff/25001/chrome/browser/ui/webui/h...
chrome/browser/ui/webui/help/help_handler.cc:166: const std::string
kResourceReportIssue = "reportAnIssue";
nit: Hmm.. just as a way to prevent future errors, can we pull this constant
into the anon-namespace and then use it here and above at * ? WDYT?

https://codereview.chromium.org/12490012/diff/25001/chrome/common/metrics/var...
File chrome/common/metrics/variations/variation_ids.h (right):

https://codereview.chromium.org/12490012/diff/25001/chrome/common/metrics/var...
chrome/common/metrics/variations/variation_ids.h:203:
SEND_FEEDBACK_LINK_LOCATION_DEFAULT = 3310249,
What's the deal with this being 3310249 when the one before it was 3310219? Are
we reserving a bunch of IDs for some reason?

[Harry McCleave, 2013-03-20 21:30:55]

https://chromiumcodereview.appspot.com/12490012/diff/25001/chrome/browser/ui/...
File chrome/browser/ui/webui/help/help_handler.cc (right):

https://chromiumcodereview.appspot.com/12490012/diff/25001/chrome/browser/ui/...
chrome/browser/ui/webui/help/help_handler.cc:166: const std::string
kResourceReportIssue = "reportAnIssue";
On 2013/03/20 15:19:44, SteveT wrote:
> nit: Hmm.. just as a way to prevent future errors, can we pull this constant
> into the anon-namespace and then use it here and above at * ? WDYT?

Done.

https://chromiumcodereview.appspot.com/12490012/diff/25001/chrome/common/metr...
File chrome/common/metrics/variations/variation_ids.h (right):

https://chromiumcodereview.appspot.com/12490012/diff/25001/chrome/common/metr...
chrome/common/metrics/variations/variation_ids.h:203:
SEND_FEEDBACK_LINK_LOCATION_DEFAULT = 3310249,
On 2013/03/20 15:19:44, SteveT wrote:
> What's the deal with this being 3310249 when the one before it was 3310219?
Are
> we reserving a bunch of IDs for some reason?

Belt & suspenders (just being sure), unless I have missed some other allocations
I figured there was no down side to reserving some extra ids in case we wanted
to add some test cases down the road.

[SteveT, 2013-03-21 05:04:39]

Okay, great! LGTM!

Very nicely done, and thanks again for tackling this :)

https://chromiumcodereview.appspot.com/12490012/diff/38001/chrome/browser/ui/...
File chrome/browser/ui/webui/help/help_handler.cc (right):

https://chromiumcodereview.appspot.com/12490012/diff/38001/chrome/browser/ui/...
chrome/browser/ui/webui/help/help_handler.cc:57: const std::string
kResourceReportIssue = "reportAnIssue";
nit: Line break here to separate variables from functions.

[SteveT, 2013-03-21 05:06:04]

Scott - mind finishing your review for UI-related stuff? And OWNERS, probably.

[sky, 2013-03-21 15:02:50]

https://chromiumcodereview.appspot.com/12490012/diff/38001/chrome/browser/ui/...
File chrome/browser/ui/send_feedback_experiment.h (right):

https://chromiumcodereview.appspot.com/12490012/diff/38001/chrome/browser/ui/...
chrome/browser/ui/send_feedback_experiment.h:13: namespace
send_feedback_experiment {
This namespace isn't necessary and is against latest style guidelines. Just use
chrome (search for "[chromium-dev] Namespace usage clarifications & rules" in
chromium-dev).

https://chromiumcodereview.appspot.com/12490012/diff/38001/chrome/browser/ui/...
chrome/browser/ui/send_feedback_experiment.h:19: // Returns True if the
alternative menu location for the Send Feedback/Report
nit: True -> true

https://chromiumcodereview.appspot.com/12490012/diff/38001/chrome/browser/ui/...
File chrome/browser/ui/webui/help/help_handler.cc (right):

https://chromiumcodereview.appspot.com/12490012/diff/38001/chrome/browser/ui/...
chrome/browser/ui/webui/help/help_handler.cc:57: const std::string
kResourceReportIssue = "reportAnIssue";
We don't use strings here as it results in a static initializer.

[Harry McCleave, 2013-03-21 20:42:09]

https://codereview.chromium.org/12490012/diff/38001/chrome/browser/ui/send_fe...
File chrome/browser/ui/send_feedback_experiment.h (right):

https://codereview.chromium.org/12490012/diff/38001/chrome/browser/ui/send_fe...
chrome/browser/ui/send_feedback_experiment.h:13: namespace
send_feedback_experiment {
On 2013/03/21 15:02:50, sky wrote:
> This namespace isn't necessary and is against latest style guidelines. Just
use
> chrome (search for "[chromium-dev] Namespace usage clarifications & rules" in
> chromium-dev).

Done.

https://codereview.chromium.org/12490012/diff/38001/chrome/browser/ui/send_fe...
chrome/browser/ui/send_feedback_experiment.h:19: // Returns True if the
alternative menu location for the Send Feedback/Report
On 2013/03/21 15:02:50, sky wrote:
> nit: True -> true

Done.

https://codereview.chromium.org/12490012/diff/38001/chrome/browser/ui/webui/h...
File chrome/browser/ui/webui/help/help_handler.cc (right):

https://codereview.chromium.org/12490012/diff/38001/chrome/browser/ui/webui/h...
chrome/browser/ui/webui/help/help_handler.cc:57: const std::string
kResourceReportIssue = "reportAnIssue";
On 2013/03/21 15:02:50, sky wrote:
> We don't use strings here as it results in a static initializer.

Done.

[sky, 2013-03-22 03:58:34]

LGTM

https://codereview.chromium.org/12490012/diff/46001/chrome/browser/ui/webui/h...
File chrome/browser/ui/webui/help/help_handler.cc (right):

https://codereview.chromium.org/12490012/diff/46001/chrome/browser/ui/webui/h...
chrome/browser/ui/webui/help/help_handler.cc:171: if
(std::string(kResourceReportIssue) == resources[i].name)
Could you remove the string construction outside the loop so it isn't done on
every iteration?

[commit-bot: I haz the power, 2013-03-22 04:24:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/harrym@chromium.org/12490012/29002

[commit-bot: I haz the power, 2013-03-22 18:16:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/harrym@chromium.org/12490012/29002

[commit-bot: I haz the power, 2013-03-23 14:54:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/harrym@chromium.org/12490012/29002

[commit-bot: I haz the power, 2013-03-23 17:49:09]

Change committed as 190049