Remove _FXBSTR* and calls to FX_BSTRC.

Remove _FXBSTR* and calls to FX_BSTRC.

This CL removes _FXBSTR and changes FPDF_AbbrPair to hold
two FX_CHAR pointers. The two remaining uses of FX_BSTRC()
were in unused FDE_CSS defines which have been removed.

BUG=pdfium:151
Committed: https://pdfium.googlesource.com/pdfium/+/d80e0a7f9fb36503e872e022af65899d55080790

[dsinclair, 2016-04-01 14:07:28]

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

[dsinclair, 2016-04-01 14:07:29]

PTAL.

[Tom Sepez, 2016-04-01 16:43:03]

https://codereview.chromium.org/1847333004/diff/1/core/fpdfapi/fpdf_page/fpdf...
File core/fpdfapi/fpdf_page/fpdf_page_parser.cpp (right):

https://codereview.chromium.org/1847333004/diff/1/core/fpdfapi/fpdf_page/fpdf...
core/fpdfapi/fpdf_page/fpdf_page_parser.cpp:57:
{MakeSimpleString("BitsPerComponent"), MakeSimpleString("BPC")},
This theoretically generates static initializers since technically strlen isn't
compile-time constant (as opposed to sizeof(), which is).

There's probably a way to use CFX_ByteStringC here, there should be form to give
us a compile time constant, and if not, we'll tweak it.

