Fixup FX_RECT and FX_SMALL_RECT classes.

Fixup FX_RECT and FX_SMALL_RECT classes.

Put these first, so later on more complicated classes can
have constructors that take these as arguments.

Add better constructors, and call appropriately.  Also don't
be afraid to return these from methods since RVO.

R=dsinclair@chromium.org

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

[Tom Sepez, 2016-02-26 23:23:49]

Description was changed from

==========
Fixup FX_RECT and FX_SMALL_RECT classes.

BUG=
==========

to

==========
Fixup FX_RECT and FX_SMALL_RECT classes.

Put these first, so later on more complicated classes can
have constructors that take these as arguments.
==========

[Tom Sepez, 2016-02-27 00:23:34]

Description was changed from

==========
Fixup FX_RECT and FX_SMALL_RECT classes.

Put these first, so later on more complicated classes can
have constructors that take these as arguments.
==========

to

==========
Fixup FX_RECT and FX_SMALL_RECT classes.

Put these first, so later on more complicated classes can
have constructors that take these as arguments.

Add better constructors, and call appropriately.  Also don't
be afraid to return these from methods since RVO.
==========

[Tom Sepez, 2016-02-27 00:29:46]

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

[Tom Sepez, 2016-02-27 00:30:05]

Dan, for review in Lei's absence.

[dsinclair, 2016-02-27 03:11:44]

I realize now most of this is moved code, so a bunch of my comments can probably
be ignored. lgtm.

https://codereview.chromium.org/1745683002/diff/20001/core/include/fxcrt/fx_c...
File core/include/fxcrt/fx_coordinates.h (right):

https://codereview.chromium.org/1745683002/diff/20001/core/include/fxcrt/fx_c...
core/include/fxcrt/fx_coordinates.h:135: int16_t Left;
Why does this one use upper case (Left) and the one below use lower case (left)?

