Implement eliding/truncating at end in RenderText

Implement eliding/truncating at end in RenderText


The eliding is determined using binary search similar to the ElideText
in ui/gfx/text_elider.cc.

The mixed LTR-RTL handling for ellipsis is done using LTR/RTL markers.
There is no other way of rendering the ellipsis with the directionality
of preceding strong-directional characters. Previous scheme worked
because it rendered LTR and RTL sub-strings as different RenderText
instances.

Added helper method to be able to determine directionality of the
trailing text.

Made StringSlicer used by text_elider.cc public to be shared by RenderText.

Additional Fix:
- Changed ellipsis width check to use render text (render_Text.cc)
- Modify tests to never have a scenario where the input is less wide than the elided.

BUG=327833
TEST=ui_unittests,base_unittests
R=msw@chromium.org
TBR=jshin@chromium.org,pkasting@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=242828

[Anuj, 2013-12-20 06:24:37]

Created Revert of Revert of Implement eliding/truncating at end in RenderText

[Anuj, 2013-12-20 07:03:28]

Patch Set 1 was created by "Revert of Revert".

Patch Set 2 has additional changes in
- render_text.cc : 
     - Use RenderText to determine lone ellipsis width
     - Additional checks for loop limits.

- render_text_unittest.cc :
     - Modified tests to have input text always cause elision when expected by
appending a long string.


Tested on Windows 8

[Anuj, 2013-12-20 08:25:32]

FYI

https://codereview.chromium.org/119813002/diff/4/chrome/browser/ui/views/omni...
File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right):

