Merge CPDFSDK_FormActionHandler into CPDFSDK_ActionHandler

The benefits of having a CPDFSDK_FormActionHandler instance in
CPDFSDK_ActionHandler are unclear. It does not add encapsulation, nor
simplicity to the logic.

CL is a driven-by clean up that merges CPDFSDK_FormActionHandler into
CPDFSDK_ActionHandler.
It takes the opportunity to delete CPDFSDK_ActionHandler (now) empty
constructor and destructor, in favor of compiler-generated ones.

No behavior change is expected, so no new tests are being added.

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

[tonikitoo, 2016-08-09 20:58:52]

Description was changed from

==========
Clean up "non JS" actions handling in fsdk_actionhandler.cpp

The CPDFSDK_ActionHandler class is supposed to be a "centralized" place
where PDF actions are handled. Well, in fact it is, but given the way
it has evolved, its implementation and logic flow is not as simple to follow
as it could.
This CL aims to be a first step to "uncomplicate"  it.

In summary, CL adds an internal class named NoJsActionHandler,
which has an interface as simple as:

  (..)
  NoJsActionHandler(action, document).Run();
  (..)

Note that the clas is designed with simplicity, and in fact centralizes
the handling of non-javascript actions:

- its constructor takes the two arguments needed set up the object
instance.
- once set, its "Run" method calls the actual handler method for
the "action" it was constructed with.

Additionally, NoJsActionHandler also incorporates CPDFSDK_FormActionHandler
class into it, also for simplicity.

Last, no behavior change is expected, so no new tests are being added.

As a follow up step, much of the existing code duplication in the remaining
methods of the CPDFSDK_ActionHandler class will be eliminated.

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

to

==========
Clean up "non JS" actions handling in fsdk_actionhandler.cpp

The CPDFSDK_ActionHandler class is supposed to be a "centralized" place
where PDF actions are handled. Well, in fact it is, but given the way
it has evolved, its implementation and logic flow is not as simple to follow
as it could.
This CL aims to be a first step to "uncomplicate"  it.

In summary, CL adds an internal class named NoJsActionHandler,
which has an interface as simple as:

  (..)
  NoJsActionHandler(action, document).Run();
  (..)

Note that the clas is designed with simplicity, and in fact centralizes
the handling of non-javascript actions:

- its constructor takes the two arguments needed set up the object
instance.
- once set, its "Run" method calls the actual handler method for
the "action" it was constructed with.

Additionally, NoJsActionHandler also incorporates CPDFSDK_FormActionHandler
class into it, also for simplicity.

Last, no behavior change is expected, so no new tests are being added.

As a follow up step, much of the existing code duplication in the remaining
methods of the CPDFSDK_ActionHandler class will be eliminated.

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

[tonikitoo, 2016-08-09 20:58:53]

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

[tonikitoo, 2016-08-09 20:59:09]

PTAL

[Tom Sepez, 2016-08-09 21:11:20]

Sorry to say, I'm not sure if this is that much simpler, despite your effort. 
Avoiding c-style indirect function calls seems preferable to me.  Also, not sure
why we'd cobble up a "closure"-like object unless we were going to pass it
around.

https://codereview.chromium.org/2227173002/diff/1/fpdfsdk/fsdk_actionhandler.cpp
File fpdfsdk/fsdk_actionhandler.cpp (right):

https://codereview.chromium.org/2227173002/diff/1/fpdfsdk/fsdk_actionhandler....
fpdfsdk/fsdk_actionhandler.cpp:51: Callback m_callback = nullptr;
Instead of storing the callback, can we store the action type and switch on it
during the Run() method?

We don't have Chromium's base::bind() here in PDFium, which would be preferred
way of doing this.

[tonikitoo, 2016-08-09 21:50:48]

On 2016/08/09 21:11:20, Tom Sepez wrote:
> Sorry to say, I'm not sure if this is that much simpler, despite your effort. 
> Avoiding c-style indirect function calls seems preferable to me.  Also, not
sure
> why we'd cobble up a "closure"-like object unless we were going to pass it
> around.
> 
>
https://codereview.chromium.org/2227173002/diff/1/fpdfsdk/fsdk_actionhandler.cpp
> File fpdfsdk/fsdk_actionhandler.cpp (right):
> 
>
https://codereview.chromium.org/2227173002/diff/1/fpdfsdk/fsdk_actionhandler....
> fpdfsdk/fsdk_actionhandler.cpp:51: Callback m_callback = nullptr;
> Instead of storing the callback, can we store the action type and switch on it
> during the Run() method?
> 
> We don't have Chromium's base::bind() here in PDFium, which would be preferred
> way of doing this.

