Re-enable warning 4201 for xfa and other clean up

Re-enable warning 4201 for xfa and other clean up

Re-enable the warning by naming the unnamed structs. Also clean up
fx_graphics files.

BUG=pdfium:29
R=tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/f89acbcf5c652dc2eb3509568b3a5a57cb4c4c5e

[Wei Li, 2016-03-12 01:30:44]

Description was changed from

==========
Re-enable warning 4201 for xfa and clean up

Ren-enable the warning by name the unnamed structs. Also clean up
fx_graphics files.

BUG=pdfium:29
==========

to

==========
Re-enable warning 4201 for xfa and other clean up

Re-enable the warning by name the unnamed structs. Also clean up
fx_graphics files.

BUG=pdfium:29
==========

[Wei Li, 2016-03-12 01:47:02]

Patchset #1 (id:1) has been deleted

[Wei Li, 2016-03-12 01:49:30]

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

[Wei Li, 2016-03-12 01:49:30]

A lot of clean up. Discovered another bug: the first  CFX_Graphics::Transfer()
never return 'succeeded'. Fortunately, it is not used.

PTAL, thanks.

[Wei Li, 2016-03-12 01:51:01]

Description was changed from

==========
Re-enable warning 4201 for xfa and other clean up

Re-enable the warning by name the unnamed structs. Also clean up
fx_graphics files.

BUG=pdfium:29
==========

to

==========
Re-enable warning 4201 for xfa and other clean up

Re-enable the warning by naming the unnamed structs. Also clean up
fx_graphics files.

BUG=pdfium:29
==========

[Tom Sepez, 2016-03-14 16:43:50]

https://codereview.chromium.org/1784323006/diff/20001/xfa/src/fxgraphics/fx_g...
File xfa/src/fxgraphics/fx_graphics.cpp (right):

https://codereview.chromium.org/1784323006/diff/20001/xfa/src/fxgraphics/fx_g...
xfa/src/fxgraphics/fx_graphics.cpp:16: m_info.graphState.SetDashCount(0);
Maybe struct TTINfo gets a ctor that does these initializations.

https://codereview.chromium.org/1784323006/diff/20001/xfa/src/fxgraphics/fx_g...
xfa/src/fxgraphics/fx_graphics.cpp:63: m_renderDevice = nullptr;
Do we need these?

https://codereview.chromium.org/1784323006/diff/20001/xfa/src/fxgraphics/fx_g...
xfa/src/fxgraphics/fx_graphics.cpp:78: int32_t deviceClass =
m_renderDevice->GetDeviceClass();
nit: local not needed.

https://codereview.chromium.org/1784323006/diff/20001/xfa/src/fxgraphics/fx_g...
xfa/src/fxgraphics/fx_graphics.cpp:97: info->graphState.Copy(m_info.graphState);
struct TInfo should have a copy ctor?

https://codereview.chromium.org/1784323006/diff/20001/xfa/src/fxgraphics/fx_g...
xfa/src/fxgraphics/fx_graphics.cpp:125:
m_info.graphState.Copy(info->graphState);
and an assignment operator?

https://codereview.chromium.org/1784323006/diff/20001/xfa/src/fxgraphics/fx_g...
xfa/src/fxgraphics/fx_graphics.cpp:136: delete info;
Maybe use a unique_ptr?

