Cleanup CJBig2_HuffmanTable and friends.

Cleanup CJBig2_HuffmanTable and friends.

R=tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/9c061e2dfb22a0e321728b73c8650eded8828d90

[Lei Zhang, 2015-12-18 23:06:09]

Patchset #1 (id:1) has been deleted

[Lei Zhang, 2015-12-18 23:06:21]

Patchset #1 (id:20001) has been deleted

[Lei Zhang, 2015-12-18 23:06:26]

Patchset #1 (id:40001) has been deleted

[Lei Zhang, 2015-12-18 23:24:19]

Patchset #1 (id:60001) has been deleted

[Lei Zhang, 2015-12-18 23:25:16]

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

[Lei Zhang, 2015-12-18 23:25:16]

And after the bug fix, do some cleanup.

[Tom Sepez, 2015-12-18 23:48:54]

https://codereview.chromium.org/1536923002/diff/80001/core/src/fxcodec/jbig2/...
File core/src/fxcodec/jbig2/JBig2_Context.cpp (right):

https://codereview.chromium.org/1536923002/diff/80001/core/src/fxcodec/jbig2/...
core/src/fxcodec/jbig2/JBig2_Context.cpp:1272: pSegment->m_Result.ht = nullptr;
nit: I might have been tempted to null this out up top, then just return here
(in case some other failure path gets introduced in the future, we'd need to
null it there, too).  Then you can lose the {} for your if.

https://codereview.chromium.org/1536923002/diff/80001/core/src/fxcodec/jbig2/...
File core/src/fxcodec/jbig2/JBig2_HuffmanTable.cpp (left):

https://codereview.chromium.org/1536923002/diff/80001/core/src/fxcodec/jbig2/...
core/src/fxcodec/jbig2/JBig2_HuffmanTable.cpp:66: 
what happened to this block?

https://codereview.chromium.org/1536923002/diff/80001/core/src/fxcodec/jbig2/...
core/src/fxcodec/jbig2/JBig2_HuffmanTable.cpp:120: NTEMP = 0;
I'm guessing the ctor did this initialization, but can we at least assert this,
in case this gets called twice?

https://codereview.chromium.org/1536923002/diff/80001/core/src/fxcodec/jbig2/...
File core/src/fxcodec/jbig2/JBig2_HuffmanTable.cpp (right):

https://codereview.chromium.org/1536923002/diff/80001/core/src/fxcodec/jbig2/...
core/src/fxcodec/jbig2/JBig2_HuffmanTable.cpp:74: ++NTEMP;
I'd like to see the ++NTEMP/ExtendBuffers() folded together into a method so
that X[NTEMP] is always good.  Hmmm.

https://codereview.chromium.org/1536923002/diff/80001/core/src/fxcodec/jbig2/...
core/src/fxcodec/jbig2/JBig2_HuffmanTable.cpp:137: }
nit: can we assert that it's big enough here?  I know we're supposed to march
lock-step through these, but should something start jumping around by large
amounts ...

https://codereview.chromium.org/1536923002/diff/80001/core/src/fxcodec/jbig2/...
File core/src/fxcodec/jbig2/JBig2_HuffmanTable.h (right):

https://codereview.chromium.org/1536923002/diff/80001/core/src/fxcodec/jbig2/...
core/src/fxcodec/jbig2/JBig2_HuffmanTable.h:47: bool m_bOK;
nit: probably packs tighter if this is adjacent to the other bool, if you care
...

[Lei Zhang, 2015-12-19 00:17:22]

https://codereview.chromium.org/1536923002/diff/80001/core/src/fxcodec/jbig2/...
File core/src/fxcodec/jbig2/JBig2_Context.cpp (right):

https://codereview.chromium.org/1536923002/diff/80001/core/src/fxcodec/jbig2/...
core/src/fxcodec/jbig2/JBig2_Context.cpp:1272: pSegment->m_Result.ht = nullptr;
On 2015/12/18 23:48:54, Tom Sepez wrote:
> nit: I might have been tempted to null this out up top, then just return here
> (in case some other failure path gets introduced in the future, we'd need to
> null it there, too).  Then you can lose the {} for your if.

Done.

https://codereview.chromium.org/1536923002/diff/80001/core/src/fxcodec/jbig2/...
File core/src/fxcodec/jbig2/JBig2_HuffmanTable.cpp (left):

https://codereview.chromium.org/1536923002/diff/80001/core/src/fxcodec/jbig2/...
core/src/fxcodec/jbig2/JBig2_HuffmanTable.cpp:66: 
On 2015/12/18 23:48:54, Tom Sepez wrote:
> what happened to this block?

The line 67-82 block was initially refactored into a for-loop, but then I
realized it looks a lot like lines 162-171, so it got folded into InitCodes().

https://codereview.chromium.org/1536923002/diff/80001/core/src/fxcodec/jbig2/...
core/src/fxcodec/jbig2/JBig2_HuffmanTable.cpp:120: NTEMP = 0;
On 2015/12/18 23:48:54, Tom Sepez wrote:
> I'm guessing the ctor did this initialization, but can we at least assert
this,
> in case this gets called twice?

ParseFromCodedBuffer() is a private method. It's probably wrong to call it twice
because |pStream| would be read from the wrong location in the stream.

https://codereview.chromium.org/1536923002/diff/80001/core/src/fxcodec/jbig2/...
File core/src/fxcodec/jbig2/JBig2_HuffmanTable.cpp (right):

https://codereview.chromium.org/1536923002/diff/80001/core/src/fxcodec/jbig2/...
core/src/fxcodec/jbig2/JBig2_HuffmanTable.cpp:74: ++NTEMP;
On 2015/12/18 23:48:54, Tom Sepez wrote:
> I'd like to see the ++NTEMP/ExtendBuffers() folded together into a method so
> that X[NTEMP] is always good.  Hmmm.

Well, here's something...

https://codereview.chromium.org/1536923002/diff/80001/core/src/fxcodec/jbig2/...
core/src/fxcodec/jbig2/JBig2_HuffmanTable.cpp:137: }
On 2015/12/18 23:48:54, Tom Sepez wrote:
> nit: can we assert that it's big enough here?  I know we're supposed to march
> lock-step through these, but should something start jumping around by large
> amounts ...

Done.

https://codereview.chromium.org/1536923002/diff/80001/core/src/fxcodec/jbig2/...
File core/src/fxcodec/jbig2/JBig2_HuffmanTable.h (right):

https://codereview.chromium.org/1536923002/diff/80001/core/src/fxcodec/jbig2/...
core/src/fxcodec/jbig2/JBig2_HuffmanTable.h:47: bool m_bOK;
On 2015/12/18 23:48:54, Tom Sepez wrote:
> nit: probably packs tighter if this is adjacent to the other bool, if you care
> ...

Done.

[Tom Sepez, 2015-12-19 00:22:54]

lgtm

[Lei Zhang, 2015-12-19 00:44:31]

Description was changed from

==========
Cleanup CJBig2_HuffmanTable and friends.
==========

to

==========
Cleanup CJBig2_HuffmanTable and friends.

R=tsepez@chromium.org

Committed:
https://pdfium.googlesource.com/pdfium/+/9c061e2dfb22a0e321728b73c8650eded882...
==========

[Lei Zhang, 2015-12-19 00:44:33]

Committed patchset #3 (id:120001) manually as
9c061e2dfb22a0e321728b73c8650eded8828d90 (presubmit successful).