Introduce TextBuffer as Output Type of copyTextTo() for Text Iterators

Introduce TextBuffer as Output Type of copyTextTo() for Text Iterators

For backwards text iterators, their |copyTextTo()| function prepends
text to the output buffer. It causes performance issues when repeatedly
prepending to vectors.

This CL introduces TextBuffer as the output class of |copyTextTo()|. The
new class wraps |WTF::Vector|, and can grow either forwards or backwards
according to its template parameter. It ensures linear time complexity
of |copyTextTo()| regardless of the direction of the text iterator.

This is part of a bigger CL [1].

[1] https://codereview.chromium.org/1604783002

BUG=n/a

[Xiaocheng, 2016-01-28 01:50:48]

Description was changed from

==========
Introduce TextBuffer as Output Type of copyTextTo() for Text Iterators

BUG=n/a
==========

to

==========
Introduce TextBuffer as Output Type of copyTextTo() for Text Iterators

This is part of a bigger CL [1].

[1] https://codereview.chromium.org/1604783002

BUG=n/a
==========

[Xiaocheng, 2016-01-28 01:50:48]

xiaochengh@chromium.org changed reviewers:
+ yosin@chromium.org

[Xiaocheng, 2016-01-28 01:54:31]

Description was changed from

==========
Introduce TextBuffer as Output Type of copyTextTo() for Text Iterators

This is part of a bigger CL [1].

[1] https://codereview.chromium.org/1604783002

BUG=n/a
==========

to

==========
Introduce TextBuffer as Output Type of copyTextTo() for Text Iterators

For backwards text iterators, their |copyTextTo()| function prepends
text to the output buffer. It causes performance issues when repeatedly
prepending to vectors.

This CL introduces TextBuffer as the output class of |copyTextTo()|. The
new class wraps |WTF::Vector|, and can grow either forwards or backwards
according to its template parameter. It ensures linear time complexity
of |copyTextTo()| regardless of the direction of the text iterator.

This is part of a bigger CL [1].

[1] https://codereview.chromium.org/1604783002

BUG=n/a
==========

[Xiaocheng, 2016-01-28 01:54:39]

PTAL.

[yosin_UTC9, 2016-01-28 04:28:21]

https://codereview.chromium.org/1647483003/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.cpp
(right):