https://codereview.chromium.org/119813002/diff/4/chrome/browser/ui/views/omni...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:397: (remaining_width <
render_text->GetContentWidth())) {
Will remove this block.

https://codereview.chromium.org/119813002/diff/4/chrome/browser/ui/views/omni...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:402:
render_text->SetElideBehavior(gfx::ELIDE_AT_END);
I haven't made this change, because my next CL needs it.

[Anuj, 2013-12-26 22:39:08]

Awaiting review. PTAL.

[msw, 2013-12-27 22:54:32]

https://codereview.chromium.org/119813002/diff/4/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/119813002/diff/4/ui/gfx/render_text.cc#newcod...
ui/gfx/render_text.cc:1151: if (text.empty())
nit: consider an early return (text or empty) for display_rect_.width() == 0.

https://codereview.chromium.org/119813002/diff/4/ui/gfx/render_text.cc#newcod...
ui/gfx/render_text.cc:1190: 
nit: remove the second blank line here.

https://codereview.chromium.org/119813002/diff/4/ui/gfx/render_text.cc#newcod...
ui/gfx/render_text.cc:1195: for (size_t guess = (lo + hi) / 2; (lo <= hi) &&
(guess < text.length());
When would guess reasonably exceed the text length? Can you solve this correctly
rather than patching it over as such? A [D]CHECK + TODO inside the loop would be
better than this and perhaps sufficient for now.

https://codereview.chromium.org/119813002/diff/4/ui/gfx/render_text.cc#newcod...
ui/gfx/render_text.cc:1231: // This shouldn't happen, but lets be paranoid.
nit: let's, but I'd prefer a real fix (or perhaps a [D]CHECK + TODO for now).
I'm also confused as to how this occurs, were you able to repro this and the
ensuing underflow described at my my comment
<https://codereview.chromium.org/107513011/diff/140001/ui/gfx/render_text.cc#n...
With guess==0, the string should be empty (or just an ellipsis), and neither of
those should be wider than the available width (unless perhaps the
cursor_enabled_ is true and possibly when the display rect has 0 width). I
wonder if this should return |insert_ellipsis ? ellipsis : base::string16()|,
since the check at 1188 should ensure that an ellipsis on its own does not
exceed the available width.

https://codereview.chromium.org/119813002/diff/4/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

https://codereview.chromium.org/119813002/diff/4/ui/gfx/render_text.h#newcode234
ui/gfx/render_text.h:234: // If both truncate and elide are specified, the
shorter of the two will be
nit: wrap this above

https://codereview.chromium.org/119813002/diff/4/ui/gfx/render_text_unittest.cc
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/119813002/diff/4/ui/gfx/render_text_unittest....
ui/gfx/render_text_unittest.cc:416: { L" . ",     L" . "    , false },
nit: this case is equivalent to kWeak, right? Remove it.

https://codereview.chromium.org/119813002/diff/4/ui/gfx/render_text_unittest....
ui/gfx/render_text_unittest.cc:453: for (size_t i = 1; i <
ARRAYSIZE_UNSAFE(cases); i++) {
Why do you skip the first test case of an empty string?

https://codereview.chromium.org/119813002/diff/4/ui/gfx/render_text_unittest....
ui/gfx/render_text_unittest.cc:459: // Extend input to always be wide enough to
be elided.
nit: add to this comment ", otherwise the layout text with its ellipsis may be
wider than the input text." Really, I wish the test cases were better designed
to avoid this workaround, but oh well...

https://codereview.chromium.org/119813002/diff/4/ui/gfx/render_text_unittest....
ui/gfx/render_text_unittest.cc:461: input += WideToUTF16(L" MMMMMMMMMMM");
Use append, and shouldn't this use characters with no/weak directionality?

https://codereview.chromium.org/119813002/diff/4/ui/gfx/text_elider.cc
File ui/gfx/text_elider.cc (right):

https://codereview.chromium.org/119813002/diff/4/ui/gfx/text_elider.cc#newcod...
ui/gfx/text_elider.cc:484: hi = guess - 1;
Shouldn't this text_elider version of the eliding code get the same fix as your
new RenderText version. Though really, I hope this is removed soon...

[Anuj, 2013-12-30 22:46:57]

https://codereview.chromium.org/119813002/diff/4/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/119813002/diff/4/ui/gfx/render_text.cc#newcod...
ui/gfx/render_text.cc:1151: if (text.empty())
That check exists on line 1137. I think this is fine, although the early return
conditions are split in two places. If you feel strongly about it, I will like
to move all of them together at one place - here at the beginning of the method,
or before the call to this method.

https://codereview.chromium.org/119813002/diff/4/ui/gfx/render_text.cc#newcod...
ui/gfx/render_text.cc:1190: 
On 2013/12/27 22:54:33, msw wrote:
> nit: remove the second blank line here.

Done.

https://codereview.chromium.org/119813002/diff/4/ui/gfx/render_text.cc#newcod...
ui/gfx/render_text.cc:1195: for (size_t guess = (lo + hi) / 2; (lo <= hi) &&
(guess < text.length());
Given guess will be used as an index of text, I think this constraint is a good
idea.

https://codereview.chromium.org/119813002/diff/4/ui/gfx/render_text.cc#newcod...
ui/gfx/render_text.cc:1231: // This shouldn't happen, but lets be paranoid.
I was able to reproduce the issue you described in your earlier comment. This
won't happen anymore because of the check on line 1188 for ellipsis_width. For
some reason (likely cursor_enabled_, but I didn't confirm), the width of
ellipsis as determined by GetStringSize is different from that determined
through a RenderText instance. But now the ellipsis width is determined using
RenderText instance.

That said, for the completeness and correctness of this binary-search for-loop,
I think it is good to have this check. At this point, the guess_width would
correspond to a render_text which contains just an ellipsis when guess == 0 and
insert_ellipsis is true. So it should return an empty string.

I have modified how I check for guess being in correct bounds of the length of
text.

https://codereview.chromium.org/119813002/diff/4/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

https://codereview.chromium.org/119813002/diff/4/ui/gfx/render_text.h#newcode234
ui/gfx/render_text.h:234: // If both truncate and elide are specified, the
shorter of the two will be
On 2013/12/27 22:54:33, msw wrote:
> nit: wrap this above 

Done.

https://codereview.chromium.org/119813002/diff/4/ui/gfx/render_text_unittest.cc
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/119813002/diff/4/ui/gfx/render_text_unittest....
ui/gfx/render_text_unittest.cc:416: { L" . ",     L" . "    , false },
On 2013/12/27 22:54:33, msw wrote:
> nit: this case is equivalent to kWeak, right? Remove it.

Done.

https://codereview.chromium.org/119813002/diff/4/ui/gfx/render_text_unittest....
ui/gfx/render_text_unittest.cc:453: for (size_t i = 1; i <
ARRAYSIZE_UNSAFE(cases); i++) {
Fixed. I was skipping it for testing the testcases. :)

https://codereview.chromium.org/119813002/diff/4/ui/gfx/render_text_unittest....
ui/gfx/render_text_unittest.cc:459: // Extend input to always be wide enough to
be elided.
Reworded the comment. I think this is a reasonable implementation of tests given
the variables of font metrics.

https://codereview.chromium.org/119813002/diff/4/ui/gfx/render_text_unittest....
ui/gfx/render_text_unittest.cc:461: input += WideToUTF16(L" MMMMMMMMMMM");
I am curious about preference for append? The two are perfectly equivalent
(https://code.google.com/p/chromium/codesearch#chromium/usr/include/c%2B%2B/4....)

M is better because it is wider (if not the widest). The directionality doesn't
matter as it will all be truncated.

https://codereview.chromium.org/119813002/diff/4/ui/gfx/text_elider.cc
File ui/gfx/text_elider.cc (right):

https://codereview.chromium.org/119813002/diff/4/ui/gfx/text_elider.cc#newcod...
ui/gfx/text_elider.cc:484: hi = guess - 1;
Done. I also want to get rid of this soon. Just resisting the temptation to pull
that change in this CL to keep scope of this CL small.

[msw, 2013-12-30 23:17:59]

https://codereview.chromium.org/119813002/diff/4/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/119813002/diff/4/ui/gfx/render_text.cc#newcod...
ui/gfx/render_text.cc:1151: if (text.empty())
On 2013/12/30 22:46:58, Anuj wrote:
> That check exists on line 1137. I think this is fine, although the early
return
> conditions are split in two places. If you feel strongly about it, I will like
> to move all of them together at one place - here at the beginning of the
method,
> or before the call to this method.

Please don't delete my original comments when replying; it's frustrating to read
replies without context, or to dig into each file to revive that context.
You can move this check out to UpdateLayoutText.

https://codereview.chromium.org/119813002/diff/4/ui/gfx/render_text.cc#newcod...
ui/gfx/render_text.cc:1231: // This shouldn't happen, but lets be paranoid.
On 2013/12/30 22:46:58, Anuj wrote:
> I was able to reproduce the issue you described in your earlier comment. This
> won't happen anymore because of the check on line 1188 for ellipsis_width. For
> some reason (likely cursor_enabled_, but I didn't confirm), the width of
> ellipsis as determined by GetStringSize is different from that determined
> through a RenderText instance. But now the ellipsis width is determined using
> RenderText instance.
> 
> That said, for the completeness and correctness of this binary-search
for-loop,
> I think it is good to have this check. At this point, the guess_width would
> correspond to a render_text which contains just an ellipsis when guess == 0
and
> insert_ellipsis is true. So it should return an empty string.
> 
> I have modified how I check for guess being in correct bounds of the length of
> text.
> 

I'm glad the revised ellipsis width checking fixed the underflow issue thanks.

https://codereview.chromium.org/119813002/diff/4/ui/gfx/render_text_unittest.cc
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/119813002/diff/4/ui/gfx/render_text_unittest....
ui/gfx/render_text_unittest.cc:461: input += WideToUTF16(L" MMMMMMMMMMM");
On 2013/12/30 22:46:58, Anuj wrote:
> M is better because it is wider (if not the widest). The directionality
doesn't
> matter as it will all be truncated.
Don't you check the trailing directionality of the elided string and make sure
that it matches the input string by adding marks? I would avoid modifying the
trailing directionality here.

https://codereview.chromium.org/119813002/diff/210002/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/119813002/diff/210002/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:1194: for (size_t guess = (lo + hi) / 2; (lo <= hi); guess
= (lo + hi) / 2) {
nit: remove the parens around lo <= hi

https://codereview.chromium.org/119813002/diff/210002/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:1196: if (guess < 0)
Guess will *never* be <0, it's unsigned... remove this.

https://codereview.chromium.org/119813002/diff/210002/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:1199: if (guess >= text.length())
Perhaps my previous question was unclear. Under what conditions will |guess| be
assigned a value greater than or equal to the text length? Shouldn't that be
impossible? I think this should be removed.

https://codereview.chromium.org/119813002/diff/210002/ui/gfx/render_text_unit...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/119813002/diff/210002/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:458: // Extend the text of testcase to ensure
that it is wider than the
nit: s/text of testcase/input text/

https://codereview.chromium.org/119813002/diff/210002/ui/gfx/text_elider.cc
File ui/gfx/text_elider.cc (right):

https://codereview.chromium.org/119813002/diff/210002/ui/gfx/text_elider.cc#n...
ui/gfx/text_elider.cc:475: if (guess < 0)
ditto, remove these checks to be consistent with the other impl.

[Anuj, 2013-12-31 00:04:03]

https://codereview.chromium.org/119813002/diff/4/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/119813002/diff/4/ui/gfx/render_text.cc#newcod...
ui/gfx/render_text.cc:1151: if (text.empty())
On 2013/12/30 23:17:59, msw wrote:
> On 2013/12/30 22:46:58, Anuj wrote:
> > That check exists on line 1137. I think this is fine, although the early
> return
> > conditions are split in two places. If you feel strongly about it, I will
like
> > to move all of them together at one place - here at the beginning of the
> method,
> > or before the call to this method.
> 
> Please don't delete my original comments when replying; it's frustrating to
read
> replies without context, or to dig into each file to revive that context.
> You can move this check out to UpdateLayoutText.

I think that comments are insufficient context, and if you want code in context
too, you have to dig into each file. I will try not to remove your comments from
context, but no promises because I think they are ugly/redundant.

https://codereview.chromium.org/119813002/diff/4/ui/gfx/render_text.cc#newcod...
ui/gfx/render_text.cc:1231: // This shouldn't happen, but lets be paranoid.
On 2013/12/30 23:17:59, msw wrote:
> On 2013/12/30 22:46:58, Anuj wrote:
> > I was able to reproduce the issue you described in your earlier comment.
This
> > won't happen anymore because of the check on line 1188 for ellipsis_width.
For
> > some reason (likely cursor_enabled_, but I didn't confirm), the width of
> > ellipsis as determined by GetStringSize is different from that determined
> > through a RenderText instance. But now the ellipsis width is determined
using
> > RenderText instance.
> > 
> > That said, for the completeness and correctness of this binary-search
> for-loop,
> > I think it is good to have this check. At this point, the guess_width would
> > correspond to a render_text which contains just an ellipsis when guess == 0
> and
> > insert_ellipsis is true. So it should return an empty string.
> > 
> > I have modified how I check for guess being in correct bounds of the length
of
> > text.
> > 
> 
> I'm glad the revised ellipsis width checking fixed the underflow issue thanks.

Done.

https://codereview.chromium.org/119813002/diff/4/ui/gfx/render_text_unittest.cc
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/119813002/diff/4/ui/gfx/render_text_unittest....
ui/gfx/render_text_unittest.cc:461: input += WideToUTF16(L" MMMMMMMMMMM");
On 2013/12/30 23:17:59, msw wrote:
> On 2013/12/30 22:46:58, Anuj wrote:
> > M is better because it is wider (if not the widest). The directionality
> doesn't
> > matter as it will all be truncated.
> Don't you check the trailing directionality of the elided string and make sure
> that it matches the input string by adding marks? I would avoid modifying the
> trailing directionality here.

I do. But the string which remains matters. This part will be removed, and its
directionality doesn't matter.

https://codereview.chromium.org/119813002/diff/210002/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/119813002/diff/210002/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:1194: for (size_t guess = (lo + hi) / 2; (lo <= hi); guess
= (lo + hi) / 2) {
On 2013/12/30 23:18:00, msw wrote:
> nit: remove the parens around lo <= hi

Done.

https://codereview.chromium.org/119813002/diff/210002/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:1196: if (guess < 0)
On 2013/12/30 23:18:00, msw wrote:
> Guess will *never* be <0, it's unsigned... remove this.

Done.

https://codereview.chromium.org/119813002/diff/210002/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:1199: if (guess >= text.length())
On 2013/12/30 23:18:00, msw wrote:
> Perhaps my previous question was unclear. Under what conditions will |guess|
be
> assigned a value greater than or equal to the text length? Shouldn't that be
> impossible? I think this should be removed.

Done.

https://codereview.chromium.org/119813002/diff/210002/ui/gfx/render_text_unit...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/119813002/diff/210002/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:458: // Extend the text of testcase to ensure
that it is wider than the
On 2013/12/30 23:18:00, msw wrote:
> nit: s/text of testcase/input text/

Done.

https://codereview.chromium.org/119813002/diff/210002/ui/gfx/text_elider.cc
File ui/gfx/text_elider.cc (right):

https://codereview.chromium.org/119813002/diff/210002/ui/gfx/text_elider.cc#n...
ui/gfx/text_elider.cc:475: if (guess < 0)
On 2013/12/30 23:18:00, msw wrote:
> ditto, remove these checks to be consistent with the other impl.

Done.

[msw, 2013-12-31 00:24:52]

LGTM with a q/nit.

https://codereview.chromium.org/119813002/diff/4/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/119813002/diff/4/ui/gfx/render_text.cc#newcod...
ui/gfx/render_text.cc:1151: if (text.empty())
ok, lol

https://codereview.chromium.org/119813002/diff/4/ui/gfx/render_text_unittest.cc
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/119813002/diff/4/ui/gfx/render_text_unittest....
ui/gfx/render_text_unittest.cc:461: input += WideToUTF16(L" MMMMMMMMMMM");
On 2013/12/31 00:04:03, Anuj wrote:
> On 2013/12/30 23:17:59, msw wrote:
> > On 2013/12/30 22:46:58, Anuj wrote:
> > > M is better because it is wider (if not the widest). The directionality
> > doesn't
> > > matter as it will all be truncated.
> > Don't you check the trailing directionality of the elided string and make
sure
> > that it matches the input string by adding marks? I would avoid modifying
the
> > trailing directionality here.
> 
> I do. But the string which remains matters. This part will be removed, and its
> directionality doesn't matter.

Ah, good point, thanks for clarifying.

https://codereview.chromium.org/119813002/diff/360001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/119813002/diff/360001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:1129: base::string16 text = GetLayoutText();
This won't make a string copy will it? Consider just inlining GetLayoutText() as
it was plus the one new GetLayoutText().empty().

[Peter Kasting, 2013-12-31 00:27:17]

https://codereview.chromium.org/119813002/diff/4/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/119813002/diff/4/ui/gfx/render_text.cc#newcod...
ui/gfx/render_text.cc:1151: if (text.empty())
On 2013/12/31 00:04:03, Anuj wrote:
> I think that comments are insufficient context, and if you want code in
context
> too, you have to dig into each file. I will try not to remove your comments
from
> context, but no promises because I think they are ugly/redundant.

FWIW, it also kinda drives me nuts when people remove all previous comments, or
reply above them instead of below.  It's really hard to recall the discussion
properly sometimes.

I usually compromise by removing all but one level of previous comments (or
sometimes more levels when more context is necessary), as I've done in this
case.

[Anuj, 2013-12-31 00:42:03]

https://codereview.chromium.org/119813002/diff/4/ui/gfx/render_text_unittest.cc
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/119813002/diff/4/ui/gfx/render_text_unittest....
ui/gfx/render_text_unittest.cc:461: input += WideToUTF16(L" MMMMMMMMMMM");
On 2013/12/31 00:24:52, msw wrote:
> On 2013/12/31 00:04:03, Anuj wrote:
> > On 2013/12/30 23:17:59, msw wrote:
> > > On 2013/12/30 22:46:58, Anuj wrote:
> > > > M is better because it is wider (if not the widest). The directionality
> > > doesn't
> > > > matter as it will all be truncated.
> > > Don't you check the trailing directionality of the elided string and make
> sure
> > > that it matches the input string by adding marks? I would avoid modifying
> the
> > > trailing directionality here.
> > 
> > I do. But the string which remains matters. This part will be removed, and
its
> > directionality doesn't matter.
> 
> Ah, good point, thanks for clarifying.

Done.

https://codereview.chromium.org/119813002/diff/360001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/119813002/diff/360001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:1129: base::string16 text = GetLayoutText();
On 2013/12/31 00:24:52, msw wrote:
> This won't make a string copy will it? Consider just inlining GetLayoutText()
as
> it was plus the one new GetLayoutText().empty().

Done.

[commit-bot: I haz the power, 2013-12-31 00:42:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skanuj@chromium.org/119813002/430001

[commit-bot: I haz the power, 2013-12-31 00:43:03]

Failed to apply patch for
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/ui/views/omnibox/omnibox_result_view.cc
  Hunk #1 FAILED at 357.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/ui/views/omnibox/omnibox_result_view.cc.rej

Patch:       chrome/browser/ui/views/omnibox/omnibox_result_view.cc
Index: chrome/browser/ui/views/omnibox/omnibox_result_view.cc
diff --git a/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
b/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
index
9938b54774812cc4cf22db8bb0cc4e2fd0506cdc..6eb06903c1db5ad45544ad6956c9aaf3c5b814d6
100644
--- a/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
+++ b/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
@@ -357,213 +357,59 @@ int OmniboxResultView::DrawString(
     }
   }
 
-  // Split the text into visual runs.  We do this first so that we don't need
to
-  // worry about whether our eliding might change the visual display in
-  // unintended ways, e.g. by removing directional markings or by adding an
-  // ellipsis that's not enclosed in appropriate markings.
-  base::i18n::BiDiLineIterator bidi_line;
-  if (!bidi_line.Open(text, base::i18n::IsRTL(), is_url))
-    return x;
-  const int num_runs = bidi_line.CountRuns();
-  ScopedVector<gfx::RenderText> render_texts;
-  Runs runs;
-  for (int run = 0; run < num_runs; ++run) {
-    int run_start_int = 0, run_length_int = 0;
-    // The index we pass to GetVisualRun corresponds to the position of the run
-    // in the displayed text. For example, the string "Google in HEBREW" (where
-    // HEBREW is text in the Hebrew language) has two runs: "Google in " which
-    // is an LTR run, and "HEBREW" which is an RTL run. In an LTR context, the
-    // run "Google in " has the index 0 (since it is the leftmost run
-    // displayed). In an RTL context, the same run has the index 1 because it
-    // is the rightmost run. This is why the order in which we traverse the
-    // runs is different depending on the locale direction.
-    const UBiDiDirection run_direction = bidi_line.GetVisualRun(
-        (base::i18n::IsRTL() && !is_url) ? (num_runs - run - 1) : run,
-        &run_start_int, &run_length_int);
-    DCHECK_GT(run_length_int, 0);
-    runs.push_back(RunData());
-    RunData* current_run = &runs.back();
-    current_run->run_start = run_start_int;
-    const size_t run_end = current_run->run_start + run_length_int;
-    current_run->visual_order = run;
-    current_run->is_rtl = !is_url && (run_direction == UBIDI_RTL);
-
-    // Compute classifications for this run.
-    for (size_t i = 0; i < classifications.size(); ++i) {
-      const size_t text_start =
-          std::max(classifications[i].offset, current_run->run_start);
-      if (text_start >= run_end)
-        break;  // We're past the last classification in the run.
-
-      const size_t text_end = (i < (classifications.size() - 1)) ?
-          std::min(classifications[i + 1].offset, run_end) : run_end;
-      if (text_end <= current_run->run_start)
-        continue;  // We haven't reached the first classification in the run.
-
-      render_texts.push_back(gfx::RenderText::CreateInstance());
-      gfx::RenderText* render_text = render_texts.back();
-      current_run->classifications.push_back(render_text);
-      render_text->SetText(text.substr(text_start, text_end - text_start));
-      render_text->SetFontList(font_list_);
-
-      // Calculate style-related data.
-      if (classifications[i].style & ACMatchClassification::MATCH)
-        render_text->SetStyle(gfx::BOLD, true);
-      const ResultViewState state = GetState();
-      if (classifications[i].style & ACMatchClassification::URL)
-        render_text->SetColor(GetColor(state, URL));
-      else if (classifications[i].style & ACMatchClassification::DIM)
-        render_text->SetColor(GetColor(state, DIMMED_TEXT));
-      else
-        render_text->SetColor(GetColor(state, force_dim ? DIMMED_TEXT : TEXT));
-
-      current_run->pixel_width += render_text->GetStringSize().width();
-    }
-    DCHECK(!current_run->classifications.empty());
-  }
-  DCHECK(!runs.empty());
-
-  // Sort into logical order so we can elide logically.
-  std::sort(runs.begin(), runs.end(), &SortRunsLogically);
-
-  // Now determine what to elide, if anything.  Several subtle points:
-  //   * Because we have the run data, we can get edge cases correct, like
-  //     whether to place an ellipsis before or after the end of a run when the
-  //     text needs to be elided at the run boundary.
-  //   * The "or one before it" comments below refer to cases where an earlier
-  //     classification fits completely, but leaves too little space for an
-  //     ellipsis that turns out to be needed later.  These cases are commented
-  //     more completely in Elide().
-  int remaining_width = mirroring_context_->remaining_width(x);
-  for (Runs::iterator i(runs.begin()); i != runs.end(); ++i) {
-    if (i->pixel_width > remaining_width) {
-      // This run or one before it needs to be elided.
-      for (Classifications::iterator j(i->classifications.begin());
-           j != i->classifications.end(); ++j) {
-        const int width = (*j)->GetStringSize().width();
-        if (width > remaining_width) {
-          // This classification or one before it needs to be elided.  Erase
all
-          // further classifications and runs so Elide() can simply reverse-
-          // iterate over everything to find the specific classification to
-          // elide.
-          i->classifications.erase(++j, i->classifications.end());
-          runs.erase(++i, runs.end());
-          Elide(&runs, remaining_width);
-          break;
-        }
-        remaining_width -= width;
-      }
+  scoped_ptr<gfx::RenderText> render_text(gfx::RenderText::CreateInstance());
+  const size_t text_length = text.length();
+  render_text->SetText(text);
+  render_text->SetFontList(font_list_);
+  render_text->SetMultiline(false);
+  render_text->SetCursorEnabled(false);
+  if (is_url)
+    render_text->SetDirectionalityMode(gfx::DIRECTIONALITY_FORCE_LTR);
+
+  // Apply classifications.
+  for (size_t i = 0; i < classifications.size(); ++i) {
+    const size_t text_start = classifications[i].offset;
+    if (text_start >= text_length)
       break;
-    }
-    remaining_width -= i->pixel_width;
-  }
 
-  // Sort back into visual order so we can display the runs correctly.
-  std::sort(runs.begin(), runs.end(), &SortRunsVisually);
-
-  // Draw the runs.
-  for (Runs::iterator i(runs.begin()); i != runs.end(); ++i) {
-    const bool reverse_visible_order = (i->is_rtl != base::i18n::IsRTL());
-    if (reverse_visible_order)
-      std::reverse(i->classifications.begin(), i->classifications.end());
-    for (Classifications::const_iterator j(i->classifications.begin());
-         j != i->classifications.end(); ++j) {
-      const gfx::Size size = (*j)->GetStringSize();
-      // Align the text runs to a common baseline.
-      const gfx::Rect rect(
-          mirroring_context_->mirrored_left_coord(x, x + size.width()), y,
-          size.width(), height());
-      (*j)->SetDisplayRect(rect);
-      (*j)->Draw(canvas);
-      x += size.width();
+    const size_t text_end = (i < (classifications.size() - 1)) ?
+        std::min(classifications[i + 1].offset, text_length) : text_length;
+    const gfx::Range current_range(text_start, text_end);
+
+    // Calculate style-related data.
+    if (classifications[i].style & ACMatchClassification::MATCH)
+      render_text->ApplyStyle(gfx::BOLD, true, current_range);
+
+    ColorKind color_kind = TEXT;
+    if (classifications[i].style & ACMatchClassification::URL) {
+      color_kind = URL;
+    } else if (force_dim ||
+        (classifications[i].style & ACMatchClassification::DIM)) {
+      color_kind = DIMMED_TEXT;
     }
+    render_text->ApplyColor(GetColor(GetState(), color_kind), current_range);
   }
 
-  return x;
-}
+  int remaining_width = mirroring_context_->remaining_width(x);
 
-void OmniboxResultView::Elide(Runs* runs, int remaining_width) const {
-  // The complexity of this function is due to edge cases like the following:
-  // We have 100 px of available space, an initial classification that takes 86
-  // px, and a font that has a 15 px wide ellipsis character.  Now if the first
-  // classification is followed by several very narrow classifications (e.g. 3
-  // px wide each), we don't know whether we need to elide or not at the time
we
-  // see the first classification -- it depends on how many subsequent
-  // classifications follow, and some of those may be in the next run (or
-  // several runs!).  This is why instead we let our caller move forward until
-  // we know we definitely need to elide, and then in this function we move
-  // backward again until we find a string that we can successfully do the
-  // eliding on.
-  bool on_trailing_classification = true;
-  for (Runs::reverse_iterator i(runs->rbegin()); i != runs->rend(); ++i) {
-    for (Classifications::reverse_iterator j(i->classifications.rbegin());
-         j != i->classifications.rend(); ++j) {
-      if (!on_trailing_classification) {
-        // We also add this classification's width (sans ellipsis) back to the
-        // available width since we want to consider the available space we'll
-        // have when we draw this classification.
-        remaining_width += (*j)->GetStringSize().width();
-
-        // If we reached here, we couldn't fit an ellipsis in the space taken
by
-        // the previous classifications we looped over (see comments at bottom
-        // of loop).  Append one here to represent those elided portions.
-        (*j)->SetText((*j)->text() + kEllipsis);
-      }
-      on_trailing_classification = false;
-
-      // Can we fit at least an ellipsis?
-      gfx::Font font((*j)->GetStyle(gfx:…
(message too large)

[commit-bot: I haz the power, 2013-12-31 01:15:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skanuj@chromium.org/119813002/390002

[commit-bot: I haz the power, 2013-12-31 01:15:07]

Failed to apply patch for
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/ui/views/omnibox/omnibox_result_view.cc
  Hunk #1 FAILED at 357.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/ui/views/omnibox/omnibox_result_view.cc.rej

Patch:       chrome/browser/ui/views/omnibox/omnibox_result_view.cc
Index: chrome/browser/ui/views/omnibox/omnibox_result_view.cc
diff --git a/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
b/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
index
9938b54774812cc4cf22db8bb0cc4e2fd0506cdc..6eb06903c1db5ad45544ad6956c9aaf3c5b814d6
100644
--- a/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
+++ b/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
@@ -357,213 +357,59 @@ int OmniboxResultView::DrawString(
     }
   }
 
-  // Split the text into visual runs.  We do this first so that we don't need
to
-  // worry about whether our eliding might change the visual display in
-  // unintended ways, e.g. by removing directional markings or by adding an
-  // ellipsis that's not enclosed in appropriate markings.
-  base::i18n::BiDiLineIterator bidi_line;
-  if (!bidi_line.Open(text, base::i18n::IsRTL(), is_url))
-    return x;
-  const int num_runs = bidi_line.CountRuns();
-  ScopedVector<gfx::RenderText> render_texts;
-  Runs runs;
-  for (int run = 0; run < num_runs; ++run) {
-    int run_start_int = 0, run_length_int = 0;
-    // The index we pass to GetVisualRun corresponds to the position of the run
-    // in the displayed text. For example, the string "Google in HEBREW" (where
-    // HEBREW is text in the Hebrew language) has two runs: "Google in " which
-    // is an LTR run, and "HEBREW" which is an RTL run. In an LTR context, the
-    // run "Google in " has the index 0 (since it is the leftmost run
-    // displayed). In an RTL context, the same run has the index 1 because it
-    // is the rightmost run. This is why the order in which we traverse the
-    // runs is different depending on the locale direction.
-    const UBiDiDirection run_direction = bidi_line.GetVisualRun(
-        (base::i18n::IsRTL() && !is_url) ? (num_runs - run - 1) : run,
-        &run_start_int, &run_length_int);
-    DCHECK_GT(run_length_int, 0);
-    runs.push_back(RunData());
-    RunData* current_run = &runs.back();
-    current_run->run_start = run_start_int;
-    const size_t run_end = current_run->run_start + run_length_int;
-    current_run->visual_order = run;
-    current_run->is_rtl = !is_url && (run_direction == UBIDI_RTL);
-
-    // Compute classifications for this run.
-    for (size_t i = 0; i < classifications.size(); ++i) {
-      const size_t text_start =
-          std::max(classifications[i].offset, current_run->run_start);
-      if (text_start >= run_end)
-        break;  // We're past the last classification in the run.
-
-      const size_t text_end = (i < (classifications.size() - 1)) ?
-          std::min(classifications[i + 1].offset, run_end) : run_end;
-      if (text_end <= current_run->run_start)
-        continue;  // We haven't reached the first classification in the run.
-
-      render_texts.push_back(gfx::RenderText::CreateInstance());
-      gfx::RenderText* render_text = render_texts.back();
-      current_run->classifications.push_back(render_text);
-      render_text->SetText(text.substr(text_start, text_end - text_start));
-      render_text->SetFontList(font_list_);
-
-      // Calculate style-related data.
-      if (classifications[i].style & ACMatchClassification::MATCH)
-        render_text->SetStyle(gfx::BOLD, true);
-      const ResultViewState state = GetState();
-      if (classifications[i].style & ACMatchClassification::URL)
-        render_text->SetColor(GetColor(state, URL));
-      else if (classifications[i].style & ACMatchClassification::DIM)
-        render_text->SetColor(GetColor(state, DIMMED_TEXT));
-      else
-        render_text->SetColor(GetColor(state, force_dim ? DIMMED_TEXT : TEXT));
-
-      current_run->pixel_width += render_text->GetStringSize().width();
-    }
-    DCHECK(!current_run->classifications.empty());
-  }
-  DCHECK(!runs.empty());
-
-  // Sort into logical order so we can elide logically.
-  std::sort(runs.begin(), runs.end(), &SortRunsLogically);
-
-  // Now determine what to elide, if anything.  Several subtle points:
-  //   * Because we have the run data, we can get edge cases correct, like
-  //     whether to place an ellipsis before or after the end of a run when the
-  //     text needs to be elided at the run boundary.
-  //   * The "or one before it" comments below refer to cases where an earlier
-  //     classification fits completely, but leaves too little space for an
-  //     ellipsis that turns out to be needed later.  These cases are commented
-  //     more completely in Elide().
-  int remaining_width = mirroring_context_->remaining_width(x);
-  for (Runs::iterator i(runs.begin()); i != runs.end(); ++i) {
-    if (i->pixel_width > remaining_width) {
-      // This run or one before it needs to be elided.
-      for (Classifications::iterator j(i->classifications.begin());
-           j != i->classifications.end(); ++j) {
-        const int width = (*j)->GetStringSize().width();
-        if (width > remaining_width) {
-          // This classification or one before it needs to be elided.  Erase
all
-          // further classifications and runs so Elide() can simply reverse-
-          // iterate over everything to find the specific classification to
-          // elide.
-          i->classifications.erase(++j, i->classifications.end());
-          runs.erase(++i, runs.end());
-          Elide(&runs, remaining_width);
-          break;
-        }
-        remaining_width -= width;
-      }
+  scoped_ptr<gfx::RenderText> render_text(gfx::RenderText::CreateInstance());
+  const size_t text_length = text.length();
+  render_text->SetText(text);
+  render_text->SetFontList(font_list_);
+  render_text->SetMultiline(false);
+  render_text->SetCursorEnabled(false);
+  if (is_url)
+    render_text->SetDirectionalityMode(gfx::DIRECTIONALITY_FORCE_LTR);
+
+  // Apply classifications.
+  for (size_t i = 0; i < classifications.size(); ++i) {
+    const size_t text_start = classifications[i].offset;
+    if (text_start >= text_length)
       break;
-    }
-    remaining_width -= i->pixel_width;
-  }
 
-  // Sort back into visual order so we can display the runs correctly.
-  std::sort(runs.begin(), runs.end(), &SortRunsVisually);
-
-  // Draw the runs.
-  for (Runs::iterator i(runs.begin()); i != runs.end(); ++i) {
-    const bool reverse_visible_order = (i->is_rtl != base::i18n::IsRTL());
-    if (reverse_visible_order)
-      std::reverse(i->classifications.begin(), i->classifications.end());
-    for (Classifications::const_iterator j(i->classifications.begin());
-         j != i->classifications.end(); ++j) {
-      const gfx::Size size = (*j)->GetStringSize();
-      // Align the text runs to a common baseline.
-      const gfx::Rect rect(
-          mirroring_context_->mirrored_left_coord(x, x + size.width()), y,
-          size.width(), height());
-      (*j)->SetDisplayRect(rect);
-      (*j)->Draw(canvas);
-      x += size.width();
+    const size_t text_end = (i < (classifications.size() - 1)) ?
+        std::min(classifications[i + 1].offset, text_length) : text_length;
+    const gfx::Range current_range(text_start, text_end);
+
+    // Calculate style-related data.
+    if (classifications[i].style & ACMatchClassification::MATCH)
+      render_text->ApplyStyle(gfx::BOLD, true, current_range);
+
+    ColorKind color_kind = TEXT;
+    if (classifications[i].style & ACMatchClassification::URL) {
+      color_kind = URL;
+    } else if (force_dim ||
+        (classifications[i].style & ACMatchClassification::DIM)) {
+      color_kind = DIMMED_TEXT;
     }
+    render_text->ApplyColor(GetColor(GetState(), color_kind), current_range);
   }
 
-  return x;
-}
+  int remaining_width = mirroring_context_->remaining_width(x);
 
-void OmniboxResultView::Elide(Runs* runs, int remaining_width) const {
-  // The complexity of this function is due to edge cases like the following:
-  // We have 100 px of available space, an initial classification that takes 86
-  // px, and a font that has a 15 px wide ellipsis character.  Now if the first
-  // classification is followed by several very narrow classifications (e.g. 3
-  // px wide each), we don't know whether we need to elide or not at the time
we
-  // see the first classification -- it depends on how many subsequent
-  // classifications follow, and some of those may be in the next run (or
-  // several runs!).  This is why instead we let our caller move forward until
-  // we know we definitely need to elide, and then in this function we move
-  // backward again until we find a string that we can successfully do the
-  // eliding on.
-  bool on_trailing_classification = true;
-  for (Runs::reverse_iterator i(runs->rbegin()); i != runs->rend(); ++i) {
-    for (Classifications::reverse_iterator j(i->classifications.rbegin());
-         j != i->classifications.rend(); ++j) {
-      if (!on_trailing_classification) {
-        // We also add this classification's width (sans ellipsis) back to the
-        // available width since we want to consider the available space we'll
-        // have when we draw this classification.
-        remaining_width += (*j)->GetStringSize().width();
-
-        // If we reached here, we couldn't fit an ellipsis in the space taken
by
-        // the previous classifications we looped over (see comments at bottom
-        // of loop).  Append one here to represent those elided portions.
-        (*j)->SetText((*j)->text() + kEllipsis);
-      }
-      on_trailing_classification = false;
-
-      // Can we fit at least an ellipsis?
-      gfx::Font font((*j)->GetStyle(gfx:…
(message too large)

[commit-bot: I haz the power, 2013-12-31 01:27:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skanuj@chromium.org/119813002/520001

[commit-bot: I haz the power, 2013-12-31 01:27:11]

Failed to apply patch for
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/ui/views/omnibox/omnibox_result_view.cc
  Hunk #1 FAILED at 357.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/ui/views/omnibox/omnibox_result_view.cc.rej

Patch:       chrome/browser/ui/views/omnibox/omnibox_result_view.cc
Index: chrome/browser/ui/views/omnibox/omnibox_result_view.cc
diff --git a/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
b/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
index
9938b54774812cc4cf22db8bb0cc4e2fd0506cdc..6eb06903c1db5ad45544ad6956c9aaf3c5b814d6
100644
--- a/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
+++ b/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
@@ -357,213 +357,59 @@ int OmniboxResultView::DrawString(
     }
   }
 
-  // Split the text into visual runs.  We do this first so that we don't need
to
-  // worry about whether our eliding might change the visual display in
-  // unintended ways, e.g. by removing directional markings or by adding an
-  // ellipsis that's not enclosed in appropriate markings.
-  base::i18n::BiDiLineIterator bidi_line;
-  if (!bidi_line.Open(text, base::i18n::IsRTL(), is_url))
-    return x;
-  const int num_runs = bidi_line.CountRuns();
-  ScopedVector<gfx::RenderText> render_texts;
-  Runs runs;
-  for (int run = 0; run < num_runs; ++run) {
-    int run_start_int = 0, run_length_int = 0;
-    // The index we pass to GetVisualRun corresponds to the position of the run
-    // in the displayed text. For example, the string "Google in HEBREW" (where
-    // HEBREW is text in the Hebrew language) has two runs: "Google in " which
-    // is an LTR run, and "HEBREW" which is an RTL run. In an LTR context, the
-    // run "Google in " has the index 0 (since it is the leftmost run
-    // displayed). In an RTL context, the same run has the index 1 because it
-    // is the rightmost run. This is why the order in which we traverse the
-    // runs is different depending on the locale direction.
-    const UBiDiDirection run_direction = bidi_line.GetVisualRun(
-        (base::i18n::IsRTL() && !is_url) ? (num_runs - run - 1) : run,
-        &run_start_int, &run_length_int);
-    DCHECK_GT(run_length_int, 0);
-    runs.push_back(RunData());
-    RunData* current_run = &runs.back();
-    current_run->run_start = run_start_int;
-    const size_t run_end = current_run->run_start + run_length_int;
-    current_run->visual_order = run;
-    current_run->is_rtl = !is_url && (run_direction == UBIDI_RTL);
-
-    // Compute classifications for this run.
-    for (size_t i = 0; i < classifications.size(); ++i) {
-      const size_t text_start =
-          std::max(classifications[i].offset, current_run->run_start);
-      if (text_start >= run_end)
-        break;  // We're past the last classification in the run.
-
-      const size_t text_end = (i < (classifications.size() - 1)) ?
-          std::min(classifications[i + 1].offset, run_end) : run_end;
-      if (text_end <= current_run->run_start)
-        continue;  // We haven't reached the first classification in the run.
-
-      render_texts.push_back(gfx::RenderText::CreateInstance());
-      gfx::RenderText* render_text = render_texts.back();
-      current_run->classifications.push_back(render_text);
-      render_text->SetText(text.substr(text_start, text_end - text_start));
-      render_text->SetFontList(font_list_);
-
-      // Calculate style-related data.
-      if (classifications[i].style & ACMatchClassification::MATCH)
-        render_text->SetStyle(gfx::BOLD, true);
-      const ResultViewState state = GetState();
-      if (classifications[i].style & ACMatchClassification::URL)
-        render_text->SetColor(GetColor(state, URL));
-      else if (classifications[i].style & ACMatchClassification::DIM)
-        render_text->SetColor(GetColor(state, DIMMED_TEXT));
-      else
-        render_text->SetColor(GetColor(state, force_dim ? DIMMED_TEXT : TEXT));
-
-      current_run->pixel_width += render_text->GetStringSize().width();
-    }
-    DCHECK(!current_run->classifications.empty());
-  }
-  DCHECK(!runs.empty());
-
-  // Sort into logical order so we can elide logically.
-  std::sort(runs.begin(), runs.end(), &SortRunsLogically);
-
-  // Now determine what to elide, if anything.  Several subtle points:
-  //   * Because we have the run data, we can get edge cases correct, like
-  //     whether to place an ellipsis before or after the end of a run when the
-  //     text needs to be elided at the run boundary.
-  //   * The "or one before it" comments below refer to cases where an earlier
-  //     classification fits completely, but leaves too little space for an
-  //     ellipsis that turns out to be needed later.  These cases are commented
-  //     more completely in Elide().
-  int remaining_width = mirroring_context_->remaining_width(x);
-  for (Runs::iterator i(runs.begin()); i != runs.end(); ++i) {
-    if (i->pixel_width > remaining_width) {
-      // This run or one before it needs to be elided.
-      for (Classifications::iterator j(i->classifications.begin());
-           j != i->classifications.end(); ++j) {
-        const int width = (*j)->GetStringSize().width();
-        if (width > remaining_width) {
-          // This classification or one before it needs to be elided.  Erase
all
-          // further classifications and runs so Elide() can simply reverse-
-          // iterate over everything to find the specific classification to
-          // elide.
-          i->classifications.erase(++j, i->classifications.end());
-          runs.erase(++i, runs.end());
-          Elide(&runs, remaining_width);
-          break;
-        }
-        remaining_width -= width;
-      }
+  scoped_ptr<gfx::RenderText> render_text(gfx::RenderText::CreateInstance());
+  const size_t text_length = text.length();
+  render_text->SetText(text);
+  render_text->SetFontList(font_list_);
+  render_text->SetMultiline(false);
+  render_text->SetCursorEnabled(false);
+  if (is_url)
+    render_text->SetDirectionalityMode(gfx::DIRECTIONALITY_FORCE_LTR);
+
+  // Apply classifications.
+  for (size_t i = 0; i < classifications.size(); ++i) {
+    const size_t text_start = classifications[i].offset;
+    if (text_start >= text_length)
       break;
-    }
-    remaining_width -= i->pixel_width;
-  }
 
-  // Sort back into visual order so we can display the runs correctly.
-  std::sort(runs.begin(), runs.end(), &SortRunsVisually);
-
-  // Draw the runs.
-  for (Runs::iterator i(runs.begin()); i != runs.end(); ++i) {
-    const bool reverse_visible_order = (i->is_rtl != base::i18n::IsRTL());
-    if (reverse_visible_order)
-      std::reverse(i->classifications.begin(), i->classifications.end());
-    for (Classifications::const_iterator j(i->classifications.begin());
-         j != i->classifications.end(); ++j) {
-      const gfx::Size size = (*j)->GetStringSize();
-      // Align the text runs to a common baseline.
-      const gfx::Rect rect(
-          mirroring_context_->mirrored_left_coord(x, x + size.width()), y,
-          size.width(), height());
-      (*j)->SetDisplayRect(rect);
-      (*j)->Draw(canvas);
-      x += size.width();
+    const size_t text_end = (i < (classifications.size() - 1)) ?
+        std::min(classifications[i + 1].offset, text_length) : text_length;
+    const gfx::Range current_range(text_start, text_end);
+
+    // Calculate style-related data.
+    if (classifications[i].style & ACMatchClassification::MATCH)
+      render_text->ApplyStyle(gfx::BOLD, true, current_range);
+
+    ColorKind color_kind = TEXT;
+    if (classifications[i].style & ACMatchClassification::URL) {
+      color_kind = URL;
+    } else if (force_dim ||
+        (classifications[i].style & ACMatchClassification::DIM)) {
+      color_kind = DIMMED_TEXT;
     }
+    render_text->ApplyColor(GetColor(GetState(), color_kind), current_range);
   }
 
-  return x;
-}
+  int remaining_width = mirroring_context_->remaining_width(x);
 
-void OmniboxResultView::Elide(Runs* runs, int remaining_width) const {
-  // The complexity of this function is due to edge cases like the following:
-  // We have 100 px of available space, an initial classification that takes 86
-  // px, and a font that has a 15 px wide ellipsis character.  Now if the first
-  // classification is followed by several very narrow classifications (e.g. 3
-  // px wide each), we don't know whether we need to elide or not at the time
we
-  // see the first classification -- it depends on how many subsequent
-  // classifications follow, and some of those may be in the next run (or
-  // several runs!).  This is why instead we let our caller move forward until
-  // we know we definitely need to elide, and then in this function we move
-  // backward again until we find a string that we can successfully do the
-  // eliding on.
-  bool on_trailing_classification = true;
-  for (Runs::reverse_iterator i(runs->rbegin()); i != runs->rend(); ++i) {
-    for (Classifications::reverse_iterator j(i->classifications.rbegin());
-         j != i->classifications.rend(); ++j) {
-      if (!on_trailing_classification) {
-        // We also add this classification's width (sans ellipsis) back to the
-        // available width since we want to consider the available space we'll
-        // have when we draw this classification.
-        remaining_width += (*j)->GetStringSize().width();
-
-        // If we reached here, we couldn't fit an ellipsis in the space taken
by
-        // the previous classifications we looped over (see comments at bottom
-        // of loop).  Append one here to represent those elided portions.
-        (*j)->SetText((*j)->text() + kEllipsis);
-      }
-      on_trailing_classification = false;
-
-      // Can we fit at least an ellipsis?
-      gfx::Font font((*j)->GetStyle(gfx:…
(message too large)

[commit-bot: I haz the power, 2013-12-31 01:27:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skanuj@chromium.org/119813002/520001

[commit-bot: I haz the power, 2013-12-31 01:28:01]

Failed to apply patch for
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/ui/views/omnibox/omnibox_result_view.cc
  Hunk #1 FAILED at 357.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/ui/views/omnibox/omnibox_result_view.cc.rej

Patch:       chrome/browser/ui/views/omnibox/omnibox_result_view.cc
Index: chrome/browser/ui/views/omnibox/omnibox_result_view.cc
diff --git a/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
b/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
index
9938b54774812cc4cf22db8bb0cc4e2fd0506cdc..6eb06903c1db5ad45544ad6956c9aaf3c5b814d6
100644
--- a/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
+++ b/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
@@ -357,213 +357,59 @@ int OmniboxResultView::DrawString(
     }
   }
 
-  // Split the text into visual runs.  We do this first so that we don't need
to
-  // worry about whether our eliding might change the visual display in
-  // unintended ways, e.g. by removing directional markings or by adding an
-  // ellipsis that's not enclosed in appropriate markings.
-  base::i18n::BiDiLineIterator bidi_line;
-  if (!bidi_line.Open(text, base::i18n::IsRTL(), is_url))
-    return x;
-  const int num_runs = bidi_line.CountRuns();
-  ScopedVector<gfx::RenderText> render_texts;
-  Runs runs;
-  for (int run = 0; run < num_runs; ++run) {
-    int run_start_int = 0, run_length_int = 0;
-    // The index we pass to GetVisualRun corresponds to the position of the run
-    // in the displayed text. For example, the string "Google in HEBREW" (where
-    // HEBREW is text in the Hebrew language) has two runs: "Google in " which
-    // is an LTR run, and "HEBREW" which is an RTL run. In an LTR context, the
-    // run "Google in " has the index 0 (since it is the leftmost run
-    // displayed). In an RTL context, the same run has the index 1 because it
-    // is the rightmost run. This is why the order in which we traverse the
-    // runs is different depending on the locale direction.
-    const UBiDiDirection run_direction = bidi_line.GetVisualRun(
-        (base::i18n::IsRTL() && !is_url) ? (num_runs - run - 1) : run,
-        &run_start_int, &run_length_int);
-    DCHECK_GT(run_length_int, 0);
-    runs.push_back(RunData());
-    RunData* current_run = &runs.back();
-    current_run->run_start = run_start_int;
-    const size_t run_end = current_run->run_start + run_length_int;
-    current_run->visual_order = run;
-    current_run->is_rtl = !is_url && (run_direction == UBIDI_RTL);
-
-    // Compute classifications for this run.
-    for (size_t i = 0; i < classifications.size(); ++i) {
-      const size_t text_start =
-          std::max(classifications[i].offset, current_run->run_start);
-      if (text_start >= run_end)
-        break;  // We're past the last classification in the run.
-
-      const size_t text_end = (i < (classifications.size() - 1)) ?
-          std::min(classifications[i + 1].offset, run_end) : run_end;
-      if (text_end <= current_run->run_start)
-        continue;  // We haven't reached the first classification in the run.
-
-      render_texts.push_back(gfx::RenderText::CreateInstance());
-      gfx::RenderText* render_text = render_texts.back();
-      current_run->classifications.push_back(render_text);
-      render_text->SetText(text.substr(text_start, text_end - text_start));
-      render_text->SetFontList(font_list_);
-
-      // Calculate style-related data.
-      if (classifications[i].style & ACMatchClassification::MATCH)
-        render_text->SetStyle(gfx::BOLD, true);
-      const ResultViewState state = GetState();
-      if (classifications[i].style & ACMatchClassification::URL)
-        render_text->SetColor(GetColor(state, URL));
-      else if (classifications[i].style & ACMatchClassification::DIM)
-        render_text->SetColor(GetColor(state, DIMMED_TEXT));
-      else
-        render_text->SetColor(GetColor(state, force_dim ? DIMMED_TEXT : TEXT));
-
-      current_run->pixel_width += render_text->GetStringSize().width();
-    }
-    DCHECK(!current_run->classifications.empty());
-  }
-  DCHECK(!runs.empty());
-
-  // Sort into logical order so we can elide logically.
-  std::sort(runs.begin(), runs.end(), &SortRunsLogically);
-
-  // Now determine what to elide, if anything.  Several subtle points:
-  //   * Because we have the run data, we can get edge cases correct, like
-  //     whether to place an ellipsis before or after the end of a run when the
-  //     text needs to be elided at the run boundary.
-  //   * The "or one before it" comments below refer to cases where an earlier
-  //     classification fits completely, but leaves too little space for an
-  //     ellipsis that turns out to be needed later.  These cases are commented
-  //     more completely in Elide().
-  int remaining_width = mirroring_context_->remaining_width(x);
-  for (Runs::iterator i(runs.begin()); i != runs.end(); ++i) {
-    if (i->pixel_width > remaining_width) {
-      // This run or one before it needs to be elided.
-      for (Classifications::iterator j(i->classifications.begin());
-           j != i->classifications.end(); ++j) {
-        const int width = (*j)->GetStringSize().width();
-        if (width > remaining_width) {
-          // This classification or one before it needs to be elided.  Erase
all
-          // further classifications and runs so Elide() can simply reverse-
-          // iterate over everything to find the specific classification to
-          // elide.
-          i->classifications.erase(++j, i->classifications.end());
-          runs.erase(++i, runs.end());
-          Elide(&runs, remaining_width);
-          break;
-        }
-        remaining_width -= width;
-      }
+  scoped_ptr<gfx::RenderText> render_text(gfx::RenderText::CreateInstance());
+  const size_t text_length = text.length();
+  render_text->SetText(text);
+  render_text->SetFontList(font_list_);
+  render_text->SetMultiline(false);
+  render_text->SetCursorEnabled(false);
+  if (is_url)
+    render_text->SetDirectionalityMode(gfx::DIRECTIONALITY_FORCE_LTR);
+
+  // Apply classifications.
+  for (size_t i = 0; i < classifications.size(); ++i) {
+    const size_t text_start = classifications[i].offset;
+    if (text_start >= text_length)
       break;
-    }
-    remaining_width -= i->pixel_width;
-  }
 
-  // Sort back into visual order so we can display the runs correctly.
-  std::sort(runs.begin(), runs.end(), &SortRunsVisually);
-
-  // Draw the runs.
-  for (Runs::iterator i(runs.begin()); i != runs.end(); ++i) {
-    const bool reverse_visible_order = (i->is_rtl != base::i18n::IsRTL());
-    if (reverse_visible_order)
-      std::reverse(i->classifications.begin(), i->classifications.end());
-    for (Classifications::const_iterator j(i->classifications.begin());
-         j != i->classifications.end(); ++j) {
-      const gfx::Size size = (*j)->GetStringSize();
-      // Align the text runs to a common baseline.
-      const gfx::Rect rect(
-          mirroring_context_->mirrored_left_coord(x, x + size.width()), y,
-          size.width(), height());
-      (*j)->SetDisplayRect(rect);
-      (*j)->Draw(canvas);
-      x += size.width();
+    const size_t text_end = (i < (classifications.size() - 1)) ?
+        std::min(classifications[i + 1].offset, text_length) : text_length;
+    const gfx::Range current_range(text_start, text_end);
+
+    // Calculate style-related data.
+    if (classifications[i].style & ACMatchClassification::MATCH)
+      render_text->ApplyStyle(gfx::BOLD, true, current_range);
+
+    ColorKind color_kind = TEXT;
+    if (classifications[i].style & ACMatchClassification::URL) {
+      color_kind = URL;
+    } else if (force_dim ||
+        (classifications[i].style & ACMatchClassification::DIM)) {
+      color_kind = DIMMED_TEXT;
     }
+    render_text->ApplyColor(GetColor(GetState(), color_kind), current_range);
   }
 
-  return x;
-}
+  int remaining_width = mirroring_context_->remaining_width(x);
 
-void OmniboxResultView::Elide(Runs* runs, int remaining_width) const {
-  // The complexity of this function is due to edge cases like the following:
-  // We have 100 px of available space, an initial classification that takes 86
-  // px, and a font that has a 15 px wide ellipsis character.  Now if the first
-  // classification is followed by several very narrow classifications (e.g. 3
-  // px wide each), we don't know whether we need to elide or not at the time
we
-  // see the first classification -- it depends on how many subsequent
-  // classifications follow, and some of those may be in the next run (or
-  // several runs!).  This is why instead we let our caller move forward until
-  // we know we definitely need to elide, and then in this function we move
-  // backward again until we find a string that we can successfully do the
-  // eliding on.
-  bool on_trailing_classification = true;
-  for (Runs::reverse_iterator i(runs->rbegin()); i != runs->rend(); ++i) {
-    for (Classifications::reverse_iterator j(i->classifications.rbegin());
-         j != i->classifications.rend(); ++j) {
-      if (!on_trailing_classification) {
-        // We also add this classification's width (sans ellipsis) back to the
-        // available width since we want to consider the available space we'll
-        // have when we draw this classification.
-        remaining_width += (*j)->GetStringSize().width();
-
-        // If we reached here, we couldn't fit an ellipsis in the space taken
by
-        // the previous classifications we looped over (see comments at bottom
-        // of loop).  Append one here to represent those elided portions.
-        (*j)->SetText((*j)->text() + kEllipsis);
-      }
-      on_trailing_classification = false;
-
-      // Can we fit at least an ellipsis?
-      gfx::Font font((*j)->GetStyle(gfx:…
(message too large)

[commit-bot: I haz the power, 2013-12-31 01:40:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skanuj@chromium.org/119813002/570001

[commit-bot: I haz the power, 2013-12-31 01:55:19]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2014-01-01 02:57:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skanuj@chromium.org/119813002/570001

[commit-bot: I haz the power, 2014-01-02 02:51:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skanuj@chromium.org/119813002/570001

[commit-bot: I haz the power, 2014-01-02 23:06:49]

Change committed as 242828