Move CPDFSDK_Annot, CPDFSDK_BAAnnot and CPDFSDK_DateTime to their own files.

Move CPDFSDK_Annot, CPDFSDK_BAAnnot and CPDFSDK_DateTime to their own files.

Committed: https://pdfium.googlesource.com/pdfium/+/2736276deff3abef9d6b226eb9f585abe1384591

[jaepark, 2016-08-10 22:44:29]

The CQ bit was checked by jaepark@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 22:44:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-10 22:56:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-10 22:56:55]

Dry run: This issue passed the CQ dry run.

[jaepark, 2016-08-10 22:58:57]

jaepark@google.com changed reviewers:
+ thestig@chromium.org

[Lei Zhang, 2016-08-10 23:12:16]

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

[Lei Zhang, 2016-08-10 23:12:16]

Deferring to others.

[dsinclair, 2016-08-11 14:01:04]

https://codereview.chromium.org/2236743002/diff/1/fpdfsdk/cpdfsdk_annot.cpp
File fpdfsdk/cpdfsdk_annot.cpp (right):

https://codereview.chromium.org/2236743002/diff/1/fpdfsdk/cpdfsdk_annot.cpp#n...
fpdfsdk/cpdfsdk_annot.cpp:103: #ifdef PDF_ENABLE_XFA
Move this up with the ifdef PDF_ENABLE_XFA block above

https://codereview.chromium.org/2236743002/diff/1/fpdfsdk/cpdfsdk_baannot.cpp
File fpdfsdk/cpdfsdk_baannot.cpp (right):