https://codereview.chromium.org/1647483003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.cpp:437:
if (m_singleCharacterBuffer) {
We prefer early return style, e.g.:

if (m_singleCharacterBuffer) {
    output.push(m_singleCharacterBuffer);
    return;
}

const int offset = m_textOffset + m_textLength - position - copyLength;
if (m_textContainer.is8Bit()) {
    output.push(m_textContainer.characters8() + offset, copyLength);
    return;
}

output.push(m_textContainer.characters16() + offset, copyLength);

https://codereview.chromium.org/1647483003/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.h
(right):

https://codereview.chromium.org/1647483003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.h:65:
int copyTextTo(BackwardsTextBuffer& output, int position = 0) const;
nit: Let's avoid to use default parameter. If we have lots of call sites, it is
OK to have TODO.

https://codereview.chromium.org/1647483003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/iterators/TextBuffer.h (right):

https://codereview.chromium.org/1647483003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/iterators/TextBuffer.h:18: : m_size(0)
nit: Use Non-Static Class Member Initializers. We can write |size_t m_size = 0|

https://codereview.chromium.org/1647483003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/iterators/TextBuffer.h:26: CharType*
data() { return Direction::dataStart(m_buffer, m_size); }
Do we really need to allow clients to modify internal buffer of |TextBuffer|?

https://codereview.chromium.org/1647483003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/iterators/TextBuffer.h:30: const
CharType& operator[](size_t index) const { return data()[index]; }
We should have assertion for |index|

https://codereview.chromium.org/1647483003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/iterators/TextBuffer.h:34: void
push(CharType c, size_t length = 1)
Please avoid to use default parameter. |pushCharacters()| is better.
https://google.github.io/styleguide/cppguide.html#Default_Arguments

https://codereview.chromium.org/1647483003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/iterators/TextBuffer.h:50: 
nit: No need to have an extra blank line.

https://codereview.chromium.org/1647483003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/iterators/TextBuffer.h:68: // I don't
think |size_t| can introduce any performance issue, since
No need to have this comment.

https://codereview.chromium.org/1647483003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/iterators/TextBuffer.h:78: static
typename Vec::const_iterator dataStart(const Vec& vector, size_t)
It seems member functions in this class and |TextBufferBackwards| are random. At
a glance, it is hard to understand why |dataStart()| needs |size_t| and this
implementation ignores |size_t|.

It seems we don't need to use |TextBuffer| template or introduces
|TextBufferBase| to implement common things between |TextBufferForwards| and
|TextBufferBackwards|.

[Xiaocheng, 2016-01-28 07:04:05]

Thanks for the review. PTAL at Patch 2.

https://codereview.chromium.org/1647483003/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.cpp
(right):

https://codereview.chromium.org/1647483003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.cpp:437:
if (m_singleCharacterBuffer) {
On 2016/01/28 at 04:28:20, Yosi_UTC9 wrote:
> We prefer early return style, e.g.:
> 
> if (m_singleCharacterBuffer) {
>     output.push(m_singleCharacterBuffer);
>     return;
> }
> 
> const int offset = m_textOffset + m_textLength - position - copyLength;
> if (m_textContainer.is8Bit()) {
>     output.push(m_textContainer.characters8() + offset, copyLength);
>     return;
> }
> 
> output.push(m_textContainer.characters16() + offset, copyLength);

Done.

https://codereview.chromium.org/1647483003/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.h
(right):

https://codereview.chromium.org/1647483003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.h:65:
int copyTextTo(BackwardsTextBuffer& output, int position = 0) const;
On 2016/01/28 at 04:28:20, Yosi_UTC9 wrote:
> nit: Let's avoid to use default parameter. If we have lots of call sites, it
is OK to have TODO.

Let's do the cleanup in another patch.

https://codereview.chromium.org/1647483003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/iterators/TextBuffer.h (right):

https://codereview.chromium.org/1647483003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/iterators/TextBuffer.h:18: : m_size(0)
On 2016/01/28 at 04:28:21, Yosi_UTC9 wrote:
> nit: Use Non-Static Class Member Initializers. We can write |size_t m_size =
0|

Done.

https://codereview.chromium.org/1647483003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/iterators/TextBuffer.h:26: CharType*
data() { return Direction::dataStart(m_buffer, m_size); }
On 2016/01/28 at 04:28:21, Yosi_UTC9 wrote:
> Do we really need to allow clients to modify internal buffer of |TextBuffer|?

Nope. The non-const public getters are removed.

https://codereview.chromium.org/1647483003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/iterators/TextBuffer.h:30: const
CharType& operator[](size_t index) const { return data()[index]; }
On 2016/01/28 at 04:28:21, Yosi_UTC9 wrote:
> We should have assertion for |index|

Done.

https://codereview.chromium.org/1647483003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/iterators/TextBuffer.h:34: void
push(CharType c, size_t length = 1)
On 2016/01/28 at 04:28:21, Yosi_UTC9 wrote:
> Please avoid to use default parameter. |pushCharacters()| is better.
> https://google.github.io/styleguide/cppguide.html#Default_Arguments

Done.

https://codereview.chromium.org/1647483003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/iterators/TextBuffer.h:50: 
On 2016/01/28 at 04:28:20, Yosi_UTC9 wrote:
> nit: No need to have an extra blank line.

Done.

https://codereview.chromium.org/1647483003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/iterators/TextBuffer.h:68: // I don't
think |size_t| can introduce any performance issue, since
On 2016/01/28 at 04:28:21, Yosi_UTC9 wrote:
> No need to have this comment.

Done.

This comment is just for discussion purpose (with you), which it has served.

https://codereview.chromium.org/1647483003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/iterators/TextBuffer.h:78: static
typename Vec::const_iterator dataStart(const Vec& vector, size_t)
On 2016/01/28 at 04:28:21, Yosi_UTC9 wrote:
> It seems member functions in this class and |TextBufferBackwards| are random.
At a glance, it is hard to understand why |dataStart()| needs |size_t| and this
implementation ignores |size_t|.
> 
> It seems we don't need to use |TextBuffer| template or introduces
|TextBufferBase| to implement common things between |TextBufferForwards| and
|TextBufferBackwards|.

Yeah, it seems better to use inheritance instead of template.

[yosin_UTC9, 2016-01-28 07:41:13]

https://codereview.chromium.org/1647483003/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.cpp
(right):

https://codereview.chromium.org/1647483003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.cpp:438:
output.pushCharacter(m_singleCharacterBuffer, 1);
nit: s/pushCharacter/pushCharacters/

https://codereview.chromium.org/1647483003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.cpp:442:
if (m_textContainer.is8Bit())
nit: we prefer early return style.

https://codereview.chromium.org/1647483003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/iterators/TextBuffer.h (right):

https://codereview.chromium.org/1647483003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/iterators/TextBuffer.h:24: virtual const
CharType* data() const = 0;
It seems strange that template class has virtual function.

Independent classes seem simpler:

template <typename CharType, size_t inlineCapacity>
class ForwardTextBufferTemplate final {
    STACK_ALLOCATED();
public:
    const CharType* data() const { return m_buffer.begin();}

    void clear() { m_size = 0; }

    void pushCharacters(CharType ch, size_t length)
    {
        std::fill_n(ensureBuffer(length), length, ch);
    }

    template <typename T>
    void pushRange(const T* other, size_t length)
    {
        if (length == 0)
            return;
        std::copy(other, other + length, ensureBuffer(length));
    }

private:
    CharType* ensureBuffer(size_t demand)
    {
        size_t current = m_size;
        m_size += demand;
        if (m_size > m_buffer.size())
            grow(demand);
        return m_buffer.begin() + current;
    }

    void grow(size_t demand)
    {
        m_buffer.resize(oldCapacity + demand);
        m_buffer.resize(m_buffer.capacity());
    }

    Vector<CharType, inlineCapacity> m_buffer;
    size_t m_size = 0;
};


template <typename CharType, size_t inlineCapacity>
class BackwardTextBufferTemplate final {
    STACK_ALLOCATED();
public:
    const CharType* data() const { return m_buffer.begin();}

    void clear() { m_size = 0; }

    void pushCharacters(CharType ch, size_t length)
    {
        std::fill_n(ensureBuffer(length), length, ch);
    }

    template <typename T>
    void pushRange(const T* other, size_t length)
    {
        if (length == 0)
            return;
        std::copy(other, other + length, ensureBuffer(length));
    }

private:
    CharType* ensureBuffer(size_t demand)
    {
        size_t current = m_size;
        m_size += demand;
        if (m_size > m_buffer.size())
            grow(demand);
        return m_buffer.end() - current;
    }

    void grow(size_t demand)
    {
        m_buffer.resize(oldCapacity + demand);
        m_buffer.resize(m_buffer.capacity());
    }

    Vector<CharType, inlineCapacity> m_buffer;
    size_t m_size = 0;
};

https://codereview.chromium.org/1647483003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/iterators/TextBuffer.h:28: void
pushCharacter(CharType c, size_t length)
nit: s/pushCharacter/pushCharacters/
nit: Please avoid one character variable name.

[Xiaocheng, 2016-01-28 08:47:36]

PTAL at Patch 3.

https://codereview.chromium.org/1647483003/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.cpp
(right):

https://codereview.chromium.org/1647483003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.cpp:438:
output.pushCharacter(m_singleCharacterBuffer, 1);
On 2016/01/28 at 07:41:13, Yosi_UTC9 wrote:
> nit: s/pushCharacter/pushCharacters/

Done.

https://codereview.chromium.org/1647483003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/iterators/SimplifiedBackwardsTextIterator.cpp:442:
if (m_textContainer.is8Bit())
On 2016/01/28 at 07:41:13, Yosi_UTC9 wrote:
> nit: we prefer early return style.

The code looks simpler to me without early return.

https://codereview.chromium.org/1647483003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/iterators/TextBuffer.h (right):

https://codereview.chromium.org/1647483003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/iterators/TextBuffer.h:24: virtual const
CharType* data() const = 0;
On 2016/01/28 at 07:41:13, Yosi_UTC9 wrote:
> It seems strange that template class has virtual function.
> 
> Independent classes seem simpler:
> 
> template <typename CharType, size_t inlineCapacity>
> class ForwardTextBufferTemplate final {
>     STACK_ALLOCATED();
> public:
>     const CharType* data() const { return m_buffer.begin();}
> 
>     void clear() { m_size = 0; }
> 
>     void pushCharacters(CharType ch, size_t length)
>     {
>         std::fill_n(ensureBuffer(length), length, ch);
>     }
> 
>     template <typename T>
>     void pushRange(const T* other, size_t length)
>     {
>         if (length == 0)
>             return;
>         std::copy(other, other + length, ensureBuffer(length));
>     }
> 
> private:
>     CharType* ensureBuffer(size_t demand)
>     {
>         size_t current = m_size;
>         m_size += demand;
>         if (m_size > m_buffer.size())
>             grow(demand);
>         return m_buffer.begin() + current;
>     }
> 
>     void grow(size_t demand)
>     {
>         m_buffer.resize(oldCapacity + demand);
>         m_buffer.resize(m_buffer.capacity());
>     }
> 
>     Vector<CharType, inlineCapacity> m_buffer;
>     size_t m_size = 0;
> };
> 
> 
> template <typename CharType, size_t inlineCapacity>
> class BackwardTextBufferTemplate final {
>     STACK_ALLOCATED();
> public:
>     const CharType* data() const { return m_buffer.begin();}
> 
>     void clear() { m_size = 0; }
> 
>     void pushCharacters(CharType ch, size_t length)
>     {
>         std::fill_n(ensureBuffer(length), length, ch);
>     }
> 
>     template <typename T>
>     void pushRange(const T* other, size_t length)
>     {
>         if (length == 0)
>             return;
>         std::copy(other, other + length, ensureBuffer(length));
>     }
> 
> private:
>     CharType* ensureBuffer(size_t demand)
>     {
>         size_t current = m_size;
>         m_size += demand;
>         if (m_size > m_buffer.size())
>             grow(demand);
>         return m_buffer.end() - current;
>     }
> 
>     void grow(size_t demand)
>     {
>         m_buffer.resize(oldCapacity + demand);
>         m_buffer.resize(m_buffer.capacity());
>     }
> 
>     Vector<CharType, inlineCapacity> m_buffer;
>     size_t m_size = 0;
> };

I would still like the two classes to be correlated so that we can reuse the
code. And they do share the same logic in many functions.

In fact we don't need things to be general, because we are the only users. Let's
just remove the template.

https://codereview.chromium.org/1647483003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/iterators/TextBuffer.h:28: void
pushCharacter(CharType c, size_t length)
On 2016/01/28 at 07:41:13, Yosi_UTC9 wrote:
> nit: s/pushCharacter/pushCharacters/
> nit: Please avoid one character variable name.

Done.

[yosin_UTC9, 2016-01-28 09:06:17]

Please split this patch into N
1. Introduce ForwardsTextBuffer and BackwardsTextBuffer w/ tests
2. Utilizing *TextBuffer in backward iterator
3. Utilizing *TextBuffer in text iterator
4. ....

In other words, this patch is all-in-one patch.

https://codereview.chromium.org/1647483003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/iterators/TextBuffer.cpp (right):

https://codereview.chromium.org/1647483003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/iterators/TextBuffer.cpp:21: UChar*
TextBufferBase::pushInternal(size_t length)
s/pushInternal/ensureDestination/ or another, this function allocates
destination in buffer.

https://codereview.chromium.org/1647483003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/iterators/TextBuffer.cpp:25: UChar* ans =
pushDestination(length);
nit: s/pushDestination/computeDestination/ or another. It doesn't add data, it
returns destination.

https://codereview.chromium.org/1647483003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/iterators/TextBuffer.h (right):

https://codereview.chromium.org/1647483003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/iterators/TextBuffer.h:9: #include
"wtf/text/WTFString.h"
nit: #include "wtf/Vector.h"

https://codereview.chromium.org/1647483003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/iterators/TextBuffer.h:14:
STACK_ALLOCATED();
 WTF_MAKE_NONCOPYABLE(TextBufferBase) see wtf/Noncopyable.h

https://codereview.chromium.org/1647483003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/iterators/TextBuffer.h:16:
TextBufferBase();
nit: |TextBufferBase()| ctor should be protected.

https://codereview.chromium.org/1647483003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/iterators/TextBuffer.h:39: virtual void
shiftData(size_t oldCapacity) {}
nit: Don't inline virtual function.

https://codereview.chromium.org/1647483003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/iterators/TextBuffer.h:46:
STACK_ALLOCATED();
 WTF_MAKE_NONCOPYABLE(ForwardsTextBuffer) see wtf/Noncopyable.h

https://codereview.chromium.org/1647483003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/iterators/TextBuffer.h:51: using
TextBufferBase::m_size;
nit: We should use protected member function instead of accessing member
variable in base class.

https://codereview.chromium.org/1647483003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/iterators/TextBuffer.h:58:
STACK_ALLOCATED();
 WTF_MAKE_NONCOPYABLE(BackwardsTextBuffer) see wtf/Noncopyable.h

[Xiaocheng, 2016-01-28 10:26:12]

On 2016/01/28 at 09:06:17, yosin wrote:
> Please split this patch into N
> 1. Introduce ForwardsTextBuffer and BackwardsTextBuffer w/ tests
> 2. Utilizing *TextBuffer in backward iterator
> 3. Utilizing *TextBuffer in text iterator
> 4. ....
> 
> In other words, this patch is all-in-one patch.
> 

OK... (╯°Д°)╯︵ ┻━┻

>
https://codereview.chromium.org/1647483003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/editing/iterators/TextBuffer.cpp (right):
> 
>
https://codereview.chromium.org/1647483003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/editing/iterators/TextBuffer.cpp:21: UChar*
TextBufferBase::pushInternal(size_t length)
> s/pushInternal/ensureDestination/ or another, this function allocates
destination in buffer.
> 
>
https://codereview.chromium.org/1647483003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/editing/iterators/TextBuffer.cpp:25: UChar* ans
= pushDestination(length);
> nit: s/pushDestination/computeDestination/ or another. It doesn't add data, it
returns destination.
> 
>
https://codereview.chromium.org/1647483003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/editing/iterators/TextBuffer.h (right):
> 
>
https://codereview.chromium.org/1647483003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/editing/iterators/TextBuffer.h:9: #include
"wtf/text/WTFString.h"
> nit: #include "wtf/Vector.h"
> 
>
https://codereview.chromium.org/1647483003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/editing/iterators/TextBuffer.h:14:
STACK_ALLOCATED();
>  WTF_MAKE_NONCOPYABLE(TextBufferBase) see wtf/Noncopyable.h
> 
>
https://codereview.chromium.org/1647483003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/editing/iterators/TextBuffer.h:16:
TextBufferBase();
> nit: |TextBufferBase()| ctor should be protected.
> 
>
https://codereview.chromium.org/1647483003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/editing/iterators/TextBuffer.h:39: virtual void
shiftData(size_t oldCapacity) {}
> nit: Don't inline virtual function.
> 
>
https://codereview.chromium.org/1647483003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/editing/iterators/TextBuffer.h:46:
STACK_ALLOCATED();
>  WTF_MAKE_NONCOPYABLE(ForwardsTextBuffer) see wtf/Noncopyable.h
> 
>
https://codereview.chromium.org/1647483003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/editing/iterators/TextBuffer.h:51: using
TextBufferBase::m_size;
> nit: We should use protected member function instead of accessing member
variable in base class.
> 
>
https://codereview.chromium.org/1647483003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/editing/iterators/TextBuffer.h:58:
STACK_ALLOCATED();
>  WTF_MAKE_NONCOPYABLE(BackwardsTextBuffer) see wtf/Noncopyable.h