Fix partial information printed with blank window covering the center

Fix partial information printed with blank window covering the center

BUG=482206
R=thestig@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/8be8a4452a947ce125dd1feea00cc9fd23714ec6

[jun_fang, 2015-12-28 07:46:11]

jun_fang@foxitsoftware.com changed reviewers:
+ thestig@chromium.org, tsepez@chromium.org

[jun_fang, 2015-12-28 07:46:12]

Hi Tom and Lei,

Please review this CL. Thanks!

[jun_fang, 2015-12-28 08:23:06]

On 2015/12/28 07:46:12, jun_fang wrote:
> Hi Tom and Lei,
> 
> Please review this CL. Thanks!

If a page has one more image masks, we need to render this page
to a bitmap rather than render it to windows DC directly. The 
reason is because windows DC can't handle transparency. At the
beginning, I investigated this issue in a wrong direction.

[Lei Zhang, 2015-12-28 19:08:18]

lgtm

https://codereview.chromium.org/1551563002/diff/20001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp (right):

https://codereview.chromium.org/1551563002/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp:713: if
(!m_pObjectList->m_bHasImageMask && m_pLastImage->IsMask())
Can't this block reduce down to the following?

m_pObjectList->m_bHasImageMask = m_pLastImage->IsMask();

[jun_fang, 2015-12-28 23:33:05]

https://codereview.chromium.org/1551563002/diff/20001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp (right):

https://codereview.chromium.org/1551563002/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp:713: if
(!m_pObjectList->m_bHasImageMask && m_pLastImage->IsMask())
On 2015/12/28 19:08:18, Lei Zhang wrote:
> Can't this block reduce down to the following?
> 
> m_pObjectList->m_bHasImageMask = m_pLastImage->IsMask();

Do you prefer the following change?

if (!m_pObjectList->m_bHasImageMask)
  m_pObjectList->m_bHasImageMask = m_pLastImage->IsMask();

[Lei Zhang, 2015-12-28 23:42:57]

https://codereview.chromium.org/1551563002/diff/20001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp (right):

https://codereview.chromium.org/1551563002/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp:713: if
(!m_pObjectList->m_bHasImageMask && m_pLastImage->IsMask())
On 2015/12/28 23:33:05, jun_fang wrote:
> On 2015/12/28 19:08:18, Lei Zhang wrote:
> > Can't this block reduce down to the following?
> > 
> > m_pObjectList->m_bHasImageMask = m_pLastImage->IsMask();
> 
> Do you prefer the following change?
> 
> if (!m_pObjectList->m_bHasImageMask)
>   m_pObjectList->m_bHasImageMask = m_pLastImage->IsMask();

Sure, I retract my suggestion because it can incorrectly set the
|m_bHasImageMask| value to false.

[jun_fang, 2015-12-28 23:55:58]

On 2015/12/28 23:42:57, Lei Zhang wrote:
>
https://codereview.chromium.org/1551563002/diff/20001/core/src/fpdfapi/fpdf_p...
> File core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp (right):
> 
>
https://codereview.chromium.org/1551563002/diff/20001/core/src/fpdfapi/fpdf_p...
> core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp:713: if
> (!m_pObjectList->m_bHasImageMask && m_pLastImage->IsMask())
> On 2015/12/28 23:33:05, jun_fang wrote:
> > On 2015/12/28 19:08:18, Lei Zhang wrote:
> > > Can't this block reduce down to the following?
> > > 
> > > m_pObjectList->m_bHasImageMask = m_pLastImage->IsMask();
> > 
> > Do you prefer the following change?
> > 
> > if (!m_pObjectList->m_bHasImageMask)
> >   m_pObjectList->m_bHasImageMask = m_pLastImage->IsMask();
> 
> Sure, I retract my suggestion because it can incorrectly set the
> |m_bHasImageMask| value to false.

There is no problem if we have a test on |m_bHasImageMask|.
We try to set the |m_bHasImageMask| value only when it's false.

[Lei Zhang, 2015-12-29 00:11:27]

On 2015/12/28 23:55:58, jun_fang wrote:
> There is no problem if we have a test on |m_bHasImageMask|.
> We try to set the |m_bHasImageMask| value only when it's false.

Yes, I agree, so I do prefer your suggested change.

[jun_fang, 2015-12-29 02:13:21]

On 2015/12/29 00:11:27, Lei Zhang wrote:
> On 2015/12/28 23:55:58, jun_fang wrote:
> > There is no problem if we have a test on |m_bHasImageMask|.
> > We try to set the |m_bHasImageMask| value only when it's false.
> 
> Yes, I agree, so I do prefer your suggested change.

Done.

[jun_fang, 2015-12-29 02:13:28]

https://codereview.chromium.org/1551563002/diff/20001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp (right):

https://codereview.chromium.org/1551563002/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp:713: if
(!m_pObjectList->m_bHasImageMask && m_pLastImage->IsMask())
On 2015/12/28 23:42:57, Lei Zhang wrote:
> On 2015/12/28 23:33:05, jun_fang wrote:
> > On 2015/12/28 19:08:18, Lei Zhang wrote:
> > > Can't this block reduce down to the following?
> > > 
> > > m_pObjectList->m_bHasImageMask = m_pLastImage->IsMask();
> > 
> > Do you prefer the following change?
> > 
> > if (!m_pObjectList->m_bHasImageMask)
> >   m_pObjectList->m_bHasImageMask = m_pLastImage->IsMask();
> 
> Sure, I retract my suggestion because it can incorrectly set the
> |m_bHasImageMask| value to false.

Done.

[jun_fang, 2015-12-29 02:18:53]

Description was changed from

==========
Fix partial information printed with blank window covering the center

BUG=482206
==========

to

==========
Fix partial information printed with blank window covering the center

BUG=482206
R=thestig@chromium.org

Committed:
https://pdfium.googlesource.com/pdfium/+/8be8a4452a947ce125dd1feea00cc9fd2371...
==========

[jun_fang, 2015-12-29 02:18:57]

Committed patchset #3 (id:40001) manually as
8be8a4452a947ce125dd1feea00cc9fd23714ec6 (presubmit successful).