Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2009673005:
AiS: Have Omnibox definitions use new multi-line elision in RenderText (Closed)
|
Created:
4 years, 7 months ago by Kevin Bailey Modified:
4 years, 6 months ago CC:
chromium-reviews, tfarina, James Su Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAiS: Have Omnibox definitions use new multi-line elision in RenderText
Use the new multi-line elision in RenderText to manage/display
a dictionary definition answer-in-suggest, which can
span multiple lines.
BUG=591803
Committed: https://crrev.com/9bc3781bd7dd8972a0b36c996e003871af23f433
Cr-Commit-Position: refs/heads/master@{#397198}
Patch Set 1 #Patch Set 2 : Removed stray comment #
Total comments: 28
Patch Set 3 : Some responses, mostly temp variables #Patch Set 4 : Streamlined RenderTexts #
Total comments: 16
Patch Set 5 : Style responses #
Messages
Total messages: 28 (10 generated)
Description was changed from ========== AiS: Have Omnibox definitions use new multi-line elision in RenderText Use the new multi-line elision in RenderText to manage/display a dictionary definition answer-in-suggest, which can span multiple lines. BUG=591803 ========== to ========== AiS: Have Omnibox definitions use new multi-line elision in RenderText Use the new multi-line elision in RenderText to manage/display a dictionary definition answer-in-suggest, which can span multiple lines. BUG=591803 ==========
krb@chromium.org changed reviewers: + pkasting@google.com
Hi pkasting@, Could you review as owner? thx, krb
pkasting@chromium.org changed reviewers: + pkasting@chromium.org
https://codereview.chromium.org/2009673005/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/2009673005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:281: if (!match_.answer->second_line().text_fields().empty() && Nit: You access match_.answer->second_line().text_fields() many times, use a const ref temp to abbreviate https://codereview.chromium.org/2009673005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:284: if (description_rendertext_.get()) { Nit: .get() not necessary https://codereview.chromium.org/2009673005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:285: num_lines = std::min(3UL, description_rendertext_->GetNumLines()); Nit: UL not necessary https://codereview.chromium.org/2009673005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:291: 3, match_.answer->second_line().text_fields()[0].num_lines()); There are three separate places that do this clamping. If we always want the value clamped, let's do it at the place where it is parsed out of the JSON, not at every consumer site. https://codereview.chromium.org/2009673005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:295: // Trigger layout. Nit: The comments in here don't really add anything to the code. https://codereview.chromium.org/2009673005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:298: num_lines = std::min(num_lines, render_text->GetNumLines()); The code below doesn't clamp GetNumLines() to num_lines, but instead uses SetMaxLines(). It seems like one of these ought to be The Right Way and used in both places. And really, I would seek to combine these two places that both do text measurement into one helper if possible. https://codereview.chromium.org/2009673005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:462: if (!match.answer->second_line().text_fields().empty() && Nit: Same comment regarding adding a temp to reduce verbosity https://codereview.chromium.org/2009673005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:466: render_text->SetDisplayRect(gfx::Rect(gfx::Size(right_x - x, 0))); Nit: Factor the "right_x - x" out of here and below into one temp, so someone won't change one without changing the other https://codereview.chromium.org/2009673005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:806: max_lines = text_field.num_lines(); This code reads the num_lines() field of every TextField, but the two places above only check for it on the 0th element. Which is correct? If it can only be on the 0th element, then remove this conditional and pull the read out of the loop. https://codereview.chromium.org/2009673005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:813: destination->SetMaxLines(std::min(max_lines, 3)); If my suggestions above are followed regarding moving clamping to the point where we read the JSON and pulling the num_lines() check out of the loop above, perhaps this function becomes simple enough that it can just be collapsed together with CreateAnswerLine(). That might be necessary anyway if we could ever have a description with additional_text() or the other fields handled there but not here.
krb@chromium.org changed reviewers: + jdonnelly@chromium.org - pkasting@google.com
Looping in jdonnely@ for a couple clarifications. https://codereview.chromium.org/2009673005/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/2009673005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:281: if (!match_.answer->second_line().text_fields().empty() && On 2016/05/27 19:10:23, Peter Kasting wrote: > Nit: You access match_.answer->second_line().text_fields() many times, use a > const ref temp to abbreviate Done. https://codereview.chromium.org/2009673005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:284: if (description_rendertext_.get()) { On 2016/05/27 19:10:22, Peter Kasting wrote: > Nit: .get() not necessary Done. https://codereview.chromium.org/2009673005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:285: num_lines = std::min(3UL, description_rendertext_->GetNumLines()); On 2016/05/27 19:10:22, Peter Kasting wrote: > Nit: UL not necessary The compiler complains. Or are you suggesting changing the return type of 'GetNumLines()'? I was told Chromium favors the use of 'size_t'. https://codereview.chromium.org/2009673005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:291: 3, match_.answer->second_line().text_fields()[0].num_lines()); It's my understanding that the JSON code is used across platforms, and that we might not want to do the same thing on each. Justin, do you think it's ok to push this down to the JSON code? https://codereview.chromium.org/2009673005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:295: // Trigger layout. On 2016/05/27 19:10:22, Peter Kasting wrote: > Nit: The comments in here don't really add anything to the code. Done. https://codereview.chromium.org/2009673005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:298: num_lines = std::min(num_lines, render_text->GetNumLines()); Sorry, I'm confused by your comment. The code that uses 'SetNumLines()' is constructing a RenderText and necessarily setting num_lines, so that it might be used above and in 'DrawRenderText()'. Regarding the second comment, I'm not sure why you'd want to do the same thing in both places since this function constructs a non-trivial RenderText, to find out it's actual width. There are two things happening here. First, we find out the maximum number of lines requested by AiS. Then, we have to calculate the actual number of lines, which depends on width, to report how much height we need. It's almost a chicken-and-egg situation. https://codereview.chromium.org/2009673005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:462: if (!match.answer->second_line().text_fields().empty() && On 2016/05/27 19:10:22, Peter Kasting wrote: > Nit: Same comment regarding adding a temp to reduce verbosity Done. https://codereview.chromium.org/2009673005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:466: render_text->SetDisplayRect(gfx::Rect(gfx::Size(right_x - x, 0))); On 2016/05/27 19:10:22, Peter Kasting wrote: > Nit: Factor the "right_x - x" out of here and below into one temp, so someone > won't change one without changing the other Done. https://codereview.chromium.org/2009673005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:806: max_lines = text_field.num_lines(); Justin, I'm curious what your opinion here is, considering your work with the AiS stuff. Mandate first text field? (This would help the presence detection above). First filled in field? Max filled in field? In any case, I'll certainly make it consistent.
https://codereview.chromium.org/2009673005/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/2009673005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:285: num_lines = std::min(3UL, description_rendertext_->GetNumLines()); On 2016/05/31 18:08:47, Kevin Bailey wrote: > On 2016/05/27 19:10:22, Peter Kasting wrote: > > Nit: UL not necessary > > The compiler complains. Or are you suggesting changing the return type of > 'GetNumLines()'? I was told Chromium favors the use of 'size_t'. Huh. Surprises me the compiler complains here. If that's the case, then "static_cast<size_t>(3)" rather than using the "UL" suffix, in part because size_t isn't always the same as an unsigned long. https://codereview.chromium.org/2009673005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:298: num_lines = std::min(num_lines, render_text->GetNumLines()); On 2016/05/31 18:08:47, Kevin Bailey wrote: > Sorry, I'm confused by your comment. The code that uses 'SetNumLines()' is > constructing a RenderText and necessarily setting num_lines, so that it might be > used above and in 'DrawRenderText()'. > > Regarding the second comment, I'm not sure why you'd want to do the same thing > in both places since this function constructs a non-trivial RenderText, to find > out it's actual width. > > There are two things happening here. First, we find out the maximum number of > lines requested by AiS. Then, we have to calculate the actual number of lines, > which depends on width, to report how much height we need. It's almost a > chicken-and-egg situation. Yeah, I think I was misreading the code. I'm wondering if it would still make sense to call SetMaxLines() here, before laying out the RenderText, in case the text would otherwise be many more lines and this would save layout time by short-circuiting after 3. Or is that not how RenderText layout works? If we did that, then presumably the std::min() call could be removed, as GetNumLines() would be limited to 3 anyway. In which case both these locations would basically do this: size_t PossibleHelperFunction(render_text, width) { render_text->SetDisplayRect(gfx::Rect(width, 0)); // Maybe deserves comment about the 0 render_text->SetMaxLines(std::min(3, match_.answer->second_line().text_fields()[0].num_lines())); render_text->GetStringSize(); // Maybe GetNumLines() should call this automatically return render_text->GetNumLines(); }
https://codereview.chromium.org/2009673005/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/2009673005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:298: num_lines = std::min(num_lines, render_text->GetNumLines()); Calling 'SetMaxLines()' here would cause the RenderText to do the additional step of eliding the text (if it went over.) If we could preserve this work, that would actually save the total time, but 'GetPreferredSize()' is part of an existing Views const interface, so we can't set 'description_rendertext_'. So I'm not setting it to save work, but it would be nice if we could streamline this WRT const.
https://codereview.chromium.org/2009673005/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/2009673005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:298: num_lines = std::min(num_lines, render_text->GetNumLines()); On 2016/05/31 20:41:49, Kevin Bailey wrote: > Calling 'SetMaxLines()' here would cause the RenderText to do the additional > step of eliding the text (if it went over.) > > If we could preserve this work, that would actually save the total time, but > 'GetPreferredSize()' is part of an existing Views const interface, so we can't > set 'description_rendertext_'. > > So I'm not setting it to save work, but it would be nice if we could streamline > this WRT const. Is |description_rendertext_| something whose state is in some way exposed by part of the class' API, or is it an implementation detail for the class? If the latter, this is a case of physical-const-versus-logical-const, and we can likely just make description_rendertext_ mutable, although you'd want to make sure the layout code is always reached before the RenderTExt actually needs to get drawn (relying on GetPreferredSize() being called first feels potentially fragile).
https://codereview.chromium.org/2009673005/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/2009673005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:298: num_lines = std::min(num_lines, render_text->GetNumLines()); Ah, I didn't read the compiler error close enough. It was complaining about the create RenderText call, not the assignment to 'description_rendertext_' (which is already mutable.) Let me clean this up.
https://codereview.chromium.org/2009673005/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/2009673005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:291: 3, match_.answer->second_line().text_fields()[0].num_lines()); On 2016/05/31 18:08:47, Kevin Bailey wrote: > It's my understanding that the JSON code is used across platforms, and that we > might not want to do the same thing on each. > > Justin, do you think it's ok to push this down to the JSON code? Yes, I think that's fine. Three strikes me as a good limit on all platforms, but if we ever changed our minds, we could just make that a parameter to the parsing code. https://codereview.chromium.org/2009673005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:806: max_lines = text_field.num_lines(); On 2016/05/31 18:08:47, Kevin Bailey wrote: > Justin, I'm curious what your opinion here is, considering your work with the > AiS stuff. Mandate first text field? (This would help the presence detection > above). First filled in field? Max filled in field? > > In any case, I'll certainly make it consistent. We should assume the first field, IMO. It's not really clear how multiple text_field elements each with their own num_lines would be handled, anyway. Can you send an email to the Suggest folks letting them know that we're planning on making this simplifying assumption? https://codereview.chromium.org/2009673005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:811: if (max_lines > 0) { Shouldn't this be > 1? https://codereview.chromium.org/2009673005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:813: destination->SetMaxLines(std::min(max_lines, 3)); On 2016/05/27 19:10:22, Peter Kasting wrote: > If my suggestions above are followed regarding moving clamping to the point > where we read the JSON and pulling the num_lines() check out of the loop above, > perhaps this function becomes simple enough that it can just be collapsed > together with CreateAnswerLine(). > > That might be necessary anyway if we could ever have a description with > additional_text() or the other fields handled there but not here. Agreed about the desirability of collapsing these two functions into one. To the extent that we can do so without adding unreasonably complexity, I'd like to support more flexibility in the format of future answers. (Or even current answers - are you sure that none of the current answer types has additional_text or status_text in the second line?)
I think it cleaned up nicely. I still prefer the per-platform max-lines to be in a per-platform file. I'm hoping that now that it's only used in one place, the urge to move it won't be as strong. https://codereview.chromium.org/2009673005/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/2009673005/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:764: for (const SuggestionAnswer::TextField& text_field : line.text_fields()) Am I the only bothered by the same variable being used for 4 different things?
LGTM https://codereview.chromium.org/2009673005/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/2009673005/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:52: constexpr int kMAX_DISPLAY_LINES = 3; Nit: Declare this in the lone scope where it's used. Name it kMaxDisplayLines. (All caps style, which we don't use, would omit the leading k.) https://codereview.chromium.org/2009673005/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:285: if (!text_fields.empty() && text_fields[0].has_num_lines()) { Nit: You use [0] here and .front() below. Pick one. (I tend to use .front(), but I think I'm an outlier.) https://codereview.chromium.org/2009673005/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:286: if (!description_rendertext_) Nit: {} https://codereview.chromium.org/2009673005/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:290: gfx::Rect(text_bounds().width(), 0)); Nit: Prefer text_bounds_ https://codereview.chromium.org/2009673005/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:296: return gfx::Size(0, GetContentLineHeight() + GetAnswerLineHeight()); Nit: Reverse the conditional above and do this as an early-return; or else do something like: int num_lines = 1; if (...) { ... num_lines = description_rendertext_->GetNumLines(); } return gfx::Size(0, GetContentLineHeight() + GetAnswerLineHeight() * num_lines); https://codereview.chromium.org/2009673005/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:450: int final_width = right_x - x; Nit: const? https://codereview.chromium.org/2009673005/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:764: for (const SuggestionAnswer::TextField& text_field : line.text_fields()) On 2016/06/01 14:56:36, Kevin Bailey wrote: > Am I the only bothered by the same variable being used for 4 different things? Well, it's really 3 variables due to scoping. If you're really bothered, you could call this one field, the next first_field, the next additional_text, and the next status_text?
https://codereview.chromium.org/2009673005/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/2009673005/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:52: constexpr int kMAX_DISPLAY_LINES = 3; On 2016/06/01 16:15:29, Peter Kasting wrote: > Nit: Declare this in the lone scope where it's used. > > Name it kMaxDisplayLines. (All caps style, which we don't use, would omit the > leading k.) Done. https://codereview.chromium.org/2009673005/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:285: if (!text_fields.empty() && text_fields[0].has_num_lines()) { On 2016/06/01 16:15:29, Peter Kasting wrote: > Nit: You use [0] here and .front() below. Pick one. (I tend to use .front(), > but I think I'm an outlier.) Done. https://codereview.chromium.org/2009673005/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:286: if (!description_rendertext_) On 2016/06/01 16:15:29, Peter Kasting wrote: > Nit: {} Done. https://codereview.chromium.org/2009673005/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:290: gfx::Rect(text_bounds().width(), 0)); On 2016/06/01 16:15:29, Peter Kasting wrote: > Nit: Prefer text_bounds_ This one surprises me, particularly since 'text_bounds()' is const, but sure. https://codereview.chromium.org/2009673005/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:296: return gfx::Size(0, GetContentLineHeight() + GetAnswerLineHeight()); On 2016/06/01 16:15:29, Peter Kasting wrote: > Nit: Reverse the conditional above and do this as an early-return; or else do > something like: > > int num_lines = 1; > if (...) { > ... > num_lines = description_rendertext_->GetNumLines(); > } > return gfx::Size(0, > GetContentLineHeight() + GetAnswerLineHeight() * num_lines); Done. https://codereview.chromium.org/2009673005/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:450: int final_width = right_x - x; On 2016/06/01 16:15:29, Peter Kasting wrote: > Nit: const? Done. https://codereview.chromium.org/2009673005/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:764: for (const SuggestionAnswer::TextField& text_field : line.text_fields()) I took care of one, but the others didn't seem like big enough improvements. Maybe the next change to this function will tip the balance.
The CQ bit was checked by krb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2009673005/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2009673005/80001
lgtm mod Peter's nits https://codereview.chromium.org/2009673005/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/2009673005/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:764: for (const SuggestionAnswer::TextField& text_field : line.text_fields()) On 2016/06/01 14:56:36, Kevin Bailey wrote: > Am I the only bothered by the same variable being used for 4 different things? To me a variable named after its type is just saying it's "a text field" which all 4 things clearly are. And it has the advantage of being relatively short - line breaks in statements tend to bother me in an unreasonable way. :) But I have no objection to giving them different names if it reads clearer to you.
On 2016/06/01 17:38:17, Justin Donnelly wrote: > > To me a variable named after its type is just saying it's "a text field" which > all 4 things clearly are. And it has the advantage of being relatively short - > line breaks in statements tend to bother me in an unreasonable way. :) But I > have no objection to giving them different names if it reads clearer to you. I guess I should have been more clear about the issue, and said, "same name". The concern is that, probably requiring extremely bad luck, someone would fat finger something and later lines in the method would be left referring to unintended or uninitialized values. And yes, there's the readability thing too. It's not a big deal; I just thought that with all the concern we have for more stylistic issues, I'm surprised to find such a tangible banana peel.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by krb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2009673005/#ps80001 (title: "Style responses")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2009673005/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2009673005/80001
Message was sent while issue was closed.
Description was changed from ========== AiS: Have Omnibox definitions use new multi-line elision in RenderText Use the new multi-line elision in RenderText to manage/display a dictionary definition answer-in-suggest, which can span multiple lines. BUG=591803 ========== to ========== AiS: Have Omnibox definitions use new multi-line elision in RenderText Use the new multi-line elision in RenderText to manage/display a dictionary definition answer-in-suggest, which can span multiple lines. BUG=591803 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== AiS: Have Omnibox definitions use new multi-line elision in RenderText Use the new multi-line elision in RenderText to manage/display a dictionary definition answer-in-suggest, which can span multiple lines. BUG=591803 ========== to ========== AiS: Have Omnibox definitions use new multi-line elision in RenderText Use the new multi-line elision in RenderText to manage/display a dictionary definition answer-in-suggest, which can span multiple lines. BUG=591803 Committed: https://crrev.com/9bc3781bd7dd8972a0b36c996e003871af23f433 Cr-Commit-Position: refs/heads/master@{#397198} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/9bc3781bd7dd8972a0b36c996e003871af23f433 Cr-Commit-Position: refs/heads/master@{#397198} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