Thanks for looking.

an even simpler approach would be just get rid of CPDFSDK_FormActionHandler
altogether (and also the declaration of m_pFormActionHandler in
CPDFSDK_ActionHandler), and move the methods of the former to the later.

This way the exiting "switch"-based logic in
CPDFSDK_ActionHandler::DoAction_NoJs which remain almost untouched. 

Basically, I do not see the point of having CPDFSDK_FormActionHandler, which as
a driven-by motivation of the clean up.

What do you think?

[Tom Sepez, 2016-08-09 22:02:40]

I like that approach.  Let's see how that goes.

[tonikitoo, 2016-08-09 22:13:50]

Description was changed from

==========
Clean up "non JS" actions handling in fsdk_actionhandler.cpp

The CPDFSDK_ActionHandler class is supposed to be a "centralized" place
where PDF actions are handled. Well, in fact it is, but given the way
it has evolved, its implementation and logic flow is not as simple to follow
as it could.
This CL aims to be a first step to "uncomplicate"  it.

In summary, CL adds an internal class named NoJsActionHandler,
which has an interface as simple as:

  (..)
  NoJsActionHandler(action, document).Run();
  (..)

Note that the clas is designed with simplicity, and in fact centralizes
the handling of non-javascript actions:

- its constructor takes the two arguments needed set up the object
instance.
- once set, its "Run" method calls the actual handler method for
the "action" it was constructed with.

Additionally, NoJsActionHandler also incorporates CPDFSDK_FormActionHandler
class into it, also for simplicity.

Last, no behavior change is expected, so no new tests are being added.

As a follow up step, much of the existing code duplication in the remaining
methods of the CPDFSDK_ActionHandler class will be eliminated.

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

to

==========
The benefits of having a CPDFSDK_FormActionHandler instance in
CPDFSDK_ActionHandler are unclear. It does not add encapsulation, nor
simplicity to the logic.

CL is a driven-by clean up that merges CPDFSDK_FormActionHandler into
CPDFSDK_ActionHandler.
It takes the opportunity to delete CPDFSDK_ActionHandler (now) empty
constructor and destructor, in favor of compiler-generated ones.

No behavior change is expected, so no new tests are being added.
==========

[tonikitoo, 2016-08-09 22:15:08]

On 2016/08/09 22:02:40, Tom Sepez wrote:
> I like that approach.  Let's see how that goes.

Done in patchset 2.

[Tom Sepez, 2016-08-09 22:32:29]

lgtm

[tonikitoo, 2016-08-09 23:48:10]

The CQ bit was checked by tonikitoo@igalia.com

[commit-bot: I haz the power, 2016-08-09 23:48:18]

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 00:07:57]

Description was changed from

==========
The benefits of having a CPDFSDK_FormActionHandler instance in
CPDFSDK_ActionHandler are unclear. It does not add encapsulation, nor
simplicity to the logic.

CL is a driven-by clean up that merges CPDFSDK_FormActionHandler into
CPDFSDK_ActionHandler.
It takes the opportunity to delete CPDFSDK_ActionHandler (now) empty
constructor and destructor, in favor of compiler-generated ones.

No behavior change is expected, so no new tests are being added.
==========

to

==========
The benefits of having a CPDFSDK_FormActionHandler instance in
CPDFSDK_ActionHandler are unclear. It does not add encapsulation, nor
simplicity to the logic.

CL is a driven-by clean up that merges CPDFSDK_FormActionHandler into
CPDFSDK_ActionHandler.
It takes the opportunity to delete CPDFSDK_ActionHandler (now) empty
constructor and destructor, in favor of compiler-generated ones.

No behavior change is expected, so no new tests are being added.

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

[commit-bot: I haz the power, 2016-08-10 00:07:58]

Committed patchset #2 (id:20001) as
https://pdfium.googlesource.com/pdfium/+/f4dc38bbc52501b322e1c39115f5ce423177...