Clean up CPDF_TextPage.

Clean up CPDF_TextPage.

- Use more enums to better describe return results.
- Simplify code.

Committed: https://pdfium.googlesource.com/pdfium/+/e7b0a2d5c47a88fb29ed9d100d670e7185b8d213

[Lei Zhang, 2016-06-14 19:20:21]

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

[Lei Zhang, 2016-06-14 19:20:21]

Here's a first pass just so I can better understand the code.

[dsinclair, 2016-06-14 19:40:13]

https://codereview.chromium.org/2064223002/diff/1/core/fpdftext/fpdf_text_int...
File core/fpdftext/fpdf_text_int.cpp (left):

https://codereview.chromium.org/2064223002/diff/1/core/fpdftext/fpdf_text_int...
core/fpdftext/fpdf_text_int.cpp:1226: m_TextBuf.AppendChar(TEXT_RETURN_CHAR);
This is different now. Previously it would append to m_TextBuf, now it will
append to m_TempTextBuf instead. Is that intentional? (Same for the if() on
1232.)

https://codereview.chromium.org/2064223002/diff/1/core/fpdftext/fpdf_text_int...
File core/fpdftext/fpdf_text_int.cpp (right):

https://codereview.chromium.org/2064223002/diff/1/core/fpdftext/fpdf_text_int...
core/fpdftext/fpdf_text_int.cpp:678: CPDF_TextPage::TextOrientation
CPDF_TextPage::FindTextlineFlowOrientation()
Is the CPDF_TextPage:: needed here?

https://codereview.chromium.org/2064223002/diff/1/core/fpdftext/fpdf_text_int...
core/fpdftext/fpdf_text_int.cpp:765: if (GenerateCharInfo(unicode,
generateChar)) {
if (!GenerateCharInfo(unicode, generateChar))
  return;

https://codereview.chromium.org/2064223002/diff/1/core/fpdftext/fpdf_text_int...
core/fpdftext/fpdf_text_int.cpp:1239: if (0x2D == curChar || 0xAD == curChar) {
nit: {}'s not needed

https://codereview.chromium.org/2064223002/diff/1/core/fpdftext/fpdf_text_int...
core/fpdftext/fpdf_text_int.cpp:1436: CPDF_TextPage::TextOrientation
CPDF_TextPage::GetTextObjectWritingMode(
nit: CPDF_TextPage:: needed?

https://codereview.chromium.org/2064223002/diff/1/core/fpdftext/fpdf_text_int...
core/fpdftext/fpdf_text_int.cpp:1502: CPDF_TextPage::GenerateCharacter
CPDF_TextPage::ProcessInsertObject(
ditto

https://codereview.chromium.org/2064223002/diff/1/core/fpdftext/fpdf_text_int...
core/fpdftext/fpdf_text_int.cpp:1644: return GenerateCharacter::Blank;
Does blank mean '' or ' '? If the latter, would Space be a better name?

[Lei Zhang, 2016-06-14 20:01:16]

https://codereview.chromium.org/2064223002/diff/1/core/fpdftext/fpdf_text_int...
File core/fpdftext/fpdf_text_int.cpp (left):

https://codereview.chromium.org/2064223002/diff/1/core/fpdftext/fpdf_text_int...
core/fpdftext/fpdf_text_int.cpp:1226: m_TextBuf.AppendChar(TEXT_RETURN_CHAR);
On 2016/06/14 19:40:13, dsinclair wrote:
> This is different now. Previously it would append to m_TextBuf, now it will
> append to m_TempTextBuf instead. Is that intentional? (Same for the if() on
> 1232.)

Whoops, bad refactoring.

https://codereview.chromium.org/2064223002/diff/1/core/fpdftext/fpdf_text_int...
File core/fpdftext/fpdf_text_int.cpp (right):

https://codereview.chromium.org/2064223002/diff/1/core/fpdftext/fpdf_text_int...
core/fpdftext/fpdf_text_int.cpp:678: CPDF_TextPage::TextOrientation
CPDF_TextPage::FindTextlineFlowOrientation()
On 2016/06/14 19:40:13, dsinclair wrote:
> Is the CPDF_TextPage:: needed here?

Yes

https://codereview.chromium.org/2064223002/diff/1/core/fpdftext/fpdf_text_int...
core/fpdftext/fpdf_text_int.cpp:765: if (GenerateCharInfo(unicode,
generateChar)) {
On 2016/06/14 19:40:12, dsinclair wrote:
> if (!GenerateCharInfo(unicode, generateChar))
>   return;

Done.

https://codereview.chromium.org/2064223002/diff/1/core/fpdftext/fpdf_text_int...
core/fpdftext/fpdf_text_int.cpp:1239: if (0x2D == curChar || 0xAD == curChar) {
On 2016/06/14 19:40:13, dsinclair wrote:
> nit: {}'s not needed

Done.

https://codereview.chromium.org/2064223002/diff/1/core/fpdftext/fpdf_text_int...
core/fpdftext/fpdf_text_int.cpp:1436: CPDF_TextPage::TextOrientation
CPDF_TextPage::GetTextObjectWritingMode(
On 2016/06/14 19:40:13, dsinclair wrote:
> nit: CPDF_TextPage:: needed?

Yes

https://codereview.chromium.org/2064223002/diff/1/core/fpdftext/fpdf_text_int...
core/fpdftext/fpdf_text_int.cpp:1502: CPDF_TextPage::GenerateCharacter
CPDF_TextPage::ProcessInsertObject(
On 2016/06/14 19:40:13, dsinclair wrote:
> ditto

ditto ditto

https://codereview.chromium.org/2064223002/diff/1/core/fpdftext/fpdf_text_int...
core/fpdftext/fpdf_text_int.cpp:1644: return GenerateCharacter::Blank;
On 2016/06/14 19:40:13, dsinclair wrote:
> Does blank mean '' or ' '? If the latter, would Space be a better name?

I was going along with TEXT_BLANK_FOO. Renamed them all.

[dsinclair, 2016-06-14 20:14:46]

The CQ bit was checked by dsinclair@chromium.org

[dsinclair, 2016-06-14 20:14:47]

lgtm

[commit-bot: I haz the power, 2016-06-14 20:14:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2064223002/20001

[commit-bot: I haz the power, 2016-06-14 20:15:15]

Description was changed from

==========
Clean up CPDF_TextPage.

- Use more enums to better describe return results.
- Simplify code.
==========

to

==========
Clean up CPDF_TextPage.

- Use more enums to better describe return results.
- Simplify code.

Committed:
https://pdfium.googlesource.com/pdfium/+/e7b0a2d5c47a88fb29ed9d100d670e7185b8...
==========

[commit-bot: I haz the power, 2016-06-14 20:15:16]

Committed patchset #2 (id:20001) as
https://pdfium.googlesource.com/pdfium/+/e7b0a2d5c47a88fb29ed9d100d670e7185b8...