Simplify XFA event handling.

Simplify XFA event handling.

- Avoid casting to void*
- Don't bother firing events that no one listens for

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

[Lei Zhang, 2016-04-14 08:52:07]

Patchset #3 (id:40001) has been deleted

[Lei Zhang, 2016-04-14 21:47:42]

Description was changed from

==========
Simplify XFA event handling.
==========

to

==========
Simplify XFA event handling.

- Avoid casting to void*
- Don't bother firing events that no one listens for
==========

[Lei Zhang, 2016-04-14 21:47:42]

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

[Lei Zhang, 2016-04-14 21:47:50]

See individual patch sets to see the progression.

[Tom Sepez, 2016-04-14 22:08:55]

https://codereview.chromium.org/1890563003/diff/1/xfa/fxfa/app/xfa_ffdocview.cpp
File xfa/fxfa/app/xfa_ffdocview.cpp (left):

https://codereview.chromium.org/1890563003/diff/1/xfa/fxfa/app/xfa_ffdocview....
xfa/fxfa/app/xfa_ffdocview.cpp:218: FX_BOOL bNotify = IsStaticNotify();
How do we know its not doctype static?

https://codereview.chromium.org/1890563003/diff/1/xfa/fxfa/app/xfa_ffwidgetac...
File xfa/fxfa/app/xfa_ffwidgetacc.cpp (right):

