[spell] Remove spelling service feedback from behind the flag

[spell] Remove spelling service feedback from behind the flag.

After this patch, all English-speaking users that have 'Ask Google for
Suggestions' enabled will send feedback with API version "v2". The users
in the "SpellingServiceFeedback.Enabled" field trial that append the
"--enable-spelling-feedback-field-trial" command-line flag will send
feedback with API version "v2-internal".

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

[please use gerrit instead, 2013-10-09 19:35:35]

Groby: PTAL.

[groby-ooo-7-16, 2013-10-11 21:36:45]

We're waiting for launch feedback, no?

[please use gerrit instead, 2013-10-11 21:38:50]

On 2013/10/11 21:36:45, groby wrote:
> We're waiting for launch feedback, no?

Correct. Tyler says that he'll start the Launch process and CC me. I think he's
busy with his other projects ATM.

[please use gerrit instead, 2013-10-25 22:49:08]

Note to self: Keep the flag. "v2-internal" API version only for people who
flipped the flag and are participating in the experiment.

[please use gerrit instead, 2013-11-21 02:35:45]

Groby: PTAL.

[groby-ooo-7-16, 2013-11-21 02:48:41]

https://codereview.chromium.org/26558006/diff/36001/chrome/browser/spellcheck...
File chrome/browser/spellchecker/feedback_sender.cc (right):

https://codereview.chromium.org/26558006/diff/36001/chrome/browser/spellcheck...
chrome/browser/spellchecker/feedback_sender.cc:149:
switches::kEnableSpellingServiceFeedback)) {
This confuses me, since we have a "FeedbackEnabled" flag, but we always do
feedback. Is that really intentional?

https://codereview.chromium.org/26558006/diff/36001/chrome/browser/spellcheck...
chrome/browser/spellchecker/feedback_sender.cc:322:
switches::kSpellingServiceFeedbackIntervalSeconds),
You might want to clamp to kMaxIntervalSeconds, too?

https://codereview.chromium.org/26558006/diff/36001/chrome/browser/spellcheck...
chrome/browser/spellchecker/feedback_sender.cc:327: timer_.Start(FROM_HERE,
I suppose timer automatically cancels pending tasks on destruction, right? (Just
double-checking)

https://codereview.chromium.org/26558006/diff/36001/chrome/browser/spellcheck...
File chrome/browser/spellchecker/feedback_sender.h (right):

https://codereview.chromium.org/26558006/diff/36001/chrome/browser/spellcheck...
chrome/browser/spellchecker/feedback_sender.h:107: void
StopFeedbackCollection();
Please put Start before Stop - makes sense both alphabetically and logically :)

https://codereview.chromium.org/26558006/diff/36001/chrome/browser/spellcheck...
File chrome/browser/spellchecker/feedback_sender_unittest.cc (right):

https://codereview.chromium.org/26558006/diff/36001/chrome/browser/spellcheck...
chrome/browser/spellchecker/feedback_sender_unittest.cc:603:
TEST_F(FeedbackSenderTest, CanReumseFeedbackCollection) {
CanResume

https://codereview.chromium.org/26558006/diff/36001/chrome/browser/spellcheck...
File chrome/browser/spellchecker/spellcheck_service.cc (right):

https://codereview.chromium.org/26558006/diff/36001/chrome/browser/spellcheck...
chrome/browser/spellchecker/spellcheck_service.cc:325:
feedback_sender_->StartFeedbackCollection();
Hm - repeated starts restart the timer interval, from what I can see, which
might delay feedback sending. Is that intentional, or should Start/Stop be
idempotent?

[please use gerrit instead, 2013-11-22 22:06:30]

Groby: PTAL.

https://codereview.chromium.org/26558006/diff/36001/chrome/browser/spellcheck...
File chrome/browser/spellchecker/feedback_sender.cc (right):

https://codereview.chromium.org/26558006/diff/36001/chrome/browser/spellcheck...
chrome/browser/spellchecker/feedback_sender.cc:149:
switches::kEnableSpellingServiceFeedback)) {
On 2013/11/21 02:48:41, groby wrote:
> This confuses me, since we have a "FeedbackEnabled" flag, but we always do
> feedback. Is that really intentional?

The flag is to switch from "v2" to "v2-internal" API version for field-trial
participants. I've renamed it to something less confusing:
--enable-spelling-feedback-dogfood.

https://codereview.chromium.org/26558006/diff/36001/chrome/browser/spellcheck...
chrome/browser/spellchecker/feedback_sender.cc:322:
switches::kSpellingServiceFeedbackIntervalSeconds),
On 2013/11/21 02:48:41, groby wrote:
> You might want to clamp to kMaxIntervalSeconds, too?

Done.

https://codereview.chromium.org/26558006/diff/36001/chrome/browser/spellcheck...
chrome/browser/spellchecker/feedback_sender.cc:327: timer_.Start(FROM_HERE,
On 2013/11/21 02:48:41, groby wrote:
> I suppose timer automatically cancels pending tasks on destruction, right?
(Just
> double-checking)

Correct. Added a comment in feedback_sender.h.

https://codereview.chromium.org/26558006/diff/36001/chrome/browser/spellcheck...
File chrome/browser/spellchecker/feedback_sender_unittest.cc (right):

https://codereview.chromium.org/26558006/diff/36001/chrome/browser/spellcheck...
chrome/browser/spellchecker/feedback_sender_unittest.cc:603:
TEST_F(FeedbackSenderTest, CanReumseFeedbackCollection) {
On 2013/11/21 02:48:41, groby wrote:
> CanResume

Done.

[please use gerrit instead, 2013-11-22 22:14:22]

Small correction.

https://codereview.chromium.org/26558006/diff/36001/chrome/browser/spellcheck...
File chrome/browser/spellchecker/feedback_sender.cc (right):

https://codereview.chromium.org/26558006/diff/36001/chrome/browser/spellcheck...
chrome/browser/spellchecker/feedback_sender.cc:149:
switches::kEnableSpellingServiceFeedback)) {
On 2013/11/22 22:06:31, Rouslan Solomakhin wrote:
> On 2013/11/21 02:48:41, groby wrote:
> > This confuses me, since we have a "FeedbackEnabled" flag, but we always do
> > feedback. Is that really intentional?
> 
> The flag is to switch from "v2" to "v2-internal" API version for field-trial
> participants. I've renamed it to something less confusing:
> --enable-spelling-feedback-dogfood.

--enable-spelling-feedback-field-trial.

[groby-ooo-7-16, 2013-11-26 19:02:13]

lgtm

[commit-bot: I haz the power, 2013-11-26 19:04:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rouslan@chromium.org/26558006/386001

[commit-bot: I haz the power, 2013-11-26 19:55:13]

Retried try job too often on mac_rel for step(s) telemetry_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-11-26 21:34:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rouslan@chromium.org/26558006/386001

[commit-bot: I haz the power, 2013-11-26 23:44:57]

Change committed as 237437