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

Issue 10807082: Add RenderText DirectionalityMode enum and support; etc. (Closed)

Created:
8 years, 5 months ago by msw
Modified:
8 years, 4 months ago
CC:
chromium-reviews
Visibility:
Public.

Description

Add RenderText DirectionalityMode enum and support; etc. Add gfx::DirectionalityMode enum. Add RenderText::SetDirectionalityMode and member. Revise/consolidate RenderText::GetTextDirection. Expand on existing unit tests; minor cleanup. Consume GetTextDirection in layout initialization: -Windows: RenderTextWin::ItemizeLogicalText() -Linux: SetupPangoLayoutWithoutFont() -Mac: Add TODO in RenderTextMac::EnsureLayout() BUG=134746 TEST=Existing/updated unit tests, no behavioral changes! Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149503

Patch Set 1 : Set RenderText TextDirection via DirectionalityMode. #

Patch Set 2 : Cleanup Canvas Skia, and Views Label directionality code. #

Patch Set 3 : Deprecate unused views::Label::SetURL(); cleanup, etc. #

Patch Set 4 : Update RenderTextMac, add TODO for forced directionality. #

Patch Set 5 : Restore Label::SetURL; derive the direction from display text. #

Total comments: 4

Patch Set 6 : Add ui/gfx/text_constants.h, revise DirectionalityMode enum names; sync and merge. #

Patch Set 7 : Update and expand on unit tests. #

Total comments: 18

Patch Set 8 : Address comments; revert Label and MessageBoxView changes. #

Total comments: 8

Patch Set 9 : Address comments; move Pango direction code; add test case. #

Total comments: 2

Patch Set 10 : Revert canvas_skia.cc behavioral changes. #