https://codereview.chromium.org/1745683002/diff/20001/core/include/fxcrt/fx_c...
core/include/fxcrt/fx_coordinates.h:156: void Intersect(int left1, int top1, int
right1, int bottom1) {
nit: Why the 1's in the names?

https://codereview.chromium.org/1745683002/diff/20001/core/include/fxcrt/fx_c...
core/include/fxcrt/fx_coordinates.h:183: return x >= left && x < right && y >=
top && y < bottom;
Why >= for left and top but < for right and bottom?

https://codereview.chromium.org/1745683002/diff/20001/core/include/fxcrt/fx_c...
core/include/fxcrt/fx_coordinates.h:209: CFX_FloatRect() { left = right = bottom
= top = 0; }
CFX_FloatRect() : left(0), right(0), bottom(0), top(0) {} ?

https://codereview.chromium.org/1745683002/diff/20001/core/include/fxcrt/fx_c...
core/include/fxcrt/fx_coordinates.h:221: CFX_FloatRect(const FX_FLOAT* pArray) {
nit: explicit

https://codereview.chromium.org/1745683002/diff/20001/core/include/fxcrt/fx_c...
core/include/fxcrt/fx_coordinates.h:228: CFX_FloatRect(const FX_RECT& rect);
nit: explicit

https://codereview.chromium.org/1745683002/diff/20001/core/include/fxcrt/fx_c...
core/include/fxcrt/fx_coordinates.h:247: 
nit: Can a bunch of these spaces be removed?

https://codereview.chromium.org/1745683002/diff/20001/core/include/fxcrt/fx_c...
core/include/fxcrt/fx_coordinates.h:254: void InitRect(FX_FLOAT x, FX_FLOAT y) {
This seems like a very weird init method? We initialize to 0 width/height and
empty?

https://codereview.chromium.org/1745683002/diff/20001/core/include/fxcrt/fx_c...
core/include/fxcrt/fx_coordinates.h:265: void Inflate(FX_FLOAT x, FX_FLOAT y) {
Maybe add a comment that this inflates by 2x and 2y?

https://codereview.chromium.org/1745683002/diff/20001/core/include/fxcrt/fx_c...
core/include/fxcrt/fx_coordinates.h:288: void Deflate(FX_FLOAT x, FX_FLOAT y) {
ditto: deflates by 2x, 2y

https://codereview.chromium.org/1745683002/diff/20001/core/include/fxcrt/fx_c...
core/include/fxcrt/fx_coordinates.h:326: // LTWH rectangles (y-axis runs
downwards).
What does LTWH stand for?

[Tom Sepez, 2016-02-29 18:42:08]

Hi Dan, quick sanity check between #3 and #2, if you would. Thanks.

https://codereview.chromium.org/1745683002/diff/20001/core/include/fxcrt/fx_c...
File core/include/fxcrt/fx_coordinates.h (right):

https://codereview.chromium.org/1745683002/diff/20001/core/include/fxcrt/fx_c...
core/include/fxcrt/fx_coordinates.h:135: int16_t Left;
On 2016/02/27 03:11:44, dsinclair wrote:
> Why does this one use upper case (Left) and the one below use lower case
(left)?

Because they're inconsistent with public members.  Usually they leave off the m_
prefix for public struct members.  It feels about 50/50 as to whether they then
use the hungarian notation (wLeft), and then lacking that, they're incosnsitet
about caps.  These are used in a few spots, I'll make them |left| to match the
below.

https://codereview.chromium.org/1745683002/diff/20001/core/include/fxcrt/fx_c...
core/include/fxcrt/fx_coordinates.h:156: void Intersect(int left1, int top1, int
right1, int bottom1) {
On 2016/02/27 03:11:44, dsinclair wrote:
> nit: Why the 1's in the names?
They conflict with instance variables in the class.  I'll make them |l| |t| |r|
|b| as above.

https://codereview.chromium.org/1745683002/diff/20001/core/include/fxcrt/fx_c...
core/include/fxcrt/fx_coordinates.h:183: return x >= left && x < right && y >=
top && y < bottom;
On 2016/02/27 03:11:44, dsinclair wrote:
> Why >= for left and top but < for right and bottom?

Beats me.  I'd expect a lot of 1px diffs if we changed it.

https://codereview.chromium.org/1745683002/diff/20001/core/include/fxcrt/fx_c...
core/include/fxcrt/fx_coordinates.h:209: CFX_FloatRect() { left = right = bottom
= top = 0; }
On 2016/02/27 03:11:44, dsinclair wrote:
> CFX_FloatRect() : left(0), right(0), bottom(0), top(0) {} ?

Done.

https://codereview.chromium.org/1745683002/diff/20001/core/include/fxcrt/fx_c...
core/include/fxcrt/fx_coordinates.h:221: CFX_FloatRect(const FX_FLOAT* pArray) {
On 2016/02/27 03:11:44, dsinclair wrote:
> nit: explicit
Yep. Good catch. Done.

https://codereview.chromium.org/1745683002/diff/20001/core/include/fxcrt/fx_c...
core/include/fxcrt/fx_coordinates.h:228: CFX_FloatRect(const FX_RECT& rect);
On 2016/02/27 03:11:44, dsinclair wrote:
> nit: explicit

Done.

https://codereview.chromium.org/1745683002/diff/20001/core/include/fxcrt/fx_c...
core/include/fxcrt/fx_coordinates.h:247: 
On 2016/02/27 03:11:44, dsinclair wrote:
> nit: Can a bunch of these spaces be removed?

Done.

https://codereview.chromium.org/1745683002/diff/20001/core/include/fxcrt/fx_c...
core/include/fxcrt/fx_coordinates.h:254: void InitRect(FX_FLOAT x, FX_FLOAT y) {
On 2016/02/27 03:11:44, dsinclair wrote:
> This seems like a very weird init method? We initialize to 0 width/height and
> empty?
yes. looking at usage, its always followed by a bunch of UpdateRect() calls to
make it non-empty.  Something for later.

https://codereview.chromium.org/1745683002/diff/20001/core/include/fxcrt/fx_c...
core/include/fxcrt/fx_coordinates.h:265: void Inflate(FX_FLOAT x, FX_FLOAT y) {
On 2016/02/27 03:11:44, dsinclair wrote:
> Maybe add a comment that this inflates by 2x and 2y?

Maybe.

https://codereview.chromium.org/1745683002/diff/20001/core/include/fxcrt/fx_c...
core/include/fxcrt/fx_coordinates.h:326: // LTWH rectangles (y-axis runs
downwards).
On 2016/02/27 03:11:44, dsinclair wrote:
> What does LTWH stand for?
Left, Top, Width, Height -- as in we store a point plus a displacement.  My
feeling is that this causes you to do a lot of needless addition along the way.
But there it is.

[dsinclair, 2016-02-29 19:00:16]

Still lgtm.

https://codereview.chromium.org/1745683002/diff/40001/core/src/fpdfapi/fpdf_f...
File core/src/fpdfapi/fpdf_font/fpdf_font_cid.cpp (right):

https://codereview.chromium.org/1745683002/diff/40001/core/src/fpdfapi/fpdf_f...
core/src/fpdfapi/fpdf_font/fpdf_font_cid.cpp:72: {"UniGB-UCS2", CIDSET_GB1,
CIDCODING_UCS2, CPDF_CMap::TwoBytes, 0, {}},
Not sure if we made the same change, or something weird happened I think you'll
conflict on update.

[Tom Sepez, 2016-02-29 19:24:32]

Description was changed from

==========
Fixup FX_RECT and FX_SMALL_RECT classes.

Put these first, so later on more complicated classes can
have constructors that take these as arguments.

Add better constructors, and call appropriately.  Also don't
be afraid to return these from methods since RVO.
==========

to

==========
Fixup FX_RECT and FX_SMALL_RECT classes.

Put these first, so later on more complicated classes can
have constructors that take these as arguments.

Add better constructors, and call appropriately.  Also don't
be afraid to return these from methods since RVO.

R=dsinclair@chromium.org

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

[Tom Sepez, 2016-02-29 19:24:33]

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

[Tom Sepez, 2016-02-29 19:25:49]

https://codereview.chromium.org/1745683002/diff/40001/core/src/fpdfapi/fpdf_f...
File core/src/fpdfapi/fpdf_font/fpdf_font_cid.cpp (right):

https://codereview.chromium.org/1745683002/diff/40001/core/src/fpdfapi/fpdf_f...
core/src/fpdfapi/fpdf_font/fpdf_font_cid.cpp:72: {"UniGB-UCS2", CIDSET_GB1,
CIDCODING_UCS2, CPDF_CMap::TwoBytes, 0, {}},
On 2016/02/29 19:00:16, dsinclair wrote:
> Not sure if we made the same change, or something weird happened I think
you'll
> conflict on update.
Ah, sorry, should have rebased and uploaded and then made changes;  you're
seeing the effect of the rebase.