Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2720193002:
Implement hot mode invocation for idle time spell checker (Closed)
|
Created:
3 years, 9 months ago by Xiaocheng Modified:
3 years, 9 months ago Reviewers:
yosin_UTC9 CC:
blink-reviews, chromium-reviews, groby+blinkspell_chromium.org, timvolodine Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement hot mode invocation for idle time spell checker
This patch implements IdleSpellCheckCallback::hotModeInvocation and
its helper functions.
BUG=517298
TEST=existing layout tests in editing/spelling
Review-Url: https://codereview.chromium.org/2720193002
Cr-Commit-Position: refs/heads/master@{#455248}
Committed: https://chromium.googlesource.com/chromium/src/+/f3f30ff20c869bcff019e748dce273d3e2f29316
Patch Set 1 #Patch Set 2 : Wed Mar 1 16:02:57 PST 2017 #
Total comments: 20
Patch Set 3 : Wed Mar 1 20:52:55 PST 2017 #Patch Set 4 : Thu Mar 2 16:35:42 PST 2017 #
Total comments: 10
Patch Set 5 : Thu Mar 2 22:22:47 PST 2017 #
Total comments: 4
Patch Set 6 : Fri Mar 3 16:05:41 PST 2017 #Patch Set 7 : Mon Mar 6 12:22:30 PST 2017 #Patch Set 8 : Mon Mar 6 12:22:30 PST 2017 #
Total comments: 4
Patch Set 9 : style fix #
Dependent Patchsets: Messages
Total messages: 58 (32 generated)
The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run
The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Implement hot mode invocation for idle time spell checker BUG=517298 ========== to ========== Implement hot mode invocation for idle time spell checker This patch implements IdleSpellCheckCallback::hotModeInvocation and its helper functions. BUG=517298 TEST=existing layout tests in editing/spelling ==========
xiaochengh@chromium.org changed reviewers: + yosin@chromium.org
PTAL.
https://codereview.chromium.org/2720193002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/Editor.h (right): https://codereview.chromium.org/2720193002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/Editor.h:201: UndoStack& undoStack() const { return *m_undoStack; } This should be |const UndoStack&|. Except for Editor, we should not change |UndoStack|. https://codereview.chromium.org/2720193002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/TypingCommand.h (right): https://codereview.chromium.org/2720193002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/TypingCommand.h:85: static TypingCommand* lastTypingCommandIfStillOpenForTyping(LocalFrame*); It seems we want to move this function to |Editor|. BTW, we want not to allow including "editing/commands/*.h" other than "Editor.cpp". https://codereview.chromium.org/2720193002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/UndoStack.h (right): https://codereview.chromium.org/2720193002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/UndoStack.h:43: typedef HeapDeque<Member<UndoStep>> UndoStepStack; nit: using UndoStepStack = HeapDeque<Member<UndoStep>>; https://codereview.chromium.org/2720193002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/UndoStack.h:61: const UndoStepStack& undoSteps() const { return m_undoStack; } Please provide high level API for usage base. We don't want to expose internal data structure. https://codereview.chromium.org/2720193002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp (right): https://codereview.chromium.org/2720193002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp:81: UndoStack& IdleSpellCheckCallback::undoStack() const { This should be |const UndoStack&| https://codereview.chromium.org/2720193002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp:159: int fullLength = TextIterator::rangeLength(fullRange.startPosition(), nit: s/int/const int/ https://codereview.chromium.org/2720193002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp:167: nit: Get rid of an extra blank line. https://codereview.chromium.org/2720193002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp:177: nit: Get rid of an extra blank line. https://codereview.chromium.org/2720193002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp:247: for (auto iter = undoSteps.rbegin(); iter != undoSteps.rend(); ++iter) { Could you we import |base::Reversed()|[1] into Blink? [1] https://cs.chromium.org/chromium/src/base/containers/adapters.h?q=base::Rever... https://codereview.chromium.org/2720193002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.h (right): https://codereview.chromium.org/2720193002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.h:68: UndoStack& undoStack() const; This should be private.
Thanks for reviewing. https://codereview.chromium.org/2720193002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/Editor.h (right): https://codereview.chromium.org/2720193002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/Editor.h:201: UndoStack& undoStack() const { return *m_undoStack; } On 2017/03/02 at 03:38:40, yosin_UTC9 wrote: > This should be |const UndoStack&|. Except for Editor, we should not change |UndoStack|. Compile error if changed to |const UndoStack&|: binding value of type 'const blink::UndoStack' to reference to type 'blink::UndoStack' drops 'const' qualifier https://codereview.chromium.org/2720193002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/TypingCommand.h (right): https://codereview.chromium.org/2720193002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/TypingCommand.h:85: static TypingCommand* lastTypingCommandIfStillOpenForTyping(LocalFrame*); On 2017/03/02 at 03:38:40, yosin_UTC9 wrote: > It seems we want to move this function to |Editor|. > > BTW, we want not to allow including "editing/commands/*.h" other than "Editor.cpp". Hmm, agreed that Editor is a better place for this function. Will do in another patch. https://codereview.chromium.org/2720193002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/UndoStack.h (right): https://codereview.chromium.org/2720193002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/UndoStack.h:61: const UndoStepStack& undoSteps() const { return m_undoStack; } On 2017/03/02 at 03:38:40, yosin_UTC9 wrote: > Please provide high level API for usage base. We don't want to expose internal data structure. Will do. https://codereview.chromium.org/2720193002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp (right): https://codereview.chromium.org/2720193002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp:81: UndoStack& IdleSpellCheckCallback::undoStack() const { On 2017/03/02 at 03:38:41, yosin_UTC9 wrote: > This should be |const UndoStack&| This one can be changed to const. Interesting... https://codereview.chromium.org/2720193002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp:159: int fullLength = TextIterator::rangeLength(fullRange.startPosition(), On 2017/03/02 at 03:38:41, yosin_UTC9 wrote: > nit: s/int/const int/ Done. https://codereview.chromium.org/2720193002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp:177: On 2017/03/02 at 03:38:40, yosin_UTC9 wrote: > nit: Get rid of an extra blank line. Done. https://codereview.chromium.org/2720193002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp:247: for (auto iter = undoSteps.rbegin(); iter != undoSteps.rend(); ++iter) { On 2017/03/02 at 03:38:41, yosin_UTC9 wrote: > Could you we import |base::Reversed()|[1] into Blink? > > [1] https://cs.chromium.org/chromium/src/base/containers/adapters.h?q=base::Rever... Never done such thing before... Can I follow the way how |WTF::Optional| is imported? https://codereview.chromium.org/2720193002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.h (right): https://codereview.chromium.org/2720193002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.h:68: UndoStack& undoStack() const; On 2017/03/02 at 03:38:41, yosin_UTC9 wrote: > This should be private. This is private.
https://codereview.chromium.org/2720193002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp (right): https://codereview.chromium.org/2720193002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp:247: for (auto iter = undoSteps.rbegin(); iter != undoSteps.rend(); ++iter) { On 2017/03/02 at 04:34:59, Xiaocheng wrote: > On 2017/03/02 at 03:38:41, yosin_UTC9 wrote: > > Could you we import |base::Reversed()|[1] into Blink? > > > > [1] https://cs.chromium.org/chromium/src/base/containers/adapters.h?q=base::Rever... > > Never done such thing before... > > Can I follow the way how |WTF::Optional| is imported? Yes. It also helps others not only us. BTW, we don't need to do this now f we expose reverse range from UndoStack class, we don't need. Since we don't want to expose HeapvEctor<UndoStep> from UndoStack. Here is sketch: class UndoStack { UndoStepRange undoSteps() { return UndoStepRange(this); } class UndoStatepRange { using Iterator = HeapVector<UndoStep>::reverse_iterator<...>; Iterator begin() const { return m_undostack.rbegin(); } Iterator end() const { return m_undoStack.rend(); } } };
The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2720193002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp (right): https://codereview.chromium.org/2720193002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp:247: for (auto iter = undoSteps.rbegin(); iter != undoSteps.rend(); ++iter) { On 2017/03/02 at 04:41:48, yosin_UTC9 wrote: > On 2017/03/02 at 04:34:59, Xiaocheng wrote: > > On 2017/03/02 at 03:38:41, yosin_UTC9 wrote: > > > Could you we import |base::Reversed()|[1] into Blink? > > > > > > [1] https://cs.chromium.org/chromium/src/base/containers/adapters.h?q=base::Rever... > > > > Never done such thing before... > > > > Can I follow the way how |WTF::Optional| is imported? > > Yes. It also helps others not only us. > > BTW, we don't need to do this now f we expose reverse range from UndoStack class, we don't need. > Since we don't want to expose HeapvEctor<UndoStep> from UndoStack. > > Here is sketch: > > class UndoStack { > UndoStepRange undoSteps() { return UndoStepRange(this); } > > class UndoStatepRange { > using Iterator = HeapVector<UndoStep>::reverse_iterator<...>; > Iterator begin() const { return m_undostack.rbegin(); } > Iterator end() const { return m_undoStack.rend(); } > } > }; Thanks. I'll do this for now and add a TODO to use |Reversed|.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/03/02 at 04:57:53, xiaochengh wrote: > https://codereview.chromium.org/2720193002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp (right): > > https://codereview.chromium.org/2720193002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp:247: for (auto iter = undoSteps.rbegin(); iter != undoSteps.rend(); ++iter) { > On 2017/03/02 at 04:41:48, yosin_UTC9 wrote: > > On 2017/03/02 at 04:34:59, Xiaocheng wrote: > > > On 2017/03/02 at 03:38:41, yosin_UTC9 wrote: > > > > Could you we import |base::Reversed()|[1] into Blink? > > > > > > > > [1] https://cs.chromium.org/chromium/src/base/containers/adapters.h?q=base::Rever... > > > > > > Never done such thing before... > > > > > > Can I follow the way how |WTF::Optional| is imported? > > > > Yes. It also helps others not only us. > > > > BTW, we don't need to do this now f we expose reverse range from UndoStack class, we don't need. > > Since we don't want to expose HeapvEctor<UndoStep> from UndoStack. > > > > Here is sketch: > > > > class UndoStack { > > UndoStepRange undoSteps() { return UndoStepRange(this); } > > > > class UndoStatepRange { > > using Iterator = HeapVector<UndoStep>::reverse_iterator<...>; > > Iterator begin() const { return m_undostack.rbegin(); } > > Iterator end() const { return m_undoStack.rend(); } > > } > > }; > > Thanks. I'll do this for now and add a TODO to use |Reversed|. I think once we have UndoStepRange, we don't need to have |Reversed|. It is better to implement |class Iterator| by ourselves to hide |HeapVector<T>|.
The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
This one is also updated. PTAL?
https://codereview.chromium.org/2720193002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/Editor.h (right): https://codereview.chromium.org/2720193002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/Editor.h:202: UndoStack& undoStack() const { return *m_undoStack; } Could you put comment that we should treat return value as |const UndoStack&| and the reason why we can't. https://codereview.chromium.org/2720193002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp (right): https://codereview.chromium.org/2720193002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp:155: const int fullLength = TextIterator::rangeLength(fullRange.startPosition(), I'm not sure why we don't have TextIterator::rangeLength(const EphemeralRange&)... https://codereview.chromium.org/2720193002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp:205: void IdleSpellCheckCallback::hotModeCheckRootEditable( Could you rename this function to start with verb? requestSpellCheckForHotModeRootEditable() or better one. https://codereview.chromium.org/2720193002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp:240: maxSequenceNumber = std::max(maxSequenceNumber, step->sequenceNumber()); It is better to update |m_lastProcessUndoStepSequence| to use |const uint64_t| const uint64_t watermark = m_lastProcessedUndoStepSequence; for (const UndoStep* step : ...) { if (step->sequenceNumber() <= watermark) break; m_lastProcessUndoStepSequence = std::max(...) ... } This tells me what m_lastProcessUndoStepSequence holding. https://codereview.chromium.org/2720193002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.h (right): https://codereview.chromium.org/2720193002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.h:77: EphemeralRange calcHotModeCheckingRange(const Element&, nit: s/calc/calculate/ We usually don't use abbrev.
Updated. PTAL. https://codereview.chromium.org/2720193002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/Editor.h (right): https://codereview.chromium.org/2720193002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/Editor.h:202: UndoStack& undoStack() const { return *m_undoStack; } On 2017/03/03 at 05:34:23, yosin_UTC9 wrote: > Could you put comment that we should treat return value as |const UndoStack&| and the reason why we can't. Done. https://codereview.chromium.org/2720193002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp (right): https://codereview.chromium.org/2720193002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp:155: const int fullLength = TextIterator::rangeLength(fullRange.startPosition(), On 2017/03/03 at 05:34:23, yosin_UTC9 wrote: > I'm not sure why we don't have TextIterator::rangeLength(const EphemeralRange&)... Maybe we don't need it that much, either... https://codereview.chromium.org/2720193002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp:205: void IdleSpellCheckCallback::hotModeCheckRootEditable( On 2017/03/03 at 05:34:23, yosin_UTC9 wrote: > Could you rename this function to start with verb? > > requestSpellCheckForHotModeRootEditable() or better one. Changed to checkRootEditableInHotMode. https://codereview.chromium.org/2720193002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp:240: maxSequenceNumber = std::max(maxSequenceNumber, step->sequenceNumber()); On 2017/03/03 at 05:34:23, yosin_UTC9 wrote: > It is better to update |m_lastProcessUndoStepSequence| to use |const uint64_t| > > const uint64_t watermark = m_lastProcessedUndoStepSequence; > for (const UndoStep* step : ...) { > if (step->sequenceNumber() <= watermark) > break; > m_lastProcessUndoStepSequence = std::max(...) > ... > } > > This tells me what m_lastProcessUndoStepSequence holding. Done. https://codereview.chromium.org/2720193002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.h (right): https://codereview.chromium.org/2720193002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.h:77: EphemeralRange calcHotModeCheckingRange(const Element&, On 2017/03/03 at 05:34:23, yosin_UTC9 wrote: > nit: s/calc/calculate/ > We usually don't use abbrev. Done.
https://codereview.chromium.org/2720193002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp (right): https://codereview.chromium.org/2720193002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp:208: HeapVector<Member<Element>>* processedRootEditables) { Let's use HeapHashSet<Member<Element>> I prefer to have a class SpellCheckRequestScope, or another good name, which doesn't request for already request element, rather than pass processed element list. class SpellCheckRequestScope { STACK_ALLOCATE() public: explicit SpellCheckRequestScope(SpellCheckeRequester*); void requestCheckingForRootEditableInHotMode(...) { ... if (m_requester.count(element)) return; ... m_spellCheckRequeset.requestCheckingFor(request); } private: SpellCheckReqter& m_requester; HeapHashSet<Member<Element>> m_requestedEleemnt; }
https://codereview.chromium.org/2720193002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp (right): https://codereview.chromium.org/2720193002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp:208: HeapVector<Member<Element>>* processedRootEditables) { On 2017/03/03 at 08:55:18, yosin_UTC9 wrote: > Let's use HeapHashSet<Member<Element>> I don't expect checking a lot of root editables in hot mode. In such case, a vector shouldn't cause any performance issue, but is much lighter than a hash set. Anyway, talk is cheap. I prefer adding a UMA of the number of distinct root editables checked in each hot mode invocation, and let data decide whether the de-duplication process should be more scalable. > > I prefer to have a class SpellCheckRequestScope, or another good name, which doesn't request for already request element, > rather than pass processed element list. > > class SpellCheckRequestScope { > STACK_ALLOCATE() > public: > explicit SpellCheckRequestScope(SpellCheckeRequester*); > void requestCheckingForRootEditableInHotMode(...) { > ... > if (m_requester.count(element)) return; > ... > m_spellCheckRequeset.requestCheckingFor(request); > } > private: > SpellCheckReqter& m_requester; > HeapHashSet<Member<Element>> m_requestedEleemnt; > } Sorry I don't get it... What's the benefit of introducing a scope here? In my understanding, we introduce scope only in two scenarios: - To pass local variables deep into the call stack - To do some work in the dtor of the scope We are in neither case here.
The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2720193002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp (right): https://codereview.chromium.org/2720193002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp:208: HeapVector<Member<Element>>* processedRootEditables) { On 2017/03/03 at 14:31:52, Xiaocheng wrote: > On 2017/03/03 at 08:55:18, yosin_UTC9 wrote: > > Let's use HeapHashSet<Member<Element>> > > I don't expect checking a lot of root editables in hot mode. In such case, a vector shouldn't cause any performance issue, but is much lighter than a hash set. > > Anyway, talk is cheap. I prefer adding a UMA of the number of distinct root editables checked in each hot mode invocation, and let data decide whether the de-duplication process should be more scalable. > > > > > I prefer to have a class SpellCheckRequestScope, or another good name, which doesn't request for already request element, > > rather than pass processed element list. > > > > class SpellCheckRequestScope { > > STACK_ALLOCATE() > > public: > > explicit SpellCheckRequestScope(SpellCheckeRequester*); > > void requestCheckingForRootEditableInHotMode(...) { > > ... > > if (m_requester.count(element)) return; > > ... > > m_spellCheckRequeset.requestCheckingFor(request); > > } > > private: > > SpellCheckReqter& m_requester; > > HeapHashSet<Member<Element>> m_requestedEleemnt; > > } > > Sorry I don't get it... > > What's the benefit of introducing a scope here? In my understanding, we introduce scope only in two scenarios: > - To pass local variables deep into the call stack > - To do some work in the dtor of the scope > > We are in neither case here. This is another use case of scope. In this case, scope manages list of requested elements to avoid multiple request on element. In your itemized list, dtor of this scope frees memory of list of element, HeapVector/HeapSet.
https://codereview.chromium.org/2720193002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp (right): https://codereview.chromium.org/2720193002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp:208: HeapVector<Member<Element>>* processedRootEditables) { On 2017/03/06 at 04:17:31, yosin_UTC9 wrote: > On 2017/03/03 at 14:31:52, Xiaocheng wrote: > > On 2017/03/03 at 08:55:18, yosin_UTC9 wrote: > > > Let's use HeapHashSet<Member<Element>> > > > > I don't expect checking a lot of root editables in hot mode. In such case, a vector shouldn't cause any performance issue, but is much lighter than a hash set. > > > > Anyway, talk is cheap. I prefer adding a UMA of the number of distinct root editables checked in each hot mode invocation, and let data decide whether the de-duplication process should be more scalable. > > > > > > > > I prefer to have a class SpellCheckRequestScope, or another good name, which doesn't request for already request element, > > > rather than pass processed element list. > > > > > > class SpellCheckRequestScope { > > > STACK_ALLOCATE() > > > public: > > > explicit SpellCheckRequestScope(SpellCheckeRequester*); > > > void requestCheckingForRootEditableInHotMode(...) { > > > ... > > > if (m_requester.count(element)) return; > > > ... > > > m_spellCheckRequeset.requestCheckingFor(request); > > > } > > > private: > > > SpellCheckReqter& m_requester; > > > HeapHashSet<Member<Element>> m_requestedEleemnt; > > > } > > > > Sorry I don't get it... > > > > What's the benefit of introducing a scope here? In my understanding, we introduce scope only in two scenarios: > > - To pass local variables deep into the call stack > > - To do some work in the dtor of the scope > > > > We are in neither case here. > > This is another use case of scope. In this case, scope manages list of requested elements to avoid multiple request > on element. > > In your itemized list, dtor of this scope frees memory of list of element, HeapVector/HeapSet. Naming wise? If "Scope" makes you confusing. We call |UniqueSpellCheckeRequester| or another name which tells we don't make multiple requests for same element. For me passing "temporary" list to function is implementation details, like: // Implement recursive function as tail-recursive. int fact(n, int acc =1) { if (n <= 1) return acc; return fact(n - 1, acc * n); } where |acc| is an implementation detail, we don't want callers don't care.
On 2017/03/06 at 04:39:15, yosin wrote: > https://codereview.chromium.org/2720193002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp (right): > > https://codereview.chromium.org/2720193002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp:208: HeapVector<Member<Element>>* processedRootEditables) { > On 2017/03/06 at 04:17:31, yosin_UTC9 wrote: > > On 2017/03/03 at 14:31:52, Xiaocheng wrote: > > > On 2017/03/03 at 08:55:18, yosin_UTC9 wrote: > > > > Let's use HeapHashSet<Member<Element>> > > > > > > I don't expect checking a lot of root editables in hot mode. In such case, a vector shouldn't cause any performance issue, but is much lighter than a hash set. > > > > > > Anyway, talk is cheap. I prefer adding a UMA of the number of distinct root editables checked in each hot mode invocation, and let data decide whether the de-duplication process should be more scalable. > > > > > > > > > > > I prefer to have a class SpellCheckRequestScope, or another good name, which doesn't request for already request element, > > > > rather than pass processed element list. > > > > > > > > class SpellCheckRequestScope { > > > > STACK_ALLOCATE() > > > > public: > > > > explicit SpellCheckRequestScope(SpellCheckeRequester*); > > > > void requestCheckingForRootEditableInHotMode(...) { > > > > ... > > > > if (m_requester.count(element)) return; > > > > ... > > > > m_spellCheckRequeset.requestCheckingFor(request); > > > > } > > > > private: > > > > SpellCheckReqter& m_requester; > > > > HeapHashSet<Member<Element>> m_requestedEleemnt; > > > > } > > > > > > Sorry I don't get it... > > > > > > What's the benefit of introducing a scope here? In my understanding, we introduce scope only in two scenarios: > > > - To pass local variables deep into the call stack > > > - To do some work in the dtor of the scope > > > > > > We are in neither case here. > > > > This is another use case of scope. In this case, scope manages list of requested elements to avoid multiple request > > on element. > > > > In your itemized list, dtor of this scope frees memory of list of element, HeapVector/HeapSet. > > Naming wise? If "Scope" makes you confusing. We call |UniqueSpellCheckeRequester| or another name which tells we don't > make multiple requests for same element. > > For me passing "temporary" list to function is implementation details, like: > > // Implement recursive function as tail-recursive. > int fact(n, int acc =1) { > if (n <= 1) return acc; > return fact(n - 1, acc * n); > } > > where |acc| is an implementation detail, we don't want callers don't care. OK, hiding implementation details makes sense. I think maybe it's better to just store the vector as a member of IdleSpellCheckCallback. Introducing a scope still seems unnecessarily complicated...
On 2017/03/06 at 06:39:36, xiaochengh wrote: > On 2017/03/06 at 04:39:15, yosin wrote: > > https://codereview.chromium.org/2720193002/diff/80001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp (right): > > > > https://codereview.chromium.org/2720193002/diff/80001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp:208: HeapVector<Member<Element>>* processedRootEditables) { > > On 2017/03/06 at 04:17:31, yosin_UTC9 wrote: > > > On 2017/03/03 at 14:31:52, Xiaocheng wrote: > > > > On 2017/03/03 at 08:55:18, yosin_UTC9 wrote: > > > > > Let's use HeapHashSet<Member<Element>> > > > > > > > > I don't expect checking a lot of root editables in hot mode. In such case, a vector shouldn't cause any performance issue, but is much lighter than a hash set. > > > > > > > > Anyway, talk is cheap. I prefer adding a UMA of the number of distinct root editables checked in each hot mode invocation, and let data decide whether the de-duplication process should be more scalable. > > > > > > > > > > > > > > I prefer to have a class SpellCheckRequestScope, or another good name, which doesn't request for already request element, > > > > > rather than pass processed element list. > > > > > > > > > > class SpellCheckRequestScope { > > > > > STACK_ALLOCATE() > > > > > public: > > > > > explicit SpellCheckRequestScope(SpellCheckeRequester*); > > > > > void requestCheckingForRootEditableInHotMode(...) { > > > > > ... > > > > > if (m_requester.count(element)) return; > > > > > ... > > > > > m_spellCheckRequeset.requestCheckingFor(request); > > > > > } > > > > > private: > > > > > SpellCheckReqter& m_requester; > > > > > HeapHashSet<Member<Element>> m_requestedEleemnt; > > > > > } > > > > > > > > Sorry I don't get it... > > > > > > > > What's the benefit of introducing a scope here? In my understanding, we introduce scope only in two scenarios: > > > > - To pass local variables deep into the call stack > > > > - To do some work in the dtor of the scope > > > > > > > > We are in neither case here. > > > > > > This is another use case of scope. In this case, scope manages list of requested elements to avoid multiple request > > > on element. > > > > > > In your itemized list, dtor of this scope frees memory of list of element, HeapVector/HeapSet. > > > > Naming wise? If "Scope" makes you confusing. We call |UniqueSpellCheckeRequester| or another name which tells we don't > > make multiple requests for same element. > > > > For me passing "temporary" list to function is implementation details, like: > > > > // Implement recursive function as tail-recursive. > > int fact(n, int acc =1) { > > if (n <= 1) return acc; > > return fact(n - 1, acc * n); > > } > > > > where |acc| is an implementation detail, we don't want callers don't care. > > OK, hiding implementation details makes sense. > > I think maybe it's better to just store the vector as a member of IdleSpellCheckCallback. Introducing a scope still seems unnecessarily complicated... I think we should avoid to make HeapSet(instead of HeapVector) as member of IdleSpellCheckCallback. Lifetime of HeapSet is longer than needed and we don't want to explicitly clear it at end of use. I guess we have other users of UniqueSpellCheckeRequester, not sure.
On 2017/03/06 at 06:46:52, yosin wrote: > On 2017/03/06 at 06:39:36, xiaochengh wrote: > > On 2017/03/06 at 04:39:15, yosin wrote: > > > https://codereview.chromium.org/2720193002/diff/80001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp (right): > > > > > > https://codereview.chromium.org/2720193002/diff/80001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp:208: HeapVector<Member<Element>>* processedRootEditables) { > > > On 2017/03/06 at 04:17:31, yosin_UTC9 wrote: > > > > On 2017/03/03 at 14:31:52, Xiaocheng wrote: > > > > > On 2017/03/03 at 08:55:18, yosin_UTC9 wrote: > > > > > > Let's use HeapHashSet<Member<Element>> > > > > > > > > > > I don't expect checking a lot of root editables in hot mode. In such case, a vector shouldn't cause any performance issue, but is much lighter than a hash set. > > > > > > > > > > Anyway, talk is cheap. I prefer adding a UMA of the number of distinct root editables checked in each hot mode invocation, and let data decide whether the de-duplication process should be more scalable. > > > > > > > > > > > > > > > > > I prefer to have a class SpellCheckRequestScope, or another good name, which doesn't request for already request element, > > > > > > rather than pass processed element list. > > > > > > > > > > > > class SpellCheckRequestScope { > > > > > > STACK_ALLOCATE() > > > > > > public: > > > > > > explicit SpellCheckRequestScope(SpellCheckeRequester*); > > > > > > void requestCheckingForRootEditableInHotMode(...) { > > > > > > ... > > > > > > if (m_requester.count(element)) return; > > > > > > ... > > > > > > m_spellCheckRequeset.requestCheckingFor(request); > > > > > > } > > > > > > private: > > > > > > SpellCheckReqter& m_requester; > > > > > > HeapHashSet<Member<Element>> m_requestedEleemnt; > > > > > > } > > > > > > > > > > Sorry I don't get it... > > > > > > > > > > What's the benefit of introducing a scope here? In my understanding, we introduce scope only in two scenarios: > > > > > - To pass local variables deep into the call stack > > > > > - To do some work in the dtor of the scope > > > > > > > > > > We are in neither case here. > > > > > > > > This is another use case of scope. In this case, scope manages list of requested elements to avoid multiple request > > > > on element. > > > > > > > > In your itemized list, dtor of this scope frees memory of list of element, HeapVector/HeapSet. > > > > > > Naming wise? If "Scope" makes you confusing. We call |UniqueSpellCheckeRequester| or another name which tells we don't > > > make multiple requests for same element. > > > > > > For me passing "temporary" list to function is implementation details, like: > > > > > > // Implement recursive function as tail-recursive. > > > int fact(n, int acc =1) { > > > if (n <= 1) return acc; > > > return fact(n - 1, acc * n); > > > } > > > > > > where |acc| is an implementation detail, we don't want callers don't care. > > > > OK, hiding implementation details makes sense. > > > > I think maybe it's better to just store the vector as a member of IdleSpellCheckCallback. Introducing a scope still seems unnecessarily complicated... > > I think we should avoid to make HeapSet(instead of HeapVector) as member of IdleSpellCheckCallback. Lifetime of HeapSet is longer than needed and we don't > want to explicitly clear it at end of use. I still prefer HeapVector than HeapSet. Anyway, this should be decided by a UMA. And what do you mean by "lifetime of HeapSet"? Isn't the lifetime the same as the scope or owner element? Or do you mean the lifetime of the members of the hash set? > > I guess we have other users of UniqueSpellCheckeRequester, not sure. ... Do we? To me, it seems to be only an implementation detail of hot mode so there's no need to make it more than 5 LOC...
On 2017/03/06 at 06:56:39, xiaochengh wrote: > On 2017/03/06 at 06:46:52, yosin wrote: > > On 2017/03/06 at 06:39:36, xiaochengh wrote: > > > On 2017/03/06 at 04:39:15, yosin wrote: > > > > https://codereview.chromium.org/2720193002/diff/80001/third_party/WebKit/Sour... > > > > File third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp (right): > > > > > > > > https://codereview.chromium.org/2720193002/diff/80001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp:208: HeapVector<Member<Element>>* processedRootEditables) { > > > > On 2017/03/06 at 04:17:31, yosin_UTC9 wrote: > > > > > On 2017/03/03 at 14:31:52, Xiaocheng wrote: > > > > > > On 2017/03/03 at 08:55:18, yosin_UTC9 wrote: > > > > > > > Let's use HeapHashSet<Member<Element>> > > > > > > > > > > > > I don't expect checking a lot of root editables in hot mode. In such case, a vector shouldn't cause any performance issue, but is much lighter than a hash set. > > > > > > > > > > > > Anyway, talk is cheap. I prefer adding a UMA of the number of distinct root editables checked in each hot mode invocation, and let data decide whether the de-duplication process should be more scalable. > > > > > > > > > > > > > > > > > > > > I prefer to have a class SpellCheckRequestScope, or another good name, which doesn't request for already request element, > > > > > > > rather than pass processed element list. > > > > > > > > > > > > > > class SpellCheckRequestScope { > > > > > > > STACK_ALLOCATE() > > > > > > > public: > > > > > > > explicit SpellCheckRequestScope(SpellCheckeRequester*); > > > > > > > void requestCheckingForRootEditableInHotMode(...) { > > > > > > > ... > > > > > > > if (m_requester.count(element)) return; > > > > > > > ... > > > > > > > m_spellCheckRequeset.requestCheckingFor(request); > > > > > > > } > > > > > > > private: > > > > > > > SpellCheckReqter& m_requester; > > > > > > > HeapHashSet<Member<Element>> m_requestedEleemnt; > > > > > > > } > > > > > > > > > > > > Sorry I don't get it... > > > > > > > > > > > > What's the benefit of introducing a scope here? In my understanding, we introduce scope only in two scenarios: > > > > > > - To pass local variables deep into the call stack > > > > > > - To do some work in the dtor of the scope > > > > > > > > > > > > We are in neither case here. > > > > > > > > > > This is another use case of scope. In this case, scope manages list of requested elements to avoid multiple request > > > > > on element. > > > > > > > > > > In your itemized list, dtor of this scope frees memory of list of element, HeapVector/HeapSet. > > > > > > > > Naming wise? If "Scope" makes you confusing. We call |UniqueSpellCheckeRequester| or another name which tells we don't > > > > make multiple requests for same element. > > > > > > > > For me passing "temporary" list to function is implementation details, like: > > > > > > > > // Implement recursive function as tail-recursive. > > > > int fact(n, int acc =1) { > > > > if (n <= 1) return acc; > > > > return fact(n - 1, acc * n); > > > > } > > > > > > > > where |acc| is an implementation detail, we don't want callers don't care. > > > > > > OK, hiding implementation details makes sense. > > > > > > I think maybe it's better to just store the vector as a member of IdleSpellCheckCallback. Introducing a scope still seems unnecessarily complicated... > > > > I think we should avoid to make HeapSet(instead of HeapVector) as member of IdleSpellCheckCallback. Lifetime of HeapSet is longer than needed and we don't > > want to explicitly clear it at end of use. > > I still prefer HeapVector than HeapSet. Anyway, this should be decided by a UMA. Just curiosity, why do you prefer |HeapVector|? For to save memory? Do we expect the size of vector is small? > And what do you mean by "lifetime of HeapSet"? Isn't the lifetime the same as the scope or owner element? Or do you mean the lifetime of the members of the hash set? If we have a list as member variable, elements in list is lasting until callback destroyed == GC will scan possibly dead element. > > I guess we have other users of UniqueSpellCheckeRequester, not sure. > > ... Do we? To me, it seems to be only an implementation detail of hot mode so there's no need to make it more than 5 LOC... I think using class encapsulate implementation detail. At first glance of your patch, I need to figure out lifetime of the list and another use the list, .e.g collecting then processing later for some how. In other word, it is ambiguous.
The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/06 at 08:20:35, yosin wrote: > On 2017/03/06 at 06:56:39, xiaochengh wrote: > > On 2017/03/06 at 06:46:52, yosin wrote: > > > On 2017/03/06 at 06:39:36, xiaochengh wrote: > > > > On 2017/03/06 at 04:39:15, yosin wrote: > > > > > https://codereview.chromium.org/2720193002/diff/80001/third_party/WebKit/Sour... > > > > > File third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp (right): > > > > > > > > > > https://codereview.chromium.org/2720193002/diff/80001/third_party/WebKit/Sour... > > > > > third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp:208: HeapVector<Member<Element>>* processedRootEditables) { > > > > > On 2017/03/06 at 04:17:31, yosin_UTC9 wrote: > > > > > > On 2017/03/03 at 14:31:52, Xiaocheng wrote: > > > > > > > On 2017/03/03 at 08:55:18, yosin_UTC9 wrote: > > > > > > > > Let's use HeapHashSet<Member<Element>> > > > > > > > > > > > > > > I don't expect checking a lot of root editables in hot mode. In such case, a vector shouldn't cause any performance issue, but is much lighter than a hash set. > > > > > > > > > > > > > > Anyway, talk is cheap. I prefer adding a UMA of the number of distinct root editables checked in each hot mode invocation, and let data decide whether the de-duplication process should be more scalable. > > > > > > > > > > > > > > > > > > > > > > > I prefer to have a class SpellCheckRequestScope, or another good name, which doesn't request for already request element, > > > > > > > > rather than pass processed element list. > > > > > > > > > > > > > > > > class SpellCheckRequestScope { > > > > > > > > STACK_ALLOCATE() > > > > > > > > public: > > > > > > > > explicit SpellCheckRequestScope(SpellCheckeRequester*); > > > > > > > > void requestCheckingForRootEditableInHotMode(...) { > > > > > > > > ... > > > > > > > > if (m_requester.count(element)) return; > > > > > > > > ... > > > > > > > > m_spellCheckRequeset.requestCheckingFor(request); > > > > > > > > } > > > > > > > > private: > > > > > > > > SpellCheckReqter& m_requester; > > > > > > > > HeapHashSet<Member<Element>> m_requestedEleemnt; > > > > > > > > } > > > > > > > > > > > > > > Sorry I don't get it... > > > > > > > > > > > > > > What's the benefit of introducing a scope here? In my understanding, we introduce scope only in two scenarios: > > > > > > > - To pass local variables deep into the call stack > > > > > > > - To do some work in the dtor of the scope > > > > > > > > > > > > > > We are in neither case here. > > > > > > > > > > > > This is another use case of scope. In this case, scope manages list of requested elements to avoid multiple request > > > > > > on element. > > > > > > > > > > > > In your itemized list, dtor of this scope frees memory of list of element, HeapVector/HeapSet. > > > > > > > > > > Naming wise? If "Scope" makes you confusing. We call |UniqueSpellCheckeRequester| or another name which tells we don't > > > > > make multiple requests for same element. > > > > > > > > > > For me passing "temporary" list to function is implementation details, like: > > > > > > > > > > // Implement recursive function as tail-recursive. > > > > > int fact(n, int acc =1) { > > > > > if (n <= 1) return acc; > > > > > return fact(n - 1, acc * n); > > > > > } > > > > > > > > > > where |acc| is an implementation detail, we don't want callers don't care. > > > > > > > > OK, hiding implementation details makes sense. > > > > > > > > I think maybe it's better to just store the vector as a member of IdleSpellCheckCallback. Introducing a scope still seems unnecessarily complicated... > > > > > > I think we should avoid to make HeapSet(instead of HeapVector) as member of IdleSpellCheckCallback. Lifetime of HeapSet is longer than needed and we don't > > > want to explicitly clear it at end of use. > > > > I still prefer HeapVector than HeapSet. Anyway, this should be decided by a UMA. > Just curiosity, why do you prefer |HeapVector|? For to save memory? Do we expect the size of vector is small? > Right. I don't expect checking a lot of editables in a hot mode invocation. > > > And what do you mean by "lifetime of HeapSet"? Isn't the lifetime the same as the scope or owner element? Or do you mean the lifetime of the members of the hash set? > If we have a list as member variable, elements in list is lasting until callback destroyed == GC will scan possibly dead element. > > > > I guess we have other users of UniqueSpellCheckeRequester, not sure. > > > > ... Do we? To me, it seems to be only an implementation detail of hot mode so there's no need to make it more than 5 LOC... > I think using class encapsulate implementation detail. At first glance of your patch, I need to figure out lifetime of the > list and another use the list, .e.g collecting then processing later for some how. In other word, it is ambiguous. Updated. PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
lgtm https://codereview.chromium.org/2720193002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/spellcheck/HotModeSpellCheckRequester.cpp (right): https://codereview.chromium.org/2720193002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/spellcheck/HotModeSpellCheckRequester.cpp:84: } nit: s"}"} // namespace" https://codereview.chromium.org/2720193002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/spellcheck/HotModeSpellCheckRequester.h (right): https://codereview.chromium.org/2720193002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/spellcheck/HotModeSpellCheckRequester.h:18: DISALLOW_COPY_AND_ASSIGN(HotModeSpellCheckRequester); nit: I think we should put |DISALLOW_COPY_AND_ASSIGN()| at end of class declaration to follow others. There are 91 occurrences in WebKit/Source, exceptional files are: - core/dom/custom/CustomElementDefinition.h - core/editing/spellcheck/IdleSpellCheckCallback.h
The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for reviewing! https://codereview.chromium.org/2720193002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/spellcheck/HotModeSpellCheckRequester.cpp (right): https://codereview.chromium.org/2720193002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/spellcheck/HotModeSpellCheckRequester.cpp:84: } On 2017/03/07 at 01:30:31, yosin_UTC9 wrote: > nit: s"}"} // namespace" Done. https://codereview.chromium.org/2720193002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/spellcheck/HotModeSpellCheckRequester.h (right): https://codereview.chromium.org/2720193002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/spellcheck/HotModeSpellCheckRequester.h:18: DISALLOW_COPY_AND_ASSIGN(HotModeSpellCheckRequester); On 2017/03/07 at 01:30:31, yosin_UTC9 wrote: > nit: I think we should put |DISALLOW_COPY_AND_ASSIGN()| at end of class declaration to follow others. > > There are 91 occurrences in WebKit/Source, exceptional files are: > - core/dom/custom/CustomElementDefinition.h > - core/editing/spellcheck/IdleSpellCheckCallback.h Done. We should also clean the above two. Btw, there is mixed usage of DISALLOW_COPY_AND_ASSIGN and WTF_MAKE_NONCOPYABLE. And WTF_MAKE_NONCOPYABLE is used at the beginning of class declaration. We should also unify them somehow...
The CQ bit was unchecked by xiaochengh@chromium.org
The CQ bit was checked by xiaochengh@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/2720193002/#ps160001 (title: "style fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/07 at 01:37:19, xiaochengh wrote: > Thanks for reviewing! > > https://codereview.chromium.org/2720193002/diff/140001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/editing/spellcheck/HotModeSpellCheckRequester.cpp (right): > > https://codereview.chromium.org/2720193002/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/spellcheck/HotModeSpellCheckRequester.cpp:84: } > On 2017/03/07 at 01:30:31, yosin_UTC9 wrote: > > nit: s"}"} // namespace" > > Done. > > https://codereview.chromium.org/2720193002/diff/140001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/editing/spellcheck/HotModeSpellCheckRequester.h (right): > > https://codereview.chromium.org/2720193002/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/spellcheck/HotModeSpellCheckRequester.h:18: DISALLOW_COPY_AND_ASSIGN(HotModeSpellCheckRequester); > On 2017/03/07 at 01:30:31, yosin_UTC9 wrote: > > nit: I think we should put |DISALLOW_COPY_AND_ASSIGN()| at end of class declaration to follow others. > > > > There are 91 occurrences in WebKit/Source, exceptional files are: > > - core/dom/custom/CustomElementDefinition.h > > - core/editing/spellcheck/IdleSpellCheckCallback.h > > Done. We should also clean the above two. > > Btw, there is mixed usage of DISALLOW_COPY_AND_ASSIGN and WTF_MAKE_NONCOPYABLE. And WTF_MAKE_NONCOPYABLE is used at the beginning of class declaration. > > We should also unify them somehow... Maybe done by global replace with clang plug-in? BTW, base/ macro doesn't have STACK_ALLOCATED()
On 2017/03/07 at 02:23:14, yosin wrote: > On 2017/03/07 at 01:37:19, xiaochengh wrote: > > Thanks for reviewing! > > > > https://codereview.chromium.org/2720193002/diff/140001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/editing/spellcheck/HotModeSpellCheckRequester.cpp (right): > > > > https://codereview.chromium.org/2720193002/diff/140001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/editing/spellcheck/HotModeSpellCheckRequester.cpp:84: } > > On 2017/03/07 at 01:30:31, yosin_UTC9 wrote: > > > nit: s"}"} // namespace" > > > > Done. > > > > https://codereview.chromium.org/2720193002/diff/140001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/editing/spellcheck/HotModeSpellCheckRequester.h (right): > > > > https://codereview.chromium.org/2720193002/diff/140001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/editing/spellcheck/HotModeSpellCheckRequester.h:18: DISALLOW_COPY_AND_ASSIGN(HotModeSpellCheckRequester); > > On 2017/03/07 at 01:30:31, yosin_UTC9 wrote: > > > nit: I think we should put |DISALLOW_COPY_AND_ASSIGN()| at end of class declaration to follow others. > > > > > > There are 91 occurrences in WebKit/Source, exceptional files are: > > > - core/dom/custom/CustomElementDefinition.h > > > - core/editing/spellcheck/IdleSpellCheckCallback.h > > > > Done. We should also clean the above two. > > > > Btw, there is mixed usage of DISALLOW_COPY_AND_ASSIGN and WTF_MAKE_NONCOPYABLE. And WTF_MAKE_NONCOPYABLE is used at the beginning of class declaration. > > > > We should also unify them somehow... > > Maybe done by global replace with clang plug-in? > BTW, base/ macro doesn't have STACK_ALLOCATED() base/ doesn't have DISALLOW_NEW, either...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by xiaochengh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1488905431213750,
"parent_rev": "5078f109f8bf4ab258c0309d58f4a9aa4fea9c07", "commit_rev":
"f3f30ff20c869bcff019e748dce273d3e2f29316"}
Message was sent while issue was closed.
Description was changed from ========== Implement hot mode invocation for idle time spell checker This patch implements IdleSpellCheckCallback::hotModeInvocation and its helper functions. BUG=517298 TEST=existing layout tests in editing/spelling ========== to ========== Implement hot mode invocation for idle time spell checker This patch implements IdleSpellCheckCallback::hotModeInvocation and its helper functions. BUG=517298 TEST=existing layout tests in editing/spelling Review-Url: https://codereview.chromium.org/2720193002 Cr-Commit-Position: refs/heads/master@{#455248} Committed: https://chromium.googlesource.com/chromium/src/+/f3f30ff20c869bcff019e748dce2... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/f3f30ff20c869bcff019e748dce2... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
