RenderText API Outline.

Add gfx::RenderText and support code.

RenderText is NativeTextFieldViews' interface to platform-specific text rendering engines.

This change doesn't hook in any new Pango or Uniscribe functionality, it will just setup the necessary API.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=93840

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=93855

[xji, 2011-07-01 18:29:44]

Thanks for writing this up!

http://codereview.chromium.org/7265011/diff/2001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

http://codereview.chromium.org/7265011/diff/2001/ui/gfx/render_text.cc#newcod...
ui/gfx/render_text.cc:289: // TODO(msw): Doesn't this crash with pos == 0?
even if it does not crash, it definitely access out-of-boundary when pos==0.

http://codereview.chromium.org/7265011/diff/2001/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

http://codereview.chromium.org/7265011/diff/2001/ui/gfx/render_text.h#newcode26
ui/gfx/render_text.h:26: ui::Range range;
also
SkColor background;
for selection background

http://codereview.chromium.org/7265011/diff/2001/ui/gfx/render_text.h#newcode111
ui/gfx/render_text.h:111: bool move_by_word) const = 0;
if we have "MoveCursorLeft() and MoveCursorRight()" as public, maybe we should
make those "GetLeftCursorPosition() and GetRightCursorPosition" as private.

http://codereview.chromium.org/7265011/diff/2001/ui/gfx/render_text.h#newcode132
ui/gfx/render_text.h:132: string16 text_;
are we going to remove those duplicated fields from NativeTextfieldViews or
TextfieldViewsModel if RenderText is a member of one of them?

http://codereview.chromium.org/7265011/diff/2001/ui/gfx/render_text.h#newcode149
ui/gfx/render_text.h:149: gfx::Point render_offset_;
what is the usage of this field?

http://codereview.chromium.org/7265011/diff/2001/ui/gfx/render_text.h#newcode164
ui/gfx/render_text.h:164: 
I do not have clear idea on what (how much) stuff we want to put in RenderText,
and what (how much) stuff we want to put in TextfieldViewsModel and/or
NativeTextfieldViews? Are we making RenderText thin enough and pass information
from TextfieldViewsModel to it if needed? Or we are making it heavy enough to do
most of TextfieldViewsModel's work?

http://codereview.chromium.org/7265011/diff/2001/views/controls/textfield/tex...
File views/controls/textfield/textfield_views_model.h (right):

http://codereview.chromium.org/7265011/diff/2001/views/controls/textfield/tex...
views/controls/textfield/textfield_views_model.h:55: // TODO(msw): Make
RenderText a member of TextfieldViewsModel?
Maybe make RenderText a member of TextfieldViewsModel instead of
NativeTextfieldViews? because the model contains the text itself plus those UI
operations, and you can access model from view, but not the other way around.

[msw, 2011-07-01 21:52:15]

Thanks for taking a look, things are still in great flux; I hope to make some
progress over the long weekend.

http://codereview.chromium.org/7265011/diff/2001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

http://codereview.chromium.org/7265011/diff/2001/ui/gfx/render_text.cc#newcod...
ui/gfx/render_text.cc:289: // TODO(msw): Doesn't this crash with pos == 0?
On 2011/07/01 18:29:44, xji wrote:
> even if it does not crash, it definitely access out-of-boundary when pos==0.

Done.

http://codereview.chromium.org/7265011/diff/2001/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

http://codereview.chromium.org/7265011/diff/2001/ui/gfx/render_text.h#newcode26
ui/gfx/render_text.h:26: ui::Range range;
On 2011/07/01 18:29:44, xji wrote:
> also
> SkColor background;
> for selection background

Interesting, we can add a background color if we want to allow stylized
backgrounds, but we shouldn't apply StyleRanges to render the selection. That
would essentially append a new StyleRange for each selection modification and
bloat the model.

http://codereview.chromium.org/7265011/diff/2001/ui/gfx/render_text.h#newcode111
ui/gfx/render_text.h:111: bool move_by_word) const = 0;
On 2011/07/01 18:29:44, xji wrote:
> if we have "MoveCursorLeft() and MoveCursorRight()" as public, maybe we should
> make those "GetLeftCursorPosition() and GetRightCursorPosition" as private.

I agree. I'm just now integrating this new API into NativeTextfieldViews, I'll
make these private if they aren't required.

http://codereview.chromium.org/7265011/diff/2001/ui/gfx/render_text.h#newcode132
ui/gfx/render_text.h:132: string16 text_;
On 2011/07/01 18:29:44, xji wrote:
> are we going to remove those duplicated fields from NativeTextfieldViews or
> TextfieldViewsModel if RenderText is a member of one of them?

I'm not sure; currently the TextfieldViewsModel operates on its text_ member,
and NativeTextfieldViews has a Textfield member with its own text_, and I was
planning to make RenderText a member of NativeTextfieldViews, but it's looking
more likely to be a member of TextfieldViewsModel now, which should eliminate
the need for TextfieldViewsModel::text_. I'm open to suggestions.

http://codereview.chromium.org/7265011/diff/2001/ui/gfx/render_text.h#newcode149
ui/gfx/render_text.h:149: gfx::Point render_offset_;
On 2011/07/01 18:29:44, xji wrote:
> what is the usage of this field?

This is for offsetting the origin of the text as rendered within the textfield,
as NativeTextfieldViews::text_offset_ does for the x dimension. This allows
panning over text that is wider (and possibly taller for multi-line?) than the
textfield can display. I may just go back to a single int, since we don't plan
to offer multi-line text immediately.

http://codereview.chromium.org/7265011/diff/2001/ui/gfx/render_text.h#newcode164
ui/gfx/render_text.h:164: 
On 2011/07/01 18:29:44, xji wrote:
> I do not have clear idea on what (how much) stuff we want to put in
RenderText,
> and what (how much) stuff we want to put in TextfieldViewsModel and/or
> NativeTextfieldViews? Are we making RenderText thin enough and pass
information
> from TextfieldViewsModel to it if needed? Or we are making it heavy enough to
do
> most of TextfieldViewsModel's work?

