Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11414282:
Improve reliability of custom spelling dictionary file. (Closed)
|
Created:
8 years ago by please use gerrit instead Modified:
8 years 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. |
DescriptionImprove 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 #
Messages
Total messages: 15 (0 generated)
Rachel: PTAL.
Preliminary review. Still have to review second half of cc file https://codereview.chromium.org/11414282/diff/2001/chrome/browser/spellchecke... File chrome/browser/spellchecker/spellcheck_custom_dictionary.cc (left): https://codereview.chromium.org/11414282/diff/2001/chrome/browser/spellchecke... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:24: DCHECK(profile); Why'd you kill the DCHECK? https://codereview.chromium.org/11414282/diff/2001/chrome/browser/spellchecke... File chrome/browser/spellchecker/spellcheck_custom_dictionary.cc (right): https://codereview.chromium.org/11414282/diff/2001/chrome/browser/spellchecke... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:69: // Clean up the dictionary file contents. ... by removing duplicates and empty words. It's nice to hace that in the comment, since stl/algorithms don't necessarily qualify as super-readable :) https://codereview.chromium.org/11414282/diff/2001/chrome/browser/spellchecke... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:195: size_t pos = 0; Can we just write out the whole dictionary here? For dictionaries less than a disk block size (16K?) that's not going to be any more expensive, and it makes much cleaner code. https://codereview.chromium.org/11414282/diff/2001/chrome/browser/spellchecke... File chrome/browser/spellchecker/spellcheck_custom_dictionary.h (right): https://codereview.chromium.org/11414282/diff/2001/chrome/browser/spellchecke... chrome/browser/spellchecker/spellcheck_custom_dictionary.h:96: Can't we just derive all those filepaths computationalluy from custom_dictionary_path_? Do we really need to store them? Also, can we store the checksum _in_ the dictionary file instead?
Rachel: PTAL. https://codereview.chromium.org/11414282/diff/2001/chrome/browser/spellchecke... File chrome/browser/spellchecker/spellcheck_custom_dictionary.cc (left): https://codereview.chromium.org/11414282/diff/2001/chrome/browser/spellchecke... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:24: DCHECK(profile); On 2012/12/03 17:12:44, groby wrote: > Why'd you kill the DCHECK? My mistake. It's back. https://codereview.chromium.org/11414282/diff/2001/chrome/browser/spellchecke... File chrome/browser/spellchecker/spellcheck_custom_dictionary.cc (right): https://codereview.chromium.org/11414282/diff/2001/chrome/browser/spellchecke... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:69: // Clean up the dictionary file contents. On 2012/12/03 17:12:44, groby wrote: > ... by removing duplicates and empty words. It's nice to hace that in the > comment, since stl/algorithms don't necessarily qualify as super-readable :) Done. https://codereview.chromium.org/11414282/diff/2001/chrome/browser/spellchecke... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:195: size_t pos = 0; On 2012/12/03 17:12:44, groby wrote: > Can we just write out the whole dictionary here? For dictionaries less than a > disk block size (16K?) that's not going to be any more expensive, and it makes > much cleaner code. Done. https://codereview.chromium.org/11414282/diff/2001/chrome/browser/spellchecke... File chrome/browser/spellchecker/spellcheck_custom_dictionary.h (right): https://codereview.chromium.org/11414282/diff/2001/chrome/browser/spellchecke... chrome/browser/spellchecker/spellcheck_custom_dictionary.h:96: On 2012/12/03 17:12:44, groby wrote: > Can't we just derive all those filepaths computationalluy from > custom_dictionary_path_? Do we really need to store them? > > Also, can we store the checksum _in_ the dictionary file instead? Done.
Sorry for the boatload of comments... https://codereview.chromium.org/11414282/diff/13004/chrome/browser/spellcheck... File chrome/browser/spellchecker/spellcheck_custom_dictionary.cc (right): https://codereview.chromium.org/11414282/diff/13004/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:27: checksum_prefix_("checksum = ") { Why a prefix? Why not just make sure it's a valid MD5 and be done? https://codereview.chromium.org/11414282/diff/13004/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:60: custom_words->clear(); It's not your CL, but I suggest just calling custom_words.clear() in any case, without the if-path. Otherwise, and entirely corrupt dictionary will not be saved out again as an empty dictionary. (Unless that's intentional?) https://codereview.chromium.org/11414282/diff/13004/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:71: custom_words->erase(std::remove_if(custom_words->begin(), While this works, it just occurred to me that only the first word can be empty. We're sorted and unique :) (In case you want to simplify this code) https://codereview.chromium.org/11414282/diff/13004/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:92: FOR_EACH_OBSERVER(Observer, observers_, OnCustomDictionaryLoaded()); +1 for awesome macros! https://codereview.chromium.org/11414282/diff/13004/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:140: ss << '\n'; Doesn't "LoadReliably" guarantee proper format? Also, not happy with using a stringstream here - why not contents.append("\n"); ? https://codereview.chromium.org/11414282/diff/13004/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:193: for (WordList::iterator it = custom_words.begin(); This would make the code easier to read: it = custom_words.find(word); if (it != custom_words.end()) custom_words.erase(it); // build ss https://codereview.chromium.org/11414282/diff/13004/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:194: it != custom_words.end(); The above remark (std::erase) brings up another point - we _always_ call SplitString after LoadDictionaryContentsReliably. Can we just have it return the list of strings? And SaveDictReliably almost always takes a string constructed from a vector, too - so I think using a vector instead of a string API makes more sense there. https://codereview.chromium.org/11414282/diff/13004/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:237: if (checksum.empty()) Hm. This looks like it will fail for any dictionaries that have been created before we added the checksum mechanism. Can we add a test case for that? (It'd be great to have some for the other behaviors, too) https://codereview.chromium.org/11414282/diff/13004/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:238: return; If you go with the one-file-to-rule-them all approach, I'd suggest making the first or the last line the checksum. And only checksumming all the other lines, obviously :) https://codereview.chromium.org/11414282/diff/13004/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:250: std::string backup; Backup is just the last copy saved before this one, right? Why do we even care about it in the load path? An MD5 checksum is a pretty strong indicator that things are OK... Either way, I'm not sure creating a backup in the Load path is a good plan - each load operation creates a new copy. Quite a bit of cost for something that's basically a startup operation. If I may make a suggestion, I'd only load the backup copy if the primary copy failed a checksum check. (And create a backup copy right before you save out a new version, in SaveReliably) https://codereview.chromium.org/11414282/diff/13004/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:275: base::ImportantFileWriter::WriteFileAtomically(custom_dictionary_path_, I might be wrong, but doesn't this just write the _checksum_ into the custom dictionary path? https://codereview.chromium.org/11414282/diff/13004/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:280: std::string checksum; You can check if something is a checksum much easier if you go 16-digit hex value: 1) Check the length == 16 2) Call digest.find_first_not_of("0123456789abcdef"). If it's std::npos, it's not a checksum. The difficult question is, what do we do if it's not a checksum - it can either be an old file without checksum, or a new file with a broken checksum. On second thought, it's worth keeping the prefix, if we include non-word characters in there. This way, if a string doesn't match the prefix & doesn't have non-word chars, we can make a reasonable guess it's a word, and we encountered an old-style dictionary. And because I worry about migrations in the future, I'd suggest a version number in the prefix so in the future, we can cleanly migrate I.e. checksum_v1 = <MD5> seems reasonable. https://codereview.chromium.org/11414282/diff/13004/chrome/browser/spellcheck... File chrome/browser/spellchecker/spellcheck_custom_dictionary.h (right): https://codereview.chromium.org/11414282/diff/13004/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.h:82: void LoadDictionaryContentsReliably(std::string* contents); We should probably document somewhere that we expect our files to be UTF8, not some fancy windows codepage or anything. https://codereview.chromium.org/11414282/diff/13004/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.h:86: // file. I'd feel more comfortable having the checksum in the dictionary file, as the first or last line. At least that way, you can still save dictionary files for later use. https://codereview.chromium.org/11414282/diff/13004/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.h:87: void SaveDictionryContentsReliably(const std::string& contents); Spelling: Diction_a_ry (Hey, y u no spellcheck! ;) https://codereview.chromium.org/11414282/diff/13004/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.h:90: std::string GetChecksum(std::string* contents); const std::string& contents - it's not an out param.
Rachel: I see your boatload of comments and raise you my boatload of replies. BTW, where is a good place to document UTF8-ness of the dictionary file? Perhaps in class comment? Then I can document the file format there, too. Right? PTAL. https://codereview.chromium.org/11414282/diff/13004/chrome/browser/spellcheck... File chrome/browser/spellchecker/spellcheck_custom_dictionary.cc (right): https://codereview.chromium.org/11414282/diff/13004/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:60: custom_words->clear(); On 2012/12/04 00:16:30, groby wrote: > It's not your CL, but I suggest just calling custom_words.clear() in any case, > without the if-path. Otherwise, and entirely corrupt dictionary will not be > saved out again as an empty dictionary. (Unless that's intentional?) Done. https://codereview.chromium.org/11414282/diff/13004/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:71: custom_words->erase(std::remove_if(custom_words->begin(), On 2012/12/04 00:16:30, groby wrote: > While this works, it just occurred to me that only the first word can be empty. > We're sorted and unique :) (In case you want to simplify this code) Done. https://codereview.chromium.org/11414282/diff/13004/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:140: ss << '\n'; On 2012/12/04 00:16:30, groby wrote: > Doesn't "LoadReliably" guarantee proper format? > > Also, not happy with using a stringstream here - why not contents.append("\n"); > ? Done. https://codereview.chromium.org/11414282/diff/13004/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:193: for (WordList::iterator it = custom_words.begin(); On 2012/12/04 00:16:30, groby wrote: > This would make the code easier to read: > it = custom_words.find(word); > if (it != custom_words.end()) custom_words.erase(it); > // build ss > > Done. https://codereview.chromium.org/11414282/diff/13004/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:194: it != custom_words.end(); On 2012/12/04 00:16:30, groby wrote: > The above remark (std::erase) brings up another point - we _always_ call > SplitString after LoadDictionaryContentsReliably. Can we just have it return the > list of strings? > > And SaveDictReliably almost always takes a string constructed from a vector, too > - so I think using a vector instead of a string API makes more sense there. Done. https://codereview.chromium.org/11414282/diff/13004/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:237: if (checksum.empty()) On 2012/12/04 00:16:30, groby wrote: > Hm. This looks like it will fail for any dictionaries that have been created > before we added the checksum mechanism. Can we add a test case for that? (It'd > be great to have some for the other behaviors, too) Empty checksum most likely indicates that this is a legacy file, created before we adding the checksum mechanism. Added a test case. https://codereview.chromium.org/11414282/diff/13004/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:238: return; On 2012/12/04 00:16:30, groby wrote: > If you go with the one-file-to-rule-them all approach, I'd suggest making the > first or the last line the checksum. And only checksumming all the other lines, > obviously :) Done. https://codereview.chromium.org/11414282/diff/13004/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:250: std::string backup; On 2012/12/04 00:16:30, groby wrote: > Backup is just the last copy saved before this one, right? Why do we even care > about it in the load path? An MD5 checksum is a pretty strong indicator that > things are OK... > > Either way, I'm not sure creating a backup in the Load path is a good plan - > each load operation creates a new copy. Quite a bit of cost for something that's > basically a startup operation. > > If I may make a suggestion, I'd only load the backup copy if the primary copy > failed a checksum check. (And create a backup copy right before you save out a > new version, in SaveReliably) Done. https://codereview.chromium.org/11414282/diff/13004/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:275: base::ImportantFileWriter::WriteFileAtomically(custom_dictionary_path_, On 2012/12/04 00:16:30, groby wrote: > I might be wrong, but doesn't this just write the _checksum_ into the custom > dictionary path? It writes the prefix, the checksum, a newline, and the contents into the custom dictionary file, which is located at the custom dictionary path. https://codereview.chromium.org/11414282/diff/13004/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:280: std::string checksum; On 2012/12/04 00:16:30, groby wrote: > You can check if something is a checksum much easier if you go 16-digit hex > value: > > 1) Check the length == 16 > 2) Call digest.find_first_not_of("0123456789abcdef"). If it's std::npos, it's > not a checksum. > > The difficult question is, what do we do if it's not a checksum - it can either > be an old file without checksum, or a new file with a broken checksum. > > On second thought, it's worth keeping the prefix, if we include non-word > characters in there. This way, if a string doesn't match the prefix & doesn't > have non-word chars, we can make a reasonable guess it's a word, and we > encountered an old-style dictionary. > > And because I worry about migrations in the future, I'd suggest a version number > in the prefix so in the future, we can cleanly migrate > > I.e. checksum_v1 = <MD5> seems reasonable. Done. https://codereview.chromium.org/11414282/diff/13004/chrome/browser/spellcheck... File chrome/browser/spellchecker/spellcheck_custom_dictionary.h (right): https://codereview.chromium.org/11414282/diff/13004/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.h:82: void LoadDictionaryContentsReliably(std::string* contents); On 2012/12/04 00:16:30, groby wrote: > We should probably document somewhere that we expect our files to be UTF8, not > some fancy windows codepage or anything. Where is the best place to document this? Class comments? Method comments? Line comments inside of LoadDictionaryContentsReliably? https://codereview.chromium.org/11414282/diff/13004/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.h:86: // file. On 2012/12/04 00:16:30, groby wrote: > I'd feel more comfortable having the checksum in the dictionary file, as the > first or last line. At least that way, you can still save dictionary files for > later use. Done. https://codereview.chromium.org/11414282/diff/13004/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.h:87: void SaveDictionryContentsReliably(const std::string& contents); On 2012/12/04 00:16:30, groby wrote: > Spelling: Diction_a_ry (Hey, y u no spellcheck! ;) Done. https://codereview.chromium.org/11414282/diff/13004/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.h:90: std::string GetChecksum(std::string* contents); On 2012/12/04 00:16:30, groby wrote: > const std::string& contents - it's not an out param. Added a comment to clarify: "If this method returns a non-empty checksum, then |contents| have been truncated to exclude the checksum."
Rachel: should we add reliability UMAs? I was thinking about an enum of these events in one histogram: LOAD_CUSTOM_DICTIONARY_CHECKSUM_GOOD LOAD_CUSTOM_DICTIONARY_CHECKSUM_BAD LOAD_CUSTOM_DICTIONARY_CHECKSUM_MISSING (this indicates conversion to new format) LOAD_CUSTOM_DICTIONARY_BACKUP_PRESENT LOAD_CUSTOM_DICTIONARY_BACKUP_MISSING LOAD_CUSTOM_DICTIONARY_BACKUP_CHECKSUM_GOOD LOAD_CUSTOM_DICTIONARY_BACKUP_CHECKSUM_BAD LOAD_CUSTOM_DICTIONARY_BACKUP_CHECKSUM_MISSING LOAD_CUSTOM_DICTIONARY_BACKUP_RESTORED
Rachel: Atomic file write also returns a boolean that we can use to write SAVE_CUSTOM_DICTIONARY_SUCCESS, SAVE_CUSTOM_DICTIONARY_FAILURE.
Just on the topic of UMA: Ideally, metrics should allow us to take action based on what we measure - otherwise, why measure. For many of the metrics, I'm not sure how we can take action based on them. Would you mind clarifying that (ideally in the UMA doc?)
On 2012/12/04 16:46:53, groby wrote: > Just on the topic of UMA: Ideally, metrics should allow us to take action based > on what we measure - otherwise, why measure. > > For many of the metrics, I'm not sure how we can take action based on them. > Would you mind clarifying that (ideally in the UMA doc?) Cannot think of actions to take based on these UMAs. Let's not add them in this case.
https://codereview.chromium.org/11414282/diff/15003/chrome/browser/spellcheck... File chrome/browser/spellchecker/spellcheck_custom_dictionary.cc (right): https://codereview.chromium.org/11414282/diff/15003/chrome/browser/spellcheck... 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/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:215: if (!custom_words->empty() && (*(custom_words->end() - 1)).empty()) Why? Just to remove trailing newlines? In that case, won't our sort/uniq process clean that out anyways? https://codereview.chromium.org/11414282/diff/15003/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:227: std::string backup; Extract that into a function - 210-217 and 228-236 are almost identical. https://codereview.chromium.org/11414282/diff/15003/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:257: if (file_util::PathExists(custom_dictionary_path_)) Do you need to check? Can't we just call CopyFile, no matter if it exists or not? https://codereview.chromium.org/11414282/diff/15003/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:264: std::string checksum; nit: You can save a bit of string rebuilding here and when you save the file if the checksum is always the _last_ entry of the file. (Use rfind to find the prefix, if you choose to do that) https://codereview.chromium.org/11414282/diff/15003/chrome/browser/spellcheck... File chrome/browser/spellchecker/spellcheck_custom_dictionary.h (right): https://codereview.chromium.org/11414282/diff/15003/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.h:81: // loads that into |custom_words| instead. "If the backup is invalid too, |custom_words| will be cleared" https://codereview.chromium.org/11414282/diff/15003/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.h:85: // Backs up the original dictionary and checksum files, saves |custom_words| - "and checksum files" https://codereview.chromium.org/11414282/diff/15003/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.h:106: const std::string checksum_prefix_; Why a member? It never changes its value, and it's only used in one function? I'd suggest a constant in anon namespace and creating the string on the fly in GetChecksum (Ditto for backup_extension_)
Rachel: PTAL. https://codereview.chromium.org/11414282/diff/15003/chrome/browser/spellcheck... File chrome/browser/spellchecker/spellcheck_custom_dictionary.cc (right): https://codereview.chromium.org/11414282/diff/15003/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:65: if (custom_words->at(0).length() == 0) On 2012/12/05 00:56:19, groby wrote: > at(0).empty() Done. https://codereview.chromium.org/11414282/diff/15003/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:215: if (!custom_words->empty() && (*(custom_words->end() - 1)).empty()) On 2012/12/05 00:56:19, groby wrote: > Why? Just to remove trailing newlines? In that case, won't our sort/uniq process > clean that out anyways? Removed. https://codereview.chromium.org/11414282/diff/15003/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:227: std::string backup; On 2012/12/05 00:56:19, groby wrote: > Extract that into a function - 210-217 and 228-236 are almost identical. Done. https://codereview.chromium.org/11414282/diff/15003/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:257: if (file_util::PathExists(custom_dictionary_path_)) On 2012/12/05 00:56:19, groby wrote: > Do you need to check? Can't we just call CopyFile, no matter if it exists or > not? Done. https://codereview.chromium.org/11414282/diff/15003/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:264: std::string checksum; On 2012/12/05 00:56:19, groby wrote: > nit: You can save a bit of string rebuilding here and when you save the file if > the checksum is always the _last_ entry of the file. (Use rfind to find the > prefix, if you choose to do that) Done. https://codereview.chromium.org/11414282/diff/15003/chrome/browser/spellcheck... File chrome/browser/spellchecker/spellcheck_custom_dictionary.h (right): https://codereview.chromium.org/11414282/diff/15003/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.h:81: // loads that into |custom_words| instead. On 2012/12/05 00:56:19, groby wrote: > "If the backup is invalid too, |custom_words| will be cleared" Done. https://codereview.chromium.org/11414282/diff/15003/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.h:85: // Backs up the original dictionary and checksum files, saves |custom_words| On 2012/12/05 00:56:19, groby wrote: > - "and checksum files" Done. https://codereview.chromium.org/11414282/diff/15003/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.h:106: const std::string checksum_prefix_; On 2012/12/05 00:56:19, groby wrote: > Why a member? It never changes its value, and it's only used in one function? > > I'd suggest a constant in anon namespace and creating the string on the fly in > GetChecksum > > (Ditto for backup_extension_) Done.
LGTM w/ nits https://codereview.chromium.org/11414282/diff/23002/chrome/browser/spellcheck... File chrome/browser/spellchecker/spellcheck_custom_dictionary.cc (right): https://codereview.chromium.org/11414282/diff/23002/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:119: NOTREACHED(); nit: Why is this NOTREACHED? Are we really guaranteed users won't ever add invalid words? https://codereview.chromium.org/11414282/diff/23002/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:180: for (content::RenderProcessHost::iterator i( Curious: Shouldn't we notify once the FILE task has completed, not while it's still in the task queue? https://codereview.chromium.org/11414282/diff/23002/chrome/browser/spellcheck... File chrome/browser/spellchecker/spellcheck_custom_dictionary_unittest.cc (right): https://codereview.chromium.org/11414282/diff/23002/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary_unittest.cc:170: TEST_F(SpellcheckCustomDictionaryTest, Reliability) { nit: Can you break this into individual test cases?
Addressed the comments. Checking in. https://codereview.chromium.org/11414282/diff/23002/chrome/browser/spellcheck... File chrome/browser/spellchecker/spellcheck_custom_dictionary.cc (right): https://codereview.chromium.org/11414282/diff/23002/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:119: NOTREACHED(); On 2012/12/11 19:25:48, groby wrote: > nit: Why is this NOTREACHED? Are we really guaranteed users won't ever add > invalid words? Removed. https://codereview.chromium.org/11414282/diff/23002/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:180: for (content::RenderProcessHost::iterator i( On 2012/12/11 19:25:48, groby wrote: > Curious: Shouldn't we notify once the FILE task has completed, not while it's > still in the task queue? Renderers receive the removed and added words in the SpellCheckMsg_WordRemoved message and do not re-read the custom dictionary from disk or write anything to disk. Therefore, there should be no need to wait for disk write to complete. https://codereview.chromium.org/11414282/diff/23002/chrome/browser/spellcheck... File chrome/browser/spellchecker/spellcheck_custom_dictionary_unittest.cc (right): https://codereview.chromium.org/11414282/diff/23002/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_custom_dictionary_unittest.cc:170: TEST_F(SpellcheckCustomDictionaryTest, Reliability) { On 2012/12/11 19:25:48, groby wrote: > nit: Can you break this into individual test cases? Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rouslan@chromium.org/11414282/33001
Message was sent while issue was closed.
Change committed as 172494 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
