Fix hebrew character highlight issue in a special document

Fix hebrew character highlight issue in a special document

There is an image object and text objects in this document, but the character in each text object is reversed.
When rendering, the image object is shown.
However, when highlighting, the text object is selected, resulting in text index issue.
Moreover, the character in the document is in reading order, which is different from normal document.

BUG=pdfium:43
R=jbreiden@google.com

Committed: https://pdfium.googlesource.com/pdfium/+/56ef173042d786281edcbbc9f1c38c8f97ef10d5

[Bo Xu, 2014-08-17 05:46:40]

Hi Jeff, I add you as reviewer, not sure if you can review it. Please let me
know what you think. Thanks!

[jbreiden, 2014-08-18 19:10:50]

This is my first time reviewing PDFium code. But I do have some experience with
PDF, so hopefully this is helpful.

https://codereview.chromium.org/484503002/diff/1/core/src/fpdftext/fpdf_text.cpp
File core/src/fpdftext/fpdf_text.cpp (right):

https://codereview.chromium.org/484503002/diff/1/core/src/fpdftext/fpdf_text....
core/src/fpdftext/fpdf_text.cpp:188: text += (FX_WCHAR)ch;
The example PDF file for this bug contains a
mapping between character codes and Unicode.
Is this mapping not being used?

====================

5 0 obj
<< /Length 353 >>
stream
/CIDInit /ProcSet findresource begin
12 dict begin
begincmap
/CIDSystemInfo
<<
  /Registry (Adobe)
  /Ordering (UCS)
  /Supplement 0
>> def
/CMapName /Adobe-Identify-UCS def
/CMapType 2 def
1 begincodespacerange
<0000> <FFFF>
endcodespacerange
1 beginbfrange
<0000> <FFFF> <0000>
endbfrange
endcmap
CMapName currentdict /CMap defineresource pop
end
end
endstream
endobj

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

https://codereview.chromium.org/484503002/diff/1/core/src/fpdftext/fpdf_text_...
core/src/fpdftext/fpdf_text_int.cpp:1686: FX_BOOL bIsBidiAndMirrosInverse =
FALSE;
What does MirrosInverse mean?