Unfortunately I haven't solved that dilemma. I thought NativeTextfieldViews
should use TextfieldViewsModel for editing/history/composition and use
RenderText for cursor/selection/coordinates/drawing. But these operations are so
intertwined that a slimmer RenderText may have to live in TextfieldViewsModel
with some redundant pass-through API. I'm not very pleased with either approach,
but leaning towards the second one now; feedback is appreciated,

http://codereview.chromium.org/7265011/diff/2001/views/controls/textfield/tex...
File views/controls/textfield/textfield_views_model.h (right):

http://codereview.chromium.org/7265011/diff/2001/views/controls/textfield/tex...
views/controls/textfield/textfield_views_model.h:55: // TODO(msw): Make
RenderText a member of TextfieldViewsModel?
On 2011/07/01 18:29:44, xji wrote:
> Maybe make RenderText a member of TextfieldViewsModel instead of
> NativeTextfieldViews? because the model contains the text itself plus those UI
> operations, and you can access model from view, but not the other way around.

Yeah, I'm leaning in that direction now, thanks for the feedback!

[xji, 2011-07-07 23:51:26]

On 2011/07/01 21:52:15, msw wrote:
> Thanks for taking a look, things are still in great flux; I hope to make some
> progress over the long weekend.
> 
> http://codereview.chromium.org/7265011/diff/2001/ui/gfx/render_text.cc
> File ui/gfx/render_text.cc (right):
> 
>
http://codereview.chromium.org/7265011/diff/2001/ui/gfx/render_text.cc#newcod...
> ui/gfx/render_text.cc:289: // TODO(msw): Doesn't this crash with pos == 0?
> On 2011/07/01 18:29:44, xji wrote:
> > even if it does not crash, it definitely access out-of-boundary when pos==0.
> 
> Done.
> 
> http://codereview.chromium.org/7265011/diff/2001/ui/gfx/render_text.h
> File ui/gfx/render_text.h (right):
> 
>
http://codereview.chromium.org/7265011/diff/2001/ui/gfx/render_text.h#newcode26
> ui/gfx/render_text.h:26: ui::Range range;
> On 2011/07/01 18:29:44, xji wrote:
> > also
> > SkColor background;
> > for selection background
> 
> Interesting, we can add a background color if we want to allow stylized
> backgrounds, but we shouldn't apply StyleRanges to render the selection. That
> would essentially append a new StyleRange for each selection modification and
> bloat the model.
> 
> 
why selection will bloat the model? as to the selection, for example "abcdefg",
if the previous selection select "a", the range is (0,1),
if it continue to select "ab", the range will be (0, 2)....

Is there any case that selection generates more than 1 range?



http://codereview.chromium.org/7265011/diff/2001/ui/gfx/render_text.h#newcode111
> ui/gfx/render_text.h:111: bool move_by_word) const = 0;
> On 2011/07/01 18:29:44, xji wrote:
> > if we have "MoveCursorLeft() and MoveCursorRight()" as public, maybe we
should
> > make those "GetLeftCursorPosition() and GetRightCursorPosition" as private.
> 
> I agree. I'm just now integrating this new API into NativeTextfieldViews, I'll
> make these private if they aren't required.
> 
>
http://codereview.chromium.org/7265011/diff/2001/ui/gfx/render_text.h#newcode132
> ui/gfx/render_text.h:132: string16 text_;
> On 2011/07/01 18:29:44, xji wrote:
> > are we going to remove those duplicated fields from NativeTextfieldViews or
> > TextfieldViewsModel if RenderText is a member of one of them?
> 
> I'm not sure; currently the TextfieldViewsModel operates on its text_ member,
> and NativeTextfieldViews has a Textfield member with its own text_, and I was
> planning to make RenderText a member of NativeTextfieldViews, but it's looking
> more likely to be a member of TextfieldViewsModel now, which should eliminate
> the need for TextfieldViewsModel::text_. I'm open to suggestions.
> 
>
http://codereview.chromium.org/7265011/diff/2001/ui/gfx/render_text.h#newcode149
> ui/gfx/render_text.h:149: gfx::Point render_offset_;
> On 2011/07/01 18:29:44, xji wrote:
> > what is the usage of this field?
> 
> This is for offsetting the origin of the text as rendered within the
textfield,
> as NativeTextfieldViews::text_offset_ does for the x dimension. This allows
> panning over text that is wider (and possibly taller for multi-line?) than the
> textfield can display. I may just go back to a single int, since we don't plan
> to offer multi-line text immediately.
> 
>
http://codereview.chromium.org/7265011/diff/2001/ui/gfx/render_text.h#newcode164
> ui/gfx/render_text.h:164: 
> On 2011/07/01 18:29:44, xji wrote:
> > I do not have clear idea on what (how much) stuff we want to put in
> RenderText,
> > and what (how much) stuff we want to put in TextfieldViewsModel and/or
> > NativeTextfieldViews? Are we making RenderText thin enough and pass
> information
> > from TextfieldViewsModel to it if needed? Or we are making it heavy enough
to
> do
> > most of TextfieldViewsModel's work?
> 
> Unfortunately I haven't solved that dilemma. I thought NativeTextfieldViews
> should use TextfieldViewsModel for editing/history/composition and use
> RenderText for cursor/selection/coordinates/drawing. But these operations are
so
> intertwined that a slimmer RenderText may have to live in TextfieldViewsModel
> with some redundant pass-through API. I'm not very pleased with either
approach,
> but leaning towards the second one now; feedback is appreciated,
> 

I am not sure. I lean towards the 1st one (make RenderText as thin as possible,
if possible, just have draw/logical-to-visual/visual-to-logical, and there will
be redundant pass-through API). Are we able to  draw a clear line (no
redundancy) if RenderText is heavy enough?

>
http://codereview.chromium.org/7265011/diff/2001/views/controls/textfield/tex...
> File views/controls/textfield/textfield_views_model.h (right):
> 
>
http://codereview.chromium.org/7265011/diff/2001/views/controls/textfield/tex...
> views/controls/textfield/textfield_views_model.h:55: // TODO(msw): Make
> RenderText a member of TextfieldViewsModel?
> On 2011/07/01 18:29:44, xji wrote:
> > Maybe make RenderText a member of TextfieldViewsModel instead of
> > NativeTextfieldViews? because the model contains the text itself plus those
UI
> > operations, and you can access model from view, but not the other way
around.
> 
> Yeah, I'm leaning in that direction now, thanks for the feedback!

