Store feedback for spellcheck results from spelling service

Store feedback for spellcheck results from spelling service

This CL begins to collect spellcheck feedback, but the only feedback actions
are PENDING and NO_ACTION. Feedback sender uploads these actions every 30
minutes. The feedback is cleared from memory when user removes misspellings
from text or every 6 hours. Follow up CLs will add the rest of the actions.

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

[please use gerrit instead, 2013-05-21 21:34:21]

Groby: PTAL.

This is mostly boilerplate code to pipe spellcheck results to feedback sender.
The feedback generates only PENDING and NO_ACTION messages at the moment.
Upcoming CLs will add the rest of the feedback actions.

Mac support comes in an upcoming CL.

[please use gerrit instead, 2013-05-21 22:36:21]

Chris: PTAL chrome/common/spellcheck_messages.h

[palmer, 2013-05-21 23:06:28]

IPC security LGTM

https://codereview.chromium.org/15318004/diff/17001/chrome/browser/spellcheck...
File chrome/browser/spellchecker/feedback.cc (right):

https://codereview.chromium.org/15318004/diff/17001/chrome/browser/spellcheck...
chrome/browser/spellchecker/feedback.cc:14: typedef std::set<uint32>
HashCollection;
Would it make sense to use these typedefs in feedback.h, too?

[please use gerrit instead, 2013-05-21 23:09:08]

https://codereview.chromium.org/15318004/diff/17001/chrome/browser/spellcheck...
File chrome/browser/spellchecker/feedback.cc (right):

https://codereview.chromium.org/15318004/diff/17001/chrome/browser/spellcheck...
chrome/browser/spellchecker/feedback.cc:14: typedef std::set<uint32>
HashCollection;
On 2013/05/21 23:06:28, Chromium Palmer wrote:
> Would it make sense to use these typedefs in feedback.h, too?

Sure, why not.

[groby-ooo-7-16, 2013-05-23 19:10:50]

LGTM w/ nits

https://codereview.chromium.org/15318004/diff/33001/chrome/browser/spellcheck...
File chrome/browser/spellchecker/feedback.cc (right):

https://codereview.chromium.org/15318004/diff/33001/chrome/browser/spellcheck...
chrome/browser/spellchecker/feedback.cc:31: std::set<uint32>
remaining_set(remaining_markers.begin(),
Isn't that a HashCollection now?

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

https://codereview.chromium.org/15318004/diff/33001/chrome/browser/spellcheck...
chrome/browser/spellchecker/feedback_sender_unittest.cc:437:
TEST_F(FeedbackSenderTest, MatchDupliateResultsWithExistingMarkers) {
Why a new test - was there new functionality?

https://codereview.chromium.org/15318004/diff/33001/chrome/browser/spellcheck...
File chrome/browser/spellchecker/feedback_unittest.cc (right):

https://codereview.chromium.org/15318004/diff/33001/chrome/browser/spellcheck...
chrome/browser/spellchecker/feedback_unittest.cc:63: TEST_F(FeedbackTest,
DuplicateMisspellingFinalization) {
Why a new test?

https://codereview.chromium.org/15318004/diff/33001/chrome/common/spellcheck_...
File chrome/common/spellcheck_marker.h (right):

https://codereview.chromium.org/15318004/diff/33001/chrome/common/spellcheck_...
chrome/common/spellcheck_marker.h:10: SpellCheckMarker() : hash(0), offset(0) {}
Please set offset to MAX_UINT - I'd like it to be very clear that this is a
default-constructed object.

(Might want to double check with security folks if that introduces problems. I
wouldn't think so, but better safe than sorry :)

[please use gerrit instead, 2013-05-23 19:28:12]

On 2013/05/23 19:10:50, groby wrote:
>
https://codereview.chromium.org/15318004/diff/33001/chrome/common/spellcheck_...
> chrome/common/spellcheck_marker.h:10: SpellCheckMarker() : hash(0), offset(0)
{}
> Please set offset to MAX_UINT - I'd like it to be very clear that this is a
> default-constructed object.
> 
> (Might want to double check with security folks if that introduces problems. I
> wouldn't think so, but better safe than sorry :)

Chris: Is it OK to initialize a default value to MAX_UINT for an integer that's
going over IPC?

[please use gerrit instead, 2013-05-23 19:50:47]

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

https://codereview.chromium.org/15318004/diff/33001/chrome/browser/spellcheck...
chrome/browser/spellchecker/feedback_sender_unittest.cc:437:
TEST_F(FeedbackSenderTest, MatchDupliateResultsWithExistingMarkers) {
On 2013/05/23 19:10:50, groby wrote:
> Why a new test - was there new functionality?

No new functionality. I was increasing test coverage.

https://codereview.chromium.org/15318004/diff/33001/chrome/browser/spellcheck...
File chrome/browser/spellchecker/feedback_unittest.cc (right):

https://codereview.chromium.org/15318004/diff/33001/chrome/browser/spellcheck...
chrome/browser/spellchecker/feedback_unittest.cc:63: TEST_F(FeedbackTest,
DuplicateMisspellingFinalization) {
On 2013/05/23 19:10:50, groby wrote:
> Why a new test?

This test verifies that I fixed a bug. If FeedbackSender added the same
misspelling to Feedback twice, then the misspelling would get finalized
prematurely.

https://codereview.chromium.org/15318004/diff/33001/chrome/common/spellcheck_...
File chrome/common/spellcheck_marker.h (right):

https://codereview.chromium.org/15318004/diff/33001/chrome/common/spellcheck_...
chrome/common/spellcheck_marker.h:10: SpellCheckMarker() : hash(0), offset(0) {}
On 2013/05/23 19:10:50, groby wrote:
> Please set offset to MAX_UINT - I'd like it to be very clear that this is a
> default-constructed object.
> 
> (Might want to double check with security folks if that introduces problems. I
> wouldn't think so, but better safe than sorry :)

Changed to MAX_UINT and asked Chris.

[please use gerrit instead, 2013-05-23 19:54:54]

https://codereview.chromium.org/15318004/diff/33001/chrome/browser/spellcheck...
File chrome/browser/spellchecker/feedback.cc (right):

https://codereview.chromium.org/15318004/diff/33001/chrome/browser/spellcheck...
chrome/browser/spellchecker/feedback.cc:31: std::set<uint32>
remaining_set(remaining_markers.begin(),
On 2013/05/23 19:10:50, groby wrote:
> Isn't that a HashCollection now?

Yep, it is. Fixed.

[please use gerrit instead, 2013-05-23 20:02:43]

Chris: PTAL again. I've changed the default constructor for SpellCheckMarker to
initialize both unsigned types to -1 (Chromium does not have a MAX_UINT constant
defined). Is this safe for IPC?

[palmer, 2013-05-23 22:58:07]

Still LGTM.

[commit-bot: I haz the power, 2013-05-23 23:01:52]

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

[commit-bot: I haz the power, 2013-05-24 06:44:21]

Change committed as 201989