Cleanup IFX_BidiChar

Clean up IFX_BidiChar

- Replace IFX_BidiChar with just CFX_BidiChar
- Document implementation
- Change out parameters to pointers
- Remove dead code
- Add an enum for bidi directions
- Move several externs to a header
- Add unit tests

R=tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/2ae87d2e8ddff79d0e96aad3db97e090db21fb99

[Lei Zhang, 2015-06-19 20:10:21]

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

[Tom Sepez, 2015-06-19 20:30:33]

https://codereview.chromium.org/1197643002/diff/1/core/include/fxcrt/fx_arb.h
File core/include/fxcrt/fx_arb.h (right):

https://codereview.chromium.org/1197643002/diff/1/core/include/fxcrt/fx_arb.h...
core/include/fxcrt/fx_arb.h:31: // should not be called after this.
nit: "may not" or "must not".

https://codereview.chromium.org/1197643002/diff/1/core/include/fxcrt/fx_arb.h...
core/include/fxcrt/fx_arb.h:33: virtual bool EndChar() = 0;
nit: it's still a mystery -- returns what?

https://codereview.chromium.org/1197643002/diff/1/core/include/fxcrt/fx_arb.h...
core/include/fxcrt/fx_arb.h:36: // The segment's start position and character
count goes in |iStart| and
nit: s/goes in/is returned in/

https://codereview.chromium.org/1197643002/diff/1/core/src/fxcrt/fx_arabic.cpp
File core/src/fxcrt/fx_arabic.cpp (left):

https://codereview.chromium.org/1197643002/diff/1/core/src/fxcrt/fx_arabic.cp...
core/src/fxcrt/fx_arabic.cpp:74: m_iCurStart = m_iCurCount;
Did we lose this assignment?

https://codereview.chromium.org/1197643002/diff/1/core/src/fxcrt/fx_arabic.cpp
File core/src/fxcrt/fx_arabic.cpp (right):

https://codereview.chromium.org/1197643002/diff/1/core/src/fxcrt/fx_arabic.cp...
core/src/fxcrt/fx_arabic.cpp:10: extern const FX_DWORD
gs_FX_TextLayout_CodeProperties[65536];
nit: This needs to be in a .h file. Does it have to be a dword, or can we save
like 128kb or 192kb with a uint16 or uint8?

https://codereview.chromium.org/1197643002/diff/1/core/src/fxcrt/fx_arabic.h
File core/src/fxcrt/fx_arabic.h (right):

https://codereview.chromium.org/1197643002/diff/1/core/src/fxcrt/fx_arabic.h#...
core/src/fxcrt/fx_arabic.h:19: // IFX_BidiChar
nit: maybe colon after IFX_BidiChar:

[Tom Sepez, 2015-06-19 21:36:11]

https://codereview.chromium.org/1197643002/diff/1/core/src/fxcrt/fx_arabic.cpp
File core/src/fxcrt/fx_arabic.cpp (right):

https://codereview.chromium.org/1197643002/diff/1/core/src/fxcrt/fx_arabic.cp...
core/src/fxcrt/fx_arabic.cpp:10: extern const FX_DWORD
gs_FX_TextLayout_CodeProperties[65536];
On 2015/06/19 20:30:32, Tom Sepez wrote:
> nit: This needs to be in a .h file. Does it have to be a dword, or can we save
> like 128kb or 192kb with a uint16 or uint8?

Answer is that its a DWORD.  I was counting the bits as defined in fx_ucd.h and
came up with 16, but there's some stuff in the high bytes that's just
manipulated with magic numbers in the .cpp

Still, needs to be in a header though.

[Tom Sepez, 2015-06-20 01:10:26]

At the risk of "piling on", it would be cool if this were renamed to something
like fx_bidi.h and had a unit test.

[Tom Sepez, 2015-07-23 19:54:21]

On 2015/06/20 01:10:26, Tom Sepez wrote:
> At the risk of "piling on", it would be cool if this were renamed to something
> like fx_bidi.h and had a unit test.

Looking back at this, it strikes me that IFX_BidiChar is pointless, since its
only implemented in one place in the same library (the fxcrt library). we should
probably have a task to get rid of IFX* where the interface isn't buying us 
anything but complexity.

[Lei Zhang, 2015-08-17 17:41:35]

Finally got back to finishing this CL.

https://codereview.chromium.org/1197643002/diff/1/core/include/fxcrt/fx_arb.h
File core/include/fxcrt/fx_arb.h (right):

https://codereview.chromium.org/1197643002/diff/1/core/include/fxcrt/fx_arb.h...
core/include/fxcrt/fx_arb.h:31: // should not be called after this.
On 2015/06/19 20:30:32, Tom Sepez wrote:
> nit: "may not" or "must not".