[xji, 2011-07-08 00:02:01]

http://codereview.chromium.org/7265011/diff/2001/ui/gfx/render_text.h#newcode26
> > ui/gfx/render_text.h:26: ui::Range range;
> > On 2011/07/01 18:29:44, xji wrote:
> > > also
> > > SkColor background;
> > > for selection background
> > 
> > Interesting, we can add a background color if we want to allow stylized
> > backgrounds, but we shouldn't apply StyleRanges to render the selection.
That
> > would essentially append a new StyleRange for each selection modification
and
> > bloat the model.
> > 
> > 
> why selection will bloat the model? as to the selection, for example
"abcdefg",
> if the previous selection select "a", the range is (0,1),
> if it continue to select "ab", the range will be (0, 2)....
> 
> Is there any case that selection generates more than 1 range?
> 
Ah, I was not thinking bidi. yes. one logical selection could have multiple
visual selection ranges. 
but I am not sure how much it will bloat the model.
I am using pango text attribute to draw selection, which is handy and which
requires background attribute, that is why the question.

[msw, 2011-07-14 20:51:56]

FYI, I made some minor progress.

[msw, 2011-07-14 21:23:01]

[+oshima]
Take a look at my current status on shuffling the text code around. There's a
bit left to do, but I welcome feedback.

[oshima, 2011-07-15 21:21:36]

Looking good, thanks mike!

I may separate Cursor class from RenderText, but we can wait until we have real
win/linux impl.

Too bad that we need to include composition. I understand why we need it, but
would have looked better if it's not in there.

Please let me know when it's ready for final review.

http://codereview.chromium.org/7265011/diff/13001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

http://codereview.chromium.org/7265011/diff/13001/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:226: const ui::Range& RenderText::GetComposition() const {
GetCompositionRange would be better (esp there are several "composition" now)

http://codereview.chromium.org/7265011/diff/13001/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:231: {
you may want to check if range is within the valid range

http://codereview.chromium.org/7265011/diff/13001/views/controls/textfield/na...
File views/controls/textfield/native_textfield_views.cc (right):

http://codereview.chromium.org/7265011/diff/13001/views/controls/textfield/na...
views/controls/textfield/native_textfield_views.cc:856:
GetRenderText()->set_insert_mode(!GetRenderText()->get_insert_mode());
may be flip_insert_mode() is simpler.

[msw, 2011-07-19 07:11:21]

Please review and invite others as necessary, thanks!

http://codereview.chromium.org/7265011/diff/13001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

http://codereview.chromium.org/7265011/diff/13001/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:226: const ui::Range& RenderText::GetComposition() const {
On 2011/07/15 21:21:36, oshima wrote:
> GetCompositionRange would be better (esp there are several "composition" now)

Done.

http://codereview.chromium.org/7265011/diff/13001/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:231: {
On 2011/07/15 21:21:36, oshima wrote:
> you may want to check if range is within the valid range

Done.

http://codereview.chromium.org/7265011/diff/13001/views/controls/textfield/na...
File views/controls/textfield/native_textfield_views.cc (right):

http://codereview.chromium.org/7265011/diff/13001/views/controls/textfield/na...
views/controls/textfield/native_textfield_views.cc:856:
GetRenderText()->set_insert_mode(!GetRenderText()->get_insert_mode());
On 2011/07/15 21:21:36, oshima wrote:
> may be flip_insert_mode() is simpler.

Done.

[xji, 2011-07-20 01:33:33]

http://codereview.chromium.org/7265011/diff/32001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

http://codereview.chromium.org/7265011/diff/32001/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:283: return base::i18n::LEFT_TO_RIGHT;
why do we need this? and why it is always LTR?

http://codereview.chromium.org/7265011/diff/32001/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

http://codereview.chromium.org/7265011/diff/32001/ui/gfx/render_text.h#newcod...
ui/gfx/render_text.h:159: ui::Range selection_range_;
I think we will need the leading or trailing information besides the logical
index.

For example, given a logical text "abcDEF" where capital letter stands for
Hebrew, the visual display is "abcFED".

According to the bidi editing guide:
1. If pointing to the right half of the cell of a LTR character, the current
position must be set after this character and the caret must be displayed after
this character.
2. If pointing to the right half of the cell of a RTL character, the current
position must be set before this character and the caret must be displayed
before this character.

So, pointing to the right half of 'c' and pointing to the right half of 'D' all
set cursor position as 3.
But the cursor displayed visually at different places:
pointing right half of 'c' displays cursor right of 'c',
pointing right half of 'D' displays cursor right of 'D'.

In chrome's flow. When pointing using mouse, FindCursorPosition() is called, and
it returns the logical position (as 3). Later, GetCursorBounds() will be called
to draw the cursor at logical position 3. For the above 2 cases, although the
logical position are both 3, but the cursor should be dawn in different places.
A single cursor position wont be able to achieve this. Instead, we will need the
trailing or leading information.

For example, pointing to right of 'c' put current cursor at trailing position of
character at index of 2. pointing to right of 'D' put current cursor at leading
position of character of index 3. (note: for a LTR character, right edge is the
trailing edge, for a RTL character, right edge is the  leading edge).

A subsequent question that I am not sure about is that:
selection_range_'s end is where the next character will be inserted.

If we save the current cursor position as (cursor_position, trailing/leading)
pair, the cursor_position means the current cursor  position, which is different
from the cursor position where next character will be inserted.
For the above example, when pointing to right of 'c', the cursor position (where
next character will be inserted) is 3. But we need to save (2, trailing) to draw
the cursor at right visual place. Then, do we need to save the cursor position
(where next character will be inserted) plus the (current cursor position,
trailing/leading)? We could generate the cursor position (where next character
will be inserted) from (current cursor position, trailing/leading) pair, but the
position wont be just increased by 1 for complex script. For example, "abcd" in
a complex script, characters a, b, and c are rendered as one grapheme, pointing
to trailing of this grapheme, the pointed to cursor position is  0, but the
logical cursor position (where the next character will be inserted) is 3.