https://codereview.chromium.org/1890563003/diff/1/xfa/fxfa/app/xfa_ffwidgetac...
xfa/fxfa/app/xfa_ffwidgetacc.cpp:379: if (iRet == XFA_EVENTERROR_Success) {
nit: might prefer early return.

https://codereview.chromium.org/1890563003/diff/200001/xfa/fxfa/app/xfa_ffnot...
File xfa/fxfa/app/xfa_ffnotify.cpp (right):

https://codereview.chromium.org/1890563003/diff/200001/xfa/fxfa/app/xfa_ffnot...
xfa/fxfa/app/xfa_ffnotify.cpp:58: CXFA_FFWidget* pWidget =
pWidgetAcc->GetNextWidget(NULL);
nit: nullptr

https://codereview.chromium.org/1890563003/diff/200001/xfa/fxfa/app/xfa_ffnot...
xfa/fxfa/app/xfa_ffnotify.cpp:77: CXFA_FFWidget* pWidget =
pWidgetAcc->GetNextWidget(NULL);
nit: maybe start with NULL, and implement loop along the lines of line 364

https://codereview.chromium.org/1890563003/diff/200001/xfa/fxfa/app/xfa_ffnot...
xfa/fxfa/app/xfa_ffnotify.cpp:366: if (pWidget->IsLoaded()) {
nit: {} not needed.

https://codereview.chromium.org/1890563003/diff/200001/xfa/fxfa/app/xfa_ffnot...
xfa/fxfa/app/xfa_ffnotify.cpp:390: if (!pWidgetAcc) {
nit: {} not needed, couple of places in this method.

https://codereview.chromium.org/1890563003/diff/200001/xfa/fxfa/app/xfa_ffnot...
xfa/fxfa/app/xfa_ffnotify.cpp:411: break;
nit: lump default together with these cases.

https://codereview.chromium.org/1890563003/diff/200001/xfa/fxfa/parser/xfa_ob...
File xfa/fxfa/parser/xfa_object_imp.cpp (right):

https://codereview.chromium.org/1890563003/diff/200001/xfa/fxfa/parser/xfa_ob...
xfa/fxfa/parser/xfa_object_imp.cpp:4850: void CXFA_Node::SetFlag(uint32_t
dwFlag, FX_BOOL bOn, bool bNotify) {
might be worth having a separate ClearFlag rather than taking a boolean arg ...
I doubt we ever pass a computed value vs. "false"

[Lei Zhang, 2016-04-14 22:11:50]

https://codereview.chromium.org/1890563003/diff/1/xfa/fxfa/app/xfa_ffdocview.cpp
File xfa/fxfa/app/xfa_ffdocview.cpp (left):

https://codereview.chromium.org/1890563003/diff/1/xfa/fxfa/app/xfa_ffdocview....
xfa/fxfa/app/xfa_ffdocview.cpp:218: FX_BOOL bNotify = IsStaticNotify();
On 2016/04/14 22:08:55, Tom Sepez wrote:
> How do we know its not doctype static?

We don't care because nobody listens for XFA_WIDGETEVENT_PostContentChanged.

[Tom Sepez, 2016-04-14 22:45:44]

Nothing but nits.  LGTM

https://codereview.chromium.org/1890563003/diff/1/xfa/fxfa/app/xfa_ffdocview.cpp
File xfa/fxfa/app/xfa_ffdocview.cpp (left):

https://codereview.chromium.org/1890563003/diff/1/xfa/fxfa/app/xfa_ffdocview....
xfa/fxfa/app/xfa_ffdocview.cpp:218: FX_BOOL bNotify = IsStaticNotify();
On 2016/04/14 22:11:50, Lei Zhang wrote:
> On 2016/04/14 22:08:55, Tom Sepez wrote:
> > How do we know its not doctype static?
> 
> We don't care because nobody listens for XFA_WIDGETEVENT_PostContentChanged.

Acknowledged.

https://codereview.chromium.org/1890563003/diff/20001/xfa/fxfa/app/xfa_ffnoti...
File xfa/fxfa/app/xfa_ffnotify.h (right):

https://codereview.chromium.org/1890563003/diff/20001/xfa/fxfa/app/xfa_ffnoti...
xfa/fxfa/app/xfa_ffnotify.h:86: CXFA_FFDoc* const m_pDoc;
nit:  blank line between methods and members.

https://codereview.chromium.org/1890563003/diff/60001/xfa/fxfa/app/xfa_ffnoti...
File xfa/fxfa/app/xfa_ffnotify.h (right):

https://codereview.chromium.org/1890563003/diff/60001/xfa/fxfa/app/xfa_ffnoti...
xfa/fxfa/app/xfa_ffnotify.h:39: void OnLayoutItemRemoving(CXFA_LayoutProcessor*
pLayout,
nit: verb tense.  OnLayoutItemAdded/Removed to match the OnChild methods below?

https://codereview.chromium.org/1890563003/diff/80001/xfa/fxfa/app/xfa_ffnoti...
File xfa/fxfa/app/xfa_ffnotify.h (right):

https://codereview.chromium.org/1890563003/diff/80001/xfa/fxfa/app/xfa_ffnoti...
xfa/fxfa/app/xfa_ffnotify.h:33: void OnLayoutItemAdd(CXFA_LayoutProcessor*
pLayout,
nit: same issue with verb tense

https://codereview.chromium.org/1890563003/diff/100001/xfa/fxfa/parser/xfa_ob...
File xfa/fxfa/parser/xfa_object_imp.cpp (right):

https://codereview.chromium.org/1890563003/diff/100001/xfa/fxfa/parser/xfa_ob...
xfa/fxfa/parser/xfa_object_imp.cpp:4869: if (bNotify && pParent) {
nit: prefer early returns.

https://codereview.chromium.org/1890563003/diff/120001/xfa/fxfa/app/xfa_ffnot...
File xfa/fxfa/app/xfa_ffnotify.cpp (right):

https://codereview.chromium.org/1890563003/diff/120001/xfa/fxfa/app/xfa_ffnot...
xfa/fxfa/app/xfa_ffnotify.cpp:59: while (pWidget) {
nit: rewrite loop in while ((pWidget = pWidgetAcc->GetNextWidget(pwidget)))
form, starting with null widget above.

https://codereview.chromium.org/1890563003/diff/140001/xfa/fxfa/parser/xfa_ob...
File xfa/fxfa/parser/xfa_object_imp.cpp (right):

https://codereview.chromium.org/1890563003/diff/140001/xfa/fxfa/parser/xfa_ob...
xfa/fxfa/parser/xfa_object_imp.cpp:3908: OnChanged(eAttr, bNotify, FALSE);
nit: false vs. FALSE ?

https://codereview.chromium.org/1890563003/diff/140001/xfa/fxfa/parser/xfa_ob...
xfa/fxfa/parser/xfa_object_imp.cpp:4124: OnChanged(eAttr, bNotify, FALSE);
ditto

https://codereview.chromium.org/1890563003/diff/140001/xfa/fxfa/parser/xfa_ob...
xfa/fxfa/parser/xfa_object_imp.cpp:4869: if (bNotify) {
nit: maybe early return?

https://codereview.chromium.org/1890563003/diff/160001/xfa/fxfa/app/xfa_ffnot...
File xfa/fxfa/app/xfa_ffnotify.cpp (right):

https://codereview.chromium.org/1890563003/diff/160001/xfa/fxfa/app/xfa_ffnot...
xfa/fxfa/app/xfa_ffnotify.cpp:366: if (pWidget->IsLoaded()) {
nit: {} not needed

https://codereview.chromium.org/1890563003/diff/160001/xfa/fxfa/app/xfa_ffnot...
xfa/fxfa/app/xfa_ffnotify.cpp:398: if (!pCapOut) {
maybe just 
  if (pCapOut)
    pCapOut->Unload();

https://codereview.chromium.org/1890563003/diff/160001/xfa/fxfa/app/xfa_ffnot...
xfa/fxfa/app/xfa_ffnotify.cpp:469: if (bLayoutReady) {
nit: {}

https://codereview.chromium.org/1890563003/diff/160001/xfa/fxfa/parser/xfa_la...
File xfa/fxfa/parser/xfa_layout_pagemgr_new.cpp (right):

https://codereview.chromium.org/1890563003/diff/160001/xfa/fxfa/parser/xfa_la...
xfa/fxfa/parser/xfa_layout_pagemgr_new.cpp:1720: CXFA_Node* pPendingPageSet =
GetRootLayoutItem()->m_pFormNode;
nit: maybe different name to avoid shadowing confusions.

https://codereview.chromium.org/1890563003/diff/160001/xfa/fxfa/parser/xfa_ob...
File xfa/fxfa/parser/xfa_object_imp.cpp (right):

https://codereview.chromium.org/1890563003/diff/160001/xfa/fxfa/parser/xfa_ob...
xfa/fxfa/parser/xfa_object_imp.cpp:1445: if (pParent->IsContainerNode()) {
nit: {}

[Lei Zhang, 2016-04-15 01:16:41]

https://codereview.chromium.org/1890563003/diff/1/xfa/fxfa/app/xfa_ffwidgetac...
File xfa/fxfa/app/xfa_ffwidgetacc.cpp (right):

https://codereview.chromium.org/1890563003/diff/1/xfa/fxfa/app/xfa_ffwidgetac...
xfa/fxfa/app/xfa_ffwidgetacc.cpp:379: if (iRet == XFA_EVENTERROR_Success) {
On 2016/04/14 22:08:55, Tom Sepez wrote:
> nit: might prefer early return.

Done.

https://codereview.chromium.org/1890563003/diff/20001/xfa/fxfa/app/xfa_ffnoti...
File xfa/fxfa/app/xfa_ffnotify.h (right):

https://codereview.chromium.org/1890563003/diff/20001/xfa/fxfa/app/xfa_ffnoti...
xfa/fxfa/app/xfa_ffnotify.h:86: CXFA_FFDoc* const m_pDoc;
On 2016/04/14 22:45:43, Tom Sepez wrote:
> nit:  blank line between methods and members.

All the methods have become public by patch set 10.

https://codereview.chromium.org/1890563003/diff/60001/xfa/fxfa/app/xfa_ffnoti...
File xfa/fxfa/app/xfa_ffnotify.h (right):

https://codereview.chromium.org/1890563003/diff/60001/xfa/fxfa/app/xfa_ffnoti...
xfa/fxfa/app/xfa_ffnotify.h:39: void OnLayoutItemRemoving(CXFA_LayoutProcessor*
pLayout,
On 2016/04/14 22:45:44, Tom Sepez wrote:
> nit: verb tense.  OnLayoutItemAdded/Removed to match the OnChild methods
below?

Added / Removing (since it's called right before the objected is removed)

https://codereview.chromium.org/1890563003/diff/80001/xfa/fxfa/app/xfa_ffnoti...
File xfa/fxfa/app/xfa_ffnotify.h (right):

https://codereview.chromium.org/1890563003/diff/80001/xfa/fxfa/app/xfa_ffnoti...
xfa/fxfa/app/xfa_ffnotify.h:33: void OnLayoutItemAdd(CXFA_LayoutProcessor*
pLayout,
On 2016/04/14 22:45:44, Tom Sepez wrote:
> nit: same issue with verb tense

Already addressed from previous patch set.

https://codereview.chromium.org/1890563003/diff/100001/xfa/fxfa/parser/xfa_ob...
File xfa/fxfa/parser/xfa_object_imp.cpp (right):

https://codereview.chromium.org/1890563003/diff/100001/xfa/fxfa/parser/xfa_ob...
xfa/fxfa/parser/xfa_object_imp.cpp:4869: if (bNotify && pParent) {
On 2016/04/14 22:45:44, Tom Sepez wrote:
> nit: prefer early returns.

Done.

https://codereview.chromium.org/1890563003/diff/120001/xfa/fxfa/app/xfa_ffnot...
File xfa/fxfa/app/xfa_ffnotify.cpp (right):

https://codereview.chromium.org/1890563003/diff/120001/xfa/fxfa/app/xfa_ffnot...
xfa/fxfa/app/xfa_ffnotify.cpp:59: while (pWidget) {
On 2016/04/14 22:45:44, Tom Sepez wrote:
> nit: rewrite loop in while ((pWidget = pWidgetAcc->GetNextWidget(pwidget)))
> form, starting with null widget above.

Done. Same for line 77.

https://codereview.chromium.org/1890563003/diff/140001/xfa/fxfa/parser/xfa_ob...
File xfa/fxfa/parser/xfa_object_imp.cpp (right):

https://codereview.chromium.org/1890563003/diff/140001/xfa/fxfa/parser/xfa_ob...
xfa/fxfa/parser/xfa_object_imp.cpp:3908: OnChanged(eAttr, bNotify, FALSE);
On 2016/04/14 22:45:44, Tom Sepez wrote:
> nit: false vs. FALSE ?

I didn't change the last parameter to a bool yet. I tried to change as many of
the |bNotify| arguments as I can in other places.

https://codereview.chromium.org/1890563003/diff/140001/xfa/fxfa/parser/xfa_ob...
xfa/fxfa/parser/xfa_object_imp.cpp:4124: OnChanged(eAttr, bNotify, FALSE);
On 2016/04/14 22:45:44, Tom Sepez wrote:
> ditto

same as above.

https://codereview.chromium.org/1890563003/diff/140001/xfa/fxfa/parser/xfa_ob...
xfa/fxfa/parser/xfa_object_imp.cpp:4869: if (bNotify) {
On 2016/04/14 22:45:44, Tom Sepez wrote:
> nit: maybe early return?

Done.

https://codereview.chromium.org/1890563003/diff/160001/xfa/fxfa/app/xfa_ffnot...
File xfa/fxfa/app/xfa_ffnotify.cpp (right):

https://codereview.chromium.org/1890563003/diff/160001/xfa/fxfa/app/xfa_ffnot...
xfa/fxfa/app/xfa_ffnotify.cpp:366: if (pWidget->IsLoaded()) {
On 2016/04/14 22:45:44, Tom Sepez wrote:
> nit: {} not needed

Done.

https://codereview.chromium.org/1890563003/diff/160001/xfa/fxfa/app/xfa_ffnot...
xfa/fxfa/app/xfa_ffnotify.cpp:398: if (!pCapOut) {
On 2016/04/14 22:45:44, Tom Sepez wrote:
> maybe just 
>   if (pCapOut)
>     pCapOut->Unload();

But the return skips all the extra work after the switch statement ends?

https://codereview.chromium.org/1890563003/diff/160001/xfa/fxfa/app/xfa_ffnot...
xfa/fxfa/app/xfa_ffnotify.cpp:469: if (bLayoutReady) {
On 2016/04/14 22:45:44, Tom Sepez wrote:
> nit: {}

Done.

https://codereview.chromium.org/1890563003/diff/160001/xfa/fxfa/parser/xfa_la...
File xfa/fxfa/parser/xfa_layout_pagemgr_new.cpp (right):

https://codereview.chromium.org/1890563003/diff/160001/xfa/fxfa/parser/xfa_la...
xfa/fxfa/parser/xfa_layout_pagemgr_new.cpp:1720: CXFA_Node* pPendingPageSet =
GetRootLayoutItem()->m_pFormNode;
On 2016/04/14 22:45:44, Tom Sepez wrote:
> nit: maybe different name to avoid shadowing confusions.

Done.

https://codereview.chromium.org/1890563003/diff/160001/xfa/fxfa/parser/xfa_ob...
File xfa/fxfa/parser/xfa_object_imp.cpp (right):

https://codereview.chromium.org/1890563003/diff/160001/xfa/fxfa/parser/xfa_ob...
xfa/fxfa/parser/xfa_object_imp.cpp:1445: if (pParent->IsContainerNode()) {
On 2016/04/14 22:45:44, Tom Sepez wrote:
> nit: {}

Done.

https://codereview.chromium.org/1890563003/diff/200001/xfa/fxfa/app/xfa_ffnot...
File xfa/fxfa/app/xfa_ffnotify.cpp (right):

https://codereview.chromium.org/1890563003/diff/200001/xfa/fxfa/app/xfa_ffnot...
xfa/fxfa/app/xfa_ffnotify.cpp:58: CXFA_FFWidget* pWidget =
pWidgetAcc->GetNextWidget(NULL);
On 2016/04/14 22:08:55, Tom Sepez wrote:
> nit: nullptr

Done.

https://codereview.chromium.org/1890563003/diff/200001/xfa/fxfa/app/xfa_ffnot...
xfa/fxfa/app/xfa_ffnotify.cpp:77: CXFA_FFWidget* pWidget =
pWidgetAcc->GetNextWidget(NULL);
On 2016/04/14 22:08:55, Tom Sepez wrote:
> nit: maybe start with NULL, and implement loop along the lines of line 364

Done.

https://codereview.chromium.org/1890563003/diff/200001/xfa/fxfa/app/xfa_ffnot...
xfa/fxfa/app/xfa_ffnotify.cpp:366: if (pWidget->IsLoaded()) {
On 2016/04/14 22:08:55, Tom Sepez wrote:
> nit: {} not needed.

Done.

https://codereview.chromium.org/1890563003/diff/200001/xfa/fxfa/app/xfa_ffnot...
xfa/fxfa/app/xfa_ffnotify.cpp:390: if (!pWidgetAcc) {
On 2016/04/14 22:08:55, Tom Sepez wrote:
> nit: {} not needed, couple of places in this method.

Done.

https://codereview.chromium.org/1890563003/diff/200001/xfa/fxfa/app/xfa_ffnot...
xfa/fxfa/app/xfa_ffnotify.cpp:411: break;
On 2016/04/14 22:08:55, Tom Sepez wrote:
> nit: lump default together with these cases.

Done.

https://codereview.chromium.org/1890563003/diff/200001/xfa/fxfa/parser/xfa_ob...
File xfa/fxfa/parser/xfa_object_imp.cpp (right):

https://codereview.chromium.org/1890563003/diff/200001/xfa/fxfa/parser/xfa_ob...
xfa/fxfa/parser/xfa_object_imp.cpp:4850: void CXFA_Node::SetFlag(uint32_t
dwFlag, FX_BOOL bOn, bool bNotify) {
On 2016/04/14 22:08:55, Tom Sepez wrote:
> might be worth having a separate ClearFlag rather than taking a boolean arg
...
> I doubt we ever pass a computed value vs. "false"

Done.

[Lei Zhang, 2016-04-15 01:18:54]

The CQ bit was checked by thestig@chromium.org

[Lei Zhang, 2016-04-15 01:18:55]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/1890563003/#ps220001
(title: "address comments")

[commit-bot: I haz the power, 2016-04-15 01:18:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1890563003/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1890563003/220001

[commit-bot: I haz the power, 2016-04-15 01:29:59]

Description was changed from

==========
Simplify XFA event handling.

- Avoid casting to void*
- Don't bother firing events that no one listens for
==========

to

==========
Simplify XFA event handling.

- Avoid casting to void*
- Don't bother firing events that no one listens for

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

[commit-bot: I haz the power, 2016-04-15 01:30:00]

Committed patchset #11 (id:220001) as
https://pdfium.googlesource.com/pdfium/+/b1a59597db4b8ce3ffc34dbfda2fb1ecb80c...