Chromium Code Reviews
Help | Chromium Project | Sign in
(2)

Issue 2227173002: Merge CPDFSDK_FormActionHandler into CPDFSDK_ActionHandler (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 months, 2 weeks ago by tonikitoo
Modified:
8 months, 2 weeks ago
CC:
pdfium-reviews_googlegroups.com
Base URL:
https://pdfium.googlesource.com/pdfium.git@master
Target Ref:
refs/heads/master
Project:
pdfium
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 1

Patch Set 2 : Merge CPDFSDK_FormActionHandler into CPDFSDK_ActionHandler #

Unified diffs Side-by-side diffs Delta from patch set Stats (+17 lines, -39 lines) Patch
M fpdfsdk/fsdk_actionhandler.cpp View 1 4 chunks +10 lines, -24 lines 0 comments Download
M fpdfsdk/include/fsdk_actionhandler.h View 1 2 chunks +7 lines, -15 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 13 (5 generated)
tonikitoo
PTAL
8 months, 2 weeks ago (2016-08-09 20:59:09 UTC) #3
Tom Sepez
Sorry to say, I'm not sure if this is that much simpler, despite your effort. ...
8 months, 2 weeks ago (2016-08-09 21:11:20 UTC) #4
tonikitoo
On 2016/08/09 21:11:20, Tom Sepez wrote: > Sorry to say, I'm not sure if this ...
8 months, 2 weeks ago (2016-08-09 21:50:48 UTC) #5
Tom Sepez
I like that approach. Let's see how that goes.
8 months, 2 weeks ago (2016-08-09 22:02:40 UTC) #6
tonikitoo
On 2016/08/09 22:02:40, Tom Sepez wrote: > I like that approach. Let's see how that ...
8 months, 2 weeks ago (2016-08-09 22:15:08 UTC) #8
Tom Sepez
lgtm
8 months, 2 weeks ago (2016-08-09 22:32:29 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2227173002/20001
8 months, 2 weeks ago (2016-08-09 23:48:18 UTC) #11
commit-bot: I haz the power
8 months, 2 weeks ago (2016-08-10 00:07:58 UTC) #13
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as
https://pdfium.googlesource.com/pdfium/+/f4dc38bbc52501b322e1c39115f5ce423177...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46