Fix Undefined-shift in CPDF_SampledFunc::v_Init().

Fix Undefined-shift in CPDF_SampledFunc::v_Init().

Also fix a divide by zero in CPDF_SampledFunc.

Do some cleanups too.

BUG=596530, 613032
Committed: https://pdfium.googlesource.com/pdfium/+/9b1a0ee2a8f24411609a2f7554119597950dbd04

[Lei Zhang, 2016-05-19 07:00:16]

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

[Lei Zhang, 2016-05-19 08:35:25]

Description was changed from

==========
Fix Undefined-shift in CPDF_SampledFunc::v_Init().

Do some cleanups too.

BUG=613032
==========

to

==========
Fix Undefined-shift in CPDF_SampledFunc::v_Init().

Also fix a divide by zero in CPDF_SampledFunc.

Do some cleanups too.

BUG=596530,613032
==========

[Tom Sepez, 2016-05-19 16:53:40]

https://codereview.chromium.org/1990843004/diff/100001/core/fpdfapi/fpdf_page...
File core/fpdfapi/fpdf_page/fpdf_page_func.cpp (right):

https://codereview.chromium.org/1990843004/diff/100001/core/fpdfapi/fpdf_page...
core/fpdfapi/fpdf_page/fpdf_page_func.cpp:507: uint32_t GetBits32(const uint8_t*
pData, int bitpos, int nbits) {
This has got to be mind-numbingly slow, consider making a better version.  I
think Cary has coded/tested the same thing more tightly in
https://codereview.chromium.org/1870463002 , please consolidate into fxcrt/
somewhere.

https://codereview.chromium.org/1990843004/diff/100001/core/fpdfapi/fpdf_page...
core/fpdfapi/fpdf_page/fpdf_page_func.cpp:821: else if (type == 4)
Shouldn't this be kTypeFourPostScript and friends?  Maybe move the enum into the
.cpp file? Or a link to the pdf spec were it says "1" instead of something saner
so that using "1" matches the spec.

[Tom Sepez, 2016-05-19 16:58:48]

https://codereview.chromium.org/1990843004/diff/100001/core/fpdfapi/fpdf_page...
File core/fpdfapi/fpdf_page/fpdf_page_func.cpp (right):

https://codereview.chromium.org/1990843004/diff/100001/core/fpdfapi/fpdf_page...
core/fpdfapi/fpdf_page/fpdf_page_func.cpp:569: 0xffffffff >> (m_nBitsPerSample
== 32 ? 0 : 32 - m_nBitsPerSample);
Not groking why we need the ? operator, isn't 32 - 32 == 0  ?  Did you mean >=
32?

[Lei Zhang, 2016-05-19 21:16:19]

https://codereview.chromium.org/1990843004/diff/100001/core/fpdfapi/fpdf_page...
File core/fpdfapi/fpdf_page/fpdf_page_func.cpp (right):

https://codereview.chromium.org/1990843004/diff/100001/core/fpdfapi/fpdf_page...
core/fpdfapi/fpdf_page/fpdf_page_func.cpp:507: uint32_t GetBits32(const uint8_t*
pData, int bitpos, int nbits) {
On 2016/05/19 16:53:40, Tom Sepez wrote:
> This has got to be mind-numbingly slow, consider making a better version.  I
> think Cary has coded/tested the same thing more tightly in
> https://codereview.chromium.org/1870463002 , please consolidate into fxcrt/
> somewhere.

Ack. Will merge with the fx_skia_device.cpp copy in another CL.

https://codereview.chromium.org/1990843004/diff/100001/core/fpdfapi/fpdf_page...
core/fpdfapi/fpdf_page/fpdf_page_func.cpp:569: 0xffffffff >> (m_nBitsPerSample
== 32 ? 0 : 32 - m_nBitsPerSample);
On 2016/05/19 16:58:48, Tom Sepez wrote:
> Not groking why we need the ? operator, isn't 32 - 32 == 0  ?  Did you mean >=
> 32?

Uh, I'm being silly. It was the m_nBitsPerSample == 0 case that I was worried
about, but now that does not even reach here.

https://codereview.chromium.org/1990843004/diff/100001/core/fpdfapi/fpdf_page...
core/fpdfapi/fpdf_page/fpdf_page_func.cpp:821: else if (type == 4)
On 2016/05/19 16:53:40, Tom Sepez wrote:
> Shouldn't this be kTypeFourPostScript and friends?  Maybe move the enum into
the
> .cpp file? Or a link to the pdf spec were it says "1" instead of something
saner
> so that using "1" matches the spec.

I put CPDF_Function::Type back. It's not obvious where else we are using ints to
represent this enum, so leaving it in the header for now.

[Tom Sepez, 2016-05-19 21:31:46]

... and windows warns about the same thing I just suggested.  LGTM once that's
fixed.

https://codereview.chromium.org/1990843004/diff/140001/core/fpdfapi/fpdf_page...
File core/fpdfapi/fpdf_page/fpdf_page_func.cpp (right):

https://codereview.chromium.org/1990843004/diff/140001/core/fpdfapi/fpdf_page...
core/fpdfapi/fpdf_page/fpdf_page_func.cpp:568: m_SampleMax = 0xffffffff >> 32 -
m_nBitsPerSample;
nit: I'd be tempted to write >> (32 - m_nBitsPerSample); just because folks can
never remember the >> precedence.  One rare case where I'd opt for
overparenthesization.

[Lei Zhang, 2016-05-19 21:58:23]

https://codereview.chromium.org/1990843004/diff/140001/core/fpdfapi/fpdf_page...
File core/fpdfapi/fpdf_page/fpdf_page_func.cpp (right):

https://codereview.chromium.org/1990843004/diff/140001/core/fpdfapi/fpdf_page...
core/fpdfapi/fpdf_page/fpdf_page_func.cpp:568: m_SampleMax = 0xffffffff >> 32 -
m_nBitsPerSample;
On 2016/05/19 21:31:46, Tom Sepez wrote:
> nit: I'd be tempted to write >> (32 - m_nBitsPerSample); just because folks
can
> never remember the >> precedence.  One rare case where I'd opt for
> overparenthesization.

Whoops. Sadness.

[Lei Zhang, 2016-05-19 22:32:35]

The CQ bit was checked by thestig@chromium.org

[Lei Zhang, 2016-05-19 22:32:35]

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/1990843004/#ps160001
(title: "compile")

[commit-bot: I haz the power, 2016-05-19 22:32:45]

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

[commit-bot: I haz the power, 2016-05-19 22:47:03]

Description was changed from

==========
Fix Undefined-shift in CPDF_SampledFunc::v_Init().

Also fix a divide by zero in CPDF_SampledFunc.

Do some cleanups too.

BUG=596530,613032
==========

to

==========
Fix Undefined-shift in CPDF_SampledFunc::v_Init().

Also fix a divide by zero in CPDF_SampledFunc.

Do some cleanups too.

BUG=596530,613032

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

[commit-bot: I haz the power, 2016-05-19 22:47:04]

Committed patchset #9 (id:160001) as
https://pdfium.googlesource.com/pdfium/+/9b1a0ee2a8f24411609a2f7554119597950d...

[Lei Zhang, 2016-05-19 23:02:28]

Oh, I think I broke Skia. Sorry.

[Lei Zhang, 2016-05-19 23:03:32]

On 2016/05/19 23:02:28, Lei Zhang wrote:
> Oh, I think I broke Skia. Sorry.

That is to say, let me whip up a CL to remedy the situation, rather than
reverting.

[Lei Zhang, 2016-05-19 23:36:53]

On 2016/05/19 23:03:32, Lei Zhang wrote:
> On 2016/05/19 23:02:28, Lei Zhang wrote:
> > Oh, I think I broke Skia. Sorry.
> 
> That is to say, let me whip up a CL to remedy the situation, rather than
> reverting.

Follow up: https://codereview.chromium.org/1996533004