Make fx_bidi sane.

Make fx_bidi sane.

Replace array of heterogenous ints with array of struct.
Create a class for traversing a string.
Flip array when R2L and process with forward iterator always.

R=thestig@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/5a5f1f1c905f2c352d2fbde143456124dd2c0fbf

[Tom Sepez, 2016-02-10 23:31:19]

Description was changed from

==========
Make fx_bidi sane.
==========

to

==========
Make fx_bidi sane.

Replace array of heterogenous ints with array of struct.
Create a class for traversing a string.
Flip array when R2L and process with forward iterator always.
==========

[Tom Sepez, 2016-02-11 18:02:15]

tsepez@chromium.org changed reviewers:
+ thestig@chromium.org

[Tom Sepez, 2016-02-11 18:02:15]

Lei, pls review.  A lot of the logic seems to be for pointless combination of
neutral segments, rather than just keeping track of the current direction.

[Tom Sepez, 2016-02-11 18:25:58]

https://codereview.chromium.org/1682983002/diff/160001/core/src/fxcrt/fx_bidi...
File core/src/fxcrt/fx_bidi.cpp (right):

https://codereview.chromium.org/1682983002/diff/160001/core/src/fxcrt/fx_bidi...
core/src/fxcrt/fx_bidi.cpp:77: std::reverse(m_Order.begin(), m_Order.end());
Note: this is the key idea. Rather than forcing our callers to figure out front
to back vs. back to front, give them back in the correct order for the
prevailing direction.

[Lei Zhang, 2016-02-12 00:46:43]

https://codereview.chromium.org/1682983002/diff/160001/core/include/fxcrt/fx_...
File core/include/fxcrt/fx_bidi.h (right):

https://codereview.chromium.org/1682983002/diff/160001/core/include/fxcrt/fx_...
core/include/fxcrt/fx_bidi.h:11: #include <memory>
memory before vector.

https://codereview.chromium.org/1682983002/diff/160001/core/include/fxcrt/fx_...
core/include/fxcrt/fx_bidi.h:27: : m_CurrentSegment({0, 0, NEUTRAL}),
m_LastSegment({0, 0, NEUTRAL}) {}
Keep the ctor impl in the .cpp file?

https://codereview.chromium.org/1682983002/diff/160001/core/include/fxcrt/fx_...
core/include/fxcrt/fx_bidi.h:53: CFX_BidiString(const CFX_WideString& str);
explicit

https://codereview.chromium.org/1682983002/diff/160001/core/include/fxcrt/fx_...
core/include/fxcrt/fx_bidi.h:56: void
SetOverallDirection(CFX_BidiChar::Direction eDirection);
If you make this private and only expose wrappers with SetOverallDirectionLeft()
/ SetOverallDirectionRight(), callers can't get it wrong.

https://codereview.chromium.org/1682983002/diff/160001/core/src/fpdftext/text...
File core/src/fpdftext/text_int.h (right):

https://codereview.chromium.org/1682983002/diff/160001/core/src/fpdftext/text...
core/src/fpdftext/text_int.h:124: void AddCharInfoByLRDirection(FX_WCHAR wChar,
PAGECHAR_INFO info);
Can you pass |info| by const ref?

https://codereview.chromium.org/1682983002/diff/160001/core/src/fxcrt/fx_bidi...
File core/src/fxcrt/fx_bidi.cpp (right):

https://codereview.chromium.org/1682983002/diff/160001/core/src/fxcrt/fx_bidi...
core/src/fxcrt/fx_bidi.cpp:65: if (m_Order.back().direction ==
CFX_BidiChar::RIGHT)
Maybe just push into |m_Order| and calculate |nR2L| / |nL2R| with std::count() ?

https://codereview.chromium.org/1682983002/diff/160001/core/src/fxcrt/fx_bidi...
core/src/fxcrt/fx_bidi.cpp:77: std::reverse(m_Order.begin(), m_Order.end());
On 2016/02/11 18:25:58, Tom Sepez wrote:
> Note: this is the key idea. Rather than forcing our callers to figure out
front
> to back vs. back to front, give them back in the correct order for the
> prevailing direction.

SGTM

https://codereview.chromium.org/1682983002/diff/160001/core/src/fxcrt/fx_bidi...
File core/src/fxcrt/fx_bidi_unittest.cpp (right):

