Continue fixing FX_BOOL / int noise

Continue fixing FX_BOOL / int noise

Committed: https://pdfium.googlesource.com/pdfium/+/95e5834ab6b6e5aa370cacd6266cecbd82c4c155

[Tom Sepez, 2016-10-28 17:15:33]

The CQ bit was checked by tsepez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 17:15:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Tom Sepez, 2016-10-28 17:17:17]

https://codereview.chromium.org/2459073002/diff/1/core/fxge/win32/fx_win32_gd...
File core/fxge/win32/fx_win32_gdipext.cpp (right):

https://codereview.chromium.org/2459073002/diff/1/core/fxge/win32/fx_win32_gd...
core/fxge/win32/fx_win32_gdipext.cpp:455:
((FuncType_GdiRemoveFontMemResourceEx)m_pGdiRemoveFontMemResourseEx)(
note: this cast scares me, but for another day.

[commit-bot: I haz the power, 2016-10-28 17:31:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-28 17:31:18]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2016-10-28 17:48:12]

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

[Tom Sepez, 2016-10-28 17:48:20]

Lei, for review.

[Lei Zhang, 2016-10-28 18:00:52]

https://codereview.chromium.org/2459073002/diff/1/core/fxcodec/jbig2/JBig2_Co...
File core/fxcodec/jbig2/JBig2_Context.cpp (right):