https://codereview.chromium.org/2236743002/diff/1/fpdfsdk/cpdfsdk_baannot.cpp...
fpdfsdk/cpdfsdk_baannot.cpp:37: CFX_FloatRect CPDFSDK_BAAnnot::GetRect() const {
Do we know what BA stands for? Might be worth a followup to expand it to
something that is recognizable.

https://codereview.chromium.org/2236743002/diff/1/fpdfsdk/cpdfsdk_datetime.cpp
File fpdfsdk/cpdfsdk_datetime.cpp (right):

https://codereview.chromium.org/2236743002/diff/1/fpdfsdk/cpdfsdk_datetime.cp...
fpdfsdk/cpdfsdk_datetime.cpp:7: #include "core/fxcrt/include/fx_ext.h"
cpdfsdk_datetime.h should come first with a blank line afterwards as it's the .h
that relates to this .cpp.

https://codereview.chromium.org/2236743002/diff/1/fpdfsdk/cpdfsdk_datetime.cp...
fpdfsdk/cpdfsdk_datetime.cpp:24: uint8_t gAfxGetMonthDays(int16_t year, uint8_t
month) {
Let's drop the gAfx prefix from these names, they don't add anything and since
they now live in an anonymous namespace won't pollute the rest of the system.

https://codereview.chromium.org/2236743002/diff/1/fpdfsdk/cpdfsdk_datetime.cp...
fpdfsdk/cpdfsdk_datetime.cpp:96: FXSYS_memcpy(&dt, &datetime.dt,
sizeof(FX_DATETIME));
If this isn't used outside the file lets move it into the CPDFSDK_DateTime
constructor.

https://codereview.chromium.org/2236743002/diff/1/fpdfsdk/cpdfsdk_datetime.cp...
fpdfsdk/cpdfsdk_datetime.cpp:100: CPDFSDK_DateTime&
CPDFSDK_DateTime::operator=(const FX_SYSTEMTIME& st) {
Is this used outside of this file? If not, lets remove it and put this code in
the FX_SYSTEMTIME constructor.

https://codereview.chromium.org/2236743002/diff/1/fpdfsdk/cpdfsdk_datetime.cp...
fpdfsdk/cpdfsdk_datetime.cpp:302: if (pTime) {
if (!pTime)
  return;

https://codereview.chromium.org/2236743002/diff/1/fpdfsdk/include/cpdfsdk_ann...
File fpdfsdk/include/cpdfsdk_annot.h (right):

https://codereview.chromium.org/2236743002/diff/1/fpdfsdk/include/cpdfsdk_ann...
fpdfsdk/include/cpdfsdk_annot.h:36: // define layout order to 5.
Any idea what this means?

https://codereview.chromium.org/2236743002/diff/1/fpdfsdk/include/cpdfsdk_ann...
fpdfsdk/include/cpdfsdk_annot.h:57: // Tab Order
This comment and the // Selection one aren't useful.

https://codereview.chromium.org/2236743002/diff/1/fpdfsdk/include/cpdfsdk_baa...
File fpdfsdk/include/cpdfsdk_baannot.h (right):

https://codereview.chromium.org/2236743002/diff/1/fpdfsdk/include/cpdfsdk_baa...
fpdfsdk/include/cpdfsdk_baannot.h:100: protected:
Can this be private?

https://codereview.chromium.org/2236743002/diff/1/fpdfsdk/include/cpdfsdk_dat...
File fpdfsdk/include/cpdfsdk_datetime.h (right):

https://codereview.chromium.org/2236743002/diff/1/fpdfsdk/include/cpdfsdk_dat...
fpdfsdk/include/cpdfsdk_datetime.h:50: } dt;
Can dt be made private?

[jaepark, 2016-08-11 17:55:08]

The CQ bit was checked by jaepark@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-11 17:55:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-11 18:06:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-11 18:06:43]

Dry run: This issue passed the CQ dry run.

[jaepark, 2016-08-11 18:09:50]

https://codereview.chromium.org/2236743002/diff/1/fpdfsdk/cpdfsdk_annot.cpp
File fpdfsdk/cpdfsdk_annot.cpp (right):

https://codereview.chromium.org/2236743002/diff/1/fpdfsdk/cpdfsdk_annot.cpp#n...
fpdfsdk/cpdfsdk_annot.cpp:103: #ifdef PDF_ENABLE_XFA
On 2016/08/11 14:01:04, dsinclair wrote:
> Move this up with the ifdef PDF_ENABLE_XFA block above

Done.

https://codereview.chromium.org/2236743002/diff/1/fpdfsdk/cpdfsdk_baannot.cpp
File fpdfsdk/cpdfsdk_baannot.cpp (right):

https://codereview.chromium.org/2236743002/diff/1/fpdfsdk/cpdfsdk_baannot.cpp...
fpdfsdk/cpdfsdk_baannot.cpp:37: CFX_FloatRect CPDFSDK_BAAnnot::GetRect() const {
On 2016/08/11 14:01:04, dsinclair wrote:
> Do we know what BA stands for? Might be worth a followup to expand it to
> something that is recognizable.

I'm not sure what BA stands for. I know that CPDFSDK_BAAnnot and
CPDFSDK_XFAWidget inherits CPDFSDK_Annot, and CPDFSDK_Widget inherits
CPDFSDK_BAAnnot. 

Since there are CPDFSDK_BFAnnotHandler and CPDFSDK_XFAAnnotHandler, my best
guess is that BAAnnot is actually supposed to be BFAnnot, where BF stands for
base form or basic form in contrast to XFA (XML Form Architecture), but I'm not
confident.

https://codereview.chromium.org/2236743002/diff/1/fpdfsdk/cpdfsdk_datetime.cpp
File fpdfsdk/cpdfsdk_datetime.cpp (right):

https://codereview.chromium.org/2236743002/diff/1/fpdfsdk/cpdfsdk_datetime.cp...
fpdfsdk/cpdfsdk_datetime.cpp:7: #include "core/fxcrt/include/fx_ext.h"
On 2016/08/11 14:01:04, dsinclair wrote:
> cpdfsdk_datetime.h should come first with a blank line afterwards as it's the
.h
> that relates to this .cpp.

Done.

https://codereview.chromium.org/2236743002/diff/1/fpdfsdk/cpdfsdk_datetime.cp...
fpdfsdk/cpdfsdk_datetime.cpp:24: uint8_t gAfxGetMonthDays(int16_t year, uint8_t
month) {
On 2016/08/11 14:01:04, dsinclair wrote:
> Let's drop the gAfx prefix from these names, they don't add anything and since
> they now live in an anonymous namespace won't pollute the rest of the system.

Done.

https://codereview.chromium.org/2236743002/diff/1/fpdfsdk/cpdfsdk_datetime.cp...
fpdfsdk/cpdfsdk_datetime.cpp:96: FXSYS_memcpy(&dt, &datetime.dt,
sizeof(FX_DATETIME));
On 2016/08/11 14:01:04, dsinclair wrote:
> If this isn't used outside the file lets move it into the CPDFSDK_DateTime
> constructor.

Done.

https://codereview.chromium.org/2236743002/diff/1/fpdfsdk/cpdfsdk_datetime.cp...
fpdfsdk/cpdfsdk_datetime.cpp:100: CPDFSDK_DateTime&
CPDFSDK_DateTime::operator=(const FX_SYSTEMTIME& st) {
On 2016/08/11 14:01:04, dsinclair wrote:
> Is this used outside of this file? If not, lets remove it and put this code in
> the FX_SYSTEMTIME constructor. 

Done.

https://codereview.chromium.org/2236743002/diff/1/fpdfsdk/cpdfsdk_datetime.cp...
fpdfsdk/cpdfsdk_datetime.cpp:302: if (pTime) {
On 2016/08/11 14:01:04, dsinclair wrote:
> if (!pTime)
>   return;

Done.

https://codereview.chromium.org/2236743002/diff/1/fpdfsdk/include/cpdfsdk_ann...
File fpdfsdk/include/cpdfsdk_annot.h (right):

https://codereview.chromium.org/2236743002/diff/1/fpdfsdk/include/cpdfsdk_ann...
fpdfsdk/include/cpdfsdk_annot.h:36: // define layout order to 5.
On 2016/08/11 14:01:04, dsinclair wrote:
> Any idea what this means?

This is used to sort the CPDFSDK_Annot, so that all CPDFSDK_Widgets come before
other CPDFSDK_Annot after being sorted in CPDFSDK_AnnotIterator. But currently,
I'm not sure why we need to sort in such an order.

https://codereview.chromium.org/2236743002/diff/1/fpdfsdk/include/cpdfsdk_ann...
fpdfsdk/include/cpdfsdk_annot.h:57: // Tab Order
On 2016/08/11 14:01:04, dsinclair wrote:
> This comment and the // Selection one aren't useful.

Removed.

https://codereview.chromium.org/2236743002/diff/1/fpdfsdk/include/cpdfsdk_baa...
File fpdfsdk/include/cpdfsdk_baannot.h (right):

https://codereview.chromium.org/2236743002/diff/1/fpdfsdk/include/cpdfsdk_baa...
fpdfsdk/include/cpdfsdk_baannot.h:100: protected:
On 2016/08/11 14:01:04, dsinclair wrote:
> Can this be private?

CPDFSDK_Widget inherits CPDFSDK_BAAnnot and uses m_pAnnot, so it needs to be
protected.

https://codereview.chromium.org/2236743002/diff/1/fpdfsdk/include/cpdfsdk_dat...
File fpdfsdk/include/cpdfsdk_datetime.h (right):

https://codereview.chromium.org/2236743002/diff/1/fpdfsdk/include/cpdfsdk_dat...
fpdfsdk/include/cpdfsdk_datetime.h:50: } dt;
On 2016/08/11 14:01:04, dsinclair wrote:
> Can dt be made private?

Done.

[dsinclair, 2016-08-11 19:11:56]

lgtm w/ nit.

https://codereview.chromium.org/2236743002/diff/1/fpdfsdk/include/cpdfsdk_ann...
File fpdfsdk/include/cpdfsdk_annot.h (right):

https://codereview.chromium.org/2236743002/diff/1/fpdfsdk/include/cpdfsdk_ann...
fpdfsdk/include/cpdfsdk_annot.h:36: // define layout order to 5.
On 2016/08/11 18:09:50, jaepark wrote:
> On 2016/08/11 14:01:04, dsinclair wrote:
> > Any idea what this means?
> 
> This is used to sort the CPDFSDK_Annot, so that all CPDFSDK_Widgets come
before
> other CPDFSDK_Annot after being sorted in CPDFSDK_AnnotIterator. But
currently,
> I'm not sure why we need to sort in such an order.


Let's remove this comment as I still don't understand what '5' is, heh. Doesn't
seem like it's useful.

[jaepark, 2016-08-11 19:19:52]

The CQ bit was checked by jaepark@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-11 19:20:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-11 19:45:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-11 19:45:02]

Dry run: This issue passed the CQ dry run.

[jaepark, 2016-08-11 20:10:02]

https://codereview.chromium.org/2236743002/diff/1/fpdfsdk/include/cpdfsdk_ann...
File fpdfsdk/include/cpdfsdk_annot.h (right):

https://codereview.chromium.org/2236743002/diff/1/fpdfsdk/include/cpdfsdk_ann...
fpdfsdk/include/cpdfsdk_annot.h:36: // define layout order to 5.
On 2016/08/11 19:11:56, dsinclair wrote:
> On 2016/08/11 18:09:50, jaepark wrote:
> > On 2016/08/11 14:01:04, dsinclair wrote:
> > > Any idea what this means?
> > 
> > This is used to sort the CPDFSDK_Annot, so that all CPDFSDK_Widgets come
> before
> > other CPDFSDK_Annot after being sorted in CPDFSDK_AnnotIterator. But
> currently,
> > I'm not sure why we need to sort in such an order.
> 
> 
> Let's remove this comment as I still don't understand what '5' is, heh.
Doesn't
> seem like it's useful.

Done.

[jaepark, 2016-08-11 20:10:13]

The CQ bit was checked by jaepark@google.com

[jaepark, 2016-08-11 20:10:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
dsinclair@chromium.org
Link to the patchset: https://codereview.chromium.org/2236743002/#ps40001
(title: "Move CPDFSDK_Annot, CPDFSDK_BAAnnot and CPDFSDK_DateTime to their own
files.")

[commit-bot: I haz the power, 2016-08-11 20:10:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-11 20:10:43]

Description was changed from

==========
Move CPDFSDK_Annot, CPDFSDK_BAAnnot and CPDFSDK_DateTime to their own files.
==========

to

==========
Move CPDFSDK_Annot, CPDFSDK_BAAnnot and CPDFSDK_DateTime to their own files.

Committed:
https://pdfium.googlesource.com/pdfium/+/2736276deff3abef9d6b226eb9f585abe138...
==========

[commit-bot: I haz the power, 2016-08-11 20:10:44]

Committed patchset #3 (id:40001) as
https://pdfium.googlesource.com/pdfium/+/2736276deff3abef9d6b226eb9f585abe138...