https://codereview.chromium.org/1682983002/diff/160001/core/src/fxcrt/fx_bidi...
core/src/fxcrt/fx_bidi_unittest.cpp:134: EXPECT_FALSE(it == bidi.end());
ASSERT_FALSE, ditto below.

[Tom Sepez, 2016-02-12 17:54:33]

https://codereview.chromium.org/1682983002/diff/160001/core/include/fxcrt/fx_...
File core/include/fxcrt/fx_bidi.h (right):

https://codereview.chromium.org/1682983002/diff/160001/core/include/fxcrt/fx_...
core/include/fxcrt/fx_bidi.h:11: #include <memory>
On 2016/02/12 00:46:42, Lei Zhang wrote:
> memory before vector.

Done.

https://codereview.chromium.org/1682983002/diff/160001/core/include/fxcrt/fx_...
core/include/fxcrt/fx_bidi.h:27: : m_CurrentSegment({0, 0, NEUTRAL}),
m_LastSegment({0, 0, NEUTRAL}) {}
On 2016/02/12 00:46:42, Lei Zhang wrote:
> Keep the ctor impl in the .cpp file?

Done.

https://codereview.chromium.org/1682983002/diff/160001/core/include/fxcrt/fx_...
core/include/fxcrt/fx_bidi.h:53: CFX_BidiString(const CFX_WideString& str);
On 2016/02/12 00:46:42, Lei Zhang wrote:
> explicit

Done.

https://codereview.chromium.org/1682983002/diff/160001/core/include/fxcrt/fx_...
core/include/fxcrt/fx_bidi.h:56: void
SetOverallDirection(CFX_BidiChar::Direction eDirection);
On 2016/02/12 00:46:42, Lei Zhang wrote:
> If you make this private and only expose wrappers with
SetOverallDirectionLeft()
> / SetOverallDirectionRight(), callers can't get it wrong.

Actually, we only call this with an argument of RIGHT, so add that one only.

https://codereview.chromium.org/1682983002/diff/160001/core/src/fpdftext/text...
File core/src/fpdftext/text_int.h (right):

https://codereview.chromium.org/1682983002/diff/160001/core/src/fpdftext/text...
core/src/fpdftext/text_int.h:124: void AddCharInfoByLRDirection(FX_WCHAR wChar,
PAGECHAR_INFO info);
On 2016/02/12 00:46:43, Lei Zhang wrote:
> Can you pass |info| by const ref?
Actually, I update it as part of the computations in this code, so I'd have to
copy it anyways there.

https://codereview.chromium.org/1682983002/diff/160001/core/src/fxcrt/fx_bidi...
File core/src/fxcrt/fx_bidi.cpp (right):

https://codereview.chromium.org/1682983002/diff/160001/core/src/fxcrt/fx_bidi...
core/src/fxcrt/fx_bidi.cpp:65: if (m_Order.back().direction ==
CFX_BidiChar::RIGHT)
On 2016/02/12 00:46:43, Lei Zhang wrote:
> Maybe just push into |m_Order| and calculate |nR2L| / |nL2R| with std::count()
?

That's really clean.

https://codereview.chromium.org/1682983002/diff/160001/core/src/fxcrt/fx_bidi...
File core/src/fxcrt/fx_bidi_unittest.cpp (right):

https://codereview.chromium.org/1682983002/diff/160001/core/src/fxcrt/fx_bidi...
core/src/fxcrt/fx_bidi_unittest.cpp:134: EXPECT_FALSE(it == bidi.end());
On 2016/02/12 00:46:43, Lei Zhang wrote:
> ASSERT_FALSE, ditto below.

Done, presumably to avoid derefin'g invalid iters, ok.

[Lei Zhang, 2016-02-12 18:10:58]

lgtm

[Tom Sepez, 2016-02-12 18:29:25]

Description was changed from

==========
Make fx_bidi sane.

Replace array of heterogenous ints with array of struct.
Create a class for traversing a string.
Flip array when R2L and process with forward iterator always.
==========

to

==========
Make fx_bidi sane.

Replace array of heterogenous ints with array of struct.
Create a class for traversing a string.
Flip array when R2L and process with forward iterator always.

R=thestig@chromium.org

Committed:
https://pdfium.googlesource.com/pdfium/+/5a5f1f1c905f2c352d2fbde143456124dd2c...
==========

[Tom Sepez, 2016-02-12 18:29:26]

Committed patchset #10 (id:180001) manually as
5a5f1f1c905f2c352d2fbde143456124dd2c0fbf (presubmit successful).