Move xfa/include/fxgraphics/fx_graphics.h to xfa/fxgraphics.

Move xfa/include/fxgraphics/fx_graphics.h to xfa/fxgraphics.

This Cl moves and splits apart the fx_graphics.h file into individual classes.
The .cpp files are renamed to match the .h files. pre.h was removed at the
same time and its contents moved to the correct places as needed.

R=tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/811b8a4f4482bb5c11ac7610e70a4c8fd34c2907

[dsinclair, 2016-03-16 14:37:37]

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

[dsinclair, 2016-03-16 14:37:38]

PTAL.

[Tom Sepez, 2016-03-16 17:08:53]

LGTM with nit and a bunch of comments for future work.

https://codereview.chromium.org/1810563002/diff/1/xfa/fxgraphics/cfx_graphics...
File xfa/fxgraphics/cfx_graphics.cpp (right):

https://codereview.chromium.org/1810563002/diff/1/xfa/fxgraphics/cfx_graphics...
xfa/fxgraphics/cfx_graphics.cpp:33: static const FX_HATCHDATA
hatchBitmapData[FX_HATCHSTYLE_Total] = {
static not needed since namespaced.

https://codereview.chromium.org/1810563002/diff/1/xfa/fxgraphics/cfx_pattern.h
File xfa/fxgraphics/cfx_pattern.h (right):

https://codereview.chromium.org/1810563002/diff/1/xfa/fxgraphics/cfx_pattern....
xfa/fxgraphics/cfx_pattern.h:17: enum { FX_PATTERN_None = 0, FX_PATTERN_Bitmap,
FX_PATTERN_Hatch };
Anon enum?  Is this the value used for |int32_t m_type|? can it be nested in the
class as private, named, and the int32_t replaced with the enum?

https://codereview.chromium.org/1810563002/diff/1/xfa/fxgraphics/cfx_pattern....
xfa/fxgraphics/cfx_pattern.h:36: int32_t m_type;
can this be const if the ctor took the type as an argument? Looking at the .cpp,
it might be a lot of changes.

https://codereview.chromium.org/1810563002/diff/1/xfa/fxgraphics/cfx_shading.h
File xfa/fxgraphics/cfx_shading.h (right):

https://codereview.chromium.org/1810563002/diff/1/xfa/fxgraphics/cfx_shading....
xfa/fxgraphics/cfx_shading.h:17: enum { FX_SHADING_None = 0, FX_SHADING_Axial,
FX_SHADING_Radial };
ditto for type here.

https://codereview.chromium.org/1810563002/diff/1/xfa/fxgraphics/include/cfx_...
File xfa/fxgraphics/include/cfx_graphics.h (right):

https://codereview.chromium.org/1810563002/diff/1/xfa/fxgraphics/include/cfx_...
xfa/fxgraphics/include/cfx_graphics.h:26: typedef int FX_ERR;
nit: use "using" while we're at it.
Can't some of these be enums in reality?

https://codereview.chromium.org/1810563002/diff/1/xfa/fxgraphics/include/cfx_...
xfa/fxgraphics/include/cfx_graphics.h:33: enum {
anon enum?

[dsinclair, 2016-03-16 19:21:08]

PTAL to diff between ps1 and ps2.

https://codereview.chromium.org/1810563002/diff/1/xfa/fxgraphics/cfx_graphics...
File xfa/fxgraphics/cfx_graphics.cpp (right):

https://codereview.chromium.org/1810563002/diff/1/xfa/fxgraphics/cfx_graphics...
xfa/fxgraphics/cfx_graphics.cpp:33: static const FX_HATCHDATA
hatchBitmapData[FX_HATCHSTYLE_Total] = {
On 2016/03/16 17:08:52, Tom Sepez wrote:
> static not needed since namespaced.

Done.

https://codereview.chromium.org/1810563002/diff/1/xfa/fxgraphics/cfx_pattern.h
File xfa/fxgraphics/cfx_pattern.h (right):

https://codereview.chromium.org/1810563002/diff/1/xfa/fxgraphics/cfx_pattern....
xfa/fxgraphics/cfx_pattern.h:17: enum { FX_PATTERN_None = 0, FX_PATTERN_Bitmap,
FX_PATTERN_Hatch };
On 2016/03/16 17:08:52, Tom Sepez wrote:
> Anon enum?  Is this the value used for |int32_t m_type|? can it be nested in
the
> class as private, named, and the int32_t replaced with the enum?

I removed the enum. The CFX_DIBitmap override is never called. We only call the
default constructor once, from xfa_ffwidget.cpp and that immediately calls
::Create with a FX_HatchStyle.

So, it can only ever be a hatch style.

https://codereview.chromium.org/1810563002/diff/1/xfa/fxgraphics/cfx_pattern....
xfa/fxgraphics/cfx_pattern.h:36: int32_t m_type;
On 2016/03/16 17:08:52, Tom Sepez wrote:
> can this be const if the ctor took the type as an argument? Looking at the
.cpp,
> it might be a lot of changes.

Ack.

https://codereview.chromium.org/1810563002/diff/1/xfa/fxgraphics/cfx_shading.h
File xfa/fxgraphics/cfx_shading.h (right):

https://codereview.chromium.org/1810563002/diff/1/xfa/fxgraphics/cfx_shading....
xfa/fxgraphics/cfx_shading.h:17: enum { FX_SHADING_None = 0, FX_SHADING_Axial,
FX_SHADING_Radial };
On 2016/03/16 17:08:52, Tom Sepez wrote:
> ditto for type here.

Done.

https://codereview.chromium.org/1810563002/diff/1/xfa/fxgraphics/include/cfx_...
File xfa/fxgraphics/include/cfx_graphics.h (right):

https://codereview.chromium.org/1810563002/diff/1/xfa/fxgraphics/include/cfx_...
xfa/fxgraphics/include/cfx_graphics.h:26: typedef int FX_ERR;
On 2016/03/16 17:08:52, Tom Sepez wrote:
> nit: use "using" while we're at it.
> Can't some of these be enums in reality?

Done.

https://codereview.chromium.org/1810563002/diff/1/xfa/fxgraphics/include/cfx_...
xfa/fxgraphics/include/cfx_graphics.h:33: enum {
On 2016/03/16 17:08:52, Tom Sepez wrote:
> anon enum?

Named as the types above.

[Tom Sepez, 2016-03-16 19:38:32]

https://codereview.chromium.org/1810563002/diff/20001/xfa/fwl/theme/widgettp.cpp
File xfa/fwl/theme/widgettp.cpp (right):

https://codereview.chromium.org/1810563002/diff/20001/xfa/fwl/theme/widgettp....
xfa/fwl/theme/widgettp.cpp:457: CFX_Shading shading(FX_SHADING_Axial);
Argh, ok, I'm wrong.  This now gives the possibility of a mismatch between the
ctor and the create method, which I really dislike.

https://codereview.chromium.org/1810563002/diff/20001/xfa/fxgraphics/cfx_patt...
File xfa/fxgraphics/cfx_pattern.h (right):

https://codereview.chromium.org/1810563002/diff/20001/xfa/fxgraphics/cfx_patt...
xfa/fxgraphics/cfx_pattern.h:22: FX_ERR Create(FX_HatchStyle hatchStyle,
Does this ever fail?  Do we check the error if it does?  Could this be the ctor
instead?

https://codereview.chromium.org/1810563002/diff/20001/xfa/fxgraphics/cfx_patt...
xfa/fxgraphics/cfx_pattern.h:31: union {
Are some of these cases unused now that the type can't differ?

https://codereview.chromium.org/1810563002/diff/20001/xfa/fxgraphics/cfx_shad...
File xfa/fxgraphics/cfx_shading.h (right):

https://codereview.chromium.org/1810563002/diff/20001/xfa/fxgraphics/cfx_shad...
xfa/fxgraphics/cfx_shading.h:21: explicit CFX_Shading(CFX_Shading_Type type);
See previous comment about mismatch between this and the subsequent create
command.  

Do the create methods ever fail?  Could they be ctors instead?

[dsinclair, 2016-03-16 19:59:16]

https://codereview.chromium.org/1810563002/diff/20001/xfa/fwl/theme/widgettp.cpp
File xfa/fwl/theme/widgettp.cpp (right):

https://codereview.chromium.org/1810563002/diff/20001/xfa/fwl/theme/widgettp....
xfa/fwl/theme/widgettp.cpp:457: CFX_Shading shading(FX_SHADING_Axial);
On 2016/03/16 19:38:31, Tom Sepez wrote:
> Argh, ok, I'm wrong.  This now gives the possibility of a mismatch between the
> ctor and the create method, which I really dislike.

Done.

https://codereview.chromium.org/1810563002/diff/20001/xfa/fxgraphics/cfx_patt...
File xfa/fxgraphics/cfx_pattern.h (right):

https://codereview.chromium.org/1810563002/diff/20001/xfa/fxgraphics/cfx_patt...
xfa/fxgraphics/cfx_pattern.h:22: FX_ERR Create(FX_HatchStyle hatchStyle,
On 2016/03/16 19:38:31, Tom Sepez wrote:
> Does this ever fail?  Do we check the error if it does?  Could this be the
ctor
> instead?

Done.

https://codereview.chromium.org/1810563002/diff/20001/xfa/fxgraphics/cfx_patt...
xfa/fxgraphics/cfx_pattern.h:31: union {
On 2016/03/16 19:38:31, Tom Sepez wrote:
> Are some of these cases unused now that the type can't differ?

Apparently m_rectInfo was never used, and we don't need m_bitmapInfo anymore.
Removed the union and just left the m_hatchInfo struct.

https://codereview.chromium.org/1810563002/diff/20001/xfa/fxgraphics/cfx_shad...
File xfa/fxgraphics/cfx_shading.h (right):

https://codereview.chromium.org/1810563002/diff/20001/xfa/fxgraphics/cfx_shad...
xfa/fxgraphics/cfx_shading.h:21: explicit CFX_Shading(CFX_Shading_Type type);
On 2016/03/16 19:38:32, Tom Sepez wrote:
> See previous comment about mismatch between this and the subsequent create
> command.  
> 
> Do the create methods ever fail?  Could they be ctors instead?

Done.

[Tom Sepez, 2016-03-16 20:53:18]

https://codereview.chromium.org/1810563002/diff/40001/xfa/fxgraphics/cfx_patt...
File xfa/fxgraphics/cfx_pattern.cpp (right):

https://codereview.chromium.org/1810563002/diff/40001/xfa/fxgraphics/cfx_patt...
xfa/fxgraphics/cfx_pattern.cpp:18: m_hatchInfo.hatchStyle = hatchStyle;
I'd move these to initializer list.

https://codereview.chromium.org/1810563002/diff/40001/xfa/fxgraphics/cfx_patt...
xfa/fxgraphics/cfx_pattern.cpp:22: m_matrix.Set(matrix->a, matrix->b, matrix->c,
matrix->d, matrix->e,
File a follow-up to make a Set(const CFX_Matrix& other) method.  Sheesh

https://codereview.chromium.org/1810563002/diff/40001/xfa/fxgraphics/cfx_patt...
File xfa/fxgraphics/cfx_pattern.h (right):

https://codereview.chromium.org/1810563002/diff/40001/xfa/fxgraphics/cfx_patt...
xfa/fxgraphics/cfx_pattern.h:30: struct {
And this struct can go, too, and the members move up a level.

https://codereview.chromium.org/1810563002/diff/40001/xfa/fxgraphics/cfx_patt...
xfa/fxgraphics/cfx_pattern.h:31: FX_HatchStyle hatchStyle;
can hatchstyle be const?

https://codereview.chromium.org/1810563002/diff/40001/xfa/fxgraphics/cfx_patt...
xfa/fxgraphics/cfx_pattern.h:33: FX_ARGB backArgb;
And these?

https://codereview.chromium.org/1810563002/diff/40001/xfa/fxgraphics/cfx_shad...
File xfa/fxgraphics/cfx_shading.cpp (right):

https://codereview.chromium.org/1810563002/diff/40001/xfa/fxgraphics/cfx_shad...
xfa/fxgraphics/cfx_shading.cpp:60: FX_FLOAT aScale = (FX_FLOAT)(1.0 * (a2 - a1)
/ f);
These casts are pointless as we assume an int's range fits in a float (modulo
loss of accurracy).

https://codereview.chromium.org/1810563002/diff/40001/xfa/fxgraphics/cfx_shad...
xfa/fxgraphics/cfx_shading.cpp:65: int32_t a3;
nit: move these inside loop.  They needn't persist across it.

https://codereview.chromium.org/1810563002/diff/40001/xfa/fxgraphics/cfx_shad...
xfa/fxgraphics/cfx_shading.cpp:70: a3 = (int32_t)(i * aScale);
nit: static_cast<>, just to emphasize that we think the float will fit.

https://codereview.chromium.org/1810563002/diff/40001/xfa/fxgraphics/cfx_shad...
xfa/fxgraphics/cfx_shading.cpp:73: b3 = (int32_t)(i * bScale);
Pity we dont define addition multiplicaton subtraction and division for argbs.

https://codereview.chromium.org/1810563002/diff/40001/xfa/fxgraphics/cfx_shad...
xfa/fxgraphics/cfx_shading.cpp:75: FXARGB_TODIB(FXARGB_MAKE((a1 + a3), (r1 +
r3), (g1 + g3), (b1 + b3)));
nit: overparenthesized, unless FXARGB_MAKE is not careful with argument
expansion.

https://codereview.chromium.org/1810563002/diff/40001/xfa/fxgraphics/cfx_shad...
File xfa/fxgraphics/cfx_shading.h (right):

https://codereview.chromium.org/1810563002/diff/40001/xfa/fxgraphics/cfx_shad...
xfa/fxgraphics/cfx_shading.h:20: CFX_Shading(const CFX_PointF& beginPoint,
add comment // Axial shading.

https://codereview.chromium.org/1810563002/diff/40001/xfa/fxgraphics/cfx_shad...
xfa/fxgraphics/cfx_shading.h:26: CFX_Shading(const CFX_PointF& beginPoint,
add comment // Radial shading.

https://codereview.chromium.org/1810563002/diff/40001/xfa/fxgraphics/cfx_shad...
xfa/fxgraphics/cfx_shading.h:42: CFX_PointF m_beginPoint;
any more of these consts?

[dsinclair, 2016-03-16 21:13:40]

https://codereview.chromium.org/1810563002/diff/40001/xfa/fxgraphics/cfx_patt...
File xfa/fxgraphics/cfx_pattern.cpp (right):

https://codereview.chromium.org/1810563002/diff/40001/xfa/fxgraphics/cfx_patt...
xfa/fxgraphics/cfx_pattern.cpp:18: m_hatchInfo.hatchStyle = hatchStyle;
On 2016/03/16 20:53:17, Tom Sepez wrote:
> I'd move these to initializer list.

Done.

https://codereview.chromium.org/1810563002/diff/40001/xfa/fxgraphics/cfx_patt...
xfa/fxgraphics/cfx_pattern.cpp:22: m_matrix.Set(matrix->a, matrix->b, matrix->c,
matrix->d, matrix->e,
On 2016/03/16 20:53:17, Tom Sepez wrote:
> File a follow-up to make a Set(const CFX_Matrix& other) method.  Sheesh

Done.

https://codereview.chromium.org/1810563002/diff/40001/xfa/fxgraphics/cfx_patt...
File xfa/fxgraphics/cfx_pattern.h (right):

https://codereview.chromium.org/1810563002/diff/40001/xfa/fxgraphics/cfx_patt...
xfa/fxgraphics/cfx_pattern.h:30: struct {
On 2016/03/16 20:53:17, Tom Sepez wrote:
> And this struct can go, too, and the members move up a level.

Done.

https://codereview.chromium.org/1810563002/diff/40001/xfa/fxgraphics/cfx_patt...
xfa/fxgraphics/cfx_pattern.h:31: FX_HatchStyle hatchStyle;
On 2016/03/16 20:53:17, Tom Sepez wrote:
> can hatchstyle be const?

Done.

https://codereview.chromium.org/1810563002/diff/40001/xfa/fxgraphics/cfx_patt...
xfa/fxgraphics/cfx_pattern.h:33: FX_ARGB backArgb;
On 2016/03/16 20:53:17, Tom Sepez wrote:
> And these?

Done.

https://codereview.chromium.org/1810563002/diff/40001/xfa/fxgraphics/cfx_shad...
File xfa/fxgraphics/cfx_shading.cpp (right):

https://codereview.chromium.org/1810563002/diff/40001/xfa/fxgraphics/cfx_shad...
xfa/fxgraphics/cfx_shading.cpp:60: FX_FLOAT aScale = (FX_FLOAT)(1.0 * (a2 - a1)
/ f);
On 2016/03/16 20:53:18, Tom Sepez wrote:
> These casts are pointless as we assume an int's range fits in a float (modulo
> loss of accurracy).

Done.

https://codereview.chromium.org/1810563002/diff/40001/xfa/fxgraphics/cfx_shad...
xfa/fxgraphics/cfx_shading.cpp:65: int32_t a3;
On 2016/03/16 20:53:18, Tom Sepez wrote:
> nit: move these inside loop.  They needn't persist across it.

Done.

https://codereview.chromium.org/1810563002/diff/40001/xfa/fxgraphics/cfx_shad...
xfa/fxgraphics/cfx_shading.cpp:70: a3 = (int32_t)(i * aScale);
On 2016/03/16 20:53:17, Tom Sepez wrote:
> nit: static_cast<>, just to emphasize that we think the float will fit.

Done.

https://codereview.chromium.org/1810563002/diff/40001/xfa/fxgraphics/cfx_shad...
xfa/fxgraphics/cfx_shading.cpp:73: b3 = (int32_t)(i * bScale);
On 2016/03/16 20:53:18, Tom Sepez wrote:
> Pity we dont define addition multiplicaton subtraction and division for argbs.
 

Done.

https://codereview.chromium.org/1810563002/diff/40001/xfa/fxgraphics/cfx_shad...
xfa/fxgraphics/cfx_shading.cpp:75: FXARGB_TODIB(FXARGB_MAKE((a1 + a3), (r1 +
r3), (g1 + g3), (b1 + b3)));
On 2016/03/16 20:53:18, Tom Sepez wrote:
> nit: overparenthesized, unless FXARGB_MAKE is not careful with argument
> expansion.

Done.

https://codereview.chromium.org/1810563002/diff/40001/xfa/fxgraphics/cfx_shad...
File xfa/fxgraphics/cfx_shading.h (right):

https://codereview.chromium.org/1810563002/diff/40001/xfa/fxgraphics/cfx_shad...
xfa/fxgraphics/cfx_shading.h:20: CFX_Shading(const CFX_PointF& beginPoint,
On 2016/03/16 20:53:18, Tom Sepez wrote:
> add comment // Axial shading.

Done.

https://codereview.chromium.org/1810563002/diff/40001/xfa/fxgraphics/cfx_shad...
xfa/fxgraphics/cfx_shading.h:26: CFX_Shading(const CFX_PointF& beginPoint,
On 2016/03/16 20:53:18, Tom Sepez wrote:
> add comment // Radial shading.

Done.

https://codereview.chromium.org/1810563002/diff/40001/xfa/fxgraphics/cfx_shad...
xfa/fxgraphics/cfx_shading.h:42: CFX_PointF m_beginPoint;
On 2016/03/16 20:53:18, Tom Sepez wrote:
> any more of these consts?

Done. And even found a bug.

[Tom Sepez, 2016-03-16 22:10:56]

lgtm

https://codereview.chromium.org/1810563002/diff/60001/xfa/fxgraphics/cfx_patt...
File xfa/fxgraphics/cfx_pattern.cpp (right):

https://codereview.chromium.org/1810563002/diff/60001/xfa/fxgraphics/cfx_patt...
xfa/fxgraphics/cfx_pattern.cpp:16: 
nit: maybe this can move to an else-branch below.

[dsinclair, 2016-03-17 12:59:30]

https://codereview.chromium.org/1810563002/diff/60001/xfa/fxgraphics/cfx_patt...
File xfa/fxgraphics/cfx_pattern.cpp (right):

https://codereview.chromium.org/1810563002/diff/60001/xfa/fxgraphics/cfx_patt...
xfa/fxgraphics/cfx_pattern.cpp:16: 
On 2016/03/16 22:10:55, Tom Sepez wrote:
> nit: maybe this can move to an else-branch below.

Done.

[dsinclair, 2016-03-17 12:59:48]

Description was changed from

==========
Move xfa/include/fxgraphics/fx_graphics.h to xfa/fxgraphics.

This Cl moves and splits apart the fx_graphics.h file into individual classes.
The .cpp files are renamed to match the .h files. pre.h was removed at the
same time and its contents moved to the correct places as needed.
==========

to

==========
Move xfa/include/fxgraphics/fx_graphics.h to xfa/fxgraphics.

This Cl moves and splits apart the fx_graphics.h file into individual classes.
The .cpp files are renamed to match the .h files. pre.h was removed at the
same time and its contents moved to the correct places as needed.

R=tsepez@chromium.org

Committed:
https://pdfium.googlesource.com/pdfium/+/811b8a4f4482bb5c11ac7610e70a4c8fd34c...
==========

[dsinclair, 2016-03-17 12:59:49]

Committed patchset #5 (id:80001) manually as
811b8a4f4482bb5c11ac7610e70a4c8fd34c2907 (presubmit successful).