Adding backgroundColor to WebCompositionUnderline and using it for InlineTextBox drawing.

Adding backgroundColor to WebCompositionUnderline and using it for InlineTextBox drawing.

This CL enables custom background color to be specified for IME composition.
Its Chrome counterpart is: https://codereview.chromium.org/313053007 .  Details:

- Adding backgroundColor to the following:
    WebCore::CompositionUnderline
    blink::WebCompositionUnderline
  for backward compabitility, we keep an old constructor, which will be deleted
  in another refactoring CL. Updated some tests to use the new constructor.
- Refactored (now) repetative code to loop over WebCompositionUnderline  that
  intersect with a range by adding class CompositionUnderlineVisitor.
- Perhaps WebCompositionUnderline should be renamed to WebCompositionSpan,
  but this refactoring task can be done in a separate CL.

We will need to split this CL when we commit.

BUG=135900
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176387

[huangs, 2014-06-04 22:30:19]

PTAL, noting the Chrome counterpart https://codereview.chromium.org/313233002/ .

[aurimas (slooooooooow), 2014-06-05 01:02:52]

On 2014/06/04 22:30:19, huangs1 wrote:
> PTAL, noting the Chrome counterpart https://codereview.chromium.org/313233002/
.

I was under impression that the goal of this bug was to allow multiple
background "spans", how will this work together with the composition underline?

[yosin_UTC9, 2014-06-05 01:18:59]

How do you manage Blink and Chromium commits? Will you land Blink patch before
Chromium?

https://codereview.chromium.org/313233002/diff/20001/Source/core/editing/Comp...
File Source/core/editing/CompositionUnderlineVisitor.h (right):

https://codereview.chromium.org/313233002/diff/20001/Source/core/editing/Comp...
Source/core/editing/CompositionUnderlineVisitor.h:42: public:
nit:  WTF_MAKE_NONCOPYABLE(CompositionUnderlineVisitor)

https://codereview.chromium.org/313233002/diff/20001/Source/core/rendering/In...
File Source/core/rendering/InlineTextBox.cpp (right):

