Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2101263002:
[Editing][CodeHealth] Refactor SpellChecker::respondToChangedSelection (Closed)
|
Created:
4 years, 5 months ago by yoichio Modified:
4 years, 5 months ago Reviewers:
yosin_UTC9 CC:
blink-reviews, chromium-reviews, groby+blinkspell_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor SpellChecker::respondToChangedSelection
Make variables const and remove |isContinuousGrammarCheckingEnabled|
because it equals to |isContinuousSpellCheckingEnabled|.
TEST=No change in behavior.
Committed: https://crrev.com/07c8f1e42b451d3f11a11ba2686e1a9a95aa6d4c
Cr-Commit-Position: refs/heads/master@{#403646}
Patch Set 1 #
Total comments: 2
Patch Set 2 : update #
Total comments: 4
Patch Set 3 : update #Messages
Total messages: 17 (6 generated)
Description was changed from ========== spellcherer BUG= ========== to ========== Refactor SpellChecker::respondToChangedSelection Make variables const and remove |isContinuousGrammarCheckingEnabled| because it equals to |isContinuousSpellCheckingEnabled|. TEST=No change in behavior. ==========
yoichio@chromium.org changed reviewers: + yosin@chromium.org
https://codereview.chromium.org/2101263002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2101263002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:823: if (!isContinuousSpellCheckingEnabled) { We prefer early return. When |isContinuousSpellCheckingEnabled| is false, this function does nothing. So, we can have following if-statement at L796 if (!isContinuousSpellCheckingEnabled) return; and reduce indentation.
https://codereview.chromium.org/2101263002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2101263002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:823: if (!isContinuousSpellCheckingEnabled) { On 2016/06/30 03:49:44, Yosi_UTC9 wrote: > We prefer early return. > > When |isContinuousSpellCheckingEnabled| is false, this function does nothing. > So, we can have following if-statement at L796 > > if (!isContinuousSpellCheckingEnabled) > return; > > and reduce indentation. > Done.
lgtm w/ small nit https://codereview.chromium.org/2101263002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2101263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:800: if (!this->isContinuousSpellCheckingEnabled()) { nit: s/this->// since we don't have local variable |isContinuousSpellCheckingEnabled| anymore.
https://codereview.chromium.org/2101263002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2101263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:800: if (!this->isContinuousSpellCheckingEnabled()) { On 2016/07/01 08:57:09, Yosi_UTC9 wrote: > nit: s/this->// since we don't have local variable > |isContinuousSpellCheckingEnabled| anymore. What do you mean?
https://codereview.chromium.org/2101263002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2101263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:800: if (!this->isContinuousSpellCheckingEnabled()) { On 2016/07/04 at 02:23:20, yoichio wrote: > On 2016/07/01 08:57:09, Yosi_UTC9 wrote: > > nit: s/this->// since we don't have local variable > > |isContinuousSpellCheckingEnabled| anymore. > > What do you mean? Before your patch, L800 has |bool isContinuousSpellCheckingEnabled = this->isContinuousSpellCheckingEnabled();|. After your patch, L800 is removed. So, we can use |SpellChecker::isContinuousSpellCheckingEnabled()| w/o |this->|
https://codereview.chromium.org/2101263002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2101263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:800: if (!this->isContinuousSpellCheckingEnabled()) { On 2016/07/04 04:29:30, Yosi_UTC9 wrote: > On 2016/07/04 at 02:23:20, yoichio wrote: > > On 2016/07/01 08:57:09, Yosi_UTC9 wrote: > > > nit: s/this->// since we don't have local variable > > > |isContinuousSpellCheckingEnabled| anymore. > > > > What do you mean? > > Before your patch, L800 has |bool isContinuousSpellCheckingEnabled = > this->isContinuousSpellCheckingEnabled();|. > After your patch, L800 is removed. So, we can use > |SpellChecker::isContinuousSpellCheckingEnabled()| w/o |this->| I see. Done.
The CQ bit was checked by yoichio@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org Link to the patchset: https://codereview.chromium.org/2101263002/#ps40001 (title: "update")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Refactor SpellChecker::respondToChangedSelection Make variables const and remove |isContinuousGrammarCheckingEnabled| because it equals to |isContinuousSpellCheckingEnabled|. TEST=No change in behavior. ========== to ========== Refactor SpellChecker::respondToChangedSelection Make variables const and remove |isContinuousGrammarCheckingEnabled| because it equals to |isContinuousSpellCheckingEnabled|. TEST=No change in behavior. ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Refactor SpellChecker::respondToChangedSelection Make variables const and remove |isContinuousGrammarCheckingEnabled| because it equals to |isContinuousSpellCheckingEnabled|. TEST=No change in behavior. ========== to ========== Refactor SpellChecker::respondToChangedSelection Make variables const and remove |isContinuousGrammarCheckingEnabled| because it equals to |isContinuousSpellCheckingEnabled|. TEST=No change in behavior. Committed: https://crrev.com/07c8f1e42b451d3f11a11ba2686e1a9a95aa6d4c Cr-Commit-Position: refs/heads/master@{#403646} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/07c8f1e42b451d3f11a11ba2686e1a9a95aa6d4c Cr-Commit-Position: refs/heads/master@{#403646} |
