Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
357603003:
Add functions searching a word boundary without VisualPosition to HTMLTextFormControlElement. (Closed)
DescriptionAdd functions searching a word boundary without VisualPosition to
HTMLTextFormControlElement.
SpellChecker::respondToChangeClient is called by FrameSelection::SetSelection.
We use VisiblePosition to find word and sentence boundaries and it cause
synchronous layout.
However we implement text field as a shadow element including only div and text
nodes.
It means we can find a word boundaries in a text field without VisiblePosition.
This CL introduce new Position functions,
HTMLTextFormControlElement::startOfWord, endOfWord, startOfSentence and
endOfSentence.
They doesn't cause synchronous layout and will be used from
SpellChecher::respondToChangeClient to avoid synchronous layout.
Following is an implementation detail.
Source/core/html/HTMLTextFormControlElement.cpp/h:
- We introduce new Position functions, startOfWord, endOfWord, startOfSentence
and EndOfSentence.
- They doesn't cause synchronous layout and be used from SpellChecher.
- Each of them works same as a corresponding 'Visible' function in VisibleUnit
except they can be used to only text fields implemented as TextFormControlElement.
Source/core/html/HTMLTextFormControlElementTest.cpp:
- To confirm those 4 functions, we introduce the
HTMLTextFormControlElementTest::WordAndSentenceBoundary test.
- It confirms each of them returns a same Position to a corresponding 'Visible'
function in VisibleUnit on each Selection in a text field.
Source/core/rendering/RenderTreeAsText.cpp/h:
- We change visibility of the nodePosition function to debug DOM tree easily.
- The function is used in HTMLTextFormControlElementTest::WordAndSentenceBoundary test.
TEST=webkit_unit_tests --gtest_filter=*TextForm*
BUG=382809
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177636
Patch Set 1 : Fix compile errors #Patch Set 2 : Update #
Total comments: 3
Patch Set 3 : Remove the SpellChecker change #
Total comments: 12
Patch Set 4 : Fix nits #
Total comments: 4
Patch Set 5 : Fix nits #
Total comments: 38
Patch Set 6 : Update #
Total comments: 12
Patch Set 7 : Use StringBuilder #
Total comments: 8
Patch Set 8 : Change test cases. #
Total comments: 8
Patch Set 9 : Add comments #
Total comments: 2
Patch Set 10 : Wrap comment #
Messages
Total messages: 26 (0 generated)
I think it is better to create plaint text version of
{start,end}Of{Word,Sentence} rather than handling internal editable element,
e.g. "editing/PlainTextUnit.{cpp,h}"
When you normalize text nodes in inner editable element, the single text node
contains whole text in text field or text area. If so, you can call functions in
platform/text, directly, e.g.
endOfFirstWordBoundaryCotnext(textNode->data()->character16(),
textNode->data()->length());
https://codereview.chromium.org/357603003/diff/160001/Source/core/editing/Spe...
File Source/core/editing/SpellChecker.cpp (right):
https://codereview.chromium.org/357603003/diff/160001/Source/core/editing/Spe...
Source/core/editing/SpellChecker.cpp:67: bool isSelectionInTextForm(const
VisibleSelection& selection)
nit: It is better to call |isSelectionInTextFormControl|. "text form" isn't
reasonable.
https://codereview.chromium.org/357603003/diff/160001/Source/core/html/HTMLTe...
File Source/core/html/HTMLTextFormControlElement.cpp (right):
https://codereview.chromium.org/357603003/diff/160001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextFormControlElement.cpp:666: HTMLElement* innerText =
toHTMLElement(innerPosition.anchorNode());
nit: |innerText| is confusing name. How about |element|?
You can split this patch further, one for {start,end}Of{Word,Sentence} and
another for integrating into spell checker.
Split out the SpellChecker change.
On 2014/07/02 09:44:03, Yosi_UTC9 wrote:
> I think it is better to create plaint text version of
> {start,end}Of{Word,Sentence} rather than handling internal editable element,
> e.g. "editing/PlainTextUnit.{cpp,h}"
>
> When you normalize text nodes in inner editable element, the single text node
> contains whole text in text field or text area. If so, you can call functions
in
> platform/text, directly, e.g.
> endOfFirstWordBoundaryCotnext(textNode->data()->character16(),
> textNode->data()->length());
Not only HTMLTextFormControlElement::setInnerEditorValue but also
HTMLElement::setInnerText inserts Div elements to innerEditorElement.
I'm not sure why a text field consists of text nodes and Div elements.
Are there rendering performance issues for a long line text including line
breaks?
I guess tkent@ would know about that:
http://trac.webkit.org/changeset/87067
Anyway, I think that that refactoring to plain text is a not matter for now. If
we can, we will do it later :)
https://codereview.chromium.org/357603003/diff/160001/Source/core/html/HTMLTe...
File Source/core/html/HTMLTextFormControlElement.cpp (right):
https://codereview.chromium.org/357603003/diff/160001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextFormControlElement.cpp:666: HTMLElement* innerText =
toHTMLElement(innerPosition.anchorNode());
On 2014/07/02 09:44:03, Yosi_UTC9 wrote:
> nit: |innerText| is confusing name. How about |element|?
Done.
kent-san, do you know why text field consists of text node and div element? why not we can only build it with a text node containing '\n' linebreaks instead of Div elements? This questions is not so related this CL. It is just my curiousity. Thanks.
On 2014/07/03 02:17:37, yoichio wrote: > kent-san, do you know why text field consists of text node and div element? why > not we can only build it with a text node containing '\n' linebreaks instead of > Div elements? > > This questions is not so related this CL. > It is just my curiousity. Thanks. div[id="inner-editor"] is shadow host, which has -webkit-user-modify: read-write-plain-text-only
https://codereview.chromium.org/357603003/diff/180001/Source/core/html/HTMLTe... File Source/core/html/HTMLTextFormControlElement.cpp (right): https://codereview.chromium.org/357603003/diff/180001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:699: static Position findWordBoundary(const HTMLElement* innerText, const Position& startPosition, const Position endPosition, bool findStart) nit: |innerText| => |innerEditor| https://codereview.chromium.org/357603003/diff/180001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:780: for (; offset >= 0; offset--) { nit: |--offset| https://codereview.chromium.org/357603003/diff/180001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:834: static inline Position startOfInnerText(const HTMLTextFormControlElement* textForm) nit: |textForm| => |textFormControl| https://codereview.chromium.org/357603003/diff/180001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:841: HTMLTextFormControlElement* textForm = enclosingTextFormControl(position); nit: |textForm| => |textFormControl| https://codereview.chromium.org/357603003/diff/180001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:877: static Position endOfInnerText(const HTMLTextFormControlElement* textForm) nit: |textForm| => |textFormControl| https://codereview.chromium.org/357603003/diff/180001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:879: HTMLElement* innerText = textForm->innerEditorElement(); nit: |innerText|=>|innerEditor|
https://codereview.chromium.org/357603003/diff/180001/Source/core/html/HTMLTe... File Source/core/html/HTMLTextFormControlElement.cpp (right): https://codereview.chromium.org/357603003/diff/180001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:699: static Position findWordBoundary(const HTMLElement* innerText, const Position& startPosition, const Position endPosition, bool findStart) On 2014/07/03 04:05:10, Yosi_UTC9 wrote: > nit: |innerText| => |innerEditor| Done. https://codereview.chromium.org/357603003/diff/180001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:780: for (; offset >= 0; offset--) { On 2014/07/03 04:05:10, Yosi_UTC9 wrote: > nit: |--offset| Done. https://codereview.chromium.org/357603003/diff/180001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:834: static inline Position startOfInnerText(const HTMLTextFormControlElement* textForm) On 2014/07/03 04:05:10, Yosi_UTC9 wrote: > nit: |textForm| => |textFormControl| Done. https://codereview.chromium.org/357603003/diff/180001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:841: HTMLTextFormControlElement* textForm = enclosingTextFormControl(position); On 2014/07/03 04:05:10, Yosi_UTC9 wrote: > nit: |textForm| => |textFormControl| Done. https://codereview.chromium.org/357603003/diff/180001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:877: static Position endOfInnerText(const HTMLTextFormControlElement* textForm) On 2014/07/03 04:05:10, Yosi_UTC9 wrote: > nit: |textForm| => |textFormControl| Done. https://codereview.chromium.org/357603003/diff/180001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:879: HTMLElement* innerText = textForm->innerEditorElement(); On 2014/07/03 04:05:10, Yosi_UTC9 wrote: > nit: |innerText|=>|innerEditor| Done.
LGTM with tiny nit. https://codereview.chromium.org/357603003/diff/200001/Source/core/html/HTMLTe... File Source/core/html/HTMLTextFormControlElement.cpp (right): https://codereview.chromium.org/357603003/diff/200001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:675: { nit: We don't need "{}" here. https://codereview.chromium.org/357603003/diff/200001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:680: { nit: We don't need "{}" here.
https://codereview.chromium.org/357603003/diff/200001/Source/core/html/HTMLTe... File Source/core/html/HTMLTextFormControlElement.cpp (right): https://codereview.chromium.org/357603003/diff/200001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:675: { On 2014/07/03 05:17:07, Yosi_UTC9 wrote: > nit: We don't need "{}" here. Done. https://codereview.chromium.org/357603003/diff/200001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:680: { On 2014/07/03 05:17:07, Yosi_UTC9 wrote: > nit: We don't need "{}" here. Done.
Ran out of time. Let me publish the comments for the part I have looked at so far. https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... File Source/core/html/HTMLTextFormControlElement.cpp (right): https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:666: HTMLElement* element = toHTMLElement(innerPosition.anchorNode()); What if the anchor node isn't an HTMLElement? https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:675: offset = std::max(0, std::min(innerPosition.computeOffsetInContainerNode(), (int)childNodes->length())); 1. Can the second argument of max() become negative? 2. nit: Prefer static_cast<> over C-style cast. https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:680: default: What about other Position types? https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:685: return Position(childNodes->item(childNodes->length() - 1), Position::PositionIsAfterAnchor); item(...) can be lastChild(). https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:694: static Position findWordBoundary(const HTMLElement* innerEditor, const Position& startPosition, const Position endPosition, bool findStart) The type of the third argument should be "const Position&". https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:700: for (Node* node = startPosition.anchorNode(); node; node = NodeTraversal::next(*node, innerEditor)) { If startPosition is of type AfterAnchor or AfterChildren, this does not work correctly. https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:702: bool isEndNode = node == endPosition.anchorNode(); These booleans are smelly, too, if they are of type BeforeAnchor or AfterAnchor (these positions are basically anchored by anchorNode()->parentNode()). https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:708: characters.append(text->data()[offset]); This loop looks error-prone. If you use StringBuilder instead of Vector<UChar>. you can make use of StringBuilder::append() which supports adding a substring of a String. https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:732: unsigned restOffset = findStart ? start : end; nit: "rest" isn't an adjective. It's a noun. You probably meant "remaining". https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... File Source/core/html/HTMLTextFormControlElement.h (right): https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.h:93: static Position endOfSentence(const Position&); nit: a blank line is needed before "protected". https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... File Source/core/html/HTMLTextFormControlElementTest.cpp (right): https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElementTest.cpp:21: using namespace blink; Is there anything under blink namespace in this file? https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElementTest.cpp:67: void testFunctionEquality(const Position& position, PositionFunction positionFunction, VisblePositionFunction visibleFunction) nit: "Equality" is not a right word to compare the functionality of different functions. It should probably be "equivalence". https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElementTest.cpp:105: innerText->appendChild(Text::create(document(), "xt form.")); Is it possible for a user to create this weird DOM structure without access to UA shadows? If such structure can't happen in the wild, this test would be pointless. https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElementTest.cpp:109: Position pos = document().frame()->selection().start(); Please avoid using a abbreviated word for variables. https://codereview.chromium.org/357603003/diff/220001/Source/core/rendering/R... File Source/core/rendering/RenderTreeAsText.h (right): https://codereview.chromium.org/357603003/diff/220001/Source/core/rendering/R... Source/core/rendering/RenderTreeAsText.h:40: class Node; nit: Please arrange the forward declarations in the alphabetical order. (Well, LocalFrame and LayoutRect are not in the correct order, but I assume that's just a mistake.) https://codereview.chromium.org/357603003/diff/220001/Source/core/rendering/R... Source/core/rendering/RenderTreeAsText.h:76: String nodePosition(Node*); Generally speaking, it is a bad idea to define a function which has such a generic name in the namespace level scope. I have several suggestions on naming: * Please make sure the function returns a String describing the position, such as "nodePositionAsString". * Please put "ForTesting" to the function name to make it clear that the function should only be used from tests.
https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... File Source/core/html/HTMLTextFormControlElement.cpp (right): https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:666: HTMLElement* element = toHTMLElement(innerPosition.anchorNode()); On 2014/07/03 08:39:27, Yuta Kitamura wrote: > What if the anchor node isn't an HTMLElement? This function is called with only innerEditorElement. https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:675: offset = std::max(0, std::min(innerPosition.computeOffsetInContainerNode(), (int)childNodes->length())); On 2014/07/03 08:39:27, Yuta Kitamura wrote: > 1. Can the second argument of max() become negative? Sorry, This must be offsetInContainerNode(), which returns just m_offset. > 2. nit: Prefer static_cast<> over C-style cast. Done https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:680: default: On 2014/07/03 08:39:27, Yuta Kitamura wrote: > What about other Position types? PositionIsBeforeChildren means offset is 0. In our use cases, the anchorType can not be PositionIsBeforeAnchor and PositionIsAfterAnchor. https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:685: return Position(childNodes->item(childNodes->length() - 1), Position::PositionIsAfterAnchor); On 2014/07/03 08:39:27, Yuta Kitamura wrote: > item(...) can be lastChild(). Done. https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:694: static Position findWordBoundary(const HTMLElement* innerEditor, const Position& startPosition, const Position endPosition, bool findStart) On 2014/07/03 08:39:27, Yuta Kitamura wrote: > The type of the third argument should be "const Position&". Done. https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:700: for (Node* node = startPosition.anchorNode(); node; node = NodeTraversal::next(*node, innerEditor)) { On 2014/07/03 08:39:27, Yuta Kitamura wrote: > If startPosition is of type AfterAnchor or AfterChildren, this > does not work correctly. A goal of this loop is to collect all strings in text nodes. The start/end position must be of type PositionIsOffsetInAnchor so I add assertions. https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:702: bool isEndNode = node == endPosition.anchorNode(); On 2014/07/03 08:39:27, Yuta Kitamura wrote: > These booleans are smelly, too, if they are of type BeforeAnchor > or AfterAnchor (these positions are basically anchored by > anchorNode()->parentNode()). ditto https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:708: characters.append(text->data()[offset]); On 2014/07/03 08:39:27, Yuta Kitamura wrote: > This loop looks error-prone. > > If you use StringBuilder instead of Vector<UChar>. you can > make use of StringBuilder::append() which supports adding > a substring of a String. Since findWordBoundary takes UChar*, we need to convert strings if it is 8bit. text->data()[offset] also do that conversion. https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:732: unsigned restOffset = findStart ? start : end; On 2014/07/03 08:39:27, Yuta Kitamura wrote: > nit: "rest" isn't an adjective. It's a noun. You probably > meant "remaining". Done. https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... File Source/core/html/HTMLTextFormControlElement.h (right): https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.h:93: static Position endOfSentence(const Position&); On 2014/07/03 08:39:27, Yuta Kitamura wrote: > nit: a blank line is needed before "protected". Done. https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... File Source/core/html/HTMLTextFormControlElementTest.cpp (right): https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElementTest.cpp:21: using namespace blink; On 2014/07/03 08:39:27, Yuta Kitamura wrote: > Is there anything under blink namespace in this file? Nothing. Thanks. https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElementTest.cpp:67: void testFunctionEquality(const Position& position, PositionFunction positionFunction, VisblePositionFunction visibleFunction) On 2014/07/03 08:39:27, Yuta Kitamura wrote: > nit: "Equality" is not a right word to compare the functionality > of different functions. It should probably be "equivalence". Done. https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElementTest.cpp:105: innerText->appendChild(Text::create(document(), "xt form.")); On 2014/07/03 08:39:27, Yuta Kitamura wrote: > Is it possible for a user to create this weird DOM structure > without access to UA shadows? If such structure can't happen > in the wild, this test would be pointless. I can't create this by only HTML functions, but at least this tree contains possible structures. I think this is useful to confirm coverage and stability of target functions. https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElementTest.cpp:109: Position pos = document().frame()->selection().start(); On 2014/07/03 08:39:27, Yuta Kitamura wrote: > Please avoid using a abbreviated word for variables. Done. https://codereview.chromium.org/357603003/diff/220001/Source/core/rendering/R... File Source/core/rendering/RenderTreeAsText.h (right): https://codereview.chromium.org/357603003/diff/220001/Source/core/rendering/R... Source/core/rendering/RenderTreeAsText.h:40: class Node; On 2014/07/03 08:39:27, Yuta Kitamura wrote: > nit: Please arrange the forward declarations in the > alphabetical order. (Well, LocalFrame and LayoutRect > are not in the correct order, but I assume that's > just a mistake.) Done. https://codereview.chromium.org/357603003/diff/220001/Source/core/rendering/R... Source/core/rendering/RenderTreeAsText.h:76: String nodePosition(Node*); On 2014/07/03 08:39:27, Yuta Kitamura wrote: > Generally speaking, it is a bad idea to define a function > which has such a generic name in the namespace level scope. > > I have several suggestions on naming: > * Please make sure the function returns a String describing > the position, such as "nodePositionAsString". > * Please put "ForTesting" to the function name to make it > clear that the function should only be used from tests. Done.
https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... File Source/core/html/HTMLTextFormControlElement.cpp (right): https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:680: default: On 2014/07/04 01:29:43, yoichio wrote: > In our use cases, the anchorType can not be PositionIsBeforeAnchor and > PositionIsAfterAnchor. Then you should ASSERT that. It's not okay to have such an implicit precondition. https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:708: characters.append(text->data()[offset]); On 2014/07/04 01:29:44, yoichio wrote: > On 2014/07/03 08:39:27, Yuta Kitamura wrote: > > This loop looks error-prone. > > > > If you use StringBuilder instead of Vector<UChar>. you can > > make use of StringBuilder::append() which supports adding > > a substring of a String. > > Since findWordBoundary takes UChar*, we need to convert strings if it is 8bit. > text->data()[offset] also do that conversion. It's silly to write this kind of loop by hand. You should make use of libraries as much as possible. In this case, you can use StringBuilder to build a string and then use String::append() to extract the content into Vector<UChar>. (I think it's pretty obvious to have an overloaded version of findWordBoundary taking LChar*, though.)
https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... File Source/core/html/HTMLTextFormControlElement.cpp (right): https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:680: default: On 2014/07/04 05:23:34, Yuta Kitamura wrote: > On 2014/07/04 01:29:43, yoichio wrote: > > In our use cases, the anchorType can not be PositionIsBeforeAnchor and > > PositionIsAfterAnchor. > > Then you should ASSERT that. It's not okay to have > such an implicit precondition. Done. https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:708: characters.append(text->data()[offset]); On 2014/07/04 05:23:34, Yuta Kitamura wrote: > On 2014/07/04 01:29:44, yoichio wrote: > > On 2014/07/03 08:39:27, Yuta Kitamura wrote: > > > This loop looks error-prone. > > > > > > If you use StringBuilder instead of Vector<UChar>. you can > > > make use of StringBuilder::append() which supports adding > > > a substring of a String. > > > > Since findWordBoundary takes UChar*, we need to convert strings if it is 8bit. > > text->data()[offset] also do that conversion. > > It's silly to write this kind of loop by hand. You should make > use of libraries as much as possible. > > In this case, you can use StringBuilder to build a string and > then use String::append() to extract the content into Vector<UChar>. > > (I think it's pretty obvious to have an overloaded version of > findWordBoundary taking LChar*, though.) Done. Thanks! I have not gotten the way :)
I'm kinda hesitant to put such a fat editing-specific functionality into html/ classes... https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... File Source/core/html/HTMLTextFormControlElement.cpp (right): https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:666: HTMLElement* element = toHTMLElement(innerPosition.anchorNode()); On 2014/07/04 01:29:44, yoichio wrote: > On 2014/07/03 08:39:27, Yuta Kitamura wrote: > > What if the anchor node isn't an HTMLElement? > This function is called with only innerEditorElement. Then that should be ASSERTed. https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... File Source/core/html/HTMLTextFormControlElementTest.cpp (right): https://codereview.chromium.org/357603003/diff/220001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElementTest.cpp:105: innerText->appendChild(Text::create(document(), "xt form.")); On 2014/07/04 01:29:44, yoichio wrote: > On 2014/07/03 08:39:27, Yuta Kitamura wrote: > > Is it possible for a user to create this weird DOM structure > > without access to UA shadows? If such structure can't happen > > in the wild, this test would be pointless. > I can't create this by only HTML functions, but > at least this tree contains possible structures. > I think this is useful to confirm coverage and stability of target functions. I don't see a point in testing impossible situations. UA-shadows are governed by the user agent, and it is safe to assume they have some specific structure. Arbitrary DOM trees won't appear in them. It's okay to use inner elements to set up something that could happen in a real world, but I'm against the idea of constructing the DOM that can't happen in the wild. https://codereview.chromium.org/357603003/diff/240001/Source/core/html/HTMLTe... File Source/core/html/HTMLTextFormControlElement.cpp (right): https://codereview.chromium.org/357603003/diff/240001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:683: ASSERT_NOT_REACHED(); This assertion fails to ASSERT if the function exits at line 670. This is a precondition, so it should be placed in the beginning of the function. https://codereview.chromium.org/357603003/diff/240001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:699: static Position findWordBoundary(const HTMLElement* innerEditor, const Position& startPosition, const Position& endPosition, bool findStart) Prefer enums to bools on function parameters. The caller passing just "true" or "false" isn't very readable. https://codereview.chromium.org/357603003/diff/240001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:734: if (!findStart && characters.data()[0] == '\n') { ".data()" isn't necessary. https://codereview.chromium.org/357603003/diff/240001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:775: nit: Unnecessary blank line. https://codereview.chromium.org/357603003/diff/240001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:788: return offset; Use String::reverseFind(). https://codereview.chromium.org/357603003/diff/240001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:795: Node* prev = NodeTraversal::previous(node, innerEditor); Please don't use abbreviated word in names.
https://codereview.chromium.org/357603003/diff/260001/Source/core/html/HTMLTe... File Source/core/html/HTMLTextFormControlElement.cpp (right): https://codereview.chromium.org/357603003/diff/260001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:804: Text* textNode = toText(prev); nit: Could you remove |textNode| varaible? https://codereview.chromium.org/357603003/diff/260001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:868: } else if (isHTMLBRElement(node) && (!isPivotNode || pivotPosition.anchorType() == Position::PositionIsAfterAnchor)) { nit: If we move this if-statement before |if (node->isTextNode()|, we can use early return pattern. https://codereview.chromium.org/357603003/diff/260001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:904: bool isPivotNode = (node == pivotPosition.anchorNode()); nit: We don't have "()". https://codereview.chromium.org/357603003/diff/260001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:911: } else if (isHTMLBRElement(node)) { nit: If we move this if-statement before |if (node->isTextNode()|, we can use early return pattern.
yutak@, you might have seen a previous patch set and commented. https://codereview.chromium.org/357603003/diff/240001/Source/core/html/HTMLTe... File Source/core/html/HTMLTextFormControlElement.cpp (right): https://codereview.chromium.org/357603003/diff/240001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:683: ASSERT_NOT_REACHED(); On 2014/07/04 06:12:35, Yuta Kitamura wrote: > This assertion fails to ASSERT if the function exits at line > 670. This is a precondition, so it should be placed in the > beginning of the function. Done. https://codereview.chromium.org/357603003/diff/240001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:699: static Position findWordBoundary(const HTMLElement* innerEditor, const Position& startPosition, const Position& endPosition, bool findStart) On 2014/07/04 06:12:35, Yuta Kitamura wrote: > Prefer enums to bools on function parameters. The caller passing > just "true" or "false" isn't very readable. Done. https://codereview.chromium.org/357603003/diff/240001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:734: if (!findStart && characters.data()[0] == '\n') { On 2014/07/04 06:12:35, Yuta Kitamura wrote: > ".data()" isn't necessary. Done. https://codereview.chromium.org/357603003/diff/240001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:775: On 2014/07/04 06:12:35, Yuta Kitamura wrote: > nit: Unnecessary blank line. Done. https://codereview.chromium.org/357603003/diff/240001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:788: return offset; On 2014/07/04 06:12:35, Yuta Kitamura wrote: > Use String::reverseFind(). Done. Removed this function and inlined at call. https://codereview.chromium.org/357603003/diff/240001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:795: Node* prev = NodeTraversal::previous(node, innerEditor); On 2014/07/04 06:12:35, Yuta Kitamura wrote: > Please don't use abbreviated word in names. Done. https://codereview.chromium.org/357603003/diff/260001/Source/core/html/HTMLTe... File Source/core/html/HTMLTextFormControlElement.cpp (right): https://codereview.chromium.org/357603003/diff/260001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:804: Text* textNode = toText(prev); On 2014/07/04 06:14:50, Yosi_UTC9 wrote: > nit: Could you remove |textNode| varaible? Done. https://codereview.chromium.org/357603003/diff/260001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:868: } else if (isHTMLBRElement(node) && (!isPivotNode || pivotPosition.anchorType() == Position::PositionIsAfterAnchor)) { On 2014/07/04 06:14:50, Yosi_UTC9 wrote: > nit: If we move this if-statement before |if (node->isTextNode()|, we can use > early return pattern. Done. https://codereview.chromium.org/357603003/diff/260001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:904: bool isPivotNode = (node == pivotPosition.anchorNode()); On 2014/07/04 06:14:50, Yosi_UTC9 wrote: > nit: We don't have "()". Done. https://codereview.chromium.org/357603003/diff/260001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:911: } else if (isHTMLBRElement(node)) { On 2014/07/04 06:14:50, Yosi_UTC9 wrote: > nit: If we move this if-statement before |if (node->isTextNode()|, we can use > early return pattern. Done.
yutak@ is gardening in this week. tkent@, could you review as OWNER? Thanks.
https://codereview.chromium.org/357603003/diff/280001/Source/core/html/HTMLTe... File Source/core/html/HTMLTextFormControlElement.cpp (right): https://codereview.chromium.org/357603003/diff/280001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:713: // Trace text nodes. nit: We don't use the word 'trace' for such operation. Use 'traverse' instead. https://codereview.chromium.org/357603003/diff/280001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:732: if (!concatTexts.length()) nit: You may write |if (concatTexts.length() == 0)|. We changed the style rule recently. https://codereview.chromium.org/357603003/diff/280001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:793: if (previousNode->hasTagName(brTag)) This is inconsistent with other functions. They use isHTMLBRElement(). https://codereview.chromium.org/357603003/diff/280001/Source/core/html/HTMLTe... File Source/core/html/HTMLTextFormControlElement.h (right): https://codereview.chromium.org/357603003/diff/280001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.h:90: static Position startOfWord(const Position&); We should have comments about: - Why we need these functions - Precondition of the argument Position (it must point inside of a text form control?)
https://codereview.chromium.org/357603003/diff/280001/Source/core/html/HTMLTe... File Source/core/html/HTMLTextFormControlElement.cpp (right): https://codereview.chromium.org/357603003/diff/280001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:713: // Trace text nodes. On 2014/07/07 23:54:18, tkent wrote: > nit: We don't use the word 'trace' for such operation. Use 'traverse' instead. Done. https://codereview.chromium.org/357603003/diff/280001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:732: if (!concatTexts.length()) On 2014/07/07 23:54:18, tkent wrote: > nit: You may write |if (concatTexts.length() == 0)|. We changed the style rule > recently. Done. https://codereview.chromium.org/357603003/diff/280001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.cpp:793: if (previousNode->hasTagName(brTag)) On 2014/07/07 23:54:18, tkent wrote: > This is inconsistent with other functions. They use isHTMLBRElement(). Done. https://codereview.chromium.org/357603003/diff/280001/Source/core/html/HTMLTe... File Source/core/html/HTMLTextFormControlElement.h (right): https://codereview.chromium.org/357603003/diff/280001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.h:90: static Position startOfWord(const Position&); On 2014/07/07 23:54:18, tkent wrote: > We should have comments about: > - Why we need these functions > - Precondition of the argument Position (it must point inside of a text form > control?) Done.
lgtm. Please wrap long lines in the CL description in 80 columns. https://codereview.chromium.org/357603003/diff/300001/Source/core/html/HTMLTe... File Source/core/html/HTMLTextFormControlElement.h (right): https://codereview.chromium.org/357603003/diff/300001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.h:90: // These functions doesn't cause synchronous layout and will be used from SpellChecher for perfermance improvement. doesn't -> don't I think |will| is unnecessary. -> SpellChecker uses them to improve performance. Also, I recommend to wrap the comment in 80 columns.
The CQ bit was checked by yoichio@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoichio@chromium.org/357603003/320001
https://codereview.chromium.org/357603003/diff/300001/Source/core/html/HTMLTe... File Source/core/html/HTMLTextFormControlElement.h (right): https://codereview.chromium.org/357603003/diff/300001/Source/core/html/HTMLTe... Source/core/html/HTMLTextFormControlElement.h:90: // These functions doesn't cause synchronous layout and will be used from SpellChecher for perfermance improvement. On 2014/07/08 01:50:10, tkent wrote: > doesn't -> don't > I think |will| is unnecessary. -> SpellChecker uses them to improve > performance. > > Also, I recommend to wrap the comment in 80 columns. Done.
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/15802)
Message was sent while issue was closed.
Change committed as 177636 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