https://codereview.chromium.org/484503002/diff/1/core/src/fpdftext/fpdf_text_...
core/src/fpdftext/fpdf_text_int.cpp:1699: if ((wstrItem.IsEmpty() || wChar == 0)
&& item.m_CharCode) {
Same question. Is the character to Unicode mapping from the PDF file used?

https://codereview.chromium.org/484503002/diff/1/core/src/fpdftext/fpdf_text_...
core/src/fpdftext/fpdf_text_int.cpp:1878: if (bIsBidiAndMirrosInverse) {
Ok, I see what is going on here. The code is reversing some
text when necessary because of BiDi. This looks like a 
heuristic.

Please, please, please let me know if there is a mistake 
with the sample PDF file. I have 100% control over the PDF 
generation and can fix them if there is a mistake. This is 
better than PDFium being "smart" and using heuristics to 
work around bad PDF files.

[Bo Xu, 2014-08-19 03:31:50]

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

https://codereview.chromium.org/484503002/diff/1/core/src/fpdftext/fpdf_text_...
core/src/fpdftext/fpdf_text_int.cpp:1699: if ((wstrItem.IsEmpty() || wChar == 0)
&& item.m_CharCode) {
On 2014/08/18 19:10:49, jbreiden wrote:
> Same question. Is the character to Unicode mapping from the PDF file used?

Yes, in CFX_WideString CPDF_Font::UnicodeFromCharCode(FX_DWORD charcode) const,
it does this :

    if (!m_bToUnicodeLoaded) {
        ((CPDF_Font*)this)->LoadUnicodeMap();
    }

to load the UnicodeMap if present.

https://codereview.chromium.org/484503002/diff/1/core/src/fpdftext/fpdf_text_...
core/src/fpdftext/fpdf_text_int.cpp:1878: if (bIsBidiAndMirrosInverse) {
On 2014/08/18 19:10:50, jbreiden wrote:
> Ok, I see what is going on here. The code is reversing some
> text when necessary because of BiDi. This looks like a 
> heuristic.
> 
> Please, please, please let me know if there is a mistake 
> with the sample PDF file. I have 100% control over the PDF 
> generation and can fix them if there is a mistake. This is 
> better than PDFium being "smart" and using heuristics to 
> work around bad PDF files.

I wonder do you generate this particular pdf file? This is indeed a very special
file, it has an image object that shows the correct text, but the actual text
objects are stored in a mirror fashion which makes the text selection goes
wrong.

[Bo Xu, 2014-08-20 17:08:05]

On 2014/08/19 03:31:50, Bo Xu wrote:
>
https://codereview.chromium.org/484503002/diff/1/core/src/fpdftext/fpdf_text_...
> File core/src/fpdftext/fpdf_text_int.cpp (right):
> 
>
https://codereview.chromium.org/484503002/diff/1/core/src/fpdftext/fpdf_text_...
> core/src/fpdftext/fpdf_text_int.cpp:1699: if ((wstrItem.IsEmpty() || wChar ==
0)
> && item.m_CharCode) {
> On 2014/08/18 19:10:49, jbreiden wrote:
> > Same question. Is the character to Unicode mapping from the PDF file used?
> 
> Yes, in CFX_WideString CPDF_Font::UnicodeFromCharCode(FX_DWORD charcode)
const,
> it does this :
> 
>     if (!m_bToUnicodeLoaded) {
>         ((CPDF_Font*)this)->LoadUnicodeMap();
>     }
> 
> to load the UnicodeMap if present.
> 
>
https://codereview.chromium.org/484503002/diff/1/core/src/fpdftext/fpdf_text_...
> core/src/fpdftext/fpdf_text_int.cpp:1878: if (bIsBidiAndMirrosInverse) {
> On 2014/08/18 19:10:50, jbreiden wrote:
> > Ok, I see what is going on here. The code is reversing some
> > text when necessary because of BiDi. This looks like a 
> > heuristic.
> > 
> > Please, please, please let me know if there is a mistake 
> > with the sample PDF file. I have 100% control over the PDF 
> > generation and can fix them if there is a mistake. This is 
> > better than PDFium being "smart" and using heuristics to 
> > work around bad PDF files.
> 
> I wonder do you generate this particular pdf file? This is indeed a very
special
> file, it has an image object that shows the correct text, but the actual text
> objects are stored in a mirror fashion which makes the text selection goes
> wrong.

Jeff, ping :)

[jbreiden, 2014-08-20 19:04:27]

I did generate this PDF file, and have 100% control. I am looking at it
carefully right now.

[jbreiden, 2014-08-21 01:18:39]

I will have a new, simpler PDF file tomorrow. Recommend waiting. Thank you for
your patience.

[Bo Xu, 2014-08-21 07:04:07]

On 2014/08/21 01:18:39, jbreiden wrote:
> I will have a new, simpler PDF file tomorrow. Recommend waiting. Thank you for
> your patience.

Thanks Jeff!

[jbreiden, 2014-08-22 16:42:14]

The CQ bit was checked by jbreiden@google.com

[jbreiden, 2014-08-22 16:42:15]

The CQ bit was unchecked by jbreiden@google.com

[jbreiden, 2014-08-22 16:58:45]

lgtm

Giving an LGTM because I have no further comments. Please use your best
judgement for what to do next.

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

https://codereview.chromium.org/484503002/diff/1/core/src/fpdftext/fpdf_text_...
core/src/fpdftext/fpdf_text_int.cpp:1699: if ((wstrItem.IsEmpty() || wChar == 0)
&& item.m_CharCode) {
Okay.  If I understand correctly:

The PDF files I provided have a UnicodeMap with full coverage. Therefore
UnicodeFromCharCode(item.m_CharCode) is successful, and line 1700 never
executes.

Looks good to me.

https://codereview.chromium.org/484503002/diff/1/core/src/fpdftext/fpdf_text_...
core/src/fpdftext/fpdf_text_int.cpp:1878: if (bIsBidiAndMirrosInverse) {
The PDF file is generated by Tesseract, which is an open source OCR program. I
am the engineer responsible for this feature and have the ability to change it.

https://code.google.com/p/tesseract-ocr/

I have no further comments, except to encourage the best possible solution. In
the future I will be working on PDF output for several other programs, so it is
good to work trouble now.

[Bo Xu, 2014-09-11 21:28:35]

Committed patchset #1 (id:1) manually as 56ef173 (presubmit successful).