https://codereview.chromium.org/313233002/diff/20001/Source/core/rendering/In...
Source/core/rendering/InlineTextBox.cpp:641: CompositionUnderlineVisitor
visitor(underlines, start(), end());
How about using begin()/end() STL iterator protocol? Once, we can use range-for,
we can write: 
for (auto underline : CompositionUnerlineIterator(underlines, start(), end())

https://codereview.chromium.org/313233002/diff/20001/Source/core/rendering/In...
Source/core/rendering/InlineTextBox.cpp:652: paintCompositionBackground(context,
boxOrigin, styleToUse, font, Color(225, 221, 85),
Could you move this hard coded color value to somewhere? e.g. RenderTheme or
appropriate.

https://codereview.chromium.org/313233002/diff/20001/Source/core/rendering/In...
Source/core/rendering/InlineTextBox.cpp:889: int ePos = min(endPos - m_start,
(int)m_len);
nit: static_cast<int>(m_len)

[huangs, 2014-06-05 15:40:33]

PTAL.

aurumas@: I'm focusing on BackgroundColorSpan, passing the data through
composition underline so we won't have to change interfaces in the plumbing. 
Meanwhile, as seen in the Chrome CL, the spans are used to instantiate multiple
background spans.  A small visual problem is that the underlines will be
separated by 2 pixels.  In the short term, I'm thinking of handling
UnderlineSpan to fix this.  In the long term, some redesign is needed.  I'm
thinking about replacing CompositionUnderline with polymorphic CompositionSpan
with accessor interfaces, and replace Vector<CompositionUnderline> with a
container that's similar to android.text.SpannableString.

yosi@: After each commit Chrome should always work properly, so I'm thinking of
the following steps:

0: Chrome: Remove code that blocks us from updating the 3 CompositionUnderline
structs separately (https://codereview.chromium.org/319553002/)

1: Blink: Add backgroundColor to blink::WebCompositionUnderline and
WebCore::CompositionUnderline; add new 5-param constructors;  keeping old
4-param constructors; cursory conversion of tests to use 5-param constructors;
changes to InlineTextBox to use 5-param -- so essentially this CL.

2: Chrome: Add background_color to ui::CompositionUnderline; same deal with
4/5-param constructors; cursory conversion of tests; Clank code to hook data --
this is the companion Chrome CL.

At this point bug 135900 should be fixed.

3: Chrome & Blink: try to remove the 4-parameter constructors, fix everything
that breaks, might need multiple CLs -- but this is just cleanup.

There are also some visual issues beyond background color -- but these are
separate issues that will be addressed after the bug fix.

https://codereview.chromium.org/313233002/diff/20001/Source/core/editing/Comp...
File Source/core/editing/CompositionUnderlineVisitor.h (right):

https://codereview.chromium.org/313233002/diff/20001/Source/core/editing/Comp...
Source/core/editing/CompositionUnderlineVisitor.h:42: public:
On 2014/06/05 01:19:00, yosi wrote:
> nit:  WTF_MAKE_NONCOPYABLE(CompositionUnderlineVisitor)

Done.

https://codereview.chromium.org/313233002/diff/20001/Source/core/rendering/In...
File Source/core/rendering/InlineTextBox.cpp (right):

https://codereview.chromium.org/313233002/diff/20001/Source/core/rendering/In...
Source/core/rendering/InlineTextBox.cpp:641: CompositionUnderlineVisitor
visitor(underlines, start(), end());
Range-for and auto aren't used in WebKit.  Also, end() is unknown until we reach
it, so we'd have to do a binary search on construction -- probably
over-designed? 

Renamed to CompositionUnderlineIterator though.

https://codereview.chromium.org/313233002/diff/20001/Source/core/rendering/In...
Source/core/rendering/InlineTextBox.cpp:652: paintCompositionBackground(context,
boxOrigin, styleToUse, font, Color(225, 221, 85),
Done.

"%02X%02X%02X" % (255, 221, 85) == "FFDD55"

and RGBA32 actually stores ARGB, so 0xFFFFDD55.

https://codereview.chromium.org/313233002/diff/20001/Source/core/rendering/In...
Source/core/rendering/InlineTextBox.cpp:889: int ePos = min(endPos - m_start,
(int)m_len);
On 2014/06/05 01:19:00, yosi wrote:
> nit: static_cast<int>(m_len)

Done, replaced globally.

[yosin_UTC9, 2014-06-06 01:23:35]

On 2014/06/05 15:40:33, huangs1 wrote:
>
https://codereview.chromium.org/313233002/diff/20001/Source/core/rendering/In...
> Source/core/rendering/InlineTextBox.cpp:641: CompositionUnderlineVisitor
> visitor(underlines, start(), end());
> Range-for and auto aren't used in WebKit.  Also, end() is unknown until we
reach
> it, so we'd have to do a binary search on construction -- probably
> over-designed? 
> 

Here is a sketch of iterator implementation. It seems simpler than visitor,
especially comparing to CompositionUnderlineIterator::next(), regarding
complexity, e.g. one if statement vs. two if-statements w/ comments.

class CompositionUnderlineRange {
public:
    class Iterator {
    public:
        Iterator(const Vector<CompositionUnderline>& underliens, size_t index) :
m_underlines(underlines), m_index(index) { }
        bool operator==(const Iterator&) const { return m_index ==
other.m_index; }
        bool operator!=(const Iterator&) const { return !operator==(other); }
        Iterator& operator++() { ++m_index; return *this; }
        const CompositionUnderline& operator*() const { return
m_underliens[m_index]; }
        const CompositionUnderline* operator->() const { return
&m_underliens[m_index]; }
    private:
        const Vector<CompositeUnderline> m_underlines;
        size_t m_index;
    };
    CompositionUnderlineRange(const Vector<CompositionUnderline>& underliens,
size_t start, size_t end)
        : m_underlines(underliens), m_startIndex(indexOf(underlines, start)),
m_endIndex(indexOf(underliens, end)) { }
    Iterator begin() const { return Iterator(m_underlines, m_startIndex); }
    Iterator end() const { return Iterator(m_underlines, m_endIndex); }
private:
    size_t indexOf(const Vector<CompositionUnderline>& underliens, size_t
offset) {
        for (size_t index = 0u; index < underliens.size(); ++index) {
            if (underline.startOffset >= offset)
                return index;
        }
        return underline.size(; // not found
    }
private:
    const Vector<CompositeUnderline> m_underlines;
    size_t m_startIndex;
    size_t m_endIndex;
};


We can write loop:

CompositionUnderlineRange underlineRange(startOffset, endOffset);
for (CompositionUnderlineRange::Iterator it = underlineRange.begin(); it !=
underlineRange.end(); ++it) {
    const CompositionUnderline* underline = *it;
}

[yosin_UTC9, 2014-06-06 01:24:27]

Thanks for explanation of plan about committing. Changing Blink/Chromium API is
complicated.

https://codereview.chromium.org/313233002/diff/40001/Source/core/editing/Comp...
File Source/core/editing/CompositionUnderlineIterator.cpp (right):

https://codereview.chromium.org/313233002/diff/40001/Source/core/editing/Comp...
Source/core/editing/CompositionUnderlineIterator.cpp:1: /*
nit: Please use following license header:
http://dev.chromium.org/blink/coding-style#TOC-License

https://codereview.chromium.org/313233002/diff/40001/Source/core/editing/Comp...
File Source/core/editing/CompositionUnderlineIterator.h (right):

https://codereview.chromium.org/313233002/diff/40001/Source/core/editing/Comp...
Source/core/editing/CompositionUnderlineIterator.h:1: /*
nit: Please use below licence header:
http://dev.chromium.org/blink/coding-style#TOC-License

[huangs, 2014-06-06 02:40:52]

Thanks for the detailed comments!  I'm fine with adding more code to make this
use the .begin(), .end() idiom instead of my weird double-pointer stuff.

However, note that the class intends to loop over a list of underline intervals
(sorted by startOffset), and considering *only* those that intersect with a
specified "query" range.

For example, consider the following semi-open intervals (underlines), and a
*closed*-interval Query:
    A: [0,2):   **.........
    B: [0,5):   ****#......
    C: [1,3):   .**........
    D: [1,10):  .***##****.
    E: [3,5):   ...*#......
    F: [5,8):   .....#**...
    G: [7,8):   .......*...
    H: [8,10):  ........**.
Query: [4,5]:   ....##.....

So we want to iterate over {B,D,E,F} only.  Note that B and D have startIdx < 4,
but they still overlap with Query.  Therefore:
- begin() has to search linearly for first overlapping interval by checking
endOffset, too (e.g., skip A).
- operator++() has to skip over some intervals (e.g., C)
- end() can use indexOf(), or binary search via std::lower_bound(), to find G.

If we do this, a unit test should be written.

Since both calls are in InlineTextBox::paint(), a simpler solution is to check
|containsComposition|, compute the list in the routine and store into a
std::vector.  However, this may result in wasted computation if
|paintInfo.phase| don't match, and it looks like allocating memory in paint() is
a bad idea?  Please let me know what you think.

[yosin_UTC9, 2014-06-06 05:08:15]

On 2014/06/06 02:40:52, huangs1 wrote:
> Thanks for the detailed comments!  I'm fine with adding more code to make this
> use the .begin(), .end() idiom instead of my weird double-pointer stuff.
> 
> However, note that the class intends to loop over a list of underline
intervals
> (sorted by startOffset), and considering *only* those that intersect with a
> specified "query" range.
> 
> For example, consider the following semi-open intervals (underlines), and a
> *closed*-interval Query:
>     A: [0,2):   **.........
>     B: [0,5):   ****#......
>     C: [1,3):   .**........
>     D: [1,10):  .***##****.
>     E: [3,5):   ...*#......
>     F: [5,8):   .....#**...
>     G: [7,8):   .......*...
>     H: [8,10):  ........**.
> Query: [4,5]:   ....##.....
> 
> So we want to iterate over {B,D,E,F} only.  Note that B and D have startIdx <
4,
> but they still overlap with Query.  Therefore:
> - begin() has to search linearly for first overlapping interval by checking
> endOffset, too (e.g., skip A).
> - operator++() has to skip over some intervals (e.g., C)
> - end() can use indexOf(), or binary search via std::lower_bound(), to find G.
> 
> If we do this, a unit test should be written.
> 
> Since both calls are in InlineTextBox::paint(), a simpler solution is to check
> |containsComposition|, compute the list in the routine and store into a
> std::vector.  However, this may result in wasted computation if
> |paintInfo.phase| don't match, and it looks like allocating memory in paint()
is
> a bad idea?  Please let me know what you think.

Do we really need to support overlapped CompositionUnderline? AFIK, Chromium/Win
never pass such underlines.
If Android passes overlapped underlines to Chromium, it should be resolved in
Chromium/Android layer rather than Blink layer.

In other words, we should keep Vector<CompositionUnderline> to have no
overlapped underlines.

[huangs, 2014-06-06 14:38:14]

To handle Spans in general, we should handle overlaps. For example, the Google
Japanese Input app creates one UnderlineSpan for the entire thing, overlapping 2
separate BackgroundColorSpan.

I think we should handle overlaps for generality.  This is also useful in
addressing a new bug I found: The current code to draw underline subtracts 1
pixel from left and right.  With 2 CompositionUnderlines for background, a gap
appears.  If we have CompositionSpan, drawing underline and drawing background
can be decoupled.

Meanwhile, for the iterator stuff, we can just make end() return a special
"done" value, and make operator++ smarter.

[huangs, 2014-06-06 20:42:28]

Updated with unit test, PTAL.

https://codereview.chromium.org/313233002/diff/40001/Source/core/editing/Comp...
File Source/core/editing/CompositionUnderlineIterator.cpp (right):

https://codereview.chromium.org/313233002/diff/40001/Source/core/editing/Comp...
Source/core/editing/CompositionUnderlineIterator.cpp:1: /*
On 2014/06/06 01:24:27, yosi wrote:
> nit: Please use following license header:
> http://dev.chromium.org/blink/coding-style#TOC-License

Done.

https://codereview.chromium.org/313233002/diff/40001/Source/core/editing/Comp...
File Source/core/editing/CompositionUnderlineIterator.h (right):

https://codereview.chromium.org/313233002/diff/40001/Source/core/editing/Comp...
Source/core/editing/CompositionUnderlineIterator.h:1: /*
On 2014/06/06 01:24:27, yosi wrote:
> nit: Please use below licence header:
> http://dev.chromium.org/blink/coding-style#TOC-License

Done.

[huangs, 2014-06-06 23:38:32]

Added reminders for stuff I should fix once I have the chance.

https://chromiumcodereview.appspot.com/313233002/diff/80001/Source/core/editi...
File Source/core/editing/CompositionUnderlineRangeFilter.cpp (right):

https://chromiumcodereview.appspot.com/313233002/diff/80001/Source/core/editi...
Source/core/editing/CompositionUnderlineRangeFilter.cpp:20: // underline is
completely before interval. This may be an underline that appear
Remove comments about "runs".

https://chromiumcodereview.appspot.com/313233002/diff/80001/Source/core/editi...
File Source/core/editing/CompositionUnderlineRangeFilter.h (right):

https://chromiumcodereview.appspot.com/313233002/diff/80001/Source/core/editi...
Source/core/editing/CompositionUnderlineRangeFilter.h:14: // visiting only
elements that inersect with specified *inclusive* range [indexLo, indexHi].
TYPO: inersect

https://chromiumcodereview.appspot.com/313233002/diff/80001/Source/core/editi...
Source/core/editing/CompositionUnderlineRangeFilter.h:55: , m_nextIndex(0) { }
Delete m_nextIndex.

[yosin_UTC9, 2014-06-09 01:49:56]

https://codereview.chromium.org/313233002/diff/80001/Source/core/editing/Comp...
File Source/core/editing/CompositionUnderlineRangeFilter.h (right):

https://codereview.chromium.org/313233002/diff/80001/Source/core/editing/Comp...
Source/core/editing/CompositionUnderlineRangeFilter.h:21: END = -0x7FFFFFFF
nit: better to use kNotFound in wtf/NotFound.h, or static_cast<size_t>(-1)

https://codereview.chromium.org/313233002/diff/80001/Source/core/editing/Comp...
Source/core/editing/CompositionUnderlineRangeFilter.h:43:
ConstIterator(CompositionUnderlineRangeFilter* filter, int index)
nit: We should use |size_t| rather than |int|. Some platform use 64 bit unsigned
int for |size_t|.

https://codereview.chromium.org/313233002/diff/80001/Source/core/rendering/In...
File Source/core/rendering/InlineTextBox.cpp (right):

https://codereview.chromium.org/313233002/diff/80001/Source/core/rendering/In...
Source/core/rendering/InlineTextBox.cpp:871: unsigned
InlineTextBox::getUnderlinePaintStart(const CompositionUnderline& underline)
nit: size_t

https://codereview.chromium.org/313233002/diff/80001/Source/core/rendering/In...
Source/core/rendering/InlineTextBox.cpp:873: return
max(static_cast<unsigned>(m_start), underline.startOffset);
nit: It is better to write |std::max|.

https://codereview.chromium.org/313233002/diff/80001/Source/core/rendering/In...
File Source/core/rendering/InlineTextBox.h (right):

https://codereview.chromium.org/313233002/diff/80001/Source/core/rendering/In...
Source/core/rendering/InlineTextBox.h:178: unsigned getUnderlinePaintStart(const
CompositionUnderline&);
nit: Blink doesn't use "get" prefix for getter. Just |underlinePaintStart| is
fine.

[huangs, 2014-06-09 06:07:14]

PTAL.

https://codereview.chromium.org/313233002/diff/80001/Source/core/editing/Comp...
File Source/core/editing/CompositionUnderlineRangeFilter.cpp (right):

https://codereview.chromium.org/313233002/diff/80001/Source/core/editing/Comp...
Source/core/editing/CompositionUnderlineRangeFilter.cpp:20: // underline is
completely before interval. This may be an underline that appear
On 2014/06/06 23:38:32, huangs1 wrote:
> Remove comments about "runs".

Done.

https://codereview.chromium.org/313233002/diff/80001/Source/core/editing/Comp...
File Source/core/editing/CompositionUnderlineRangeFilter.h (right):

https://codereview.chromium.org/313233002/diff/80001/Source/core/editing/Comp...
Source/core/editing/CompositionUnderlineRangeFilter.h:14: // visiting only
elements that inersect with specified *inclusive* range [indexLo, indexHi].
On 2014/06/06 23:38:32, huangs1 wrote:
> TYPO: inersect

Done.

https://codereview.chromium.org/313233002/diff/80001/Source/core/editing/Comp...
Source/core/editing/CompositionUnderlineRangeFilter.h:21: END = -0x7FFFFFFF
Ah neat. Using kNotFound.  Note the change in operator++(), to prevent the
ability to advance end() (-1 would have this problem).

https://codereview.chromium.org/313233002/diff/80001/Source/core/editing/Comp...
Source/core/editing/CompositionUnderlineRangeFilter.h:43:
ConstIterator(CompositionUnderlineRangeFilter* filter, int index)
On 2014/06/09 01:49:56, yosi wrote:
> nit: We should use |size_t| rather than |int|. Some platform use 64 bit
unsigned
> int for |size_t|.

Done.

https://codereview.chromium.org/313233002/diff/80001/Source/core/editing/Comp...
Source/core/editing/CompositionUnderlineRangeFilter.h:55: , m_nextIndex(0) { }
On 2014/06/06 23:38:32, huangs1 wrote:
> Delete m_nextIndex.

Done.

https://codereview.chromium.org/313233002/diff/80001/Source/core/rendering/In...
File Source/core/rendering/InlineTextBox.cpp (right):

https://codereview.chromium.org/313233002/diff/80001/Source/core/rendering/In...
Source/core/rendering/InlineTextBox.cpp:871: unsigned
InlineTextBox::getUnderlinePaintStart(const CompositionUnderline& underline)
On 2014/06/09 01:49:56, yosi wrote:
> nit: size_t

Done.  Note that I only changed return types, but the intermediate values are
still unsigned, since the original variables are int or unsigned. Let me know if
you'd prefer everything to be size_t.

https://codereview.chromium.org/313233002/diff/80001/Source/core/rendering/In...
Source/core/rendering/InlineTextBox.cpp:873: return
max(static_cast<unsigned>(m_start), underline.startOffset);
Done, replaced globally in file, and included <algorithm>.

However, note that the file has "using namespace std;".  Should this be removed?

https://codereview.chromium.org/313233002/diff/80001/Source/core/rendering/In...
File Source/core/rendering/InlineTextBox.h (right):

https://codereview.chromium.org/313233002/diff/80001/Source/core/rendering/In...
Source/core/rendering/InlineTextBox.h:178: unsigned getUnderlinePaintStart(const
CompositionUnderline&);
On 2014/06/09 01:49:56, yosi wrote:
> nit: Blink doesn't use "get" prefix for getter. Just |underlinePaintStart| is
> fine.

Done.

[huangs, 2014-06-09 06:33:25]

https://chromiumcodereview.appspot.com/313233002/diff/100001/Source/core/edit...
File Source/core/editing/CompositionUnderlineRangeFilter.h (right):

https://chromiumcodereview.appspot.com/313233002/diff/100001/Source/core/edit...
Source/core/editing/CompositionUnderlineRangeFilter.h:40: friend
CompositionUnderlineRangeFilter;
friend class CompositionUnderlineRangeFilter;

https://chromiumcodereview.appspot.com/313233002/diff/100001/Source/core/edit...
Source/core/editing/CompositionUnderlineRangeFilter.h:59: friend ConstIterator;
friend class friend ConstIterator;

[huangs, 2014-06-09 20:27:22]

https://codereview.chromium.org/313233002/diff/100001/Source/core/editing/Com...
File Source/core/editing/CompositionUnderlineRangeFilter.h (right):

https://codereview.chromium.org/313233002/diff/100001/Source/core/editing/Com...
Source/core/editing/CompositionUnderlineRangeFilter.h:40: friend
CompositionUnderlineRangeFilter;
On 2014/06/09 06:33:25, huangs1 wrote:
> friend class CompositionUnderlineRangeFilter;

Done.

https://codereview.chromium.org/313233002/diff/100001/Source/core/editing/Com...
Source/core/editing/CompositionUnderlineRangeFilter.h:59: friend ConstIterator;
On 2014/06/09 06:33:25, huangs1 wrote:
> friend class friend ConstIterator;

Done.

[huangs, 2014-06-10 04:40:47]

Ping.

[huangs, 2014-06-12 05:39:17]

Ping yosin@. Please let me know if you have more comments.  Thanks!

[yosin_UTC9, 2014-06-12 06:23:18]

https://chromiumcodereview.appspot.com/313233002/diff/160001/Source/core/edit...
File Source/core/editing/CompositionUnderlineRangeFilter.h (right):

https://chromiumcodereview.appspot.com/313233002/diff/160001/Source/core/edit...
Source/core/editing/CompositionUnderlineRangeFilter.h:24: ASSERT(m_index >= 0);
nit: We don't need to have this |ASSERT|, since |m_index| is |unsigned|. It
always greater than or equal to zero.

https://chromiumcodereview.appspot.com/313233002/diff/160001/Source/core/edit...
Source/core/editing/CompositionUnderlineRangeFilter.h:49:
CompositionUnderlineRangeFilter(const Vector<CompositionUnderline>& underlines,
size_t indexLo, size_t indexHi)
nit: Let's move multiple line implementation functions into CPP file to reduce
build time. Compilers may pick them up for inline if suitable.

https://chromiumcodereview.appspot.com/313233002/diff/160001/Source/core/rend...
File Source/core/rendering/InlineTextBox.cpp (right):

https://chromiumcodereview.appspot.com/313233002/diff/160001/Source/core/rend...
Source/core/rendering/InlineTextBox.cpp:144: int sPos = std::max(startPos -
m_start, 0);
nit: Sorry for confusion about my comment. You only need to to use
|std::min|/|std:max| where you changed rather than all |min|/|max|. We prefer
small changes.

https://chromiumcodereview.appspot.com/313233002/diff/160001/Source/core/rend...
Source/core/rendering/InlineTextBox.cpp:200: int ePos = std::min(endPos -
m_start, static_cast<int>(m_len));
nit: You don't need to change |(int)| to |static_cast<int>(...)| unless you
changed. We prefer small patch.

https://chromiumcodereview.appspot.com/313233002/diff/160001/Source/core/rend...
Source/core/rendering/InlineTextBox.cpp:650: if ((*it).backgroundColor !=
Color::transparent) {
nit: It is better to use |it->backgroundColor|. We may want to have |const
CompositionUnderline* operator->() const| in |CompositionUnderlineRangeFilter|.

https://chromiumcodereview.appspot.com/313233002/diff/160001/Source/core/rend...
Source/core/rendering/InlineTextBox.cpp:878: size_t
InlineTextBox::underlinePaintStart(const CompositionUnderline& underline)
nit: |size_t| -> |unsigned|

https://chromiumcodereview.appspot.com/313233002/diff/160001/Source/core/rend...
Source/core/rendering/InlineTextBox.cpp:883: size_t
InlineTextBox::underlinePaintEnd(const CompositionUnderline& underline)
nit: |size_t| -> |unsigned|

It seems both |InlineTextBox| and |CompositeUnderline| use |unsigned|.

[huangs, 2014-06-12 23:22:10]

Updated, PTAL.

https://codereview.chromium.org/313233002/diff/160001/Source/core/editing/Com...
File Source/core/editing/CompositionUnderlineRangeFilter.h (right):

https://codereview.chromium.org/313233002/diff/160001/Source/core/editing/Com...
Source/core/editing/CompositionUnderlineRangeFilter.h:24: ASSERT(m_index >= 0);
On 2014/06/12 06:23:17, yosi wrote:
> nit: We don't need to have this |ASSERT|, since |m_index| is |unsigned|. It
> always greater than or equal to zero.

Oops, should use

ASSERT(m_index != kNotFound);

https://codereview.chromium.org/313233002/diff/160001/Source/core/editing/Com...
Source/core/editing/CompositionUnderlineRangeFilter.h:49:
CompositionUnderlineRangeFilter(const Vector<CompositionUnderline>& underlines,
size_t indexLo, size_t indexHi)
On 2014/06/12 06:23:17, yosi wrote:
> nit: Let's move multiple line implementation functions into CPP file to reduce
> build time. Compilers may pick them up for inline if suitable.

Done.

https://codereview.chromium.org/313233002/diff/160001/Source/core/rendering/I...
File Source/core/rendering/InlineTextBox.cpp (right):

https://codereview.chromium.org/313233002/diff/160001/Source/core/rendering/I...
Source/core/rendering/InlineTextBox.cpp:144: int sPos = std::max(startPos -
m_start, 0);
Done; undid changes to code I didn't touch.

https://codereview.chromium.org/313233002/diff/160001/Source/core/rendering/I...
Source/core/rendering/InlineTextBox.cpp:200: int ePos = std::min(endPos -
m_start, static_cast<int>(m_len));
Done; undid changes to code I didn't touch.

https://codereview.chromium.org/313233002/diff/160001/Source/core/rendering/I...
Source/core/rendering/InlineTextBox.cpp:650: if ((*it).backgroundColor !=
Color::transparent) {
On 2014/06/12 06:23:17, yosi wrote:
> nit: It is better to use |it->backgroundColor|. We may want to have |const
> CompositionUnderline* operator->() const| in
|CompositionUnderlineRangeFilter|.

Done.

https://codereview.chromium.org/313233002/diff/160001/Source/core/rendering/I...
Source/core/rendering/InlineTextBox.cpp:878: size_t
InlineTextBox::underlinePaintStart(const CompositionUnderline& underline)
On 2014/06/12 06:23:17, yosi wrote:
> nit: |size_t| -> |unsigned|

Done.

https://codereview.chromium.org/313233002/diff/160001/Source/core/rendering/I...
Source/core/rendering/InlineTextBox.cpp:883: size_t
InlineTextBox::underlinePaintEnd(const CompositionUnderline& underline)
On 2014/06/12 06:23:17, yosi wrote:
> nit: |size_t| -> |unsigned|
> 
> It seems both |InlineTextBox| and |CompositeUnderline| use |unsigned|.

Done.

[yosin_UTC9, 2014-06-13 04:08:27]

LGTM

Add tkent@ for OWNERE review.

https://codereview.chromium.org/313233002/diff/180001/Source/core/rendering/I...
File Source/core/rendering/InlineTextBox.cpp (right):

https://codereview.chromium.org/313233002/diff/180001/Source/core/rendering/I...
Source/core/rendering/InlineTextBox.cpp:1157: int fontHeightInt =
(int)(styleToUse->fontMetrics().floatHeight() + 0.5);
super nit: If you have chance to update this patch, please revert this line as
is.

[tkent, 2014-06-13 04:31:43]

looks ok except rendering/.
Add some rendering people.

https://codereview.chromium.org/313233002/diff/180001/Source/core/editing/Com...
File Source/core/editing/CompositionUnderline.h (right):

https://codereview.chromium.org/313233002/diff/180001/Source/core/editing/Com...
Source/core/editing/CompositionUnderline.h:39: , backgroundColor(0) { }
0 Color is not good for readability.
, backgroundColor(Color::transparent) { }

or omit backgroundColor initialization.  Color has the default constructor with
no arguments.

https://codereview.chromium.org/313233002/diff/180001/Source/core/editing/Com...
Source/core/editing/CompositionUnderline.h:41: // TODO(huangs): remove this
constructor.
Use FIXME: instead of TODO(name).

https://codereview.chromium.org/313233002/diff/180001/Source/core/editing/Com...
Source/core/editing/CompositionUnderline.h:47: , backgroundColor(0) { }
0 Color is not good.

https://codereview.chromium.org/313233002/diff/180001/Source/core/editing/Com...
File Source/core/editing/CompositionUnderlineRangeFilter.cpp (right):

https://codereview.chromium.org/313233002/diff/180001/Source/core/editing/Com...
Source/core/editing/CompositionUnderlineRangeFilter.cpp:22: while
(static_cast<size_t>(index) < numUnderlines) {
static_cast<> is unnecessary.

https://codereview.chromium.org/313233002/diff/180001/Source/core/editing/Com...
File Source/core/editing/CompositionUnderlineRangeFilter.h (right):

https://codereview.chromium.org/313233002/diff/180001/Source/core/editing/Com...
Source/core/editing/CompositionUnderlineRangeFilter.h:14: // A visitor class to
yield elements of a sorted (by startOffset) list of underlines,
nit: I recommend to wrap code comments in 80 columns.

https://codereview.chromium.org/313233002/diff/180001/Source/core/rendering/I...
File Source/core/rendering/InlineTextBox.cpp (right):

https://codereview.chromium.org/313233002/diff/180001/Source/core/rendering/I...
Source/core/rendering/InlineTextBox.cpp:650: if (it->backgroundColor !=
Color::transparent) {
We prefer early continue.

if (it->backgroundColor == Color::transparent)
    continue;

https://codereview.chromium.org/313233002/diff/180001/Source/core/rendering/I...
File Source/core/rendering/InlineTextBox.h (right):

https://codereview.chromium.org/313233002/diff/180001/Source/core/rendering/I...
Source/core/rendering/InlineTextBox.h:177: private:
|private:| is unnecessary.

https://codereview.chromium.org/313233002/diff/180001/public/web/WebCompositi...
File public/web/WebCompositionUnderline.h (right):

https://codereview.chromium.org/313233002/diff/180001/public/web/WebCompositi...
public/web/WebCompositionUnderline.h:48: // TODO(huangs): remove this
constructor.
Use FIXME: instead of TODO().

[huangs, 2014-06-13 17:22:25]

Updated. Note change in behavior: we skip drawing transparent underlines when
iterating over them.

Note that in the Chrome CL https://codereview.chromium.org/313053007/ ,
BackgroundColorSpan is given transparent underline (underlines are drawn by
UnderlineSpan).

https://codereview.chromium.org/313233002/diff/180001/Source/core/editing/Com...
File Source/core/editing/CompositionUnderline.h (right):

https://codereview.chromium.org/313233002/diff/180001/Source/core/editing/Com...
Source/core/editing/CompositionUnderline.h:39: , backgroundColor(0) { }
On 2014/06/13 04:31:42, tkent wrote:
> 0 Color is not good for readability.
> , backgroundColor(Color::transparent) { }
> 
> or omit backgroundColor initialization.  Color has the default constructor
with
> no arguments.

Done.

https://codereview.chromium.org/313233002/diff/180001/Source/core/editing/Com...
Source/core/editing/CompositionUnderline.h:41: // TODO(huangs): remove this
constructor.
On 2014/06/13 04:31:42, tkent wrote:
> Use FIXME: instead of TODO(name).

Done.

https://codereview.chromium.org/313233002/diff/180001/Source/core/editing/Com...
Source/core/editing/CompositionUnderline.h:47: , backgroundColor(0) { }
On 2014/06/13 04:31:43, tkent wrote:
> 0 Color is not good.

Done.

https://codereview.chromium.org/313233002/diff/180001/Source/core/editing/Com...
File Source/core/editing/CompositionUnderlineRangeFilter.cpp (right):

https://codereview.chromium.org/313233002/diff/180001/Source/core/editing/Com...
Source/core/editing/CompositionUnderlineRangeFilter.cpp:22: while
(static_cast<size_t>(index) < numUnderlines) {
On 2014/06/13 04:31:43, tkent wrote:
> static_cast<> is unnecessary.

Done.

https://codereview.chromium.org/313233002/diff/180001/Source/core/editing/Com...
File Source/core/editing/CompositionUnderlineRangeFilter.h (right):

https://codereview.chromium.org/313233002/diff/180001/Source/core/editing/Com...
Source/core/editing/CompositionUnderlineRangeFilter.h:14: // A visitor class to
yield elements of a sorted (by startOffset) list of underlines,
On 2014/06/13 04:31:43, tkent wrote:
> nit: I recommend to wrap code comments in 80 columns.

Done, also applied to my other changes in this CL.

https://codereview.chromium.org/313233002/diff/180001/Source/core/rendering/I...
File Source/core/rendering/InlineTextBox.cpp (right):

https://codereview.chromium.org/313233002/diff/180001/Source/core/rendering/I...
Source/core/rendering/InlineTextBox.cpp:650: if (it->backgroundColor !=
Color::transparent) {
Done, also applied to the other loop near line 791, i.e., skip drawing underline
if the underline color is transparent.

https://codereview.chromium.org/313233002/diff/180001/Source/core/rendering/I...
Source/core/rendering/InlineTextBox.cpp:1157: int fontHeightInt =
(int)(styleToUse->fontMetrics().floatHeight() + 0.5);
On 2014/06/13 04:08:26, yosi wrote:
> super nit: If you have chance to update this patch, please revert this line as
> is.

Done.

https://codereview.chromium.org/313233002/diff/180001/Source/core/rendering/I...
File Source/core/rendering/InlineTextBox.h (right):

https://codereview.chromium.org/313233002/diff/180001/Source/core/rendering/I...
Source/core/rendering/InlineTextBox.h:177: private:
On 2014/06/13 04:31:43, tkent wrote:
> |private:| is unnecessary.

Done.

https://codereview.chromium.org/313233002/diff/180001/public/web/WebCompositi...
File public/web/WebCompositionUnderline.h (right):

https://codereview.chromium.org/313233002/diff/180001/public/web/WebCompositi...
public/web/WebCompositionUnderline.h:48: // TODO(huangs): remove this
constructor.
On 2014/06/13 04:31:43, tkent wrote:
> Use FIXME: instead of TODO().

Done.

[tkent, 2014-06-15 23:46:37]

lgtm

[commit-bot: I haz the power, 2014-06-15 23:46:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/huangs@chromium.org/313233002/200001

[tkent, 2014-06-15 23:46:46]

The CQ bit was unchecked by tkent@chromium.org

[tkent, 2014-06-15 23:47:42]

Need review for rendering/.

[huangs, 2014-06-16 02:19:21]

Ping leviw@ for OWNER review of

Source/core/rendering/*

[leviw_travelin_and_unemployed, 2014-06-17 19:09:56]

LGTM with 2 nits.

https://codereview.chromium.org/313233002/diff/200001/Source/core/rendering/I...
File Source/core/rendering/InlineTextBox.cpp (right):

https://codereview.chromium.org/313233002/diff/200001/Source/core/rendering/I...
Source/core/rendering/InlineTextBox.cpp:645: if (useCustomUnderlines) {
How about extracting this whole if/else block into a named function. This paint
function is already pretty long and deeply nested.

https://codereview.chromium.org/313233002/diff/200001/Source/core/rendering/I...
Source/core/rendering/InlineTextBox.cpp:1389: // start of line to draw, relative
to tx
Can you update this comment? tx has been dead for ages. It's relative to
paintOffset.

[huangs, 2014-06-17 21:23:27]

Updated. Thanks!

https://codereview.chromium.org/313233002/diff/200001/Source/core/rendering/I...
File Source/core/rendering/InlineTextBox.cpp (right):

https://codereview.chromium.org/313233002/diff/200001/Source/core/rendering/I...
Source/core/rendering/InlineTextBox.cpp:645: if (useCustomUnderlines) {
Done.  The new function is 

InlineTextBox::paintCompositionBackgrounds()

the old function InlineTextBox::paintCompositionBackground() is renamed to

InlineTextBox::paintSingleCompositionBackgroundRun()

https://codereview.chromium.org/313233002/diff/200001/Source/core/rendering/I...
Source/core/rendering/InlineTextBox.cpp:1389: // start of line to draw, relative
to tx
On 2014/06/17 19:09:56, leviw wrote:
> Can you update this comment? tx has been dead for ages. It's relative to
> paintOffset.

Done.

[huangs, 2014-06-17 21:24:08]

The CQ bit was checked by huangs@chromium.org

[commit-bot: I haz the power, 2014-06-17 21:24:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/huangs@chromium.org/313233002/220001

[huangs, 2014-06-17 21:25:46]

The CQ bit was unchecked by huangs@chromium.org

[huangs, 2014-06-18 04:08:30]

The CQ bit was checked by huangs@chromium.org

[commit-bot: I haz the power, 2014-06-18 04:09:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/huangs@chromium.org/313233002/220001

[commit-bot: I haz the power, 2014-06-18 04:41:22]

Change committed as 176387