Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
121523002:
[Spellcheck] Unify dictionary handling. (Closed)
|
Created:
6 years, 12 months ago by groby-ooo-7-16 Modified:
6 years, 11 months ago CC:
chromium-reviews, groby+spellwatch_chromium.org, rpetterson, rouslan+spellwatch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Spellcheck] Unify dictionary handling.
Remove platform-specific handling for dictionary.
R=rlp@chromium.org
BUG=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243978
Patch Set 1 #
Total comments: 4
Messages
Total messages: 9 (0 generated)
lgtm https://codereview.chromium.org/121523002/diff/1/chrome/browser/spellchecker/... File chrome/browser/spellchecker/spellcheck_service.cc (right): https://codereview.chromium.org/121523002/diff/1/chrome/browser/spellchecker/... chrome/browser/spellchecker/spellcheck_service.cc:172: hunspell_dictionary_->GetDictionaryFile(), process->GetHandle(), false); DHCECK(file != base::kInvalidPlatformFileValue) << logging::GetLastSystemErrorCode();
https://codereview.chromium.org/121523002/diff/1/chrome/browser/spellchecker/... File chrome/browser/spellchecker/spellcheck_service.cc (right): https://codereview.chromium.org/121523002/diff/1/chrome/browser/spellchecker/... chrome/browser/spellchecker/spellcheck_service.cc:172: hunspell_dictionary_->GetDictionaryFile(), process->GetHandle(), false); On 2013/12/30 17:21:24, Rouslan Solomakhin wrote: > DHCECK(file != base::kInvalidPlatformFileValue) > << logging::GetLastSystemErrorCode(); Which error is this supposed to catch?
https://codereview.chromium.org/121523002/diff/1/chrome/browser/spellchecker/... File chrome/browser/spellchecker/spellcheck_service.cc (right): https://codereview.chromium.org/121523002/diff/1/chrome/browser/spellchecker/... chrome/browser/spellchecker/spellcheck_service.cc:172: hunspell_dictionary_->GetDictionaryFile(), process->GetHandle(), false); On 2014/01/02 17:48:44, groby wrote: > On 2013/12/30 17:21:24, Rouslan Solomakhin wrote: > > DHCECK(file != base::kInvalidPlatformFileValue) > > << logging::GetLastSystemErrorCode(); > > Which error is this supposed to catch? This would catch ::DuplicateHandle() returning false, like in the existing code. http://msdn.microsoft.com/en-us/library/windows/desktop/ms724251(v=vs.85).aspx is not clear on why that function could return false.
https://codereview.chromium.org/121523002/diff/1/chrome/browser/spellchecker/... File chrome/browser/spellchecker/spellcheck_service.cc (right): https://codereview.chromium.org/121523002/diff/1/chrome/browser/spellchecker/... chrome/browser/spellchecker/spellcheck_service.cc:172: hunspell_dictionary_->GetDictionaryFile(), process->GetHandle(), false); On 2014/01/02 17:54:25, Rouslan Solomakhin wrote: > On 2014/01/02 17:48:44, groby wrote: > > On 2013/12/30 17:21:24, Rouslan Solomakhin wrote: > > > DHCECK(file != base::kInvalidPlatformFileValue) > > > << logging::GetLastSystemErrorCode(); > > > > Which error is this supposed to catch? > > This would catch ::DuplicateHandle() returning false, like in the existing code. > http://msdn.microsoft.com/en-us/library/windows/desktop/ms724251%28v=vs.85%29... > is not clear on why that function could return false. Technically, DCHECK should document invalid states. If DuplicateHandle can fail, it's not an invalid state :) Further, GetFileHandleForProcess handles DuplicateHandle failures correctly, and the render process handles invalid file handles correctly. I don't think this DCHECK has a point, so keeping it removed. If we're concerned that this actually fails in the field, this should either be a metric or (shudder!) a CHECK
On 2014/01/02 19:15:27, groby wrote: > https://codereview.chromium.org/121523002/diff/1/chrome/browser/spellchecker/... > File chrome/browser/spellchecker/spellcheck_service.cc (right): > > https://codereview.chromium.org/121523002/diff/1/chrome/browser/spellchecker/... > chrome/browser/spellchecker/spellcheck_service.cc:172: > hunspell_dictionary_->GetDictionaryFile(), process->GetHandle(), false); > On 2014/01/02 17:54:25, Rouslan Solomakhin wrote: > > On 2014/01/02 17:48:44, groby wrote: > > > On 2013/12/30 17:21:24, Rouslan Solomakhin wrote: > > > > DHCECK(file != base::kInvalidPlatformFileValue) > > > > << logging::GetLastSystemErrorCode(); > > > > > > Which error is this supposed to catch? > > > > This would catch ::DuplicateHandle() returning false, like in the existing > code. > > > http://msdn.microsoft.com/en-us/library/windows/desktop/ms724251%2528v=vs.85%... > > is not clear on why that function could return false. > > Technically, DCHECK should document invalid states. If DuplicateHandle can fail, > it's not an invalid state :) > > Further, GetFileHandleForProcess handles DuplicateHandle failures correctly, and > the render process handles invalid file handles correctly. I don't think this > DCHECK has a point, so keeping it removed. > > If we're concerned that this actually fails in the field, this should either be > a metric or (shudder!) a CHECK sounds good.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/groby@chromium.org/121523002/1
Message was sent while issue was closed.
Change committed as 243978 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