http://codereview.chromium.org/7265011/diff/32001/views/controls/textfield/na...
File views/controls/textfield/native_textfield_views.cc (left):

http://codereview.chromium.org/7265011/diff/32001/views/controls/textfield/na...
views/controls/textfield/native_textfield_views.cc:765: void
NativeTextfieldViews::UpdateCursorBoundsAndTextOffset(size_t cursor_pos,
So, the cursor_bounds_ is updated inside RepaintCursor() and that is the only
place that is needed?

[msw, 2011-07-20 09:22:46]

Thanks for the feedback, let's continue our discussion tomorrow.

http://codereview.chromium.org/7265011/diff/32001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

http://codereview.chromium.org/7265011/diff/32001/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:283: return base::i18n::LEFT_TO_RIGHT;
On 2011/07/20 01:33:33, xji wrote:
> why do we need this? and why it is always LTR?

It's just a stub right now, intended to replace the functionality added in
crrev.com/91881 (discussed in codereview.chromium.org/7324011).

http://codereview.chromium.org/7265011/diff/32001/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

http://codereview.chromium.org/7265011/diff/32001/ui/gfx/render_text.h#newcod...
ui/gfx/render_text.h:159: ui::Range selection_range_;
On 2011/07/20 01:33:33, xji wrote:
> I think we will need the leading or trailing information besides the logical
> index.
> 
> For example, given a logical text "abcDEF" where capital letter stands for
> Hebrew, the visual display is "abcFED".
> 
> According to the bidi editing guide:
> 1. If pointing to the right half of the cell of a LTR character, the current
> position must be set after this character and the caret must be displayed
after
> this character.
> 2. If pointing to the right half of the cell of a RTL character, the current
> position must be set before this character and the caret must be displayed
> before this character.
> 
> So, pointing to the right half of 'c' and pointing to the right half of 'D'
all
> set cursor position as 3.
> But the cursor displayed visually at different places:
> pointing right half of 'c' displays cursor right of 'c',
> pointing right half of 'D' displays cursor right of 'D'.
> 
> In chrome's flow. When pointing using mouse, FindCursorPosition() is called,
and
> it returns the logical position (as 3). Later, GetCursorBounds() will be
called
> to draw the cursor at logical position 3. For the above 2 cases, although the
> logical position are both 3, but the cursor should be dawn in different
places.
> A single cursor position wont be able to achieve this. Instead, we will need
the
> trailing or leading information.
> 
> For example, pointing to right of 'c' put current cursor at trailing position
of
> character at index of 2. pointing to right of 'D' put current cursor at
leading
> position of character of index 3. (note: for a LTR character, right edge is
the
> trailing edge, for a RTL character, right edge is the  leading edge).
> 
> A subsequent question that I am not sure about is that:
> selection_range_'s end is where the next character will be inserted.
> 
> If we save the current cursor position as (cursor_position, trailing/leading)
> pair, the cursor_position means the current cursor  position, which is
different
> from the cursor position where next character will be inserted.
> For the above example, when pointing to right of 'c', the cursor position
(where
> next character will be inserted) is 3. But we need to save (2, trailing) to
draw
> the cursor at right visual place. Then, do we need to save the cursor position
> (where next character will be inserted) plus the (current cursor position,
> trailing/leading)? We could generate the cursor position (where next character
> will be inserted) from (current cursor position, trailing/leading) pair, but
the
> position wont be just increased by 1 for complex script. For example, "abcd"
in
> a complex script, characters a, b, and c are rendered as one grapheme,
pointing
> to trailing of this grapheme, the pointed to cursor position is  0, but the
> logical cursor position (where the next character will be inserted) is 3.

You're right, this model doesn't currently support two potential visual cursor
positions for the same logical index (visually 'abc|FEDghi' and 'abcFED|ghi',
both logically at [3] 'abc|DEFghi'). I think it's bad behavior*. However, we
could implement it by adding a member: either (cursor_position,
leading/trailing) as you suggest, or a visual_cursor_index_ disjoint from the
logical cursor (selection_range_.end()), and adding a function
RenderText::MoveCursorTo(const gfx::Point& point, bool select). Perhaps I'll
just add a note for now, and we can address it later.

*How are we supposed to render the non insert-mode cursor (which surrounds the
glyph) in these cases? Do both display as 'abcFE[D]ghi', even if you click the
right side of 'c'? Also, this behavior makes visual cursors at level changes
ambiguous ('abcFED|ghi' could be from clicking the right of 'c' or the left of
'g'), perhaps we want to show directionality?

http://codereview.chromium.org/7265011/diff/32001/views/controls/textfield/na...
File views/controls/textfield/native_textfield_views.cc (left):

http://codereview.chromium.org/7265011/diff/32001/views/controls/textfield/na...
views/controls/textfield/native_textfield_views.cc:765: void
NativeTextfieldViews::UpdateCursorBoundsAndTextOffset(size_t cursor_pos,
On 2011/07/20 01:33:33, xji wrote:
> So, the cursor_bounds_ is updated inside RepaintCursor() and that is the only
> place that is needed?

The visual cursor bounds aren't cached anywhere currently, they're actually
drawn in RenderText::Draw and used elsewhere; check out
RenderText::GetCursorBounds and its call sites. Their use in RepaintCursor is
just to indicate the dirty region on the scheduled paint call.

[xji, 2011-07-20 19:59:36]

http://codereview.chromium.org/7265011/diff/32001/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

http://codereview.chromium.org/7265011/diff/32001/ui/gfx/render_text.h#newcod...
ui/gfx/render_text.h:159: ui::Range selection_range_;
On 2011/07/20 09:22:46, msw wrote:
> On 2011/07/20 01:33:33, xji wrote:
> > I think we will need the leading or trailing information besides the logical
> > index.
> > 
> > For example, given a logical text "abcDEF" where capital letter stands for
> > Hebrew, the visual display is "abcFED".
> > 
> > According to the bidi editing guide:
> > 1. If pointing to the right half of the cell of a LTR character, the current
> > position must be set after this character and the caret must be displayed
> after
> > this character.
> > 2. If pointing to the right half of the cell of a RTL character, the current
> > position must be set before this character and the caret must be displayed
> > before this character.
> > 
> > So, pointing to the right half of 'c' and pointing to the right half of 'D'
> all
> > set cursor position as 3.
> > But the cursor displayed visually at different places:
> > pointing right half of 'c' displays cursor right of 'c',
> > pointing right half of 'D' displays cursor right of 'D'.
> > 
> > In chrome's flow. When pointing using mouse, FindCursorPosition() is called,
> and
> > it returns the logical position (as 3). Later, GetCursorBounds() will be
> called
> > to draw the cursor at logical position 3. For the above 2 cases, although
the
> > logical position are both 3, but the cursor should be dawn in different
> places.
> > A single cursor position wont be able to achieve this. Instead, we will need
> the
> > trailing or leading information.
> > 
> > For example, pointing to right of 'c' put current cursor at trailing
position
> of
> > character at index of 2. pointing to right of 'D' put current cursor at
> leading
> > position of character of index 3. (note: for a LTR character, right edge is
> the
> > trailing edge, for a RTL character, right edge is the  leading edge).
> > 
> > A subsequent question that I am not sure about is that:
> > selection_range_'s end is where the next character will be inserted.
> > 
> > If we save the current cursor position as (cursor_position,
trailing/leading)
> > pair, the cursor_position means the current cursor  position, which is
> different
> > from the cursor position where next character will be inserted.
> > For the above example, when pointing to right of 'c', the cursor position
> (where
> > next character will be inserted) is 3. But we need to save (2, trailing) to
> draw
> > the cursor at right visual place. Then, do we need to save the cursor
position
> > (where next character will be inserted) plus the (current cursor position,
> > trailing/leading)? We could generate the cursor position (where next
character
> > will be inserted) from (current cursor position, trailing/leading) pair, but
> the
> > position wont be just increased by 1 for complex script. For example, "abcd"
> in
> > a complex script, characters a, b, and c are rendered as one grapheme,
> pointing
> > to trailing of this grapheme, the pointed to cursor position is  0, but the
> > logical cursor position (where the next character will be inserted) is 3.
> 
> You're right, this model doesn't currently support two potential visual cursor
> positions for the same logical index (visually 'abc|FEDghi' and 'abcFED|ghi',
> both logically at [3] 'abc|DEFghi'). I think it's bad behavior*. However, we
> could implement it by adding a member: either (cursor_position,
> leading/trailing) as you suggest, or a visual_cursor_index_ disjoint from the
> logical cursor (selection_range_.end()), and adding a function
> RenderText::MoveCursorTo(const gfx::Point& point, bool select). Perhaps I'll
> just add a note for now, and we can address it later.
> 
I'am resending/republishing my comments since it was mailed correctly but not
published last time.....

in bidi text, the mapping between logical and visual is not one-to-one.
Webkit implemented it as one-to-one mapping, and that caused quite a few
problems, such as you are not able to point the cursor between Arabic number and
Arabic letter when embedding levels are high (because there is no position
points there), the cursor is at the rightmost position when you type Hebrew in
LTR context (findInPage in Linux chrome does the same, but omnibox does it
correctly), extending selection by mouse dragging from the rightmost Hebrew
character in LTR context is not working (the rightmost character is not
selected).

If we support split caret like Pango does (but the consensus from native
speakers is NO), one logical position might rendered with 2 different visual
carets. Otherwise, we need to have a flag to indicate where we want the single
caret to be drawn.

That should be fine to address it later.


> *How are we supposed to render the non insert-mode cursor (which surrounds the
> glyph) in these cases? Do both display as 'abcFE[D]ghi', even if you click the
> right side of 'c'? Also, this behavior makes visual cursors at level changes
> ambiguous ('abcFED|ghi' could be from clicking the right of 'c' or the left of
> 'g'), perhaps we want to show directionality?

I did not quite get the idea of render cursor in non-insert-mode.
Does it render cursor as a selection of (cursor_position, cursor_position + 1)?


If the cursor is rendered as a selection of (3, 4), yes, [D] should be displayed
even if you click the right side of 'c'.
It is similar as if you type in a Hebrew character X (no matter after you right
click 'c' or right click 'D'), X will always be rendered at right of 'D' because
the logical position is 3 before insert 'X'. But although the logical position
is 3, the visual display of cursor is different if click right of 'c' and right
of 'D'.


Visual cursor is also mapped to multiple logical positions.
"abcFEDghi", click right of 'D' put logical position as 3, click left of 'g' put
logical position as 6. But the cursor display between 'D' and 'g' for both.

That is confusion, and it is one of the discussions in bidi editing standard. We
will need an indicator to indicate the logical position. But I do not think I
will implement that in my 1st version.

[msw, 2011-07-21 00:19:55]

Please take a look, thanks!

http://codereview.chromium.org/7265011/diff/32001/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

http://codereview.chromium.org/7265011/diff/32001/ui/gfx/render_text.h#newcod...
ui/gfx/render_text.h:159: ui::Range selection_range_;
On 2011/07/20 19:59:36, xji wrote:
> On 2011/07/20 09:22:46, msw wrote:
> > On 2011/07/20 01:33:33, xji wrote:
> > > I think we will need the leading or trailing information besides the
logical
> > > index.
> > > 
> > > For example, given a logical text "abcDEF" where capital letter stands for
> > > Hebrew, the visual display is "abcFED".
> > > 
> > > According to the bidi editing guide:
> > > 1. If pointing to the right half of the cell of a LTR character, the
current
> > > position must be set after this character and the caret must be displayed
> > after
> > > this character.
> > > 2. If pointing to the right half of the cell of a RTL character, the
current
> > > position must be set before this character and the caret must be displayed
> > > before this character.
> > > 
> > > So, pointing to the right half of 'c' and pointing to the right half of
'D'
> > all
> > > set cursor position as 3.
> > > But the cursor displayed visually at different places:
> > > pointing right half of 'c' displays cursor right of 'c',
> > > pointing right half of 'D' displays cursor right of 'D'.
> > > 
> > > In chrome's flow. When pointing using mouse, FindCursorPosition() is
called,
> > and
> > > it returns the logical position (as 3). Later, GetCursorBounds() will be
> > called
> > > to draw the cursor at logical position 3. For the above 2 cases, although
> the
> > > logical position are both 3, but the cursor should be dawn in different
> > places.
> > > A single cursor position wont be able to achieve this. Instead, we will
need
> > the
> > > trailing or leading information.
> > > 
> > > For example, pointing to right of 'c' put current cursor at trailing
> position
> > of
> > > character at index of 2. pointing to right of 'D' put current cursor at
> > leading
> > > position of character of index 3. (note: for a LTR character, right edge
is
> > the
> > > trailing edge, for a RTL character, right edge is the  leading edge).
> > > 
> > > A subsequent question that I am not sure about is that:
> > > selection_range_'s end is where the next character will be inserted.
> > > 
> > > If we save the current cursor position as (cursor_position,
> trailing/leading)
> > > pair, the cursor_position means the current cursor  position, which is
> > different
> > > from the cursor position where next character will be inserted.
> > > For the above example, when pointing to right of 'c', the cursor position
> > (where
> > > next character will be inserted) is 3. But we need to save (2, trailing)
to
> > draw
> > > the cursor at right visual place. Then, do we need to save the cursor
> position
> > > (where next character will be inserted) plus the (current cursor position,
> > > trailing/leading)? We could generate the cursor position (where next
> character
> > > will be inserted) from (current cursor position, trailing/leading) pair,
but
> > the
> > > position wont be just increased by 1 for complex script. For example,
"abcd"
> > in
> > > a complex script, characters a, b, and c are rendered as one grapheme,
> > pointing
> > > to trailing of this grapheme, the pointed to cursor position is  0, but
the
> > > logical cursor position (where the next character will be inserted) is 3.
> > 
> > You're right, this model doesn't currently support two potential visual
cursor
> > positions for the same logical index (visually 'abc|FEDghi' and
'abcFED|ghi',
> > both logically at [3] 'abc|DEFghi'). I think it's bad behavior*. However, we
> > could implement it by adding a member: either (cursor_position,
> > leading/trailing) as you suggest, or a visual_cursor_index_ disjoint from
the
> > logical cursor (selection_range_.end()), and adding a function
> > RenderText::MoveCursorTo(const gfx::Point& point, bool select). Perhaps I'll
> > just add a note for now, and we can address it later.
> > 
> I'am resending/republishing my comments since it was mailed correctly but not
> published last time.....
> 
> in bidi text, the mapping between logical and visual is not one-to-one.
> Webkit implemented it as one-to-one mapping, and that caused quite a few
> problems, such as you are not able to point the cursor between Arabic number
and
> Arabic letter when embedding levels are high (because there is no position
> points there), the cursor is at the rightmost position when you type Hebrew in
> LTR context (findInPage in Linux chrome does the same, but omnibox does it
> correctly), extending selection by mouse dragging from the rightmost Hebrew
> character in LTR context is not working (the rightmost character is not
> selected).
> 
> If we support split caret like Pango does (but the consensus from native
> speakers is NO), one logical position might rendered with 2 different visual
> carets. Otherwise, we need to have a flag to indicate where we want the single
> caret to be drawn.
> 
> That should be fine to address it later.
> 
> 
> > *How are we supposed to render the non insert-mode cursor (which surrounds
the
> > glyph) in these cases? Do both display as 'abcFE[D]ghi', even if you click
the
> > right side of 'c'? Also, this behavior makes visual cursors at level changes
> > ambiguous ('abcFED|ghi' could be from clicking the right of 'c' or the left
of
> > 'g'), perhaps we want to show directionality?
> 
> I did not quite get the idea of render cursor in non-insert-mode.
> Does it render cursor as a selection of (cursor_position, cursor_position +
1)?
> 
> 
> If the cursor is rendered as a selection of (3, 4), yes, [D] should be
displayed
> even if you click the right side of 'c'.
> It is similar as if you type in a Hebrew character X (no matter after you
right
> click 'c' or right click 'D'), X will always be rendered at right of 'D'
because
> the logical position is 3 before insert 'X'. But although the logical position
> is 3, the visual display of cursor is different if click right of 'c' and
right
> of 'D'.
> 
> 
> Visual cursor is also mapped to multiple logical positions.
> "abcFEDghi", click right of 'D' put logical position as 3, click left of 'g'
put
> logical position as 6. But the cursor display between 'D' and 'g' for both.
> 
> That is confusion, and it is one of the discussions in bidi editing standard.
We
> will need an indicator to indicate the logical position. But I do not think I
> will implement that in my 1st version.

Yes, the non insert-mode cursor is essentially a movable selection.
I've added a pair of TODO notes with explanations for now.

[xji, 2011-07-21 17:16:03]

LGTM. 
Thanks, Michael!

[msw, 2011-07-21 17:59:48]

oshima: ping!

[xji, 2011-07-21 20:02:08]

http://codereview.chromium.org/7265011/diff/41004/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

http://codereview.chromium.org/7265011/diff/41004/ui/gfx/render_text.h#newcode85
ui/gfx/render_text.h:85: bool get_cursor_visible() { return is_cursor_visible_;
}
bool get_cursor_visible() const {...}

http://codereview.chromium.org/7265011/diff/41004/ui/gfx/render_text.h#newcode91
ui/gfx/render_text.h:91: bool get_focused() { return is_focused_; }
ditto

http://codereview.chromium.org/7265011/diff/41004/ui/gfx/render_text.h#newcode94
ui/gfx/render_text.h:94: const StyleRange& get_default_style() { return
default_style_; }
ditto

[oshima, 2011-07-23 09:51:12]

LGTM, once you address comments. No need to wait for anther LGTM.

http://codereview.chromium.org/7265011/diff/32001/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

http://codereview.chromium.org/7265011/diff/32001/ui/gfx/render_text.h#newcode34
ui/gfx/render_text.h:34: 
move constructor to .cc as this has non trivial objects.

http://codereview.chromium.org/7265011/diff/41004/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

http://codereview.chromium.org/7265011/diff/41004/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:637: if (emphasize) {
While I understand what it's doing, it is now a bit harder to follow, probably
because this is now more procedural while it was more declarative.
 Could you try to come up with better way?

http://codereview.chromium.org/7265011/diff/41004/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

http://codereview.chromium.org/7265011/diff/41004/ui/gfx/render_text.h#newcode9
ui/gfx/render_text.h:9: #include <string>
isn't string16 enough?

http://codereview.chromium.org/7265011/diff/41004/ui/gfx/render_text.h#newcode44
ui/gfx/render_text.h:44: StyleRange()
move this to .cc as it has non trivial member variables.

http://codereview.chromium.org/7265011/diff/41004/ui/gfx/render_text.h#newcode85
ui/gfx/render_text.h:85: bool get_cursor_visible() { return is_cursor_visible_;
}
On 2011/07/21 20:02:08, xji wrote:
> bool get_cursor_visible() const {...}

and getter method name should be is_cursor_visible() per style guide.

http://codereview.chromium.org/7265011/diff/41004/ui/gfx/render_text.h#newcode88
ui/gfx/render_text.h:88: bool get_insert_mode() const { return insert_mode_; }
insert_mode()

http://codereview.chromium.org/7265011/diff/41004/ui/gfx/render_text.h#newcode91
ui/gfx/render_text.h:91: bool get_focused() { return is_focused_; }
On 2011/07/21 20:02:08, xji wrote:
> ditto

ditto

http://codereview.chromium.org/7265011/diff/41004/ui/gfx/render_text.h#newcode94
ui/gfx/render_text.h:94: const StyleRange& get_default_style() { return
default_style_; }
On 2011/07/21 20:02:08, xji wrote:
> ditto

ditto

http://codereview.chromium.org/7265011/diff/41004/ui/gfx/render_text.h#newcode97
ui/gfx/render_text.h:97: const gfx::Rect& get_display_rect() const { return
display_rect_; }
display_rect()

http://codereview.chromium.org/7265011/diff/41004/ui/gfx/render_text.h#newcod...
ui/gfx/render_text.h:101: void SetCursor(const size_t position);
Cursor implies the type of cursor, so let's call it SetCursorPosition.

http://codereview.chromium.org/7265011/diff/41004/ui/gfx/render_text.h#newcod...
ui/gfx/render_text.h:136: virtual void ApplyStyleRange(StyleRange style_range);
 new line

http://codereview.chromium.org/7265011/diff/41004/ui/gfx/render_text.h#newcod...
ui/gfx/render_text.h:166: const StyleRanges& get_style_ranges() const { return
style_ranges_; }
ditto

http://codereview.chromium.org/7265011/diff/41004/ui/gfx/render_text.h#newcod...
ui/gfx/render_text.h:168: const gfx::Point& get_display_offset() const { return
display_offset_; }
ditto

http://codereview.chromium.org/7265011/diff/41004/ui/ui.gyp
File ui/ui.gyp (right):

http://codereview.chromium.org/7265011/diff/41004/ui/ui.gyp#newcode384
ui/ui.gyp:384: ],
we don't need for non views linux, do we?

http://codereview.chromium.org/7265011/diff/41004/views/controls/textfield/te...
File views/controls/textfield/textfield_views_model.cc (right):

http://codereview.chromium.org/7265011/diff/41004/views/controls/textfield/te...
views/controls/textfield/textfield_views_model.cc:287: delete render_text_;
why not scoped_ptr?

http://codereview.chromium.org/7265011/diff/41004/views/controls/textfield/te...
views/controls/textfield/textfield_views_model.cc:497:
render_text_->SetSelection(ui::Range(selection.end(), selection.start()));
doesn't range have a method to swap position?

http://codereview.chromium.org/7265011/diff/41004/views/controls/textfield/te...
views/controls/textfield/textfield_views_model.cc:642: else if (!HasSelection())
{
you need {} for the 1st if, i believe. check style guide.

http://codereview.chromium.org/7265011/diff/41004/views/controls/textfield/te...
File views/controls/textfield/textfield_views_model.h (right):

http://codereview.chromium.org/7265011/diff/41004/views/controls/textfield/te...
views/controls/textfield/textfield_views_model.h:85: gfx::RenderText*
get_render_text() { return render_text_; }
ditto

[msw, 2011-07-25 05:09:53]

Comments addressed, landing.

http://codereview.chromium.org/7265011/diff/41004/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

http://codereview.chromium.org/7265011/diff/41004/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:637: if (emphasize) {
On 2011/07/23 09:51:12, oshima wrote:
> While I understand what it's doing, it is now a bit harder to follow, probably
> because this is now more procedural while it was more declarative.
>  Could you try to come up with better way?

Done.

http://codereview.chromium.org/7265011/diff/41004/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

http://codereview.chromium.org/7265011/diff/41004/ui/gfx/render_text.h#newcode9
ui/gfx/render_text.h:9: #include <string>
On 2011/07/23 09:51:12, oshima wrote:
> isn't string16 enough?

Done.

http://codereview.chromium.org/7265011/diff/41004/ui/gfx/render_text.h#newcode44
ui/gfx/render_text.h:44: StyleRange()
On 2011/07/23 09:51:12, oshima wrote:
> move this to .cc as it has non trivial member variables.

Done.

http://codereview.chromium.org/7265011/diff/41004/ui/gfx/render_text.h#newcode85
ui/gfx/render_text.h:85: bool get_cursor_visible() { return is_cursor_visible_;
}
On 2011/07/21 20:02:08, xji wrote:
> bool get_cursor_visible() const {...}

Done.

http://codereview.chromium.org/7265011/diff/41004/ui/gfx/render_text.h#newcode85
ui/gfx/render_text.h:85: bool get_cursor_visible() { return is_cursor_visible_;
}
On 2011/07/23 09:51:12, oshima wrote:
> On 2011/07/21 20:02:08, xji wrote:
> > bool get_cursor_visible() const {...}
> 
> and getter method name should be is_cursor_visible() per style guide.

Done.

http://codereview.chromium.org/7265011/diff/41004/ui/gfx/render_text.h#newcode88
ui/gfx/render_text.h:88: bool get_insert_mode() const { return insert_mode_; }
On 2011/07/23 09:51:12, oshima wrote:
> insert_mode()

Done.

http://codereview.chromium.org/7265011/diff/41004/ui/gfx/render_text.h#newcode91
ui/gfx/render_text.h:91: bool get_focused() { return is_focused_; }
On 2011/07/21 20:02:08, xji wrote:
> ditto

Done.

http://codereview.chromium.org/7265011/diff/41004/ui/gfx/render_text.h#newcode91
ui/gfx/render_text.h:91: bool get_focused() { return is_focused_; }
On 2011/07/23 09:51:12, oshima wrote:
> On 2011/07/21 20:02:08, xji wrote:
> > ditto
> 
> ditto

Done.

http://codereview.chromium.org/7265011/diff/41004/ui/gfx/render_text.h#newcode94
ui/gfx/render_text.h:94: const StyleRange& get_default_style() { return
default_style_; }
On 2011/07/21 20:02:08, xji wrote:
> ditto

Done.

http://codereview.chromium.org/7265011/diff/41004/ui/gfx/render_text.h#newcode94
ui/gfx/render_text.h:94: const StyleRange& get_default_style() { return
default_style_; }
On 2011/07/23 09:51:12, oshima wrote:
> On 2011/07/21 20:02:08, xji wrote:
> > ditto
> 
> ditto

Done.

http://codereview.chromium.org/7265011/diff/41004/ui/gfx/render_text.h#newcode97
ui/gfx/render_text.h:97: const gfx::Rect& get_display_rect() const { return
display_rect_; }
On 2011/07/23 09:51:12, oshima wrote:
> display_rect()

Done.

http://codereview.chromium.org/7265011/diff/41004/ui/gfx/render_text.h#newcod...
ui/gfx/render_text.h:101: void SetCursor(const size_t position);
On 2011/07/23 09:51:12, oshima wrote:
> Cursor implies the type of cursor, so let's call it SetCursorPosition.

Done.

http://codereview.chromium.org/7265011/diff/41004/ui/gfx/render_text.h#newcod...
ui/gfx/render_text.h:136: virtual void ApplyStyleRange(StyleRange style_range);
On 2011/07/23 09:51:12, oshima wrote:
>  new line

Done.

http://codereview.chromium.org/7265011/diff/41004/ui/gfx/render_text.h#newcod...
ui/gfx/render_text.h:166: const StyleRanges& get_style_ranges() const { return
style_ranges_; }
On 2011/07/23 09:51:12, oshima wrote:
> ditto

Done.

http://codereview.chromium.org/7265011/diff/41004/ui/gfx/render_text.h#newcod...
ui/gfx/render_text.h:168: const gfx::Point& get_display_offset() const { return
display_offset_; }
On 2011/07/23 09:51:12, oshima wrote:
> ditto

Done.

http://codereview.chromium.org/7265011/diff/41004/ui/ui.gyp
File ui/ui.gyp (right):

http://codereview.chromium.org/7265011/diff/41004/ui/ui.gyp#newcode384
ui/ui.gyp:384: ],
On 2011/07/23 09:51:12, oshima wrote:
> we don't need for non views linux, do we?

Ok, I excluded these from all non-views builds.
I originally just excluded mac to avoid compile failures.

http://codereview.chromium.org/7265011/diff/41004/views/controls/textfield/te...
File views/controls/textfield/textfield_views_model.cc (right):

http://codereview.chromium.org/7265011/diff/41004/views/controls/textfield/te...
views/controls/textfield/textfield_views_model.cc:287: delete render_text_;
On 2011/07/23 09:51:12, oshima wrote:
> why not scoped_ptr?

Done.

http://codereview.chromium.org/7265011/diff/41004/views/controls/textfield/te...
views/controls/textfield/textfield_views_model.cc:497:
render_text_->SetSelection(ui::Range(selection.end(), selection.start()));
On 2011/07/23 09:51:12, oshima wrote:
> doesn't range have a method to swap position?

Not that I see.

http://codereview.chromium.org/7265011/diff/41004/views/controls/textfield/te...
views/controls/textfield/textfield_views_model.cc:642: else if (!HasSelection())
{
On 2011/07/23 09:51:12, oshima wrote:
> you need {} for the 1st if, i believe. check style guide.

Done.

http://codereview.chromium.org/7265011/diff/41004/views/controls/textfield/te...
File views/controls/textfield/textfield_views_model.h (right):

http://codereview.chromium.org/7265011/diff/41004/views/controls/textfield/te...
views/controls/textfield/textfield_views_model.h:85: gfx::RenderText*
get_render_text() { return render_text_; }
On 2011/07/23 09:51:12, oshima wrote:
> ditto

Done.

[xji, 2011-07-25 20:42:59]

On 2011/07/20 09:22:46, msw wrote:

> On 2011/07/20 01:33:33, xji wrote:
> > So, the cursor_bounds_ is updated inside RepaintCursor() and that is the
only
> > place that is needed?
> 
> The visual cursor bounds aren't cached anywhere currently, they're actually
> drawn in RenderText::Draw and used elsewhere; check out
> RenderText::GetCursorBounds and its call sites. Their use in RepaintCursor is
> just to indicate the dirty region on the scheduled paint call.


I know it is committed, but I guess I can still type in comments here, right?

Sorry that I just noticed that we have to cache cursor_bounds.
If not, when we mouse over the text and stops the mouse at somewhere inside the
text *without* mouse click, the cursor will be drawn at where we stopped mouse
at, which is the latest cursor position computed by FindCursorPosition() during
mouse movement.

Previously, cursor_bounds is updated in UpdateCursorBoundsAndTextOffset() and
UpdateCursorBoundsAndTextOffset() is only called in several events.

[msw, 2011-07-27 08:31:44]

Trung, this landed (r93855), but could you review .gyp[i]s?

ui/ui.gyp
ui/ui_unittests.gypi
views/views.gyp

I'm curious, since you were evaluating performance and style on these and I
don't have much gyp experience.

[viettrungluu, 2011-07-27 17:10:28]

http://codereview.chromium.org/7265011/diff/46004/ui/ui.gyp
File ui/ui.gyp (right):

http://codereview.chromium.org/7265011/diff/46004/ui/ui.gyp#newcode407
ui/ui.gyp:407: ['exclude', '^gfx/render_text.cc'],
Don't do this. Each of these regexes needs to be applied to each element of the
sources list. (Plus the '.'s match anything and theoretically these regexes
would also match filenames with more stuff appended.)

Use a 'sources!' list (see other examples in this file).