PDF: Add a bunch of DCHECKs to make sure we do not go out of bounds.

PDF: Add a bunch of DCHECKs to make sure we do not go out of bounds.

Committed: https://crrev.com/9660cfaaac3ffc1b2f79efce49af7ae73cab9d60
Cr-Commit-Position: refs/heads/master@{#312744}

[Lei Zhang, 2015-01-22 01:38:50]

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

[raymes, 2015-01-22 22:50:03]

lgtm

https://codereview.chromium.org/863043003/diff/1/pdf/pdfium/pdfium_engine.cc
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/863043003/diff/1/pdf/pdfium/pdfium_engine.cc#...
pdf/pdfium/pdfium_engine.cc:1057: DCHECK(pending);
Are these actually useful? I've heard various opinions but some people say that
if something is NULL when it's not supposed to be then there is no real
advantage to adding a DCHECK as a crash with a useful error should appear on the
line where it is first accessed.

With that said, I'm ok to add these iyl.

https://codereview.chromium.org/863043003/diff/1/pdf/pdfium/pdfium_engine.cc#...
pdf/pdfium/pdfium_engine.cc:1091: if (progressive == -1) {
can we make this an "else" branch of the previous if?

https://codereview.chromium.org/863043003/diff/1/pdf/pdfium/pdfium_engine.cc#...
pdf/pdfium/pdfium_engine.cc:1095: progressive_paint_timeout_ =
kMaxProgressivePaintTimeMs;
Can this go into the previous if-statement?

[Lei Zhang, 2015-01-22 23:00:09]

https://codereview.chromium.org/863043003/diff/1/pdf/pdfium/pdfium_engine.cc
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/863043003/diff/1/pdf/pdfium/pdfium_engine.cc#...
pdf/pdfium/pdfium_engine.cc:1057: DCHECK(pending);
On 2015/01/22 22:50:03, raymes wrote:
> Are these actually useful? I've heard various opinions but some people say
that
> if something is NULL when it's not supposed to be then there is no real
> advantage to adding a DCHECK as a crash with a useful error should appear on
the
> line where it is first accessed.
> 
> With that said, I'm ok to add these iyl.

In the case of:

DCHECK(ptr_var);
ptr_var->Method();

I would also just let it crash and omit the DCHECK(). When put at the top of the
file, they serve as a sanity check and as a form of documentation. If I see this
at the top of a function, I would know/expect the function to use the pointers
without checking if they are null.

https://codereview.chromium.org/863043003/diff/1/pdf/pdfium/pdfium_engine.cc#...
pdf/pdfium/pdfium_engine.cc:1091: if (progressive == -1) {
On 2015/01/22 22:50:03, raymes wrote:
> can we make this an "else" branch of the previous if?

I thought about it, but I can also see the argument that this is the "next step"
so it should be separate.

[Lei Zhang, 2015-01-23 01:06:17]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2015-01-23 01:09:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/863043003/1

[commit-bot: I haz the power, 2015-01-23 03:08:56]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-01-23 03:10:40]

Patchset 1 (id:??) landed as
https://crrev.com/9660cfaaac3ffc1b2f79efce49af7ae73cab9d60
Cr-Commit-Position: refs/heads/master@{#312744}