https://codereview.chromium.org/2459073002/diff/1/core/fxcodec/jbig2/JBig2_Co...
core/fxcodec/jbig2/JBig2_Context.cpp:33: size_t GetRefAggContextSize(int bVal) {
Why can't this be a bool?

https://codereview.chromium.org/2459073002/diff/1/core/fxge/win32/fx_win32_gd...
File core/fxge/win32/fx_win32_gdipext.cpp (right):

https://codereview.chromium.org/2459073002/diff/1/core/fxge/win32/fx_win32_gd...
core/fxge/win32/fx_win32_gdipext.cpp:455:
((FuncType_GdiRemoveFontMemResourceEx)m_pGdiRemoveFontMemResourseEx)(
On 2016/10/28 17:17:17, Tom Sepez wrote:
> note: this cast scares me, but for another day.

You may want to check to see if this actually gets called, and if the call stack
actually goes somewhere.

[Tom Sepez, 2016-10-28 18:04:44]

https://codereview.chromium.org/2459073002/diff/1/core/fxcodec/jbig2/JBig2_Co...
File core/fxcodec/jbig2/JBig2_Context.cpp (right):

https://codereview.chromium.org/2459073002/diff/1/core/fxcodec/jbig2/JBig2_Co...
core/fxcodec/jbig2/JBig2_Context.cpp:33: size_t GetRefAggContextSize(int bVal) {
On 2016/10/28 18:00:52, Lei Zhang wrote:
> Why can't this be a bool?

It's only called with things that are ints, so we'd be sticking a bunch of !!s
in several places.  Would you prefer that?

[Tom Sepez, 2016-10-28 18:06:27]

https://codereview.chromium.org/2459073002/diff/1/core/fxge/win32/fx_win32_gd...
File core/fxge/win32/fx_win32_gdipext.cpp (right):

https://codereview.chromium.org/2459073002/diff/1/core/fxge/win32/fx_win32_gd...
core/fxge/win32/fx_win32_gdipext.cpp:455:
((FuncType_GdiRemoveFontMemResourceEx)m_pGdiRemoveFontMemResourseEx)(
On 2016/10/28 18:00:52, Lei Zhang wrote:
> On 2016/10/28 17:17:17, Tom Sepez wrote:
> > note: this cast scares me, but for another day.
> 
> You may want to check to see if this actually gets called, and if the call
stack
> actually goes somewhere.

Fixed in a follow-up CL.

[Lei Zhang, 2016-10-28 18:09:50]

https://codereview.chromium.org/2459073002/diff/1/core/fxcodec/jbig2/JBig2_Co...
File core/fxcodec/jbig2/JBig2_Context.cpp (right):

https://codereview.chromium.org/2459073002/diff/1/core/fxcodec/jbig2/JBig2_Co...
core/fxcodec/jbig2/JBig2_Context.cpp:33: size_t GetRefAggContextSize(int bVal) {
On 2016/10/28 18:04:44, Tom Sepez wrote:
> On 2016/10/28 18:00:52, Lei Zhang wrote:
> > Why can't this be a bool?
> 
> It's only called with things that are ints, so we'd be sticking a bunch of !!s
> in several places.  Would you prefer that?

All those ints can be converted to bools: GRTEMPLATE, SBRTEMPLATE, and
SDRTEMPLATE. Want to leave this one alone and do a separate CL?

[Tom Sepez, 2016-10-28 18:13:26]

On 2016/10/28 18:09:50, Lei Zhang wrote:
>
https://codereview.chromium.org/2459073002/diff/1/core/fxcodec/jbig2/JBig2_Co...
> File core/fxcodec/jbig2/JBig2_Context.cpp (right):
> 
>
https://codereview.chromium.org/2459073002/diff/1/core/fxcodec/jbig2/JBig2_Co...
> core/fxcodec/jbig2/JBig2_Context.cpp:33: size_t GetRefAggContextSize(int bVal)
{
> On 2016/10/28 18:04:44, Tom Sepez wrote:
> > On 2016/10/28 18:00:52, Lei Zhang wrote:
> > > Why can't this be a bool?
> > 
> > It's only called with things that are ints, so we'd be sticking a bunch of
!!s
> > in several places.  Would you prefer that?
> 
> All those ints can be converted to bools: GRTEMPLATE, SBRTEMPLATE, and
> SDRTEMPLATE. Want to leave this one alone and do a separate CL?

Well, no, because I think GRTEMPLATE, SBRTEMPLATE, SDRTEMPLATE get used in int
expressions, which generates its own noise.

[Tom Sepez, 2016-10-28 18:16:26]

On 2016/10/28 18:13:26, Tom Sepez wrote:
> On 2016/10/28 18:09:50, Lei Zhang wrote:
> >
>
https://codereview.chromium.org/2459073002/diff/1/core/fxcodec/jbig2/JBig2_Co...
> > File core/fxcodec/jbig2/JBig2_Context.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2459073002/diff/1/core/fxcodec/jbig2/JBig2_Co...
> > core/fxcodec/jbig2/JBig2_Context.cpp:33: size_t GetRefAggContextSize(int
bVal)
> {
> > On 2016/10/28 18:04:44, Tom Sepez wrote:
> > > On 2016/10/28 18:00:52, Lei Zhang wrote:
> > > > Why can't this be a bool?
> > > 
> > > It's only called with things that are ints, so we'd be sticking a bunch of
> !!s
> > > in several places.  Would you prefer that?
> > 
> > All those ints can be converted to bools: GRTEMPLATE, SBRTEMPLATE, and
> > SDRTEMPLATE. Want to leave this one alone and do a separate CL?
> 
> Well, no, because I think GRTEMPLATE, SBRTEMPLATE, SDRTEMPLATE get used in int
> expressions, which generates its own noise.

https://cs.chromium.org/chromium/src/third_party/pdfium/core/fxcodec/jbig2/JB...
puts two bits into this value, for example

[Lei Zhang, 2016-10-28 18:18:35]

On 2016/10/28 18:16:26, Tom Sepez wrote:
>
https://cs.chromium.org/chromium/src/third_party/pdfium/core/fxcodec/jbig2/JB...
> puts two bits into this value, for example

Ya, but I don't see the 2 bits being used for anything other than an on/off
switch.

[Tom Sepez, 2016-10-28 18:19:17]

On 2016/10/28 18:18:35, Lei Zhang wrote:
> On 2016/10/28 18:16:26, Tom Sepez wrote:
> >
>
https://cs.chromium.org/chromium/src/third_party/pdfium/core/fxcodec/jbig2/JB...
> > puts two bits into this value, for example
> 
> Ya, but I don't see the 2 bits being used for anything other than an on/off
> switch.

Ok, Follow-up.  Makes sense.

[Lei Zhang, 2016-10-28 18:20:08]

On 2016/10/28 18:19:17, Tom Sepez wrote:
> Ok, Follow-up.  Makes sense.

I already started on it just to sanity check my statement. Drop the
JBig2_Context.cpp change in this CL then? Everything else lgtm.

[Tom Sepez, 2016-10-28 18:21:39]

On 2016/10/28 18:20:08, Lei Zhang wrote:
> On 2016/10/28 18:19:17, Tom Sepez wrote:
> > Ok, Follow-up.  Makes sense.
> 
> I already started on it just to sanity check my statement. Drop the
> JBig2_Context.cpp change in this CL then? Everything else lgtm.

Dropped.

[Tom Sepez, 2016-10-28 18:21:43]

The CQ bit was checked by tsepez@chromium.org

[Tom Sepez, 2016-10-28 18:21:44]

The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/2459073002/#ps40001
(title: "Drop file")

[commit-bot: I haz the power, 2016-10-28 18:21:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-28 18:34:45]

Description was changed from

==========
Continue fixing FX_BOOL / int noise
==========

to

==========
Continue fixing FX_BOOL / int noise

Committed:
https://pdfium.googlesource.com/pdfium/+/95e5834ab6b6e5aa370cacd6266cecbd82c4...
==========

[commit-bot: I haz the power, 2016-10-28 18:34:46]

Committed patchset #3 (id:40001) as
https://pdfium.googlesource.com/pdfium/+/95e5834ab6b6e5aa370cacd6266cecbd82c4...