https://codereview.chromium.org/1847333004/diff/1/core/fpdfapi/fpdf_page/fpdf...
core/fpdfapi/fpdf_page/fpdf_page_parser.cpp:104: for (size_t i = 0; i < count;
++i) {
std::find using cfx_ByteStringC's == operator should do here.

[dsinclair, 2016-04-01 20:18:34]

https://codereview.chromium.org/1847333004/diff/1/core/fpdfapi/fpdf_page/fpdf...
File core/fpdfapi/fpdf_page/fpdf_page_parser.cpp (right):

https://codereview.chromium.org/1847333004/diff/1/core/fpdfapi/fpdf_page/fpdf...
core/fpdfapi/fpdf_page/fpdf_page_parser.cpp:57:
{MakeSimpleString("BitsPerComponent"), MakeSimpleString("BPC")},
On 2016/04/01 16:43:03, Tom Sepez wrote:
> This theoretically generates static initializers since technically strlen
isn't
> compile-time constant (as opposed to sizeof(), which is).
> 
> There's probably a way to use CFX_ByteStringC here, there should be form to
give
> us a compile time constant, and if not, we'll tweak it.

Ah, I failed to understand the comment in pdfium:151. I thought the compiler
would optimize the strlen("foo") to 3 at compile time. Will look into switching
to CFX_ByteStringC.

[Tom Sepez, 2016-04-01 20:44:37]

> Ah, I failed to understand the comment in pdfium:151. I thought the compiler
> would optimize the strlen("foo") to 3 at compile time. Will look into
switching
> to CFX_ByteStringC.
Actually, can't be done at present without a small tweak. We insist that

CFX_ByteStringC fred;

be initialized, hence the user-supplied default ctor, alas that makes it
non-POD.

[Tom Sepez, 2016-04-01 21:45:54]

https://codereview.chromium.org/1847333004/diff/1/core/fpdfapi/fpdf_page/fpdf...
File core/fpdfapi/fpdf_page/fpdf_page_parser.cpp (right):

https://codereview.chromium.org/1847333004/diff/1/core/fpdfapi/fpdf_page/fpdf...
core/fpdfapi/fpdf_page/fpdf_page_parser.cpp:51: struct PDF_AbbrPairs {
nit: singular (e.g AbbrPair).

https://codereview.chromium.org/1847333004/diff/1/core/fpdfapi/fpdf_page/fpdf...
core/fpdfapi/fpdf_page/fpdf_page_parser.cpp:52: SimpleString full_name;
Actually, its hard to justify this being anything other than 
struct PDf_AbbrPair {
  const FX_CHAR* full_name;
  const FX_CHAR* abbr;
};

[dsinclair, 2016-04-02 00:13:33]

https://codereview.chromium.org/1847333004/diff/1/core/fpdfapi/fpdf_page/fpdf...
File core/fpdfapi/fpdf_page/fpdf_page_parser.cpp (right):

https://codereview.chromium.org/1847333004/diff/1/core/fpdfapi/fpdf_page/fpdf...
core/fpdfapi/fpdf_page/fpdf_page_parser.cpp:51: struct PDF_AbbrPairs {
On 2016/04/01 21:45:53, Tom Sepez wrote:
> nit: singular (e.g AbbrPair).

Done.

https://codereview.chromium.org/1847333004/diff/1/core/fpdfapi/fpdf_page/fpdf...
core/fpdfapi/fpdf_page/fpdf_page_parser.cpp:52: SimpleString full_name;
On 2016/04/01 21:45:53, Tom Sepez wrote:
> Actually, its hard to justify this being anything other than 
> struct PDf_AbbrPair {
>   const FX_CHAR* full_name;
>   const FX_CHAR* abbr;
> };
> 

Done.

https://codereview.chromium.org/1847333004/diff/1/core/fpdfapi/fpdf_page/fpdf...
core/fpdfapi/fpdf_page/fpdf_page_parser.cpp:104: for (size_t i = 0; i < count;
++i) {
On 2016/04/01 16:43:03, Tom Sepez wrote:
> std::find using cfx_ByteStringC's == operator should do here.

Acknowledged.

https://codereview.chromium.org/1847333004/diff/20001/core/fpdfapi/fpdf_page/...
File core/fpdfapi/fpdf_page/fpdf_page_parser.cpp (right):

https://codereview.chromium.org/1847333004/diff/20001/core/fpdfapi/fpdf_page/...
core/fpdfapi/fpdf_page/fpdf_page_parser.cpp:83: CFX_ByteStringC
PDF_FindFullName(const PDF_AbbrPair* table,
What's your preference for testing something like this? Move it out of the
anonymous namespace, make a PDF_FindFullNameForTesting outside that calls into
this one?

[Tom Sepez, 2016-04-02 18:11:54]

CL description needs updating

https://codereview.chromium.org/1847333004/diff/20001/core/fpdfapi/fpdf_page/...
File core/fpdfapi/fpdf_page/fpdf_page_parser.cpp (right):

https://codereview.chromium.org/1847333004/diff/20001/core/fpdfapi/fpdf_page/...
core/fpdfapi/fpdf_page/fpdf_page_parser.cpp:83: CFX_ByteStringC
PDF_FindFullName(const PDF_AbbrPair* table,
On 2016/04/02 00:13:33, dsinclair wrote:
> What's your preference for testing something like this? Move it out of the
> anonymous namespace, make a PDF_FindFullNameForTesting outside that calls into
> this one?
Dunno. May not require a unit test, even, if we hit this somewhere in real pdfs
... 

auto it = std::find_if(table, table + count, [abbr](const PDF_AbbrPair& pair) {
  return pair.abbr == abbr;
});
return it != table + count ? CFX_ByteString(it->full_name) : CFX_ByteString();


or some such ...

[dsinclair, 2016-04-04 15:21:07]

Description was changed from

==========
Remove _FXBSTR* and calls to FX_BSTRC.

This CL replaces _FXBSTR with SimpleString and changes the macro to use strlen
instead of sizeof() - 1. The two remaining uses of FX_BSTRC() were in unused
FDE_CSS defines which have been removed.

BUG=pdfium:151
==========

to

==========
Remove _FXBSTR* and calls to FX_BSTRC.

This CL removes _FXBSTR and changes FPDF_AbbrPair to hold
two FX_CHAR pointers. The two remaining uses of FX_BSTRC()
were in unused FDE_CSS defines which have been removed.

BUG=pdfium:151
==========

[dsinclair, 2016-04-04 15:21:14]

https://codereview.chromium.org/1847333004/diff/20001/core/fpdfapi/fpdf_page/...
File core/fpdfapi/fpdf_page/fpdf_page_parser.cpp (right):

https://codereview.chromium.org/1847333004/diff/20001/core/fpdfapi/fpdf_page/...
core/fpdfapi/fpdf_page/fpdf_page_parser.cpp:83: CFX_ByteStringC
PDF_FindFullName(const PDF_AbbrPair* table,
On 2016/04/02 18:11:54, Tom Sepez wrote:
> On 2016/04/02 00:13:33, dsinclair wrote:
> > What's your preference for testing something like this? Move it out of the
> > anonymous namespace, make a PDF_FindFullNameForTesting outside that calls
into
> > this one?
> Dunno. May not require a unit test, even, if we hit this somewhere in real
pdfs
> ... 
> 
> auto it = std::find_if(table, table + count, [abbr](const PDF_AbbrPair& pair)
{
>   return pair.abbr == abbr;
> });
> return it != table + count ? CFX_ByteString(it->full_name) : CFX_ByteString();
> 
> 
> or some such ...
> 

Done.  Add ForTesting wrappers and some simple unittests.

[Tom Sepez, 2016-04-04 15:57:30]

lgtm

https://codereview.chromium.org/1847333004/diff/60001/core/fpdfapi/fpdf_page/...
File core/fpdfapi/fpdf_page/fpdf_page_parser.cpp (right):

https://codereview.chromium.org/1847333004/diff/60001/core/fpdfapi/fpdf_page/...
core/fpdfapi/fpdf_page/fpdf_page_parser.cpp:45: const FX_CHAR* full_name;
If you'd like, you might consider flipping these around

const Fx_CHAR* abbr;
const FX_CHAR* full_name

so that the "key" comes before the "value". Right now, we've got value/key pairs
rather than key/value pairs, since we always replace the shorter with the longer
...

[Tom Sepez, 2016-04-04 16:02:38]

On 2016/04/04 15:57:30, Tom Sepez wrote:
> lgtm
> 
>
https://codereview.chromium.org/1847333004/diff/60001/core/fpdfapi/fpdf_page/...
> File core/fpdfapi/fpdf_page/fpdf_page_parser.cpp (right):
> 
>
https://codereview.chromium.org/1847333004/diff/60001/core/fpdfapi/fpdf_page/...
> core/fpdfapi/fpdf_page/fpdf_page_parser.cpp:45: const FX_CHAR* full_name;
> If you'd like, you might consider flipping these around
> 
> const Fx_CHAR* abbr;
> const FX_CHAR* full_name
> 
> so that the "key" comes before the "value". Right now, we've got value/key
pairs
> rather than key/value pairs, since we always replace the shorter with the
longer
> ...

Any idea what's up with the tests?

[Tom Sepez, 2016-04-04 16:11:37]

Ha!

https://codereview.chromium.org/1847333004/diff/60001/core/fpdfapi/fpdf_page/...
File core/fpdfapi/fpdf_page/fpdf_page_parser.cpp (right):

https://codereview.chromium.org/1847333004/diff/60001/core/fpdfapi/fpdf_page/...
core/fpdfapi/fpdf_page/fpdf_page_parser.cpp:89: return it != table + count ?
CFX_ByteString(it->full_name) : CFX_ByteString();
This should be ByteStringC, not ByteString. argh.  Damn silent demotion from
bytestring to bytestringc.

[dsinclair, 2016-04-04 16:26:52]

https://codereview.chromium.org/1847333004/diff/60001/core/fpdfapi/fpdf_page/...
File core/fpdfapi/fpdf_page/fpdf_page_parser.cpp (right):

https://codereview.chromium.org/1847333004/diff/60001/core/fpdfapi/fpdf_page/...
core/fpdfapi/fpdf_page/fpdf_page_parser.cpp:45: const FX_CHAR* full_name;
On 2016/04/04 15:57:30, Tom Sepez wrote:
> If you'd like, you might consider flipping these around
> 
> const Fx_CHAR* abbr;
> const FX_CHAR* full_name
> 
> so that the "key" comes before the "value". Right now, we've got value/key
pairs
> rather than key/value pairs, since we always replace the shorter with the
longer
> ...

Done.

https://codereview.chromium.org/1847333004/diff/60001/core/fpdfapi/fpdf_page/...
core/fpdfapi/fpdf_page/fpdf_page_parser.cpp:89: return it != table + count ?
CFX_ByteString(it->full_name) : CFX_ByteString();
On 2016/04/04 16:11:37, Tom Sepez wrote:
> This should be ByteStringC, not ByteString. argh.  Damn silent demotion from
> bytestring to bytestringc.

Good catch. Didn't notice the missing C even reviewing this line knowing
something was wrong, heh.

[dsinclair, 2016-04-04 16:27:16]

The CQ bit was checked by dsinclair@chromium.org

[dsinclair, 2016-04-04 16:27:17]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/1847333004/#ps80001
(title: " ")

[commit-bot: I haz the power, 2016-04-04 16:27:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1847333004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1847333004/80001

[Tom Sepez, 2016-04-04 16:29:26]

++LGTM

https://codereview.chromium.org/1847333004/diff/80001/core/fpdfapi/fpdf_page/...
File core/fpdfapi/fpdf_page/fpdf_page_parser.cpp (right):

https://codereview.chromium.org/1847333004/diff/80001/core/fpdfapi/fpdf_page/...
core/fpdfapi/fpdf_page/fpdf_page_parser.cpp:50: {"BPC", "BitsPerComponent"},
{"CS", "ColorSpace"}, {"D", "Decode"},
That's so much better in that order, it gives a clue as to what's about to
happen.

https://codereview.chromium.org/1847333004/diff/80001/core/fpdfapi/fpdf_page/...
core/fpdfapi/fpdf_page/fpdf_page_parser.cpp:89: return it != table + count ?
CFX_ByteStringC(it->full_name)
I'm going to try to make the ctor explicit.  Its long overdue, and would have
caught this.

[commit-bot: I haz the power, 2016-04-04 16:39:00]

Description was changed from

==========
Remove _FXBSTR* and calls to FX_BSTRC.

This CL removes _FXBSTR and changes FPDF_AbbrPair to hold
two FX_CHAR pointers. The two remaining uses of FX_BSTRC()
were in unused FDE_CSS defines which have been removed.

BUG=pdfium:151
==========

to

==========
Remove _FXBSTR* and calls to FX_BSTRC.

This CL removes _FXBSTR and changes FPDF_AbbrPair to hold
two FX_CHAR pointers. The two remaining uses of FX_BSTRC()
were in unused FDE_CSS defines which have been removed.

BUG=pdfium:151

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

[commit-bot: I haz the power, 2016-04-04 16:39:01]

Committed patchset #5 (id:80001) as
https://pdfium.googlesource.com/pdfium/+/d80e0a7f9fb36503e872e022af65899d5508...