Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1375)

Issue 10693061: Fix RenderTextWin base dir and adjacent char code; remove test exceptions; etc. (Closed)

Created:
8 years, 5 months ago by msw
Modified:
8 years, 5 months ago
CC:
chromium-reviews, tfarina, James Su, penghuang+watch_chromium.org, yusukes+watch_chromium.org, oshima
Visibility:
Public.

Description

Fix RenderTextWin base direction; remove test exceptions; etc. Use the first strong character's direction to set SCRIPT_STATE.uBidiLevel. RenderTextWin::GetTextDirection returns this calculated base direction. Fix RenderTextWin::AdjacentCharSelectionModel: Handle all out-of-bounds run indicies as invalid runs. Use the visual ordering of runs (implicit on Linux) to determine edges. Add new RenderTextTest.GetTextDirection unit test. Add SetRTL test utility function which works with GTK. Fix RenderTextTest.DisplayRectShowsCursorRTL on linux with SetRTL. Remove relevant Windows-specific unit test exceptions. TODO(msw): Cleanup canvas_skia.cc in a followup CL. TODO(msw): Use UI dir by default in GetFirstStrongCharacterDirection. BUG=134746, 134009 TEST=Updated unit tests; no RTL/LTR regressions. TBR=sky@chromium.org Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=145476

Patch Set 1 : Sync and rebase. #

Total comments: 4

Patch Set 2 : Address comments; add unit test. #

Total comments: 4

Patch Set 3 : Use the application text direction by default; update unit test; add TODO. #

Patch Set 4 : Revert base/i18n/rtl.cc; use LTR by default for now; update test. #

Patch Set 5 : Fix RenderTextTest.DisplayRectShowsCursorRTL on linux with SetRTL(). #

Patch Set 6 : Revert gypi change to keep Mac working. #

Total comments: 6

Patch Set 7 : Address comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+74 lines, -113 lines) Patch
M ui/gfx/canvas_skia.cc View 1 2 3 4 5 6 1 chunk +3 lines, -2 lines 0 comments Download
M ui/gfx/render_text_unittest.cc View 1 2 3 4 5 6 12 chunks +57 lines, -70 lines 0 comments Download
M ui/gfx/render_text_win.cc View 1 2 3 4 chunks +14 lines, -7 lines 0 comments Download
M ui/views/controls/textfield/native_textfield_views_unittest.cc View 4 chunks +0 lines, -30 lines 0 comments Download
M ui/views/controls/textfield/textfield_views_model_unittest.cc View 1 chunk +0 lines, -4 lines 0 comments Download

Messages

Total messages: 14 (0 generated)
msw
Hey Xiaomei and Alexei, please take a look; thanks!
8 years, 5 months ago (2012-07-01 19:06:23 UTC) #1
Alexei Svitkine (slow)
Have you checked that text drawn via canvas_skia.cc still looks the same? i.e. checking RTL/BiDi ...
8 years, 5 months ago (2012-07-02 16:21:11 UTC) #2
Alexei Svitkine (slow)
Also, consider adding a unit test for GetTextDirection().
8 years, 5 months ago (2012-07-02 16:22:42 UTC) #3
msw
Done, please take another look; thanks! https://chromiumcodereview.appspot.com/10693061/diff/2001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://chromiumcodereview.appspot.com/10693061/diff/2001/ui/gfx/render_text_win.cc#newcode306 ui/gfx/render_text_win.cc:306: return base::i18n::GetFirstStrongCharacterDirection(text()); On ...
8 years, 5 months ago (2012-07-02 19:54:49 UTC) #4
msw
Oh, and I tested tab titles, they're the same as ToT/Canary. But FYI, I think ...
8 years, 5 months ago (2012-07-03 00:07:53 UTC) #5
Alexei Svitkine (slow)
I think this will break the gfx::Canvas flag FORCE_LTR_DIRECTIONALITY as currently implemented in canvas_skia.cc The ...
8 years, 5 months ago (2012-07-03 18:34:12 UTC) #6
Alexei Svitkine (slow)
http://codereview.chromium.org/10693061/diff/8002/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): http://codereview.chromium.org/10693061/diff/8002/ui/gfx/render_text_win.cc#newcode599 ui/gfx/render_text_win.cc:599: script_state_.uBidiLevel = 0; Can you add a case to ...
8 years, 5 months ago (2012-07-03 18:37:57 UTC) #7
msw
I made GetFirstStrongCharacterDirection return the application text direction (depends on locale) by default (if blank ...
8 years, 5 months ago (2012-07-03 22:34:04 UTC) #8
msw
Okay, I reverted the GetFirstStrongCharacterDirection change and updated the tests (wasn't working on Linux/CrOS...). I ...
8 years, 5 months ago (2012-07-04 01:22:50 UTC) #9
Alexei Svitkine (slow)
Nice! https://chromiumcodereview.appspot.com/10693061/diff/7007/ui/gfx/render_text_unittest.cc File ui/gfx/render_text_unittest.cc (right): https://chromiumcodereview.appspot.com/10693061/diff/7007/ui/gfx/render_text_unittest.cc#newcode372 ui/gfx/render_text_unittest.cc:372: void SetRTL(bool rtl) { Nit: Move it to ...
8 years, 5 months ago (2012-07-04 15:05:33 UTC) #10
msw
Done, please take another look; thanks! https://chromiumcodereview.appspot.com/10693061/diff/7007/ui/gfx/render_text_unittest.cc File ui/gfx/render_text_unittest.cc (right): https://chromiumcodereview.appspot.com/10693061/diff/7007/ui/gfx/render_text_unittest.cc#newcode372 ui/gfx/render_text_unittest.cc:372: void SetRTL(bool rtl) ...
8 years, 5 months ago (2012-07-04 15:58:08 UTC) #11
Alexei Svitkine (slow)
LGTM, thanks!
8 years, 5 months ago (2012-07-04 16:52:19 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/msw@chromium.org/10693061/3006
8 years, 5 months ago (2012-07-04 16:56:25 UTC) #13
commit-bot: I haz the power
8 years, 5 months ago (2012-07-04 18:29:03 UTC) #14
Change committed as 145476

Powered by Google App Engine
This is Rietveld 408576698