Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1678803002:
Introduce TextAccumulator for Progressive Extraction from Text Iterators (Closed)
DescriptionIntroduce TextAccumulator for Progressive Extraction from Text Iterators
This CL introduces |ForwardsTextAccumulator| and |BackwardsTextAccumulator|
which allow progressively accumulating text from a text iterator on a
large text node. It improves efficiency when we need to iteratively
accumulate and examine text from a text iterator.
This is part of a larger CL [1].
[1] https://codereview.chromium.org/1604783002
BUG=n/a
TEST=webkit_unit_tests --gtest_filter=*TextAccumulatorTest.*
Patch Set 1 #Patch Set 2 : #
Total comments: 15
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #
Total comments: 4
Patch Set 5 : #Patch Set 6 : Use plain accumulator #
Total comments: 2
Patch Set 7 : Revision on naming #
Total comments: 3
Depends on Patchset: Messages
Total messages: 18 (3 generated)
Description was changed from ========== Introduce TextAccumulator for Progressive Extraction from Text Iterators This CL introduces |ForwardsTextAccumulator| and |BackwardsTextAccumulator| which allow progressively accumulating text from a text iterator on a large text node. It improves efficiency when we need to iteratively accumulate and examine text from a text iterator. This is part of a larger CL [1]. BUG=n/a TEST=webkit_unit_tests --gtest_filter=*TextAccumulatorTest.* ========== to ========== Introduce TextAccumulator for Progressive Extraction from Text Iterators This CL introduces |ForwardsTextAccumulator| and |BackwardsTextAccumulator| which allow progressively accumulating text from a text iterator on a large text node. It improves efficiency when we need to iteratively accumulate and examine text from a text iterator. This is part of a larger CL [1]. [1] https://codereview.chromium.org/1604783002 BUG=n/a TEST=webkit_unit_tests --gtest_filter=*TextAccumulatorTest.* ==========
xiaochengh@chromium.org changed reviewers: + yosin@chromium.org
xiaochengh@chromium.org changed reviewers: + tkent@chromium.org
PTAL.
yosin@ should be the primary reviewer. https://codereview.chromium.org/1678803002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/iterators/TextAccumulator.h (right): https://codereview.chromium.org/1678803002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextAccumulator.h:14: template<typename Buffer> Please add a comment explaining what's this class.
https://codereview.chromium.org/1678803002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/iterators/TextAccumulator.h (right): https://codereview.chromium.org/1678803002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextAccumulator.h:14: template<typename Buffer> nit: clang-format inserts a space between |template| and |<|. You may want to have a space to reduce changes after clang-format applied. https://codereview.chromium.org/1678803002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextAccumulator.h:16: STACK_ALLOCATED(); Please include WTF_MAKE_NONCOPYABLE(TextAccumulator) from "wtf/Noncopyable.h" https://codereview.chromium.org/1678803002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextAccumulator.h:20: int accumulatedLengthInCurrentNode() const { return m_accumulatedInCurrentNode; } Since, |TextAccumulator| doesn't have concept about |Noe|. The name of |accumulatedLengthInCurrentNode| is non-sense in this class. https://codereview.chromium.org/1678803002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextAccumulator.h:25: void extract(const Iterator& it) |copyFromIterator()|? https://codereview.chromium.org/1678803002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextAccumulator.h:39: if (accumulateLength >= m_maxAccumulate) nit: Prefer early return style. https://codereview.chromium.org/1678803002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextAccumulator.h:44: void advance(Iterator* it) It seems this function does nothing when |m_accumulatedInCurrentNode < it->length()|. So, name |advance()| doesn't explain what this function does. https://codereview.chromium.org/1678803002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextAccumulator.h:50: if (m_accumulatedInCurrentNode == it->length()) { nit: Prefer early return style.
Thanks for your reviews! PTAL at patch 3. https://codereview.chromium.org/1678803002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/iterators/TextAccumulator.h (right): https://codereview.chromium.org/1678803002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextAccumulator.h:14: template<typename Buffer> tkent@: Sorry, do you mean |Buffer| or |TextAccumulator| by "this class"? On 2016/02/09 at 01:29:40, Yosi_UTC9 wrote: > nit: clang-format inserts a space between |template| and |<|. > You may want to have a space to reduce changes after clang-format applied. Done. I guess I should do the same to template member functions, right? https://codereview.chromium.org/1678803002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextAccumulator.h:16: STACK_ALLOCATED(); On 2016/02/09 at 01:29:40, Yosi_UTC9 wrote: > Please include WTF_MAKE_NONCOPYABLE(TextAccumulator) from "wtf/Noncopyable.h" Done. https://codereview.chromium.org/1678803002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextAccumulator.h:20: int accumulatedLengthInCurrentNode() const { return m_accumulatedInCurrentNode; } On 2016/02/09 at 01:29:40, Yosi_UTC9 wrote: > Since, |TextAccumulator| doesn't have concept about |Noe|. > The name of |accumulatedLengthInCurrentNode| is non-sense in this class. Renamed to |accumulatedSinceLastAdvance()|. To make the semantic complete, I also added a |bool| return type to |advanceIfNeeded()| indicating whether the text iterator is advanced (though we don't have any scenario that needs this return value yet). https://codereview.chromium.org/1678803002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextAccumulator.h:25: void extract(const Iterator& it) On 2016/02/09 at 01:29:40, Yosi_UTC9 wrote: > |copyFromIterator()|? Done https://codereview.chromium.org/1678803002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextAccumulator.h:39: if (accumulateLength >= m_maxAccumulate) On 2016/02/09 at 01:29:40, Yosi_UTC9 wrote: > nit: Prefer early return style. The current form looks simpler to me :) https://codereview.chromium.org/1678803002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextAccumulator.h:44: void advance(Iterator* it) On 2016/02/09 at 01:29:40, Yosi_UTC9 wrote: > It seems this function does nothing when |m_accumulatedInCurrentNode < it->length()|. > So, name |advance()| doesn't explain what this function does. Renamed to |advanceIfNeeded()|. https://codereview.chromium.org/1678803002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextAccumulator.h:50: if (m_accumulatedInCurrentNode == it->length()) { On 2016/02/09 at 01:29:40, Yosi_UTC9 wrote: > nit: Prefer early return style. Done.
https://codereview.chromium.org/1678803002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/iterators/TextAccumulator.h (right): https://codereview.chromium.org/1678803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextAccumulator.h:28: void copyFromIterator(const Iterator& it) We should use push model rather than pull model for |TextAccumulator|. TextAccumulator should concentrate accumulating characters and know nothing about detail of |TextIterator|. Clients of |TextAccumulator| should be: TextIterator it; TextAccumulator acc; while (!it.AtEnd()) { for (;;) { it.copyTo(acc, acc.size(), acc.capacity()); DoSomethingOnAcc(acc); if (acc.size() == it.length()) break; acc.grow(); } ++it; } Handling secured text should be in |TextIterator| rather than |TextAccumulator| to follow |TextIteratorEmitsOriginalText|, which is used for copying password field for copy to clipboard. // Copies characters from |start| to |end|(exclusive). If code unit at |end| is // trailing code unit of surrogate, we don't copy leading surrogate. TextIterator::copyTo(TextAccumulator* acc, int start, int end) { ASSERT(start < end); // We don't allow copy nothing. ASSERT(end - start >= 2); // |acc| should hold at least one surrogate pair. if (!emitOriginalText() && isInTextSurityMode()) { acc.pushCharacters('x', std::min(end - start, length()); return; } if (end >= length()) { // |acc| can holds everything. copyCodeUnitsTo(acc, start, length()); return; } if (isTrainlingSurrogatePair(characterAt(end))) { copyCodeUnitsTo(acc, start end - 1); return; } copyCodeUnitTo(acc, start, end); } https://codereview.chromium.org/1678803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextAccumulator.h:49: ASSERT(it); nit: We don't need to have null check assertion.
PTAL at Patch 4, which has switched to the push model as suggested.
https://codereview.chromium.org/1678803002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.cpp (right): https://codereview.chromium.org/1678803002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.cpp:450: void SimplifiedBackwardsTextIteratorAlgorithm<Strategy>::copyTextProgressivelyTo(BackwardsTextAccumulator* output, bool textSecurityAware) const |textSecurityAware()| should be managed by |TextIterator| with |TextIteratorEmitsOriginalText| by |TextIterator:: emitsOriginalText()|. Note: also we should avoid bool parameter. https://codereview.chromium.org/1678803002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.cpp:455: int tentativeLength = std::min(remainLength, output->getLimit()); It is better to pass number of character to read rather than using specific member function. The basic idea is like read(2), read(int fd, void* buffer, size_t buffer_size) Fumm... If we follow read(2), we can make - copyTo(UChar* destination, int source_start, int source_end), or - copyTo(UChar* destination, int source_start, int destination_size); This makes |TextIterator| independent from Accumulator.
https://codereview.chromium.org/1678803002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.cpp (right): https://codereview.chromium.org/1678803002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.cpp:450: void SimplifiedBackwardsTextIteratorAlgorithm<Strategy>::copyTextProgressivelyTo(BackwardsTextAccumulator* output, bool textSecurityAware) const On 2016/02/10 at 08:27:53, Yosi_UTC9 wrote: > |textSecurityAware()| should be managed by |TextIterator| with |TextIteratorEmitsOriginalText| by |TextIterator:: emitsOriginalText()|. > Note: also we should avoid bool parameter. I'll upload another patch for it. https://codereview.chromium.org/1678803002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.cpp:455: int tentativeLength = std::min(remainLength, output->getLimit()); On 2016/02/10 at 08:27:53, Yosi_UTC9 wrote: > It is better to pass number of character to read rather than using specific member function. > The basic idea is like read(2), read(int fd, void* buffer, size_t buffer_size) > > Fumm... If we follow read(2), we can make > - copyTo(UChar* destination, int source_start, int source_end), or > - copyTo(UChar* destination, int source_start, int destination_size); > > This makes |TextIterator| independent from Accumulator. Sorry I don't get it. A tricky thing about Accumulator is that we can't determine the number of characters to copy at the beginning. We need to first compare with Accumulator's limit, and then adjust with the text iterator to avoid breaking surrogates. Besides, passing a UCHAR* destination exposes the memory management details that should be handled by TextBuffer --- and we cannot determine this destination due to vector reallocation, unless we want to expose more details from TextBuffer. Do we really want to do that?
On 2016/02/10 at 08:42:13, xiaochengh wrote: > https://codereview.chromium.org/1678803002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.cpp (right): > > https://codereview.chromium.org/1678803002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.cpp:450: void SimplifiedBackwardsTextIteratorAlgorithm<Strategy>::copyTextProgressivelyTo(BackwardsTextAccumulator* output, bool textSecurityAware) const > On 2016/02/10 at 08:27:53, Yosi_UTC9 wrote: > > |textSecurityAware()| should be managed by |TextIterator| with |TextIteratorEmitsOriginalText| by |TextIterator:: emitsOriginalText()|. > > Note: also we should avoid bool parameter. > > I'll upload another patch for it. > > https://codereview.chromium.org/1678803002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.cpp:455: int tentativeLength = std::min(remainLength, output->getLimit()); > On 2016/02/10 at 08:27:53, Yosi_UTC9 wrote: > > It is better to pass number of character to read rather than using specific member function. > > The basic idea is like read(2), read(int fd, void* buffer, size_t buffer_size) > > > > Fumm... If we follow read(2), we can make > > - copyTo(UChar* destination, int source_start, int source_end), or > > - copyTo(UChar* destination, int source_start, int destination_size); > > > > This makes |TextIterator| independent from Accumulator. > > Sorry I don't get it. > > A tricky thing about Accumulator is that we can't determine the number of characters to copy at the beginning. We need to first compare with Accumulator's limit, and then adjust with the text iterator to avoid breaking surrogates. We know the size of buffer, e.g. |output->getLimit()| See comment #8. It support surrogate pair. > Besides, passing a UCHAR* destination exposes the memory management details that should be handled by TextBuffer --- and we cannot determine this destination due to vector reallocation, unless we want to expose more details from TextBuffer. Do we really want to do that? It should be OK for performance reason. |std::vector<T>| had |T* data()|, |size_t size()| and |size_t capacity()|. What we want to hide is backward filling of buffer and *highlight* of copy function is partial copy. #8 hides details of buffer implementation except for it has |size()| and |capacity()|, |grow()|, |enlarge()| is better name?, like |std::vector<T>|.
On 2016/02/10 at 08:56:11, Yosi_UTC9 wrote: > On 2016/02/10 at 08:42:13, xiaochengh wrote: > > https://codereview.chromium.org/1678803002/diff/60001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.cpp (right): > > > > https://codereview.chromium.org/1678803002/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.cpp:450: void SimplifiedBackwardsTextIteratorAlgorithm<Strategy>::copyTextProgressivelyTo(BackwardsTextAccumulator* output, bool textSecurityAware) const > > On 2016/02/10 at 08:27:53, Yosi_UTC9 wrote: > > > |textSecurityAware()| should be managed by |TextIterator| with |TextIteratorEmitsOriginalText| by |TextIterator:: emitsOriginalText()|. > > > Note: also we should avoid bool parameter. > > > > I'll upload another patch for it. > > > > https://codereview.chromium.org/1678803002/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.cpp:455: int tentativeLength = std::min(remainLength, output->getLimit()); > > On 2016/02/10 at 08:27:53, Yosi_UTC9 wrote: > > > It is better to pass number of character to read rather than using specific member function. > > > The basic idea is like read(2), read(int fd, void* buffer, size_t buffer_size) > > > > > > Fumm... If we follow read(2), we can make > > > - copyTo(UChar* destination, int source_start, int source_end), or > > > - copyTo(UChar* destination, int source_start, int destination_size); > > > > > > This makes |TextIterator| independent from Accumulator. > > > > Sorry I don't get it. > > > > A tricky thing about Accumulator is that we can't determine the number of characters to copy at the beginning. We need to first compare with Accumulator's limit, and then adjust with the text iterator to avoid breaking surrogates. > > We know the size of buffer, e.g. |output->getLimit()| > See comment #8. It support surrogate pair. > > > > Besides, passing a UCHAR* destination exposes the memory management details that should be handled by TextBuffer --- and we cannot determine this destination due to vector reallocation, unless we want to expose more details from TextBuffer. Do we really want to do that? > > It should be OK for performance reason. |std::vector<T>| had |T* data()|, |size_t size()| and |size_t capacity()|. > What we want to hide is backward filling of buffer and *highlight* of copy function is partial copy. > > #8 hides details of buffer implementation except for it has |size()| and |capacity()|, |grow()|, |enlarge()| is better name?, like |std::vector<T>|. In other words, we prefer - simple API, - no new convention, - follow exiting pattern.
PTAL at Patch 6. The previous discussion suggests decoupling |Accumulator| from |TextIterator|. I think we can go further in this direction and even decouple |Accumulator| from |TextBuffer|. Now |Accumulator| is nothing but a simple holder of two values. For the text-security-related topic, the replacement of bullets by 'x' is useful only in the two boundary finding functions. So it shouldn't be placed in any of |TextIterator|, |TextBuffer| or |Accumulator|. Instead, just put it in the two functions as an ad-hoc adaption for ICU is fine.
https://codereview.chromium.org/1678803002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/iterators/ExponentialAccumulator.h (right): https://codereview.chromium.org/1678803002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/iterators/ExponentialAccumulator.h:17: int getValue() const { return m_value; } In Blink naming convention, we don't have "get" prefix. So, just use |value()| and |limit()|. BTW, it is better to use |size()| and |capacity()| to follow STL.
PTAL at patch 7. https://codereview.chromium.org/1678803002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/iterators/ExponentialAccumulator.h (right): https://codereview.chromium.org/1678803002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/iterators/ExponentialAccumulator.h:17: int getValue() const { return m_value; } On 2016/02/12 at 08:07:37, Yosi_UTC9 wrote: > In Blink naming convention, we don't have "get" prefix. So, just use |value()| and |limit()|. > > BTW, it is better to use |size()| and |capacity()| to follow STL. Done.
https://codereview.chromium.org/1678803002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/iterators/ExponentialAccumulator.h (right): https://codereview.chromium.org/1678803002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/iterators/ExponentialAccumulator.h:8: #include "core/CoreExport.h" nit: Since nothing to export, we don't need to include "CoreExport.h" https://codereview.chromium.org/1678803002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/iterators/ExponentialAccumulator.h:12: class ExponentialAccumulator { |ExponentialAccumulator| does accumulate nothing. It is counter. Do you want to make this class as base class of |BackwardsTextBuffer| and |ForwardsTextBuffer|? Or template class? template <typename TextBuffer> class ExponentialAccumulator { const UChar* data() const { return m_buffer.data() } int capacity() const { return m_buffer.capacity(); } int size() const { return m_buffer.size(); } void push(TextIterator* it) { int accumulated = it.copyTextTo(&m_buffer, size(), capacity()); m_size += accumulated; } void enlarge() { m_buffer.enlarge(); } TextBuffer m_buffer; }; static VisiblePositionTemplate<Strategy> previousBoundary(...) { ... ExponentialAccumulator<BackwardsTextBuffer> accumulator; ... if (!inTextSecurityMode) { while (!next && accumulator.size() < it.length()) { accumulator.push(&it); next = searchFunction(accumulator.data(), accumulator.size(), accumulaotr.size() - suffixLength, MayHaveMoreContext, needMoreContext); if (next) break; // |it| hold characters more than |accumulator| buffer, we enlarge |accumulator| to hold more characters. accumulator.enlarge(); } } else { accumulator.pushCharacters('x', it.length()); next = 0; } it.advance(); accumulator.rest(); } https://codereview.chromium.org/1678803002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/iterators/ExponentialAccumulator.h:20: void grow(int delta) { m_size += delta; m_capacity += delta; } nit: enlarge() is better name? nit: Don't have two statements in a line.
I've been struggling with how much information has to leak from the Accumulator
class. On one hand, we don't want to couple it with text iterators; On the other
hand, letting each client manually call |enlarge()| after each push doesn't seem
very good, either. Patch 7 is an experiment where the Accumulator doesn't manage
buffer at all, but it doesn't look good.
In fact, I realized that this accumulator class may not be necessary at all.
Given that |WTFVector| maintains a capacity that grows exponentially every time
a reallocation occurs, it is already doing exactly the progressive accumulation
that we want, and I'm just reinventing the wheel (for such a long time!). Each
client only needs to maintain an |int| counter as follows:
In TextBufferBase.h:
class TextBufferBase {
public:
size_t capacity() const { return m_buffer.capacity(); }
...
};
In VisibleUnits.cpp:
static VisiblePositionTemplate<Strategy> previousBoundary(...) {
...
BackwardsTextBuffer string;
int runLength = 0;
...
if (!inTextSecurityMode) {
while (!next && runLength < it.length()) {
runLength += it.copyTextTo(&string, runLength, string.capacity());
next = searchFunction(string.data(), string.size(), string.size() -
suffixLength, MayHaveMoreContext, needMoreContext);
if (next)
break;
}
} else {
string.pushCharacters('x', it.length());
next = 0;
}
it.advance();
runLength = 0();
}
On 2016/02/15 at 05:58:30, yosin wrote:
>
https://codereview.chromium.org/1678803002/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/editing/iterators/ExponentialAccumulator.h
(right):
>
>
https://codereview.chromium.org/1678803002/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/editing/iterators/ExponentialAccumulator.h:8:
#include "core/CoreExport.h"
> nit: Since nothing to export, we don't need to include "CoreExport.h"
>
>
https://codereview.chromium.org/1678803002/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/editing/iterators/ExponentialAccumulator.h:12:
class ExponentialAccumulator {
> |ExponentialAccumulator| does accumulate nothing. It is counter.
> Do you want to make this class as base class of |BackwardsTextBuffer| and
|ForwardsTextBuffer|?
>
> Or template class?
>
> template <typename TextBuffer>
> class ExponentialAccumulator {
> const UChar* data() const { return m_buffer.data() }
> int capacity() const { return m_buffer.capacity(); }
> int size() const { return m_buffer.size(); }
>
> void push(TextIterator* it) {
> int accumulated = it.copyTextTo(&m_buffer, size(), capacity());
> m_size += accumulated;
> }
>
> void enlarge() { m_buffer.enlarge(); }
>
> TextBuffer m_buffer;
> };
>
> static VisiblePositionTemplate<Strategy> previousBoundary(...) {
> ...
> ExponentialAccumulator<BackwardsTextBuffer> accumulator;
> ...
>
> if (!inTextSecurityMode) {
> while (!next && accumulator.size() < it.length()) {
> accumulator.push(&it);
> next = searchFunction(accumulator.data(), accumulator.size(),
accumulaotr.size() - suffixLength, MayHaveMoreContext, needMoreContext);
> if (next)
> break;
> // |it| hold characters more than |accumulator| buffer, we enlarge
|accumulator| to hold more characters.
> accumulator.enlarge();
> }
> } else {
> accumulator.pushCharacters('x', it.length());
> next = 0;
> }
> it.advance();
> accumulator.rest();
> }
>
>
https://codereview.chromium.org/1678803002/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/editing/iterators/ExponentialAccumulator.h:20:
void grow(int delta) { m_size += delta; m_capacity += delta; }
> nit: enlarge() is better name?
> nit: Don't have two statements in a line.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