Done.

https://codereview.chromium.org/1197643002/diff/1/core/include/fxcrt/fx_arb.h...
core/include/fxcrt/fx_arb.h:33: virtual bool EndChar() = 0;
On 2015/06/19 20:30:32, Tom Sepez wrote:
> nit: it's still a mystery -- returns what?

Done.

https://codereview.chromium.org/1197643002/diff/1/core/include/fxcrt/fx_arb.h...
core/include/fxcrt/fx_arb.h:36: // The segment's start position and character
count goes in |iStart| and
On 2015/06/19 20:30:32, Tom Sepez wrote:
> nit: s/goes in/is returned in/

Done.

https://codereview.chromium.org/1197643002/diff/1/core/src/fxcrt/fx_arabic.cpp
File core/src/fxcrt/fx_arabic.cpp (left):

https://codereview.chromium.org/1197643002/diff/1/core/src/fxcrt/fx_arabic.cp...
core/src/fxcrt/fx_arabic.cpp:74: m_iCurStart = m_iCurCount;
On 2015/06/19 20:30:32, Tom Sepez wrote:
> Did we lose this assignment?

Yep, don't remember why I did that.

https://codereview.chromium.org/1197643002/diff/1/core/src/fxcrt/fx_arabic.cpp
File core/src/fxcrt/fx_arabic.cpp (right):

https://codereview.chromium.org/1197643002/diff/1/core/src/fxcrt/fx_arabic.cp...
core/src/fxcrt/fx_arabic.cpp:10: extern const FX_DWORD
gs_FX_TextLayout_CodeProperties[65536];
On 2015/06/19 21:36:10, Tom Sepez wrote:
> On 2015/06/19 20:30:32, Tom Sepez wrote:
> > nit: This needs to be in a .h file. Does it have to be a dword, or can we
save
> > like 128kb or 192kb with a uint16 or uint8?
> 
> Answer is that its a DWORD.  I was counting the bits as defined in fx_ucd.h
and
> came up with 16, but there's some stuff in the high bytes that's just
> manipulated with magic numbers in the .cpp
> 
> Still, needs to be in a header though.

Done.

[Tom Sepez, 2015-08-17 18:55:16]

lgtm

https://codereview.chromium.org/1197643002/diff/120001/core/src/fxcrt/fx_ucdd...
File core/src/fxcrt/fx_ucddata.cpp (right):

https://codereview.chromium.org/1197643002/diff/120001/core/src/fxcrt/fx_ucdd...
core/src/fxcrt/fx_ucddata.cpp:9: extern const FX_DWORD
kTextLayout_CodeProperties[1 << (8 * sizeof(FX_WORD))] = {
nit: I find this more confusing than they old way.  Also, since this is a
definition, not a declaration, no need for "extern"

https://codereview.chromium.org/1197643002/diff/120001/core/src/fxcrt/fx_ucdd...
core/src/fxcrt/fx_ucddata.cpp:10935: extern const FX_WCHAR
gs_FX_TextLayout_VerticalMirror[64] = {
nit: ditto for no "extern". Probably other places in the file, too.

[Lei Zhang, 2015-08-17 22:01:26]

https://codereview.chromium.org/1197643002/diff/120001/core/src/fxcrt/fx_ucdd...
File core/src/fxcrt/fx_ucddata.cpp (right):

https://codereview.chromium.org/1197643002/diff/120001/core/src/fxcrt/fx_ucdd...
core/src/fxcrt/fx_ucddata.cpp:9: extern const FX_DWORD
kTextLayout_CodeProperties[1 << (8 * sizeof(FX_WORD))] = {
On 2015/08/17 18:55:16, Tom Sepez wrote:
> nit: I find this more confusing than they old way.  Also, since this is a
> definition, not a declaration, no need for "extern"

I rewrote it. PTAL.

For the extern, because I didn't include fx_ucd.h, the compiler decided this was
useless code.

https://codereview.chromium.org/1197643002/diff/120001/core/src/fxcrt/fx_ucdd...
core/src/fxcrt/fx_ucddata.cpp:10935: extern const FX_WCHAR
gs_FX_TextLayout_VerticalMirror[64] = {
On 2015/08/17 18:55:16, Tom Sepez wrote:
> nit: ditto for no "extern". Probably other places in the file, too.

Done.

[Tom Sepez, 2015-08-17 23:06:36]

lgtm

[Lei Zhang, 2015-08-18 01:00:52]

Committed patchset #8 (id:140001) manually as
2ae87d2e8ddff79d0e96aad3db97e090db21fb99 (presubmit successful).