Patch Set 11 : Remove errant blank line. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+182 lines, -94 lines) Patch
M ui/gfx/pango_util.h View 1 2 3 4 5 6 7 8 1 chunk +3 lines, -4 lines 0 comments Download
M ui/gfx/pango_util.cc View 1 2 3 4 5 6 7 8 1 chunk +4 lines, -2 lines 0 comments Download
M ui/gfx/render_text.h View 1 2 3 4 5 6 7 8 4 chunks +11 lines, -19 lines 0 comments Download
M ui/gfx/render_text.cc View 1 2 3 4 5 4 chunks +43 lines, -0 lines 0 comments Download
M ui/gfx/render_text_linux.h View 1 chunk +0 lines, -1 line 0 comments Download
M ui/gfx/render_text_linux.cc View 1 2 3 4 5 6 7 8 2 chunks +7 lines, -16 lines 0 comments Download
M ui/gfx/render_text_mac.h View 1 2 3 1 chunk +0 lines, -1 line 0 comments Download
M ui/gfx/render_text_mac.cc View 1 2 3 2 chunks +3 lines, -4 lines 0 comments Download
M ui/gfx/render_text_unittest.cc View 1 2 3 4 5 6 7 8 6 chunks +60 lines, -36 lines 0 comments Download
M ui/gfx/render_text_win.h View 1 chunk +0 lines, -1 line 0 comments Download
M ui/gfx/render_text_win.cc View 1 2 3 4 5 6 7 2 chunks +2 lines, -10 lines 0 comments Download
A ui/gfx/text_constants.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +45 lines, -0 lines 0 comments Download
M ui/ui.gyp View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M ui/views/controls/textfield/native_textfield_views.cc View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M ui/views/controls/textfield/textfield_views_model.h View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M ui/views/controls/textfield/textfield_views_model.cc View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 20 (0 generated)
msw
Hey Alexei, please take a look; thanks!
8 years, 5 months ago (2012-07-26 20:43:21 UTC) #1
Alexei Svitkine (slow)
Can you delete Label::SetURL() in a separate CL? This one is already pretty crowded.
8 years, 5 months ago (2012-07-26 23:23:10 UTC) #2
msw
Done, I'll make a separate CL for that. I also fixed tests by using GetDisplayText ...
8 years, 5 months ago (2012-07-26 23:45:38 UTC) #3
Alexei Svitkine (slow)
I'm travelling today, so I don't have time to do a thorough review yet, but ...
8 years, 4 months ago (2012-07-27 11:40:35 UTC) #4
msw
Done, and I expanded on unit tests, please take a look as you have time, ...
8 years, 4 months ago (2012-07-28 01:17:10 UTC) #5
msw
Xiaomei, please take a look too; thanks!
8 years, 4 months ago (2012-07-28 01:18:45 UTC) #6
xji
Very nice! http://codereview.chromium.org/10807082/diff/8016/ui/gfx/canvas_skia.cc File ui/gfx/canvas_skia.cc (right): http://codereview.chromium.org/10807082/diff/8016/ui/gfx/canvas_skia.cc#newcode35 ui/gfx/canvas_skia.cc:35: render_text->SetDirectionalityMode(gfx::DIRECTIONALITY_FORCE_RTL); this is DIRECTIONALITY_FROM_UI. we probably should ...
8 years, 4 months ago (2012-07-30 18:32:16 UTC) #7
xji
http://codereview.chromium.org/10807082/diff/8016/ui/views/controls/label.cc File ui/views/controls/label.cc (right): http://codereview.chromium.org/10807082/diff/8016/ui/views/controls/label.cc#newcode438 ui/views/controls/label.cc:438: const base::i18n::TextDirection direction = if we introduce gfx::Canvas::DIRECTIONALITY_FROM_TEXT, then, ...
8 years, 4 months ago (2012-07-30 19:00:04 UTC) #8
Alexei Svitkine (slow)
Sorry, I still need to find time to review this in full. But here's some ...
8 years, 4 months ago (2012-07-30 22:29:16 UTC) #9
msw
I addressed the comments and reverted the Label and MessageBoxView changes (I'll follow up in ...
8 years, 4 months ago (2012-07-31 03:03:06 UTC) #10
xji
http://codereview.chromium.org/10807082/diff/8016/ui/gfx/canvas_skia.cc File ui/gfx/canvas_skia.cc (right): http://codereview.chromium.org/10807082/diff/8016/ui/gfx/canvas_skia.cc#newcode35 ui/gfx/canvas_skia.cc:35: render_text->SetDirectionalityMode(gfx::DIRECTIONALITY_FORCE_RTL); On 2012/07/31 03:03:06, msw wrote: > On 2012/07/30 ...
8 years, 4 months ago (2012-07-31 23:03:25 UTC) #11
Alexei Svitkine (slow)
Here some more comments. Please be sure to test RTL and BiDi UI text with ...
8 years, 4 months ago (2012-07-31 23:33:29 UTC) #12
msw
Comments addressed, pics attached at http://crbug.com/134746 Please take another look; thanks! https://chromiumcodereview.appspot.com/10807082/diff/8016/ui/gfx/canvas_skia.cc File ui/gfx/canvas_skia.cc (right): ...
8 years, 4 months ago (2012-08-01 17:30:10 UTC) #13
msw
I removed the behavioral changes to simplify this CL. Please take a look.
8 years, 4 months ago (2012-08-01 18:28:56 UTC) #14
Alexei Svitkine (slow)
LGTM with a possible nit. https://chromiumcodereview.appspot.com/10807082/diff/14006/ui/gfx/text_constants.h File ui/gfx/text_constants.h (right): https://chromiumcodereview.appspot.com/10807082/diff/14006/ui/gfx/text_constants.h#newcode1 ui/gfx/text_constants.h:1: Nit: Unnecessary blank line?
8 years, 4 months ago (2012-08-01 18:29:13 UTC) #15
msw
Done. +Ben for OWNERS review; please take a look, thanks! https://chromiumcodereview.appspot.com/10807082/diff/14006/ui/gfx/text_constants.h File ui/gfx/text_constants.h (right): https://chromiumcodereview.appspot.com/10807082/diff/14006/ui/gfx/text_constants.h#newcode1 ...
8 years, 4 months ago (2012-08-01 18:48:31 UTC) #16
Ben Goodger (Google)
owner lgtm
8 years, 4 months ago (2012-08-01 19:08:34 UTC) #17
xji
lgtm
8 years, 4 months ago (2012-08-01 19:54:57 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/msw@chromium.org/10807082/22009
8 years, 4 months ago (2012-08-01 20:26:18 UTC) #19
commit-bot: I haz the power
8 years, 4 months ago (2012-08-01 21:57:32 UTC) #20
Change committed as 149503

Powered by Google App Engine
This is Rietveld 408576698