Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
112063003:
Implement eliding/truncating at end in RenderText (Closed)
|
Created:
7 years ago by Anuj Modified:
7 years ago CC:
chromium-reviews, tfarina, James Su, erikwright+watch_chromium.org, jshin+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionImplement 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.
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=240572
Patch Set 1 #
Total comments: 76
Patch Set 2 : Addressed comments and added unit tests #
Total comments: 38
Patch Set 3 : Addresss comments, update bugs #
Total comments: 2
Patch Set 4 : Addressed comments (Patch Set 4) #
Total comments: 1
Patch Set 5 : Synced #
Messages
Total messages: 20 (0 generated)
PTAL. And soon please!
Please add a BUG=, TEST=, and R= to the CL description. Please add unit tests. https://codereview.chromium.org/112063003/diff/1/base/i18n/rtl.h File base/i18n/rtl.h (right): https://codereview.chromium.org/112063003/diff/1/base/i18n/rtl.h#newcode76 base/i18n/rtl.h:76: // Given the string in |text|, returns the directionality of the last nit: combine this with the comment above, via "of the first or last character." https://codereview.chromium.org/112063003/diff/1/chrome/browser/ui/views/omni... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/112063003/diff/1/chrome/browser/ui/views/omni... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:361: const size_t text_size = text.size(); nit: I'd prefer text_length and .length(), but either is okay. https://codereview.chromium.org/112063003/diff/1/chrome/browser/ui/views/omni... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:367: render_text->SetDirectionalityMode(gfx::DIRECTIONALITY_FORCE_LTR); This shouldn't be necessary, URLs should always have LTR directionality. Please remove this or give an example of when this would be necessary. https://codereview.chromium.org/112063003/diff/1/chrome/browser/ui/views/omni... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:377: optional nit: remove blank line. https://codereview.chromium.org/112063003/diff/1/chrome/browser/ui/views/omni... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:383: optional nit: remove blank line. https://codereview.chromium.org/112063003/diff/1/chrome/browser/ui/views/omni... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:386: render_text->ApplyColor(GetColor(state, URL), current_range); nit: I suggest making this if/else translate the classification style to a local ColorKind value (URL, DIMMED_TEXT, or TEXT), then in one line simply do: render_text->ApplyColor(GetColor(GetState(), kind), current_range); https://codereview.chromium.org/112063003/diff/1/chrome/browser/ui/views/omni... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:405: const int display_width = std::min(remaining_width, size.width()); Shouldn't eliding ensure that the string size is always less than or equal to the provided width? If so, this std::min is not necessary. https://codereview.chromium.org/112063003/diff/1/chrome/browser/ui/views/omni... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:407: const gfx::Rect rect( optional nit: inline this in the SetDisplayRect call. https://codereview.chromium.org/112063003/diff/1/chrome/browser/ui/views/omni... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:413: x += display_width; nit: remove this and simply return x + display_width. https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:442: ResetLayout(); nit: Would you mind adding a ResetLayout() call to the end of UpdateLayoutText() call? Then remove all the explicitly paired ResetLayout() calls. https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:1134: base::string16 elided_text = ElideText(text); This disregards the obscuring and truncation done above, which seems bad. You should do something like the |text| const reference above that conditionally uses |text_| or |layout_text_|. Both this and the |text| assignment above can actually call GetLayoutText() to do the same. https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:1136: // This doesn't trim styles so ellipsis may get rendered as a different I suppose it'll be tough to temporarily apply styles based on the elided text length, but otherwise you might get some weird styling of the ellipsis. I would suggest trying to modify ApplyCompositionAndSelectionStyles to ApplyLayoutStyles or similar, and extending the last style of the unelided text to the ellipsis there. If you want, you can file an issue, add a comment here, just deal with potentially buggy elided styles for now, and address this in a followup CL. https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:1143: base::string16 RenderText::ElideText(const base::string16& text) { This is quite similar to ElideText from text_elider... please at least file a bug against yourself to reduce the redundancy of these codepaths and add a TODO comment with the bug number here. https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:1147: const float current_text_pixel_width = GetStringSizeF().width(); GetStringSizeF will actually EnsureLayout and calculate the size of the current GetLayoutText(), which should coincide with the supplied |text| argument if you address the changes I suggest above, but I suspect that you can forego the argument altogether and use GetLayoutText() below in this function. https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:1163: const base::string16 cut = slicer.CutString(text.length() / 2, false); Shouldn't this actually insert an ellipsis if |insert_ellipsis| is true? Otherwise, if we cut the string in half, and that happens to fit in the available space, we'd have a string that's truncated instead of elided, when the next current_text_pixel_width <= available_pixel_width_ returns the halved text. Ditto for the text_elider implemention from which this was copied. https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:1177: size_t guess; nit: declare guess in the for loop's control. https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:1191: base::string16 newtext = slicer.CutString(guess, false); nit: use the unix_hacker naming convention |new_text|. https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:1205: base::i18n::GetLastStrongCharacterDirection(newtext); nit: indent two more spaces. https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:1208: newtext += base::i18n::kLeftToRightMark; I think you should use WrapStringWithLTRFormatting and WrapStringWithRTLFormatting on the ellipsis substring, then append that to |newtext|? Otherwise, don't you need to append the directionality control character before appending the ellipsis, and probably also add a kPopDirectionalFormatting in case text is ever appended to the result? https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:1215: // We check the length of the whole desired string at once to ensure we nit: s/length/width/, s/guess_length/guess_width/; ditto for text_elider. https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:1218: // Check again that we didn't hit a Pango width overflow. If so, cut the This shouldn't be necessary here or in the text_elider implementation; above, |text| is under the Pango limit, then it's cut in half and we append an ellipsis and perhaps directionality control characters. Halving a sufficiently short string then adding a few characters should not exceed the Pango limit, right? https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:1220: if (guess_length <= 0) { nit: remove unnecessary curly braces. https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:1223: if (guess_length > available_pixel_width_) { Shouldn't this break if guess_length == available_pixel_width_? https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:1226: if (hi == lo) Sorry, I'm failing to understand how this can happen, and the original text_elider implementation doesn't do this; can you give an example of how this might happen? Does this happen only because there's no break on equal widths as I noted above? Should the original text_elider code be updated? Can you add a unit test to ensure this doesn't happen and verify that without this workaround the algorithm might yield a string wider than appropriate? https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.h#newcode232 ui/gfx/render_text.h:232: // specified |elide_behavior|. Only one of elide or truncate should be used. We should support simultaneous truncation and eliding, since the Windows implementation of RenderText uses a 10k code point truncation limit by default to avoid performance degradation. I posted a comment in the implementation of UpdateLayoutText about how to accomplish this. https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.h#newcode234 ui/gfx/render_text.h:234: void Elide(float available_pixel_width, ElideBehavior elide_behavior); I would rather this function be SetElideBehavior(ElideBehavior elide_behavior), and the available pixel width is simply the width of |display_rect_|, that avoids requiring clients to deal with two RenderText widths when eliding text. https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.h#newcode642 ui/gfx/render_text.h:642: // The maxmium width available to display the text. nit: " // The maximum width used for eliding, disjoint from the display_rect width."... but I hope this goes away. https://codereview.chromium.org/112063003/diff/1/ui/gfx/text_elider.h File ui/gfx/text_elider.h (right): https://codereview.chromium.org/112063003/diff/1/ui/gfx/text_elider.h#newcode48 ui/gfx/text_elider.h:48: // Returns a valid cut boundary at or before |index|. nit: make this "before/after", ditch the second comment and blank line. https://codereview.chromium.org/112063003/diff/1/ui/gfx/text_elider.h#newcode107 ui/gfx/text_elider.h:107: NO_ELISION Consider making this NO_ELIDE to match the views::Label::ElideBehavior name; I've been meaning to consolidate these enums for some time.
Responding with some comments for clarification. I will address the rest of the comments shortly. https://codereview.chromium.org/112063003/diff/1/chrome/browser/ui/views/omni... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/112063003/diff/1/chrome/browser/ui/views/omni... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:367: render_text->SetDirectionalityMode(gfx::DIRECTIONALITY_FORCE_LTR); Are you suggesting URLs should always have LTR directionality because they will start with a protocol string (http/https/file) and default directionality is DIRECTIONALITY_FROM_TEXT? I added this just to be on safe side, in case the default directionality is ever changed to DIRECTIONALITY_FROM_UI. https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:1208: newtext += base::i18n::kLeftToRightMark; See implicit markers here - http://www.iamcal.com/understanding-bidirectional-text/. What you are referring to are explicit markers, and they need a PDF (pop-directional-formatting). I also wanted to check if similar change should be done in text_elider, probably based on an additional param. Finally, I have tested it manually, it works. https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:1226: if (hi == lo) I had a scenario where the following happened available = 182 guess = 21, guess_length = 181 guess = 23, guess_length =193 guess = 22, guess_length=186 (hi = 22, lo = 22) I don't remember the previous hi-lo values. So guess == available never happens. I can add this to text_elider too if you think this is the right fix. https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.h#newcode232 ui/gfx/render_text.h:232: // specified |elide_behavior|. Only one of elide or truncate should be used. The second part of the comment mentions what will happen if both are used. I was just trying to recommend ("should") that only one should be used. https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.h#newcode234 ui/gfx/render_text.h:234: void Elide(float available_pixel_width, ElideBehavior elide_behavior); So I will need to add UpdateLayoutText to both of SetElideBehavior and SetDisplayRect. Does that seem ok to you? Just confirming.
https://codereview.chromium.org/112063003/diff/1/chrome/browser/ui/views/omni... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/112063003/diff/1/chrome/browser/ui/views/omni... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:367: render_text->SetDirectionalityMode(gfx::DIRECTIONALITY_FORCE_LTR); On 2013/12/11 18:41:32, Anuj wrote: > Are you suggesting URLs should always have LTR directionality because they will > start with a protocol string (http/https/file) and default directionality is > DIRECTIONALITY_FROM_TEXT? > > I added this just to be on safe side, in case the default directionality is ever > changed to DIRECTIONALITY_FROM_UI. I guess it's fine as a precautionary measure. https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:1208: newtext += base::i18n::kLeftToRightMark; On 2013/12/11 18:41:32, Anuj wrote: > See implicit markers here - > http://www.iamcal.com/understanding-bidirectional-text/. What you are referring > to are explicit markers, and they need a PDF (pop-directional-formatting). > > I also wanted to check if similar change should be done in text_elider, probably > based on an additional param. > > Finally, I have tested it manually, it works. Cool, using that mark seems correct, thanks for the info. Yes, you'll need to update text_elider similarly, why do you need an extra param? https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:1226: if (hi == lo) On 2013/12/11 18:41:32, Anuj wrote: > I had a scenario where the following happened > available = 182 > guess = 21, guess_length = 181 > guess = 23, guess_length =193 > guess = 22, guess_length=186 (hi = 22, lo = 22) > > I don't remember the previous hi-lo values. So guess == available never happens. > I can add this to text_elider too if you think this is the right fix. > Yeah, I worked through a few conceptual examples and I think you're right, but I'm not sure this is a sufficient solution. Consider a 4-char string where two characters will fit, but three won't "<<>>": a: low=0, hi=3, guess=1 ("<<" is sufficiently short) b: low=2, hi=3, guess=2 ("<<>" is too long) c: low=2, hi=1... We terminate with "<<>", which is too long. Yes, you'll need to update the text_elider implementation, that's one of the fundamental problems with code duplication, one is fixed and the other isn't... https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.h#newcode232 ui/gfx/render_text.h:232: // specified |elide_behavior|. Only one of elide or truncate should be used. On 2013/12/11 18:41:32, Anuj wrote: > The second part of the comment mentions what will happen if both are used. I was > just trying to recommend ("should") that only one should be used. Then this says elide shouldn't be used on windows, since truncate is always used. I think clarifying the interaction of the two limitations is good, but the advice against using both simultaneously is probably unnecessary. https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.h#newcode234 ui/gfx/render_text.h:234: void Elide(float available_pixel_width, ElideBehavior elide_behavior); On 2013/12/11 18:41:32, Anuj wrote: > So I will need to add UpdateLayoutText to both of SetElideBehavior and > SetDisplayRect. Does that seem ok to you? Just confirming. I think that'll be okay, but you'll only need to UpdateLayoutText on SetDisplayRect if the ElideBehavior is not NO_ELIDE.
RenderText deserves more tests. But I think I deserve a break from this CL, so that I can finish Infinite suggest. I promise I will add more tests before the end of the year. https://codereview.chromium.org/112063003/diff/1/base/i18n/rtl.h File base/i18n/rtl.h (right): https://codereview.chromium.org/112063003/diff/1/base/i18n/rtl.h#newcode76 base/i18n/rtl.h:76: // Given the string in |text|, returns the directionality of the last On 2013/12/11 08:10:14, msw wrote: > nit: combine this with the comment above, via "of the first or last character." Done. https://codereview.chromium.org/112063003/diff/1/chrome/browser/ui/views/omni... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/112063003/diff/1/chrome/browser/ui/views/omni... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:361: const size_t text_size = text.size(); On 2013/12/11 08:10:14, msw wrote: > nit: I'd prefer text_length and .length(), but either is okay. Done. https://codereview.chromium.org/112063003/diff/1/chrome/browser/ui/views/omni... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:367: render_text->SetDirectionalityMode(gfx::DIRECTIONALITY_FORCE_LTR); On 2013/12/11 19:39:57, msw wrote: > On 2013/12/11 18:41:32, Anuj wrote: > > Are you suggesting URLs should always have LTR directionality because they > will > > start with a protocol string (http/https/file) and default directionality is > > DIRECTIONALITY_FROM_TEXT? > > > > I added this just to be on safe side, in case the default directionality is > ever > > changed to DIRECTIONALITY_FROM_UI. > > I guess it's fine as a precautionary measure. Done. https://codereview.chromium.org/112063003/diff/1/chrome/browser/ui/views/omni... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:377: On 2013/12/11 08:10:14, msw wrote: > optional nit: remove blank line. Done. https://codereview.chromium.org/112063003/diff/1/chrome/browser/ui/views/omni... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:383: I like it here for keeping the next if-block distinct. https://codereview.chromium.org/112063003/diff/1/chrome/browser/ui/views/omni... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:386: render_text->ApplyColor(GetColor(state, URL), current_range); On 2013/12/11 08:10:14, msw wrote: > nit: I suggest making this if/else translate the classification style to a local > ColorKind value (URL, DIMMED_TEXT, or TEXT), then in one line simply do: > render_text->ApplyColor(GetColor(GetState(), kind), current_range); Done. https://codereview.chromium.org/112063003/diff/1/chrome/browser/ui/views/omni... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:405: const int display_width = std::min(remaining_width, size.width()); I thought as much, and the paranoid in me left it :) https://codereview.chromium.org/112063003/diff/1/chrome/browser/ui/views/omni... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:407: const gfx::Rect rect( On 2013/12/11 08:10:14, msw wrote: > optional nit: inline this in the SetDisplayRect call. Done. https://codereview.chromium.org/112063003/diff/1/chrome/browser/ui/views/omni... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:413: x += display_width; On 2013/12/11 08:10:14, msw wrote: > nit: remove this and simply return x + display_width. Done. https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:442: ResetLayout(); On 2013/12/11 08:10:14, msw wrote: > nit: Would you mind adding a ResetLayout() call to the end of UpdateLayoutText() > call? Then remove all the explicitly paired ResetLayout() calls. Done. https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:1134: base::string16 elided_text = ElideText(text); On 2013/12/11 08:10:14, msw wrote: > This disregards the obscuring and truncation done above, which seems bad. You > should do something like the |text| const reference above that conditionally > uses |text_| or |layout_text_|. Both this and the |text| assignment above can > actually call GetLayoutText() to do the same. Done. https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:1136: // This doesn't trim styles so ellipsis may get rendered as a different Filed crbug.com/327850 https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:1143: base::string16 RenderText::ElideText(const base::string16& text) { On 2013/12/11 08:10:14, msw wrote: > This is quite similar to ElideText from text_elider... please at least file a > bug against yourself to reduce the redundancy of these codepaths and add a TODO > comment with the bug number here. Done. https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:1147: const float current_text_pixel_width = GetStringSizeF().width(); I can't forgo the argument because of the recursive call for width overflow. I moved the render text creation up here to handle that. https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:1163: const base::string16 cut = slicer.CutString(text.length() / 2, false); On 2013/12/11 08:10:14, msw wrote: > Shouldn't this actually insert an ellipsis if |insert_ellipsis| is true? > Otherwise, if we cut the string in half, and that happens to fit in the > available space, we'd have a string that's truncated instead of elided, when the > next current_text_pixel_width <= available_pixel_width_ returns the halved text. > Ditto for the text_elider implemention from which this was copied. Done. https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:1177: size_t guess; On 2013/12/11 08:10:14, msw wrote: > nit: declare guess in the for loop's control. Done. https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:1191: base::string16 newtext = slicer.CutString(guess, false); On 2013/12/11 08:10:14, msw wrote: > nit: use the unix_hacker naming convention |new_text|. Done. https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:1205: base::i18n::GetLastStrongCharacterDirection(newtext); On 2013/12/11 08:10:14, msw wrote: > nit: indent two more spaces. Done. https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:1208: newtext += base::i18n::kLeftToRightMark; I suggested an extra param because existing users of that method do not expect RTL marker. And what if someone has chosen to inspect size of the returned string from that method, or has an assumption that the last character will be ellipsis? So adding a variant in text_elider.cc https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:1215: // We check the length of the whole desired string at once to ensure we On 2013/12/11 08:10:14, msw wrote: > nit: s/length/width/, s/guess_length/guess_width/; ditto for text_elider. Done. https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:1218: // Check again that we didn't hit a Pango width overflow. If so, cut the I thought adding ellipsis can cause that behavior. But I think you are right. The guess starts at ~length/2, and since we do not reach here until the pango width is valid, this should not be needed. Done. https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:1220: if (guess_length <= 0) { On 2013/12/11 08:10:14, msw wrote: > nit: remove unnecessary curly braces. Done. https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:1223: if (guess_length > available_pixel_width_) { On 2013/12/11 08:10:14, msw wrote: > Shouldn't this break if guess_length == available_pixel_width_? Done. https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:1226: if (hi == lo) I got a new interview question ;) https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.h#newcode232 ui/gfx/render_text.h:232: // specified |elide_behavior|. Only one of elide or truncate should be used. On 2013/12/11 19:39:57, msw wrote: > On 2013/12/11 18:41:32, Anuj wrote: > > The second part of the comment mentions what will happen if both are used. I > was > > just trying to recommend ("should") that only one should be used. > > Then this says elide shouldn't be used on windows, since truncate is always > used. I think clarifying the interaction of the two limitations is good, but the > advice against using both simultaneously is probably unnecessary. Done. https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.h#newcode234 ui/gfx/render_text.h:234: void Elide(float available_pixel_width, ElideBehavior elide_behavior); On 2013/12/11 19:39:57, msw wrote: > On 2013/12/11 18:41:32, Anuj wrote: > > So I will need to add UpdateLayoutText to both of SetElideBehavior and > > SetDisplayRect. Does that seem ok to you? Just confirming. > > I think that'll be okay, but you'll only need to UpdateLayoutText on > SetDisplayRect if the ElideBehavior is not NO_ELIDE. Done. https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.h#newcode642 ui/gfx/render_text.h:642: // The maxmium width available to display the text. On 2013/12/11 08:10:14, msw wrote: > nit: " // The maximum width used for eliding, disjoint from the display_rect > width."... but I hope this goes away. Done. https://codereview.chromium.org/112063003/diff/1/ui/gfx/text_elider.h File ui/gfx/text_elider.h (right): https://codereview.chromium.org/112063003/diff/1/ui/gfx/text_elider.h#newcode48 ui/gfx/text_elider.h:48: // Returns a valid cut boundary at or before |index|. On 2013/12/11 08:10:14, msw wrote: > nit: make this "before/after", ditch the second comment and blank line. Done. https://codereview.chromium.org/112063003/diff/1/ui/gfx/text_elider.h#newcode107 ui/gfx/text_elider.h:107: NO_ELISION On 2013/12/11 08:10:14, msw wrote: > Consider making this NO_ELIDE to match the views::Label::ElideBehavior name; > I've been meaning to consolidate these enums for some time. Done.
Added jshin@ for base/i18n/rtl*
Looking pretty good. The tests you've added (with my suggested changes) are probably sufficient for now; perhaps crbug.com/327846 should mention porting (some of) the tests in text_elider_unittest to render_text_unittest, and/or you can add a TODO to that effect in render_text_unittest. https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:442: ResetLayout(); On 2013/12/12 08:08:41, Anuj wrote: > On 2013/12/11 08:10:14, msw wrote: > > nit: Would you mind adding a ResetLayout() call to the end of > UpdateLayoutText() > > call? Then remove all the explicitly paired ResetLayout() calls. > > Done. Thanks! https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:1147: const float current_text_pixel_width = GetStringSizeF().width(); On 2013/12/12 08:08:41, Anuj wrote: > I can't forgo the argument because of the recursive call for width overflow. I > moved the render text creation up here to handle that. Hmm, nice! https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:1208: newtext += base::i18n::kLeftToRightMark; On 2013/12/12 08:08:41, Anuj wrote: > I suggested an extra param because existing users of that method do not expect > RTL marker. And what if someone has chosen to inspect size of the returned > string from that method, or has an assumption that the last character will be > ellipsis? > > So adding a variant in text_elider.cc I suggest making the default behavior add the directionality control marks because it's the right thing to do; we can update callers as needed. https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:1226: if (hi == lo) On 2013/12/12 08:08:41, Anuj wrote: > I got a new interview question ;) I thought binary searches were supposed to be easy! Your new solution looks good. https://codereview.chromium.org/112063003/diff/20001/base/i18n/rtl.h File base/i18n/rtl.h (right): https://codereview.chromium.org/112063003/diff/20001/base/i18n/rtl.h#newcode75 base/i18n/rtl.h:75: nit: remove blank line https://codereview.chromium.org/112063003/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/112063003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:383: const ResultViewState state = GetState(); optional nit: inline GetState() https://codereview.chromium.org/112063003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:401: render_text->SetElideBehavior(gfx::ELIDE_AT_END); Should this only happen if the unelided text won't fit in the remaining width? https://codereview.chromium.org/112063003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:407: // Readjust display rectangle. Is this necessary? It doesn't make sense to save a couple of pixels at the expense of another elide pass, but if you expect a substantial amount of space to remain for showing additional trailing text, you should be able to leave the display rect as-is, return x+render_text->GetContentWidth() and allow the empty portion of this display rect to intersect with the next RenderText. https://codereview.chromium.org/112063003/diff/20001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/112063003/diff/20001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:436: if (elide_behavior_ != gfx::NO_ELIDE) This should check if the elide behavior changed, and run even if it was changed from ELIDE_AT_END to NO_ELIDE. https://codereview.chromium.org/112063003/diff/20001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:1125: int content_width = GetContentWidth(); Inline this, it's value is only needed once, conditionally. https://codereview.chromium.org/112063003/diff/20001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:1144: // Create a copy render text. We copy the attributes which affect the optional nit: // Create a RenderText copy with attributes that affect the rendering width. https://codereview.chromium.org/112063003/diff/20001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:1186: render_text->SetText(new_text); Oh darn, I think you'll need to (conditionally?) reapply the original styles and colors here, because otherwise if a short guess truncates styles applied to the end of the string, and then another loop pass restores some of that string length, it'll ignore the intended styles for that range... https://codereview.chromium.org/112063003/diff/20001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:1191: nit: remove this blank line. https://codereview.chromium.org/112063003/diff/20001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/112063003/diff/20001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:646: float available_pixel_width_; Remove this. https://codereview.chromium.org/112063003/diff/20001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/112063003/diff/20001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:403: TEST_F(RenderTextTest, ElidedText) { I think this test might be too fragile with hard-coded exact pixel widths; we've seen the available fonts change with test bot upgrades, variations between platforms, etc. Could this test measure the width of the expected layout_text and use those widths for the display rect? https://codereview.chromium.org/112063003/diff/20001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:414: { L"", L"" , 30}, nit: align the comments after the preceding entry: { L"", L"", 30}, https://codereview.chromium.org/112063003/diff/20001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:425: { L"012" L"abc", L"012a\x2026" , 44}, nit: align the strings within each commented section. https://codereview.chromium.org/112063003/diff/20001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:450: render_text->SetText(base::string16()); Why is this needed? https://codereview.chromium.org/112063003/diff/20001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:464: render_text->SetDisplayRect(gfx::Rect(0, 0, 30, 100)); Ditto here, consider using the width of L"**\x2026" instead of hard-coded 30px. https://codereview.chromium.org/112063003/diff/20001/ui/gfx/text_elider.h File ui/gfx/text_elider.h (right): https://codereview.chromium.org/112063003/diff/20001/ui/gfx/text_elider.h#new... ui/gfx/text_elider.h:50: nit: remove blank line
I have added TODOs as well as updated bugs about more work/tests needed. PTAL. https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:442: ResetLayout(); On 2013/12/12 19:28:29, msw wrote: > On 2013/12/12 08:08:41, Anuj wrote: > > On 2013/12/11 08:10:14, msw wrote: > > > nit: Would you mind adding a ResetLayout() call to the end of > > UpdateLayoutText() > > > call? Then remove all the explicitly paired ResetLayout() calls. > > > > Done. > > Thanks! Done. https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:1147: const float current_text_pixel_width = GetStringSizeF().width(); On 2013/12/12 19:28:29, msw wrote: > On 2013/12/12 08:08:41, Anuj wrote: > > I can't forgo the argument because of the recursive call for width overflow. I > > moved the render text creation up here to handle that. > > Hmm, nice! Done. https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:1208: newtext += base::i18n::kLeftToRightMark; Adding a TODO, because ellipsis in middle also needs some additional handling. https://codereview.chromium.org/112063003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:1226: if (hi == lo) On 2013/12/12 19:28:29, msw wrote: > On 2013/12/12 08:08:41, Anuj wrote: > > I got a new interview question ;) > > I thought binary searches were supposed to be easy! > Your new solution looks good. Done. https://codereview.chromium.org/112063003/diff/20001/base/i18n/rtl.h File base/i18n/rtl.h (right): https://codereview.chromium.org/112063003/diff/20001/base/i18n/rtl.h#newcode75 base/i18n/rtl.h:75: On 2013/12/12 19:28:29, msw wrote: > nit: remove blank line Done. https://codereview.chromium.org/112063003/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/112063003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:383: const ResultViewState state = GetState(); On 2013/12/12 19:28:29, msw wrote: > optional nit: inline GetState() Done. https://codereview.chromium.org/112063003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:401: render_text->SetElideBehavior(gfx::ELIDE_AT_END); Shouldn't matter, but no harm in safeguard either. Done. https://codereview.chromium.org/112063003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:407: // Readjust display rectangle. On 2013/12/12 19:28:29, msw wrote: > Is this necessary? It doesn't make sense to save a couple of pixels at the > expense of another elide pass, but if you expect a substantial amount of space > to remain for showing additional trailing text, you should be able to leave the > display rect as-is, return x+render_text->GetContentWidth() and allow the empty > portion of this display rect to intersect with the next RenderText. Done. https://codereview.chromium.org/112063003/diff/20001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/112063003/diff/20001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:436: if (elide_behavior_ != gfx::NO_ELIDE) Good point. Thanks. https://codereview.chromium.org/112063003/diff/20001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:1125: int content_width = GetContentWidth(); On 2013/12/12 19:28:29, msw wrote: > Inline this, it's value is only needed once, conditionally. Done. https://codereview.chromium.org/112063003/diff/20001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:1144: // Create a copy render text. We copy the attributes which affect the On 2013/12/12 19:28:29, msw wrote: > optional nit: // Create a RenderText copy with attributes that affect the > rendering width. Done. https://codereview.chromium.org/112063003/diff/20001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:1186: render_text->SetText(new_text); No. The unconditional reapplying of original styles works fine. Note that we are restoring from original styles which are not getting modified, and correspond to the full text length. And assignment is creating a copy if that is your doubt. https://codereview.chromium.org/112063003/diff/20001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:1191: On 2013/12/12 19:28:29, msw wrote: > nit: remove this blank line. Done. https://codereview.chromium.org/112063003/diff/20001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/112063003/diff/20001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:646: float available_pixel_width_; On 2013/12/12 19:28:29, msw wrote: > Remove this. Done. https://codereview.chromium.org/112063003/diff/20001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/112063003/diff/20001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:403: TEST_F(RenderTextTest, ElidedText) { On 2013/12/12 19:28:29, msw wrote: > I think this test might be too fragile with hard-coded exact pixel widths; we've > seen the available fonts change with test bot upgrades, variations between > platforms, etc. Could this test measure the width of the expected layout_text > and use those widths for the display rect? Done. https://codereview.chromium.org/112063003/diff/20001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:414: { L"", L"" , 30}, Didn't understand. https://codereview.chromium.org/112063003/diff/20001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:425: { L"012" L"abc", L"012a\x2026" , 44}, For the record, I find this column formatting insane!! https://codereview.chromium.org/112063003/diff/20001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:450: render_text->SetText(base::string16()); To clear the layout text. https://codereview.chromium.org/112063003/diff/20001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:464: render_text->SetDisplayRect(gfx::Rect(0, 0, 30, 100)); On 2013/12/12 19:28:29, msw wrote: > Ditto here, consider using the width of L"**\x2026" instead of hard-coded 30px. Done. https://codereview.chromium.org/112063003/diff/20001/ui/gfx/text_elider.h File ui/gfx/text_elider.h (right): https://codereview.chromium.org/112063003/diff/20001/ui/gfx/text_elider.h#new... ui/gfx/text_elider.h:50: On 2013/12/12 19:28:29, msw wrote: > nit: remove blank line Done.
LGTM with a Q, thanks for working on this, please do help with consolidating the newly duplicated code as you have time; I suspect we could just have text_elider setup a RenderText and run eliding at the specified width, then return its layout text. https://codereview.chromium.org/112063003/diff/20001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/112063003/diff/20001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:1186: render_text->SetText(new_text); On 2013/12/13 01:23:50, Anuj wrote: > No. The unconditional reapplying of original styles works fine. Note that we are > restoring from original styles which are not getting modified, and correspond to > the full text length. And assignment is creating a copy if that is your doubt. Gah, I just missed copying done above... Sorry for the false alarm. https://codereview.chromium.org/112063003/diff/20001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/112063003/diff/20001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:414: { L"", L"" , 30}, On 2013/12/13 01:23:50, Anuj wrote: > Didn't understand. > D'oh I meant "align the commas", so that the commas are after the strings, not after some spaces and just one space before the pixel widths. https://codereview.chromium.org/112063003/diff/20001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:425: { L"012" L"abc", L"012a\x2026" , 44}, On 2013/12/13 01:23:50, Anuj wrote: > For the record, I find this column formatting insane!! > I think it makes reading test cases easier :) https://codereview.chromium.org/112063003/diff/40001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/112063003/diff/40001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:453: expected_render_text->SetText(base::string16()); nit: Won't the SetText calls on the next loop iteration clear the LayoutText? Are you doing this to avoid a recalculation of the elided text when you call render_text->SetDisplayRect?
This was a fun CL. If it were not for M33 cut date, I would have done more. I thought about text_elider using RenderText, but elide-in-middle is a difficult one for RenderText along with styles. I feel we will know better when we actually change the code. https://codereview.chromium.org/112063003/diff/20001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/112063003/diff/20001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:1186: render_text->SetText(new_text); On 2013/12/13 01:56:08, msw wrote: > On 2013/12/13 01:23:50, Anuj wrote: > > No. The unconditional reapplying of original styles works fine. Note that we > are > > restoring from original styles which are not getting modified, and correspond > to > > the full text length. And assignment is creating a copy if that is your doubt. > > Gah, I just missed copying done above... Sorry for the false alarm. Done. https://codereview.chromium.org/112063003/diff/20001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/112063003/diff/20001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:414: { L"", L"" , 30}, On 2013/12/13 01:56:08, msw wrote: > On 2013/12/13 01:23:50, Anuj wrote: > > Didn't understand. > > > > D'oh I meant "align the commas", so that the commas are after the strings, not > after some spaces and just one space before the pixel widths. Done. https://codereview.chromium.org/112063003/diff/20001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:425: { L"012" L"abc", L"012a\x2026" , 44}, Looks like I have some motivation to hack on clang-format. https://codereview.chromium.org/112063003/diff/40001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/112063003/diff/40001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:453: expected_render_text->SetText(base::string16()); Some previous incarnation of tests failed without this. You are right this is not needed. Removed.
Still LGTM. It should be trivial enough to clear any style/color breaks in the logical text range being elided for elide-in-middle, hopefully one of us will have some time to take a look in the near future. https://codereview.chromium.org/112063003/diff/60001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/112063003/diff/60001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:413: { L"" , L"" }, I actually wanted the opposite of this, but it's not worth your time to change.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skanuj@chromium.org/112063003/60001
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
4ba9ae390740e529075d10e4cc1f32b994ca19df..f7b8e27a4cf324f721fa4224d467d680746e7956
100644
--- a/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
+++ b/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
@@ -357,213 +357,58 @@ 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 colorKind = TEXT;
+ if (classifications[i].style & ACMatchClassification::URL)
+ colorKind = URL;
+ else if (force_dim ||
+ (classifications[i].style & ACMatchClassification::DIM))
+ colorKind = DIMMED_TEXT;
+ render_text->ApplyColor(GetColor(GetState(), colorKind), 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::BOLD) ?
- …
(message too large)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skanuj@chromium.org/112063003/60001
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
4ba9ae390740e529075d10e4cc1f32b994ca19df..f7b8e27a4cf324f721fa4224d467d680746e7956
100644
--- a/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
+++ b/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
@@ -357,213 +357,58 @@ 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 colorKind = TEXT;
+ if (classifications[i].style & ACMatchClassification::URL)
+ colorKind = URL;
+ else if (force_dim ||
+ (classifications[i].style & ACMatchClassification::DIM))
+ colorKind = DIMMED_TEXT;
+ render_text->ApplyColor(GetColor(GetState(), colorKind), 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::BOLD) ?
- …
(message too large)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skanuj@chromium.org/112063003/70001
Message was sent while issue was closed.
Change committed as 240572
Message was sent while issue was closed.
On 2013/12/13 09:34:24, I haz the power (commit-bot) wrote: > Change committed as 240572 RenderTextTest.ElidedText fails on XP Tests (3)
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/103493003/ by szym@chromium.org. The reason for reverting is: RenderTextTest.ElidedText fails on XP Tests (3).
Message was sent while issue was closed.
FYI, I left comments on the version of this patch that appears on https://codereview.chromium.org/107513011/ . |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
