Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
326123003:
RenderTextHarfBuzz: Decide run direction by BiDi embedding level (Closed)
|
Created:
6 years, 6 months ago by ckocagil Modified:
6 years, 6 months ago Reviewers:
msw CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRenderTextHarfBuzz: Decide run direction by BiDi embedding level
BUG=382178
NOTRY=true
R=msw
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276379
Patch Set 1 #
Total comments: 7
Patch Set 2 : comments addressed #
Total comments: 2
Patch Set 3 : gcc pls #
Messages
Total messages: 17 (0 generated)
Also does minor refactoring by replacing the fake_runs case with an early return.
https://codereview.chromium.org/326123003/diff/1/ui/gfx/render_text_harfbuzz.cc File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/326123003/diff/1/ui/gfx/render_text_harfbuzz.... ui/gfx/render_text_harfbuzz.cc:868: // to crash/misbehave since they expect non-zero text metrics from a non-empty nit: remove "crash/" to shorten the comment by a line. https://codereview.chromium.org/326123003/diff/1/ui/gfx/render_text_harfbuzz.... ui/gfx/render_text_harfbuzz.cc:900: run->is_rtl = (run->level % 2) == 1; Is this also true in RTL UI (--lang=he)?
Also, please add a unit test :)
https://codereview.chromium.org/326123003/diff/1/ui/gfx/render_text_harfbuzz.cc File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/326123003/diff/1/ui/gfx/render_text_harfbuzz.... ui/gfx/render_text_harfbuzz.cc:868: // to crash/misbehave since they expect non-zero text metrics from a non-empty On 2014/06/10 17:42:52, msw wrote: > nit: remove "crash/" to shorten the comment by a line. Done. https://codereview.chromium.org/326123003/diff/1/ui/gfx/render_text_harfbuzz.... ui/gfx/render_text_harfbuzz.cc:900: run->is_rtl = (run->level % 2) == 1; On 2014/06/10 17:42:51, msw wrote: > Is this also true in RTL UI (--lang=he)? AFAICT, yes.
https://codereview.chromium.org/326123003/diff/1/ui/gfx/render_text_harfbuzz.cc File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/326123003/diff/1/ui/gfx/render_text_harfbuzz.... ui/gfx/render_text_harfbuzz.cc:900: run->is_rtl = (run->level % 2) == 1; On 2014/06/10 19:12:52, ckocagil wrote: > On 2014/06/10 17:42:51, msw wrote: > > Is this also true in RTL UI (--lang=he)? > > AFAICT, yes. I wonder if that's right. Do we need to set a base direction, like RenderTextWin::ItemizeLogicalText()? // Set Uniscribe's base text direction. script_state_.uBidiLevel = (GetTextDirection() == base::i18n::RIGHT_TO_LEFT) ? 1 : 0; https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/render_text... https://codereview.chromium.org/326123003/diff/20001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/326123003/diff/20001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1980: TEST_F(RenderTextTest, HarfBuzz_RunDirection) { Does this fail without the functional changes of the CL? (I hope the test would fail if the defect regressed)
https://codereview.chromium.org/326123003/diff/1/ui/gfx/render_text_harfbuzz.cc File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/326123003/diff/1/ui/gfx/render_text_harfbuzz.... ui/gfx/render_text_harfbuzz.cc:900: run->is_rtl = (run->level % 2) == 1; I think base directions are only relevant *before* establishing the run levels. Once the levels are established, the directions are already decided. The levels generated by i18n::BiDiLineIterator take into account the GetTextDirection() value we pass it. I tested this by making GetTextDirection() return RTL and typing a bunch of weak characters (periods, commas) in an empty textfield. It was correctly displayed as RTL. So yeah, this seems to work right. https://codereview.chromium.org/326123003/diff/20001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/326123003/diff/20001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1980: TEST_F(RenderTextTest, HarfBuzz_RunDirection) { On 2014/06/10 20:47:56, msw wrote: > Does this fail without the functional changes of the CL? > (I hope the test would fail if the defect regressed) Yes, I tried this by locally reverting the functional part.
lgtm with an important question for followup. https://codereview.chromium.org/326123003/diff/1/ui/gfx/render_text_harfbuzz.cc File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/326123003/diff/1/ui/gfx/render_text_harfbuzz.... ui/gfx/render_text_harfbuzz.cc:900: run->is_rtl = (run->level % 2) == 1; On 2014/06/11 07:18:08, ckocagil wrote: > I think base directions are only relevant *before* establishing the run levels. > Once the levels are established, the directions are already decided. The levels > generated by i18n::BiDiLineIterator take into account the GetTextDirection() > value we pass it. > > I tested this by making GetTextDirection() return RTL and typing a bunch of weak > characters (periods, commas) in an empty textfield. It was correctly displayed > as RTL. > > So yeah, this seems to work right. You're right about the function ordering, but my question is still relevant: in --lang=he RTL UI, does weak text still yield RTL runs? If not, we need to do something as I suggest (setting a base direction before itemizing the runs).
On 2014/06/11 07:54:19, msw wrote: > lgtm with an important question for followup. > > https://codereview.chromium.org/326123003/diff/1/ui/gfx/render_text_harfbuzz.cc > File ui/gfx/render_text_harfbuzz.cc (right): > > https://codereview.chromium.org/326123003/diff/1/ui/gfx/render_text_harfbuzz.... > ui/gfx/render_text_harfbuzz.cc:900: run->is_rtl = (run->level % 2) == 1; > On 2014/06/11 07:18:08, ckocagil wrote: > > I think base directions are only relevant *before* establishing the run > levels. > > Once the levels are established, the directions are already decided. The > levels > > generated by i18n::BiDiLineIterator take into account the GetTextDirection() > > value we pass it. > > > > I tested this by making GetTextDirection() return RTL and typing a bunch of > weak > > characters (periods, commas) in an empty textfield. It was correctly displayed > > as RTL. > > > > So yeah, this seems to work right. > > You're right about the function ordering, but my question is still relevant: in > --lang=he RTL UI, does weak text still yield RTL runs? If not, we need to do > something as I suggest (setting a base direction before itemizing the runs). Ah, I answered this myself, looking at the RenderText::GetTextDirection impl, and as per your experimentation with that, this LGTM.
The CQ bit was checked by ckocagil@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ckocagil@chromium.org/326123003/20001
The CQ bit was checked by ckocagil@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ckocagil@chromium.org/326123003/40001
The CQ bit was unchecked by ckocagil@chromium.org
The CQ bit was checked by ckocagil@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ckocagil@chromium.org/326123003/40001
Message was sent while issue was closed.
Change committed as 276379
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/324413002/ by ckocagil@chromium.org. The reason for reverting is: Memory leak: http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te.... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
