Implement right-to-left text rendering in Pepper.

Implement right-to-left text rendering in Pepper.

TEST=included
BUG=http://crbug.com/134394

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=144623

[viettrungluu, 2012-06-26 17:35:07]

Hmmm, can't you at least write an automated test for measurement?

[viettrungluu, 2012-06-26 17:59:40]

https://chromiumcodereview.appspot.com/10658037/diff/2001/ppapi/shared_impl/p...
File ppapi/shared_impl/private/ppb_browser_font_trusted_shared.cc (right):

https://chromiumcodereview.appspot.com/10658037/diff/2001/ppapi/shared_impl/p...
ppapi/shared_impl/private/ppb_browser_font_trusted_shared.cc:61:
TextRunCollection(const PP_BrowserFont_Trusted_TextRun& run)
explicit?

https://chromiumcodereview.appspot.com/10658037/diff/2001/ppapi/shared_impl/p...
ppapi/shared_impl/private/ppb_browser_font_trusted_shared.cc:76:
ubidi_setPara(bidi_, text_.c_str(), text_.size(), run.rtl, NULL, &uerror);
text_.data() instead .c_str()?

https://chromiumcodereview.appspot.com/10658037/diff/2001/ppapi/shared_impl/p...
ppapi/shared_impl/private/ppb_browser_font_trusted_shared.cc:94: bool run_rtl =
!!ubidi_getVisualRun(bidi_, index, run_start, run_len);
If ubidi_setPara() failed, do you really want to do this?
(Also, I suppose you're relying on int = int32_t. That's okay, but I'm not
entirely sure if we want to do that.)

More generally, should you always DCHECK(index < num_runs_) (outside this |if|)?

https://chromiumcodereview.appspot.com/10658037/diff/2001/ppapi/shared_impl/p...
ppapi/shared_impl/private/ppb_browser_font_trusted_shared.cc:354: int run_begin
= 0, run_len = 0;
Does our style guide really allow this?

https://chromiumcodereview.appspot.com/10658037/diff/2001/ppapi/shared_impl/p...
ppapi/shared_impl/private/ppb_browser_font_trusted_shared.cc:356: int cur_width
= font_->calculateWidth(run);
I think "run_width" would be a better name for this variable.

https://chromiumcodereview.appspot.com/10658037/diff/2001/ppapi/shared_impl/p...
ppapi/shared_impl/private/ppb_browser_font_trusted_shared.cc:374: int run_begin
= 0, run_len = 0;
...

https://chromiumcodereview.appspot.com/10658037/diff/2001/ppapi/shared_impl/p...
ppapi/shared_impl/private/ppb_browser_font_trusted_shared.cc:417: int run_begin
= 0, run_len = 0;
...

[brettw, 2012-06-27 17:42:46]

new patch up

https://chromiumcodereview.appspot.com/10658037/diff/2001/ppapi/shared_impl/p...
File ppapi/shared_impl/private/ppb_browser_font_trusted_shared.cc (right):

https://chromiumcodereview.appspot.com/10658037/diff/2001/ppapi/shared_impl/p...
ppapi/shared_impl/private/ppb_browser_font_trusted_shared.cc:94: bool run_rtl =
!!ubidi_getVisualRun(bidi_, index, run_start, run_len);
If setPara failed, num_runs will be 0 and we'll never get here.

[viettrungluu, 2012-06-27 20:33:58]

https://chromiumcodereview.appspot.com/10658037/diff/6001/ppapi/shared_impl/p...
File ppapi/shared_impl/private/ppb_browser_font_trusted_shared.cc (right):

https://chromiumcodereview.appspot.com/10658037/diff/6001/ppapi/shared_impl/p...
ppapi/shared_impl/private/ppb_browser_font_trusted_shared.cc:103: *run_len =
static_cast<int>(text_.size());
int32_t

https://chromiumcodereview.appspot.com/10658037/diff/6001/ppapi/tests/test_br...
File ppapi/tests/test_browser_font.cc (right):

https://chromiumcodereview.appspot.com/10658037/diff/6001/ppapi/tests/test_br...
ppapi/tests/test_browser_font.cc:60: // Mixed string, two chars of LTR, two of
RTL, than two of LTR.
s/than/then/

https://chromiumcodereview.appspot.com/10658037/diff/6001/ppapi/tests/test_br...
ppapi/tests/test_browser_font.cc:66: // Note that since this is UTF8, the two
RTL chars are two bytes each.
Nit: "UTF8" or "UTF-8"?

https://chromiumcodereview.appspot.com/10658037/diff/6001/ppapi/tests/test_br...
ppapi/tests/test_browser_font.cc:78: ASSERT_TRUE(len[2] > len[1]);
Instead of this convoluted logic, I don't see why you don't just do:

I don't see why you don't just continue:

// Visually, left-to-write, the characters occur in the order:
// 0, 1, 3, 2, 4, 5.

ASSERT_TRUE(len[0] >= 0);
ASSERT_TRUE(len[1] > len[0]);
ASSERT_TRUE(len[3] > len[1]);
ASSERT_TRUE(len[2] > len[3]);
ASSERT_TRUE(len[4] > len[2]);
ASSERT_TRUE(len[5] > len[4]);

(You also forgot to look at len[5].)

[viettrungluu, 2012-06-27 20:34:28]

https://chromiumcodereview.appspot.com/10658037/diff/6001/ppapi/tests/test_br...
File ppapi/tests/test_browser_font.cc (right):

https://chromiumcodereview.appspot.com/10658037/diff/6001/ppapi/tests/test_br...
ppapi/tests/test_browser_font.cc:23: RUN_TEST(Draw, filter);
You forgot CharPosRTL.

[brettw, 2012-06-27 21:48:38]

New snap up.

[viettrungluu, 2012-06-27 21:50:01]

LGTM. You can probably update the TEST= line in the CL description.

[commit-bot: I haz the power, 2012-06-27 21:58:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brettw@chromium.org/10658037/11001

[commit-bot: I haz the power, 2012-06-28 00:31:57]

Change committed as 144623