https://codereview.chromium.org/1784323006/diff/20001/xfa/src/fxgraphics/fx_g...
xfa/src/fxgraphics/fx_graphics.cpp:493: FX_BOOL result =
bmp.Create((int32_t)dstRect.width, (int32_t)dstRect.height,
nit: local not needed.

https://codereview.chromium.org/1784323006/diff/20001/xfa/src/fxgraphics/fx_g...
xfa/src/fxgraphics/fx_graphics.cpp:732: delete bmp2;
unique_ptr? then you can return at will.

https://codereview.chromium.org/1784323006/diff/20001/xfa/src/fxgraphics/fx_g...
xfa/src/fxgraphics/fx_graphics.cpp:764: delete bmp2;
ditto

https://codereview.chromium.org/1784323006/diff/20001/xfa/src/fxgraphics/fx_g...
xfa/src/fxgraphics/fx_graphics.cpp:1001: delete bmp2;
ditto

https://codereview.chromium.org/1784323006/diff/20001/xfa/src/fxgraphics/fx_g...
xfa/src/fxgraphics/fx_graphics.cpp:1249: CFX_Color::CFX_Color() {
nit: maybe initializer list for m_type while we're at it.

[Wei Li, 2016-03-14 19:47:45]

thanks for reviewing.

https://codereview.chromium.org/1784323006/diff/20001/xfa/src/fxgraphics/fx_g...
File xfa/src/fxgraphics/fx_graphics.cpp (right):

https://codereview.chromium.org/1784323006/diff/20001/xfa/src/fxgraphics/fx_g...
xfa/src/fxgraphics/fx_graphics.cpp:16: m_info.graphState.SetDashCount(0);
On 2016/03/14 16:43:49, Tom Sepez wrote:
> Maybe struct TTINfo gets a ctor that does these initializations.

Done.

https://codereview.chromium.org/1784323006/diff/20001/xfa/src/fxgraphics/fx_g...
xfa/src/fxgraphics/fx_graphics.cpp:63: m_renderDevice = nullptr;
On 2016/03/14 16:43:50, Tom Sepez wrote:
> Do we need these?

Done.

https://codereview.chromium.org/1784323006/diff/20001/xfa/src/fxgraphics/fx_g...
xfa/src/fxgraphics/fx_graphics.cpp:78: int32_t deviceClass =
m_renderDevice->GetDeviceClass();
On 2016/03/14 16:43:50, Tom Sepez wrote:
> nit: local not needed.

Done.

https://codereview.chromium.org/1784323006/diff/20001/xfa/src/fxgraphics/fx_g...
xfa/src/fxgraphics/fx_graphics.cpp:97: info->graphState.Copy(m_info.graphState);
On 2016/03/14 16:43:49, Tom Sepez wrote:
> struct TInfo should have a copy ctor?

Done.

https://codereview.chromium.org/1784323006/diff/20001/xfa/src/fxgraphics/fx_g...
xfa/src/fxgraphics/fx_graphics.cpp:125:
m_info.graphState.Copy(info->graphState);
On 2016/03/14 16:43:50, Tom Sepez wrote:
> and an assignment operator?

Done.

https://codereview.chromium.org/1784323006/diff/20001/xfa/src/fxgraphics/fx_g...
xfa/src/fxgraphics/fx_graphics.cpp:136: delete info;
On 2016/03/14 16:43:49, Tom Sepez wrote:
> Maybe use a unique_ptr?

Done.

https://codereview.chromium.org/1784323006/diff/20001/xfa/src/fxgraphics/fx_g...
xfa/src/fxgraphics/fx_graphics.cpp:493: FX_BOOL result =
bmp.Create((int32_t)dstRect.width, (int32_t)dstRect.height,
On 2016/03/14 16:43:49, Tom Sepez wrote:
> nit: local not needed.

Done.

https://codereview.chromium.org/1784323006/diff/20001/xfa/src/fxgraphics/fx_g...
xfa/src/fxgraphics/fx_graphics.cpp:732: delete bmp2;
On 2016/03/14 16:43:50, Tom Sepez wrote:
> unique_ptr? then you can return at will.

Done.

https://codereview.chromium.org/1784323006/diff/20001/xfa/src/fxgraphics/fx_g...
xfa/src/fxgraphics/fx_graphics.cpp:764: delete bmp2;
On 2016/03/14 16:43:50, Tom Sepez wrote:
> ditto

Done.

https://codereview.chromium.org/1784323006/diff/20001/xfa/src/fxgraphics/fx_g...
xfa/src/fxgraphics/fx_graphics.cpp:1001: delete bmp2;
On 2016/03/14 16:43:50, Tom Sepez wrote:
> ditto

Done.

https://codereview.chromium.org/1784323006/diff/20001/xfa/src/fxgraphics/fx_g...
xfa/src/fxgraphics/fx_graphics.cpp:1249: CFX_Color::CFX_Color() {
On 2016/03/14 16:43:49, Tom Sepez wrote:
> nit: maybe initializer list for m_type while we're at it.

Done.

[Tom Sepez, 2016-03-14 20:02:03]

lgtm

https://codereview.chromium.org/1784323006/diff/40001/xfa/include/fxgraphics/...
File xfa/include/fxgraphics/fx_graphics.h (right):

https://codereview.chromium.org/1784323006/diff/40001/xfa/include/fxgraphics/...
xfa/include/fxgraphics/fx_graphics.h:252: explicit TInfo(const TInfo& info);
Copy ctor shouldn't be marked explicit, only when the one and only arg is a
different type (which could get silently promoted).

[Wei Li, 2016-03-14 20:23:04]

thanks

https://codereview.chromium.org/1784323006/diff/40001/xfa/include/fxgraphics/...
File xfa/include/fxgraphics/fx_graphics.h (right):

https://codereview.chromium.org/1784323006/diff/40001/xfa/include/fxgraphics/...
xfa/include/fxgraphics/fx_graphics.h:252: explicit TInfo(const TInfo& info);
On 2016/03/14 20:02:03, Tom Sepez wrote:
> Copy ctor shouldn't be marked explicit, only when the one and only arg is a
> different type (which could get silently promoted).

'Explicit' forces the copy ctor not to be called implicitly such as passing
through argument, which I think we would prefer.

[Tom Sepez, 2016-03-14 20:30:06]

On 2016/03/14 20:23:04, Wei Li wrote:
> thanks
> 
>
https://codereview.chromium.org/1784323006/diff/40001/xfa/include/fxgraphics/...
> File xfa/include/fxgraphics/fx_graphics.h (right):
> 
>
https://codereview.chromium.org/1784323006/diff/40001/xfa/include/fxgraphics/...
> xfa/include/fxgraphics/fx_graphics.h:252: explicit TInfo(const TInfo& info);
> On 2016/03/14 20:02:03, Tom Sepez wrote:
> > Copy ctor shouldn't be marked explicit, only when the one and only arg is a
> > different type (which could get silently promoted).
> 
> 'Explicit' forces the copy ctor not to be called implicitly such as passing
> through argument, which I think we would prefer.

OK.

[Wei Li, 2016-03-14 22:10:57]

Description was changed from

==========
Re-enable warning 4201 for xfa and other clean up

Re-enable the warning by naming the unnamed structs. Also clean up
fx_graphics files.

BUG=pdfium:29
==========

to

==========
Re-enable warning 4201 for xfa and other clean up

Re-enable the warning by naming the unnamed structs. Also clean up
fx_graphics files.

BUG=pdfium:29
R=tsepez@chromium.org

Committed:
https://pdfium.googlesource.com/pdfium/+/f89acbcf5c652dc2eb3509568b3a5a57cb4c...
==========

[Wei Li, 2016-03-14 22:10:59]

Committed patchset #4 (id:80001) manually as
f89acbcf5c652dc2eb3509568b3a5a57cb4c4c5e (presubmit successful).

[Tom Sepez, 2016-03-22 21:02:30]

On 2016/03/14 22:10:59, Wei Li wrote:
> Committed patchset #4 (id:80001) manually as
> f89acbcf5c652dc2eb3509568b3a5a57cb4c4c5e (presubmit successful).

Bisecting show we broke something with this commit.  Rendering the PDF from bug
434 is blank after this change.
I'll see if I cant figure it out.  Thanks.