Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2414323002:
Move AlreadySpellCheckedFlag from NodeFlags to ElementFlags (Closed)
|
Created:
4 years, 2 months ago by Xiaocheng Modified:
4 years, 2 months ago CC:
blink-reviews, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, groby+blinkspell_chromium.org, rwlbuis, sof, timvolodine Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove AlreadySpellCheckedFlag from NodeFlags to ElementFlags
This patch moves the flag from NodeFlags to ElementFlags to make
vacant space in NodeFlags. Currently, NodeFlags is full.
BUG=655939
Committed: https://crrev.com/2acb119a211ed79d61f76b41070a209375140ff3
Cr-Commit-Position: refs/heads/master@{#425291}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Revise comment #
Messages
Total messages: 22 (10 generated)
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...
Description was changed from ========== Move AlreadySpellCheckedFlag from NodeFlags to ElementFlags BUG= ========== to ========== Move AlreadySpellCheckedFlag from NodeFlags to ElementFlags BUG=655939 ==========
Description was changed from ========== Move AlreadySpellCheckedFlag from NodeFlags to ElementFlags BUG=655939 ========== to ========== Move AlreadySpellCheckedFlag from NodeFlags to ElementFlags This patch moves the flag from NodeFlags to ElementFlags to make vacant space in NodeFlags. Currently, NodeFlags is full. BUG=655939 ==========
xiaochengh@chromium.org changed reviewers: + tkent@chromium.org, yosin@chromium.org
lgtm Thanks for quick work!
lgtm
https://codereview.chromium.org/2414323002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Node.h (right): https://codereview.chromium.org/2414323002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Node.h:843: // 0 bits remaining. Update the comment.
https://codereview.chromium.org/2414323002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2414323002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:172: toElement(node).setAlreadySpellChecked(false); I realized this adds ElementRareData to every Elements in the document. not lgtm.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2414323002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2414323002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:172: toElement(node).setAlreadySpellChecked(false); On 2016/10/14 at 09:16:47, tkent wrote: > I realized this adds ElementRareData to every Elements in the document. not lgtm. Setting a false ElementFlag on doesn't create ElementRareData: void Element::setElementFlag(ElementFlags mask, bool value) { if (!hasRareData() && !value) return; ensureElementRareData().setElementFlag(mask, value); } https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Eleme...
https://codereview.chromium.org/2414323002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2414323002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:172: toElement(node).setAlreadySpellChecked(false); Oh, you're right! So, ElementRareData is added to only editing host elements, INPUT, and TEXTAREA. lgtm
The CQ bit was checked by xiaochengh@chromium.org
Thanks for the review. Modified the comment in Node.h into "1 bit remaining". Committing... https://codereview.chromium.org/2414323002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Node.h (right): https://codereview.chromium.org/2414323002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Node.h:843: // 0 bits remaining. On 2016/10/14 at 09:12:26, tkent wrote: > Update the comment. Done.
The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org, yosin@chromium.org Link to the patchset: https://codereview.chromium.org/2414323002/#ps20001 (title: "Revise comment")
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.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Move AlreadySpellCheckedFlag from NodeFlags to ElementFlags This patch moves the flag from NodeFlags to ElementFlags to make vacant space in NodeFlags. Currently, NodeFlags is full. BUG=655939 ========== to ========== Move AlreadySpellCheckedFlag from NodeFlags to ElementFlags This patch moves the flag from NodeFlags to ElementFlags to make vacant space in NodeFlags. Currently, NodeFlags is full. BUG=655939 Committed: https://crrev.com/2acb119a211ed79d61f76b41070a209375140ff3 Cr-Commit-Position: refs/heads/master@{#425291} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/2acb119a211ed79d61f76b41070a209375140ff3 Cr-Commit-Position: refs/heads/master@{#425291}
Message was sent while issue was closed.
Thank you so much for this xiaochengh@! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
