Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10692101:
Use RenderText directly in omnibox_result_view.cc. (Closed)
|
Created:
8 years, 5 months ago by Alexei Svitkine (slow) Modified:
8 years, 5 months ago CC:
chromium-reviews, James Su, msw Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionUse RenderText directly in omnibox_result_view.cc and baseline-align the runs.
BUG=128027
TEST=Text is drawn correctly in omnibox suggest drop down.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=147947
Patch Set 1 : #
Total comments: 13
Patch Set 2 : #Messages
Total messages: 16 (0 generated)
Hey Peter, Could you take a look at this? I've tested it and the new code seems to render fine for regular English LTR text and I believe it should also work correctly in RTL mode, but I don't have a good way to test it (that's why I have been asking you about how to test this). Is there someone we could get to patch this in locally and try it in RTL mode who'll be able to confirm whether this works right in that case? (i.e. some of the folks in Israel?)
You can try and test manually by running with --lang=he and putting in various English and Hebrew strings, and then comparing to the existing build. Also contact playmobil@google.com regarding more extensive RTL testing. http://codereview.chromium.org/10692101/diff/14001/chrome/browser/ui/views/om... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (left): http://codereview.chromium.org/10692101/diff/14001/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:447: flags |= gfx::Canvas::FORCE_RTL_DIRECTIONALITY; It worries me that you have no equivalent of this statement. This seems like a big red flag for RTL.
+jeremy (playmobil) Jeremy, would you be able to apply this patch locally to your Windows checkout and see whether omnibox suggestions look right w.r.t. RTL? http://codereview.chromium.org/10692101/diff/14001/chrome/browser/ui/views/om... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (left): http://codereview.chromium.org/10692101/diff/14001/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:447: flags |= gfx::Canvas::FORCE_RTL_DIRECTIONALITY; On 2012/07/12 17:51:48, Peter Kasting wrote: > It worries me that you have no equivalent of this statement. This seems like a > big red flag for RTL. RenderText will use RTL directionality in the presence of strong RTL characters in the string. Is there a case where this will not give the results we want?
http://codereview.chromium.org/10692101/diff/14001/chrome/browser/ui/views/om... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (left): http://codereview.chromium.org/10692101/diff/14001/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:447: flags |= gfx::Canvas::FORCE_RTL_DIRECTIONALITY; On 2012/07/12 17:58:02, Alexei Svitkine wrote: > On 2012/07/12 17:51:48, Peter Kasting wrote: > > It worries me that you have no equivalent of this statement. This seems like > a > > big red flag for RTL. > > RenderText will use RTL directionality in the presence of strong RTL characters > in the string. Is there a case where this will not give the results we want? We may get bad results for URLs with Hebrew characters in them where we render those sections in reversed order (note how when calculating is_rtl we exclude the is_url cases). I don't know how coherent the results of the GetVisualRun() call on line 357 will be with RenderText's algorithm otherwise so I can't speculate much.
On 2012/07/12 18:05:12, Peter Kasting wrote: > http://codereview.chromium.org/10692101/diff/14001/chrome/browser/ui/views/om... > File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (left): > > http://codereview.chromium.org/10692101/diff/14001/chrome/browser/ui/views/om... > chrome/browser/ui/views/omnibox/omnibox_result_view.cc:447: flags |= > gfx::Canvas::FORCE_RTL_DIRECTIONALITY; > On 2012/07/12 17:58:02, Alexei Svitkine wrote: > > On 2012/07/12 17:51:48, Peter Kasting wrote: > > > It worries me that you have no equivalent of this statement. This seems > like > > a > > > big red flag for RTL. > > > > RenderText will use RTL directionality in the presence of strong RTL > characters > > in the string. Is there a case where this will not give the results we want? > > We may get bad results for URLs with Hebrew characters in them where we render > those sections in reversed order (note how when calculating is_rtl we exclude > the is_url cases). I don't know how coherent the results of the GetVisualRun() > call on line 357 will be with RenderText's algorithm otherwise so I can't > speculate much. I've sent a build with this patch applied for Jeremy to try for RTL/BiDi issues in the omnibox drop down and he says he didn't see any problems.
http://codereview.chromium.org/10692101/diff/14001/chrome/browser/ui/views/om... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (left): http://codereview.chromium.org/10692101/diff/14001/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:447: flags |= gfx::Canvas::FORCE_RTL_DIRECTIONALITY; On 2012/07/12 18:05:14, Peter Kasting wrote: > On 2012/07/12 17:58:02, Alexei Svitkine wrote: > > On 2012/07/12 17:51:48, Peter Kasting wrote: > > > It worries me that you have no equivalent of this statement. This seems > like > > a > > > big red flag for RTL. > > > > RenderText will use RTL directionality in the presence of strong RTL > characters > > in the string. Is there a case where this will not give the results we want? > > We may get bad results for URLs with Hebrew characters in them where we render > those sections in reversed order (note how when calculating is_rtl we exclude > the is_url cases). I don't know how coherent the results of the GetVisualRun() > call on line 357 will be with RenderText's algorithm otherwise so I can't > speculate much. I'll write a patch to force base LTR/RTL with a flag (at least for RenderTextWin), which should be done with Uniscribe settings instead of wrapping the text with control characters like ui/gfx/canvas_skia.cc. http://codereview.chromium.org/10692101/diff/14001/chrome/browser/ui/views/om... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): http://codereview.chromium.org/10692101/diff/14001/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:337: // Split the text into visual runs. We do this first so that we don't need to We should try to accomplish this styling and eliding/fading of text via RenderText StyleRange and CanvasSkia instead of duplicating functionality here.
http://codereview.chromium.org/10692101/diff/14001/chrome/browser/ui/views/om... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): http://codereview.chromium.org/10692101/diff/14001/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:337: // Split the text into visual runs. We do this first so that we don't need to On 2012/07/23 15:48:12, msw wrote: > We should try to accomplish this styling and eliding/fading of text via > RenderText StyleRange and CanvasSkia instead of duplicating functionality here. I agree that it would be nice, but currently there's some support missing in RenderText to do this. This code basically splits the string into runs and then elides individual runs as necessary according to omnibox suggestion-specific logic. This isn't really possible currently with RenderText because it does not expose its runs to the user of the API. Even if it did, it would also have to be smarter about re-calculating partial layout - as we wouldn't want an eliding of a single run to cause the other runs to invalidated. So until we add support to RenderText for those, we can't really convert to use a single instance of RenderText for this, but I agree that we should strive to add such support and eventually do that.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/asvitkine@chromium.org/10692101/14001
LGTM with nits. https://chromiumcodereview.appspot.com/10692101/diff/14001/chrome/browser/ui/... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://chromiumcodereview.appspot.com/10692101/diff/14001/chrome/browser/ui/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:337: // Split the text into visual runs. We do this first so that we don't need to On 2012/07/23 15:59:27, Alexei Svitkine wrote: > On 2012/07/23 15:48:12, msw wrote: > > We should try to accomplish this styling and eliding/fading of text via > > RenderText StyleRange and CanvasSkia instead of duplicating functionality > here. > > I agree that it would be nice, but currently there's some support missing in > RenderText to do this. > > This code basically splits the string into runs and then elides individual runs > as necessary according to omnibox suggestion-specific logic. > > This isn't really possible currently with RenderText because it does not expose > its runs to the user of the API. Even if it did, it would also have to be > smarter about re-calculating partial layout - as we wouldn't want an eliding of > a single run to cause the other runs to invalidated. > > So until we add support to RenderText for those, we can't really convert to use > a single instance of RenderText for this, but I agree that we should strive to > add such support and eventually do that. ok, sgtm. https://chromiumcodereview.appspot.com/10692101/diff/14001/chrome/browser/ui/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:454: const int text_height = GetTextHeight(); nit: remove unused local variable. https://chromiumcodereview.appspot.com/10692101/diff/14001/chrome/browser/ui/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:533: (elided_text.length() == 1) && optional nit: fit on line above. https://chromiumcodereview.appspot.com/10692101/diff/14001/chrome/browser/ui/... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:535: (prior_classification->font == &normal_font_))) { nit: re-indent by one space.
Try job failure for 10692101-14001 (retry) on linux_chromeos for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
http://codereview.chromium.org/10692101/diff/14001/chrome/browser/ui/views/om... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): http://codereview.chromium.org/10692101/diff/14001/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:454: const int text_height = GetTextHeight(); On 2012/07/23 18:56:08, msw wrote: > nit: remove unused local variable. Done. http://codereview.chromium.org/10692101/diff/14001/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:533: (elided_text.length() == 1) && On 2012/07/23 18:56:08, msw wrote: > optional nit: fit on line above. Done. http://codereview.chromium.org/10692101/diff/14001/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:535: (prior_classification->font == &normal_font_))) { On 2012/07/23 18:56:08, msw wrote: > nit: re-indent by one space. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/asvitkine@chromium.org/10692101/19002
Try job failure for 10692101-19002 (retry) on mac_rel for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/asvitkine@chromium.org/10692101/19002
Change committed as 147947 |
