Make a bunch of JBig2 classes independent of CJBig2_Object.

Make a bunch of JBig2 classes independent of CJBig2_Object.

R=tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/800bf5ac07305ccf9c9522c8c76f728a4fd99da5

[Lei Zhang, 2015-09-03 05:02:03]

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

[Lei Zhang, 2015-09-03 05:02:03]

Less pModule->JBig2[Malloc/Free]() indirection.

There's a bunch of cleanup in here too, so it's a bit bigger.

[Tom Sepez, 2015-09-03 20:24:58]

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
File core/src/fxcodec/jbig2/JBig2_ArithDecoder.h (right):

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_ArithDecoder.h:30: unsigned char B;
nit: prefer blank line above and no blank line at 28 if you're mucking with this
...

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_ArithDecoder.h:41: inline
CJBig2_ArithDecoder::~CJBig2_ArithDecoder() {}
Do we need this?

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
File core/src/fxcodec/jbig2/JBig2_ArithIntDecoder.cpp (right):

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_ArithIntDecoder.cpp:9: #include
"../../../include/fxcrt/fx_memory.h"
Does this introduce a new dependency between JBIG2 and fxcrt?  In theory, could
someone snarf just the JBig2 directory independently and use it for some other
purpose?  Or is it already dependent?

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
File core/src/fxcodec/jbig2/JBig2_Context.cpp (right):

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:148: } else if (nRet != JBIG2_SUCCESS)
{
nit: else after break.

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:197: } else if (nRet != JBIG2_SUCCESS)
{
nit: else after break

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:638: HuffmanTable_B4,
sizeof(HuffmanTable_B4) / sizeof(JBig2TableLine),
nit: use FX_ArraySize() if possible.

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
File core/src/fxcodec/jbig2/JBig2_Context.h (right):

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.h:23: enum JBig2State {
nit: private, scoped by CJBig2_Context class.

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.h:43: class CJBig2_Context : public
CJBig2_Object {
missed one?

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.h:126: 
nit: maybe kill blank lines to be consistent with lines 141 and following.

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
File core/src/fxcodec/jbig2/JBig2_GeneralDecoder.cpp (right):

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_GeneralDecoder.cpp:1144: JBIG2_ALLOC(SBREG,
CJBig2_Image(SBW, SBH));
nit: Did you want to fix this one too?

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_GeneralDecoder.cpp:1760: HuffmanTable_B6,
sizeof(HuffmanTable_B6) / sizeof(JBig2TableLine),
nit: FX_ArraySize() usage?

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
File core/src/fxcodec/jbig2/JBig2_List.h (right):

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_List.h:13: class CJBig2_List {
do we want to just kill the entire class?  All it buys is the clear() method;
maybe inline as needed?

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
File core/src/fxcodec/jbig2/JBig2_PatternDict.h (right):

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_PatternDict.h:17: public:
nit: duplicate public:

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
File core/src/fxcodec/jbig2/JBig2_Segment.h (right):

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Segment.h:37: public:
nit: duplicate public

[Lei Zhang, 2015-09-03 22:52:13]

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
File core/src/fxcodec/jbig2/JBig2_ArithDecoder.h (right):

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_ArithDecoder.h:30: unsigned char B;
On 2015/09/03 20:24:57, Tom Sepez wrote:
> nit: prefer blank line above and no blank line at 28 if you're mucking with
this
> ...

Done.

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_ArithDecoder.h:41: inline
CJBig2_ArithDecoder::~CJBig2_ArithDecoder() {}
On 2015/09/03 20:24:57, Tom Sepez wrote:
> Do we need this?

As is, I can't build without out it. I'll create JBig2_ArithDecoder.cpp at some
point.

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
File core/src/fxcodec/jbig2/JBig2_ArithIntDecoder.cpp (right):

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_ArithIntDecoder.cpp:9: #include
"../../../include/fxcrt/fx_memory.h"
On 2015/09/03 20:24:58, Tom Sepez wrote:
> Does this introduce a new dependency between JBIG2 and fxcrt?  In theory,
could
> someone snarf just the JBig2 directory independently and use it for some other
> purpose?  Or is it already dependent?

Already dependent. Besides, fxcrt is like 'base'.

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
File core/src/fxcodec/jbig2/JBig2_Context.cpp (right):

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:148: } else if (nRet != JBIG2_SUCCESS)
{
On 2015/09/03 20:24:58, Tom Sepez wrote:
> nit: else after break.

Done, changed the break to a return as well.

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:197: } else if (nRet != JBIG2_SUCCESS)
{
On 2015/09/03 20:24:58, Tom Sepez wrote:
> nit: else after break

Done, break -> return.

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:638: HuffmanTable_B4,
sizeof(HuffmanTable_B4) / sizeof(JBig2TableLine),
On 2015/09/03 20:24:58, Tom Sepez wrote:
> nit: use FX_ArraySize() if possible.

Done.

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
File core/src/fxcodec/jbig2/JBig2_Context.h (right):

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.h:23: enum JBig2State {
On 2015/09/03 20:24:58, Tom Sepez wrote:
> nit: private, scoped by CJBig2_Context class.

Done.

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.h:43: class CJBig2_Context : public
CJBig2_Object {
On 2015/09/03 20:24:58, Tom Sepez wrote:
> missed one?

No, next CL...

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.h:126: 
On 2015/09/03 20:24:58, Tom Sepez wrote:
> nit: maybe kill blank lines to be consistent with lines 141 and following.

Done.

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
File core/src/fxcodec/jbig2/JBig2_GeneralDecoder.cpp (right):

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_GeneralDecoder.cpp:1144: JBIG2_ALLOC(SBREG,
CJBig2_Image(SBW, SBH));
On 2015/09/03 20:24:58, Tom Sepez wrote:
> nit: Did you want to fix this one too?

CJBig2_Image is still a CJBig2_Object subclass for now. I have a follow up CL to
kill CJBig2_Object altogether.

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_GeneralDecoder.cpp:1760: HuffmanTable_B6,
sizeof(HuffmanTable_B6) / sizeof(JBig2TableLine),
On 2015/09/03 20:24:58, Tom Sepez wrote:
> nit: FX_ArraySize() usage?

Done.

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
File core/src/fxcodec/jbig2/JBig2_List.h (right):

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_List.h:13: class CJBig2_List {
On 2015/09/03 20:24:58, Tom Sepez wrote:
> do we want to just kill the entire class?  All it buys is the clear() method;
> maybe inline as needed?

The class is basically being used as a poor man's ScopedVector. I'd like to keep
it for now.

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
File core/src/fxcodec/jbig2/JBig2_PatternDict.h (right):

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_PatternDict.h:17: public:
On 2015/09/03 20:24:58, Tom Sepez wrote:
> nit: duplicate public:

Done.

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
File core/src/fxcodec/jbig2/JBig2_Segment.h (right):

https://codereview.chromium.org/1319713003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Segment.h:37: public:
On 2015/09/03 20:24:58, Tom Sepez wrote:
> nit: duplicate public

Done.

[Tom Sepez, 2015-09-03 22:58:36]

lgtm

[Lei Zhang, 2015-09-03 23:03:46]

Committed patchset #3 (id:40001) manually as
800bf5ac07305ccf9c9522c8c76f728a4fd99da5 (presubmit successful).