Cleanup FWL Event and Message code.

Cleanup FWL Event and Message code.

This CL removes any CFWL_Evt classes for which nothing was listening to the
events. For events with no parameters the override class was removed and
the event type passed into CFWL_Event which is used instead. Any event or
message parameters which are un-read have been removed.

Committed: https://pdfium.googlesource.com/pdfium/+/4614b45a6592530c4cd930f139c366ce20d359f4

[dsinclair, 2016-11-25 04:31:33]

The CQ bit was checked by dsinclair@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-25 04:31:38]

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

[dsinclair, 2016-11-25 04:37:40]

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

[dsinclair, 2016-11-25 04:37:41]

PTAL.

[commit-bot: I haz the power, 2016-11-25 04:42:07]

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

[commit-bot: I haz the power, 2016-11-25 04:42:08]

Dry run: This issue passed the CQ dry run.

[npm, 2016-11-28 15:16:57]

https://codereview.chromium.org/2530993002/diff/1/xfa/fwl/core/cfwl_widget.cpp
File xfa/fwl/core/cfwl_widget.cpp (left):

https://codereview.chromium.org/2530993002/diff/1/xfa/fwl/core/cfwl_widget.cp...
xfa/fwl/core/cfwl_widget.cpp:92: if (FXSYS_fabs(rtOld.width - rect.width) > 0.5f
||
Why is this being deleted?

https://codereview.chromium.org/2530993002/diff/1/xfa/fwl/core/cfwl_widget.cp...
xfa/fwl/core/cfwl_widget.cpp:610: DispatchEvent(pEvent.get());
Ditto

[Tom Sepez, 2016-11-28 18:26:33]

https://codereview.chromium.org/2530993002/diff/1/xfa/fwl/core/cfwl_event.h
File xfa/fwl/core/cfwl_event.h (right):

https://codereview.chromium.org/2530993002/diff/1/xfa/fwl/core/cfwl_event.h#n...
xfa/fwl/core/cfwl_event.h:18: CheckStateChanged,
Maybe want None = 0, worry about implicit cast to bools?

https://codereview.chromium.org/2530993002/diff/1/xfa/fwl/core/cfwl_pushbutto...
File xfa/fwl/core/cfwl_pushbutton.cpp (left):

https://codereview.chromium.org/2530993002/diff/1/xfa/fwl/core/cfwl_pushbutto...
xfa/fwl/core/cfwl_pushbutton.cpp:310: if (pMsg->m_dwKeyCode != FWL_VKEY_Tab)
Do we still want to dispatch tab events?

[dsinclair, 2016-12-05 20:14:05]

https://codereview.chromium.org/2530993002/diff/1/xfa/fwl/core/cfwl_event.h
File xfa/fwl/core/cfwl_event.h (right):

https://codereview.chromium.org/2530993002/diff/1/xfa/fwl/core/cfwl_event.h#n...
xfa/fwl/core/cfwl_event.h:18: CheckStateChanged,
On 2016/11/28 18:26:33, Tom Sepez wrote:
> Maybe want None = 0, worry about implicit cast to bools?

There should be no implicit cast as it's an enum class. At least, that's my
understanding, is that incorrect?

https://codereview.chromium.org/2530993002/diff/1/xfa/fwl/core/cfwl_pushbutto...
File xfa/fwl/core/cfwl_pushbutton.cpp (left):

https://codereview.chromium.org/2530993002/diff/1/xfa/fwl/core/cfwl_pushbutto...
xfa/fwl/core/cfwl_pushbutton.cpp:310: if (pMsg->m_dwKeyCode != FWL_VKEY_Tab)
On 2016/11/28 18:26:33, Tom Sepez wrote:
> Do we still want to dispatch tab events?

There is nothing that listens to key events, so dispatching the tab event will
have no effect that I can see.

https://codereview.chromium.org/2530993002/diff/1/xfa/fwl/core/cfwl_widget.cpp
File xfa/fwl/core/cfwl_widget.cpp (left):

https://codereview.chromium.org/2530993002/diff/1/xfa/fwl/core/cfwl_widget.cp...
xfa/fwl/core/cfwl_widget.cpp:92: if (FXSYS_fabs(rtOld.width - rect.width) > 0.5f
||
On 2016/11/28 15:16:57, npm wrote:
> Why is this being deleted?

Nothing listens for EvtSizeChanged so this code ends up doing nothing.

https://codereview.chromium.org/2530993002/diff/1/xfa/fwl/core/cfwl_widget.cp...
xfa/fwl/core/cfwl_widget.cpp:610: DispatchEvent(pEvent.get());
On 2016/11/28 15:16:57, npm wrote:
> Ditto

Nothing listens for EvtKey.

[npm, 2016-12-05 20:42:38]

https://codereview.chromium.org/2530993002/diff/1/xfa/fwl/core/cfwl_widget.cpp
File xfa/fwl/core/cfwl_widget.cpp (left):

https://codereview.chromium.org/2530993002/diff/1/xfa/fwl/core/cfwl_widget.cp...
xfa/fwl/core/cfwl_widget.cpp:92: if (FXSYS_fabs(rtOld.width - rect.width) > 0.5f
||
On 2016/12/05 20:14:05, dsinclair wrote:
> On 2016/11/28 15:16:57, npm wrote:
> > Why is this being deleted?
> 
> Nothing listens for EvtSizeChanged so this code ends up doing nothing.

But EvtSizeChanged is a CFWL_Event, and m_pSrcTarget belongs there.

https://codereview.chromium.org/2530993002/diff/1/xfa/fwl/core/cfwl_widget.cp...
xfa/fwl/core/cfwl_widget.cpp:610: DispatchEvent(pEvent.get());
On 2016/12/05 20:14:05, dsinclair wrote:
> On 2016/11/28 15:16:57, npm wrote:
> > Ditto
> 
> Nothing listens for EvtKey.

Similar reply as above.

[dsinclair, 2016-12-06 14:05:50]

https://codereview.chromium.org/2530993002/diff/1/xfa/fwl/core/cfwl_widget.cpp
File xfa/fwl/core/cfwl_widget.cpp (left):

https://codereview.chromium.org/2530993002/diff/1/xfa/fwl/core/cfwl_widget.cp...
xfa/fwl/core/cfwl_widget.cpp:92: if (FXSYS_fabs(rtOld.width - rect.width) > 0.5f
||
On 2016/12/05 20:42:38, npm wrote:
> On 2016/12/05 20:14:05, dsinclair wrote:
> > On 2016/11/28 15:16:57, npm wrote:
> > > Why is this being deleted?
> > 
> > Nothing listens for EvtSizeChanged so this code ends up doing nothing.
> 
> But EvtSizeChanged is a CFWL_Event, and m_pSrcTarget belongs there.


The CFWL_Event classes are used in the OnProcessEvent methods. I've gone through
all of those and they all explicitly check for the event type before doing any
processing.

https://codereview.chromium.org/2530993002/diff/1/xfa/fwl/core/cfwl_widget.cp...
xfa/fwl/core/cfwl_widget.cpp:610: DispatchEvent(pEvent.get());
On 2016/12/05 20:42:38, npm wrote:
> On 2016/12/05 20:14:05, dsinclair wrote:
> > On 2016/11/28 15:16:57, npm wrote:
> > > Ditto
> > 
> > Nothing listens for EvtKey.
> 
> Similar reply as above.

ditto.

[dsinclair, 2016-12-06 14:05:55]

The CQ bit was checked by dsinclair@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 14:05:57]

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-12-06 14:18:28]

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

[commit-bot: I haz the power, 2016-12-06 14:18:28]

Dry run: This issue passed the CQ dry run.

[dsinclair, 2016-12-07 13:55:16]

ping

[Tom Sepez, 2016-12-07 18:48:27]

Up to you how much of this you want to do.  LGTM otherwise.

If you want to be really compulsive, I'm not crazy about the names
CFWL_EvtScroll and so forth, a a subclasses of CFWL_Event should be called
something like CFWL_ScrollEvent, just cause adjectives come first in English.

https://codereview.chromium.org/2530993002/diff/20001/xfa/fwl/core/cfwl_event.h
File xfa/fwl/core/cfwl_event.h (right):

https://codereview.chromium.org/2530993002/diff/20001/xfa/fwl/core/cfwl_event...
xfa/fwl/core/cfwl_event.h:17: enum class CFWL_EventType {
nit: If I were really compulsive, I'd nest this as CFWL_Event::Type.

https://codereview.chromium.org/2530993002/diff/20001/xfa/fwl/core/cfwl_event...
xfa/fwl/core/cfwl_event.h:38: explicit CFWL_Event(CFWL_EventType type);
nit: let's add two more ctors:

CFWL_Event(CFWL_EventType type, CFWL_Widget* pSrcTarget);
CFWL_Event(CFWL_EventType type, CFWL_Widget* pSrcTarget, CFWL_Widget*
pDstTarget);

since we nearly always set one or both these immediately after making one.

https://codereview.chromium.org/2530993002/diff/20001/xfa/fwl/core/cfwl_event...
xfa/fwl/core/cfwl_event.h:41: CFWL_EventType GetClassID() const { return m_type;
}
nit: bad name, maybe GetEventType() ?

https://codereview.chromium.org/2530993002/diff/20001/xfa/fwl/core/cfwl_evtch...
File xfa/fwl/core/cfwl_evtcheckword.cpp (right):

https://codereview.chromium.org/2530993002/diff/20001/xfa/fwl/core/cfwl_evtch...
xfa/fwl/core/cfwl_evtcheckword.cpp:9: CFWL_EvtCheckWord::CFWL_EvtCheckWord()
nit: maybe additional ctor forms for all of these with src/dst targets.

https://codereview.chromium.org/2530993002/diff/20001/xfa/fwl/core/cfwl_evtsc...
File xfa/fwl/core/cfwl_evtscroll.h (right):

https://codereview.chromium.org/2530993002/diff/20001/xfa/fwl/core/cfwl_evtsc...
xfa/fwl/core/cfwl_evtscroll.h:12: enum class FWL_SCBCODE {
nitto with nesting as CFWL_EvtScroll::Code

https://codereview.chromium.org/2530993002/diff/20001/xfa/fwl/core/cfwl_messa...
File xfa/fwl/core/cfwl_message.h (right):

https://codereview.chromium.org/2530993002/diff/20001/xfa/fwl/core/cfwl_messa...
xfa/fwl/core/cfwl_message.h:17: enum class CFWL_MessageType {
nitto

[dsinclair, 2016-12-07 20:02:02]

PTAL.

I agree about the class names, but am holding off for a followup. There are a
bunch of changes in the new patchset so please give a re-review.

https://codereview.chromium.org/2530993002/diff/20001/xfa/fwl/core/cfwl_event.h
File xfa/fwl/core/cfwl_event.h (right):

https://codereview.chromium.org/2530993002/diff/20001/xfa/fwl/core/cfwl_event...
xfa/fwl/core/cfwl_event.h:17: enum class CFWL_EventType {
On 2016/12/07 18:48:26, Tom Sepez wrote:
> nit: If I were really compulsive, I'd nest this as CFWL_Event::Type.

Done.

https://codereview.chromium.org/2530993002/diff/20001/xfa/fwl/core/cfwl_event...
xfa/fwl/core/cfwl_event.h:38: explicit CFWL_Event(CFWL_EventType type);
On 2016/12/07 18:48:26, Tom Sepez wrote:
> nit: let's add two more ctors:
> 
> CFWL_Event(CFWL_EventType type, CFWL_Widget* pSrcTarget);
> CFWL_Event(CFWL_EventType type, CFWL_Widget* pSrcTarget, CFWL_Widget*
> pDstTarget);
> 
> since we nearly always set one or both these immediately after making one.

Done.

https://codereview.chromium.org/2530993002/diff/20001/xfa/fwl/core/cfwl_event...
xfa/fwl/core/cfwl_event.h:41: CFWL_EventType GetClassID() const { return m_type;
}
On 2016/12/07 18:48:26, Tom Sepez wrote:
> nit: bad name, maybe GetEventType() ?

Went with GetType()

https://codereview.chromium.org/2530993002/diff/20001/xfa/fwl/core/cfwl_evtch...
File xfa/fwl/core/cfwl_evtcheckword.cpp (right):

https://codereview.chromium.org/2530993002/diff/20001/xfa/fwl/core/cfwl_evtch...
xfa/fwl/core/cfwl_evtcheckword.cpp:9: CFWL_EvtCheckWord::CFWL_EvtCheckWord()
On 2016/12/07 18:48:26, Tom Sepez wrote:
> nit: maybe additional ctor forms for all of these with src/dst targets.

Done.

https://codereview.chromium.org/2530993002/diff/20001/xfa/fwl/core/cfwl_evtsc...
File xfa/fwl/core/cfwl_evtscroll.h (right):

https://codereview.chromium.org/2530993002/diff/20001/xfa/fwl/core/cfwl_evtsc...
xfa/fwl/core/cfwl_evtscroll.h:12: enum class FWL_SCBCODE {
On 2016/12/07 18:48:26, Tom Sepez wrote:
> nitto with nesting as CFWL_EvtScroll::Code

Done.

https://codereview.chromium.org/2530993002/diff/20001/xfa/fwl/core/cfwl_messa...
File xfa/fwl/core/cfwl_message.h (right):

https://codereview.chromium.org/2530993002/diff/20001/xfa/fwl/core/cfwl_messa...
xfa/fwl/core/cfwl_message.h:17: enum class CFWL_MessageType {
On 2016/12/07 18:48:26, Tom Sepez wrote:
> nitto 

Done.

[dsinclair, 2016-12-07 20:02:11]

The CQ bit was checked by dsinclair@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 20:02:22]

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-12-07 20:13:52]

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

[commit-bot: I haz the power, 2016-12-07 20:13:53]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2016-12-08 00:56:46]

lgtm

https://codereview.chromium.org/2530993002/diff/40001/xfa/fwl/core/cfwl_event.h
File xfa/fwl/core/cfwl_event.h (right):

https://codereview.chromium.org/2530993002/diff/40001/xfa/fwl/core/cfwl_event...
xfa/fwl/core/cfwl_event.h:22: enum class Type {
nit:  given that we're already qualifiying the names with CFWL_Event, you might
go back to regular eums here, and avoid typing a bunch of |::Type|.

[dsinclair, 2016-12-08 00:59:54]

https://codereview.chromium.org/2530993002/diff/40001/xfa/fwl/core/cfwl_event.h
File xfa/fwl/core/cfwl_event.h (right):

https://codereview.chromium.org/2530993002/diff/40001/xfa/fwl/core/cfwl_event...
xfa/fwl/core/cfwl_event.h:22: enum class Type {
On 2016/12/08 00:56:46, Tom Sepez wrote:
> nit:  given that we're already qualifiying the names with CFWL_Event, you
might
> go back to regular eums here, and avoid typing a bunch of |::Type|.

I prefer to keep this an enum class so that we can't accidentally case to an int
or back. We know these are the only values.

[dsinclair, 2016-12-08 01:01:24]

The CQ bit was checked by dsinclair@chromium.org

[commit-bot: I haz the power, 2016-12-08 01:01:29]

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-12-08 01:01:59]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481158884374890,
"parent_rev": "248cb27e64b3a25230f53fc2f4ab9d483facc5f9", "commit_rev":
"4614b45a6592530c4cd930f139c366ce20d359f4"}

[commit-bot: I haz the power, 2016-12-08 01:02:01]

Description was changed from

==========
Cleanup FWL Event and Message code.

This CL removes any CFWL_Evt classes for which nothing was listening to the
events. For events with no parameters the override class was removed and
the event type passed into CFWL_Event which is used instead. Any event or
message parameters which are un-read have been removed.
==========

to

==========
Cleanup FWL Event and Message code.

This CL removes any CFWL_Evt classes for which nothing was listening to the
events. For events with no parameters the override class was removed and
the event type passed into CFWL_Event which is used instead. Any event or
message parameters which are un-read have been removed.

Committed:
https://pdfium.googlesource.com/pdfium/+/4614b45a6592530c4cd930f139c366ce20d3...
==========

[commit-bot: I haz the power, 2016-12-08 01:02:03]

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