Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(47)

Issue 11414282: Improve reliability of custom spelling dictionary file. (Closed)

Created:
6 years, 7 months ago by please use gerrit instead
Modified:
6 years, 7 months ago
Reviewers:
groby-ooo-7-16
CC:
chromium-reviews, groby+spellwatch_chromium.org, rpetterson
Base URL:
http://git.chromium.org/chromium/src.git@master
Visibility:
Public.

Description

Improve reliability of custom spelling dictionary file. This improves reliability of custom spelling dictionary file through checksums, backups, and atomic file writes. When a user alters their custom dictionary, we back it up, calculate the checksum of the new contents, and then write out the dictionary file with its checksum atomically. When the user restarts the browser and starts using spellchecker again, we load the custom dictionary and check its checksum. If the dictionary checksum does not match, but the backup checksum matches for the backup dictionary, we restore the backup and use it instead. BUG=161905 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=172494

Patch Set 1 : #

Total comments: 8

Patch Set 2 : #

Total comments: 30

Patch Set 3 : #

Total comments: 16

Patch Set 4 : #

Patch Set 5 : #

Patch Set 6 : #

Total comments: 6

Patch Set 7 : Address comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+248 lines, -73 lines) Patch
M chrome/browser/spellchecker/spellcheck_custom_dictionary.h View 1 2 3 4 5 6 chunks +30 lines, -8 lines 0 comments Download
M chrome/browser/spellchecker/spellcheck_custom_dictionary.cc View 1 2 3 4 5 6 11 chunks +114 lines, -65 lines 0 comments Download
M chrome/browser/spellchecker/spellcheck_custom_dictionary_unittest.cc View 1 2 3 4 5 6 2 chunks +104 lines, -0 lines 0 comments Download

Messages

Total messages: 15 (0 generated)
please use gerrit instead
Rachel: PTAL.
6 years, 7 months ago (2012-12-01 02:39:56 UTC) #1
groby-ooo-7-16
Preliminary review. Still have to review second half of cc file https://codereview.chromium.org/11414282/diff/2001/chrome/browser/spellchecker/spellcheck_custom_dictionary.cc File chrome/browser/spellchecker/spellcheck_custom_dictionary.cc (left): ...
6 years, 7 months ago (2012-12-03 17:12:44 UTC) #2
please use gerrit instead
Rachel: PTAL. https://codereview.chromium.org/11414282/diff/2001/chrome/browser/spellchecker/spellcheck_custom_dictionary.cc File chrome/browser/spellchecker/spellcheck_custom_dictionary.cc (left): https://codereview.chromium.org/11414282/diff/2001/chrome/browser/spellchecker/spellcheck_custom_dictionary.cc#oldcode24 chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:24: DCHECK(profile); On 2012/12/03 17:12:44, groby wrote: > ...
6 years, 7 months ago (2012-12-03 23:16:51 UTC) #3
groby-ooo-7-16
Sorry for the boatload of comments... https://codereview.chromium.org/11414282/diff/13004/chrome/browser/spellchecker/spellcheck_custom_dictionary.cc File chrome/browser/spellchecker/spellcheck_custom_dictionary.cc (right): https://codereview.chromium.org/11414282/diff/13004/chrome/browser/spellchecker/spellcheck_custom_dictionary.cc#newcode27 chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:27: checksum_prefix_("checksum = ") ...
6 years, 7 months ago (2012-12-04 00:16:30 UTC) #4
please use gerrit instead
Rachel: I see your boatload of comments and raise you my boatload of replies. BTW, ...
6 years, 7 months ago (2012-12-04 05:46:40 UTC) #5
please use gerrit instead
Rachel: should we add reliability UMAs? I was thinking about an enum of these events ...
6 years, 7 months ago (2012-12-04 16:12:17 UTC) #6
please use gerrit instead
Rachel: Atomic file write also returns a boolean that we can use to write SAVE_CUSTOM_DICTIONARY_SUCCESS, ...
6 years, 7 months ago (2012-12-04 16:13:31 UTC) #7
groby-ooo-7-16
Just on the topic of UMA: Ideally, metrics should allow us to take action based ...
6 years, 7 months ago (2012-12-04 16:46:53 UTC) #8
please use gerrit instead
On 2012/12/04 16:46:53, groby wrote: > Just on the topic of UMA: Ideally, metrics should ...
6 years, 7 months ago (2012-12-04 18:11:01 UTC) #9
groby-ooo-7-16
https://codereview.chromium.org/11414282/diff/15003/chrome/browser/spellchecker/spellcheck_custom_dictionary.cc File chrome/browser/spellchecker/spellcheck_custom_dictionary.cc (right): https://codereview.chromium.org/11414282/diff/15003/chrome/browser/spellchecker/spellcheck_custom_dictionary.cc#newcode65 chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:65: if (custom_words->at(0).length() == 0) at(0).empty() https://codereview.chromium.org/11414282/diff/15003/chrome/browser/spellchecker/spellcheck_custom_dictionary.cc#newcode215 chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:215: if (!custom_words->empty() ...
6 years, 7 months ago (2012-12-05 00:56:18 UTC) #10
please use gerrit instead
Rachel: PTAL. https://codereview.chromium.org/11414282/diff/15003/chrome/browser/spellchecker/spellcheck_custom_dictionary.cc File chrome/browser/spellchecker/spellcheck_custom_dictionary.cc (right): https://codereview.chromium.org/11414282/diff/15003/chrome/browser/spellchecker/spellcheck_custom_dictionary.cc#newcode65 chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:65: if (custom_words->at(0).length() == 0) On 2012/12/05 00:56:19, ...
6 years, 7 months ago (2012-12-05 16:51:26 UTC) #11
groby-ooo-7-16
LGTM w/ nits https://codereview.chromium.org/11414282/diff/23002/chrome/browser/spellchecker/spellcheck_custom_dictionary.cc File chrome/browser/spellchecker/spellcheck_custom_dictionary.cc (right): https://codereview.chromium.org/11414282/diff/23002/chrome/browser/spellchecker/spellcheck_custom_dictionary.cc#newcode119 chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:119: NOTREACHED(); nit: Why is this NOTREACHED? ...
6 years, 7 months ago (2012-12-11 19:25:48 UTC) #12
please use gerrit instead
Addressed the comments. Checking in. https://codereview.chromium.org/11414282/diff/23002/chrome/browser/spellchecker/spellcheck_custom_dictionary.cc File chrome/browser/spellchecker/spellcheck_custom_dictionary.cc (right): https://codereview.chromium.org/11414282/diff/23002/chrome/browser/spellchecker/spellcheck_custom_dictionary.cc#newcode119 chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:119: NOTREACHED(); On 2012/12/11 19:25:48, ...
6 years, 7 months ago (2012-12-11 22:32:58 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rouslan@chromium.org/11414282/33001
6 years, 7 months ago (2012-12-11 22:35:20 UTC) #14
commit-bot: I haz the power
6 years, 7 months ago (2012-12-12 01:53:06 UTC) #15
Message was sent while issue was closed.
Change committed as 172494

Powered by Google App Engine
This is Rietveld 408576698