Pass objects instead of strings for undo/redo records.

Pass objects instead of strings for undo/redo records.

Currently the Undo/Redo records are serialized as byte strings and stored
into a CFX_ByteStringArray. They are deserialized when used.

This CL removes the serialization and stores the objects in a deque of unique
pointers.

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

[dsinclair, 2016-05-17 19:21:23]

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

[dsinclair, 2016-05-17 19:21:24]

PTAL.

[Tom Sepez, 2016-05-17 20:11:01]

On 2016/05/17 19:21:24, dsinclair wrote:
> PTAL.

Sure we don't persist these when saving a file, for example?  So that you have
undo when you open it again?

[dsinclair, 2016-05-17 20:16:05]

On 2016/05/17 20:11:01, Tom Sepez wrote:
> On 2016/05/17 19:21:24, dsinclair wrote:
> > PTAL.
> 
> Sure we don't persist these when saving a file, for example?  So that you have
> undo when you open it again?


It doesn't look like it. The only place we stored them was in the array in
fwl_editimp which I changed to a deque. I don't see any usage which is just
reading them other then to pass them to Undo/Redo.

[Tom Sepez, 2016-05-17 20:37:59]

https://codereview.chromium.org/1980293004/diff/20001/xfa/fwl/basewidget/fwl_...
File xfa/fwl/basewidget/fwl_comboboximp.cpp (right):

https://codereview.chromium.org/1980293004/diff/20001/xfa/fwl/basewidget/fwl_...
xfa/fwl/basewidget/fwl_comboboximp.cpp:793: if (!m_pEdit)
nit: or just return m_pEdit && m_pEdit->Redo(pRecord);

https://codereview.chromium.org/1980293004/diff/20001/xfa/fwl/basewidget/fwl_...
xfa/fwl/basewidget/fwl_comboboximp.cpp:800: return m_pEdit->Undo(pRecord);
same here.

https://codereview.chromium.org/1980293004/diff/20001/xfa/fwl/basewidget/fwl_...
File xfa/fwl/basewidget/fwl_editimp.cpp (right):

https://codereview.chromium.org/1980293004/diff/20001/xfa/fwl/basewidget/fwl_...
xfa/fwl/basewidget/fwl_editimp.cpp:1475: for (int32_t i = nCount - 1; i >
m_iCurRecord; i--)
There's got to be an erase call that does this in one fell swoop?

https://codereview.chromium.org/1980293004/diff/20001/xfa/fwl/lightwidget/cfw...
File xfa/fwl/lightwidget/cfwl_edit.cpp (right):

https://codereview.chromium.org/1980293004/diff/20001/xfa/fwl/lightwidget/cfw...
xfa/fwl/lightwidget/cfwl_edit.cpp:160: return
static_cast<IFWL_Edit*>(m_pIface)->Redo(pRecord);
same here, return m_pIface && ...

https://codereview.chromium.org/1980293004/diff/20001/xfa/fwl/lightwidget/cfw...
xfa/fwl/lightwidget/cfwl_edit.cpp:166: return
static_cast<IFWL_Edit*>(m_pIface)->Undo(pRecord);
and here

[dsinclair, 2016-05-17 20:50:03]

https://codereview.chromium.org/1980293004/diff/20001/xfa/fwl/basewidget/fwl_...
File xfa/fwl/basewidget/fwl_comboboximp.cpp (right):

https://codereview.chromium.org/1980293004/diff/20001/xfa/fwl/basewidget/fwl_...
xfa/fwl/basewidget/fwl_comboboximp.cpp:793: if (!m_pEdit)
On 2016/05/17 20:37:59, Tom Sepez wrote:
> nit: or just return m_pEdit && m_pEdit->Redo(pRecord);

Done.

https://codereview.chromium.org/1980293004/diff/20001/xfa/fwl/basewidget/fwl_...
xfa/fwl/basewidget/fwl_comboboximp.cpp:800: return m_pEdit->Undo(pRecord);
On 2016/05/17 20:37:59, Tom Sepez wrote:
> same here.

Done.

https://codereview.chromium.org/1980293004/diff/20001/xfa/fwl/basewidget/fwl_...
File xfa/fwl/basewidget/fwl_editimp.cpp (right):

https://codereview.chromium.org/1980293004/diff/20001/xfa/fwl/basewidget/fwl_...
xfa/fwl/basewidget/fwl_editimp.cpp:1475: for (int32_t i = nCount - 1; i >
m_iCurRecord; i--)
On 2016/05/17 20:37:59, Tom Sepez wrote:
> There's got to be an erase call that does this in one fell swoop?

Done.

https://codereview.chromium.org/1980293004/diff/20001/xfa/fwl/lightwidget/cfw...
File xfa/fwl/lightwidget/cfwl_edit.cpp (right):

https://codereview.chromium.org/1980293004/diff/20001/xfa/fwl/lightwidget/cfw...
xfa/fwl/lightwidget/cfwl_edit.cpp:160: return
static_cast<IFWL_Edit*>(m_pIface)->Redo(pRecord);
On 2016/05/17 20:37:59, Tom Sepez wrote:
> same here, return m_pIface && ...

Done.

https://codereview.chromium.org/1980293004/diff/20001/xfa/fwl/lightwidget/cfw...
xfa/fwl/lightwidget/cfwl_edit.cpp:166: return
static_cast<IFWL_Edit*>(m_pIface)->Undo(pRecord);
On 2016/05/17 20:37:59, Tom Sepez wrote:
> and here

Done.

[Tom Sepez, 2016-05-17 21:24:38]

lgtm

https://codereview.chromium.org/1980293004/diff/40001/xfa/fwl/basewidget/fwl_...
File xfa/fwl/basewidget/fwl_editimp.cpp (right):

https://codereview.chromium.org/1980293004/diff/40001/xfa/fwl/basewidget/fwl_...
xfa/fwl/basewidget/fwl_editimp.cpp:1475: m_DoRecords.erase(m_DoRecords.begin() +
m_iCurRecord + 1,
Maybe m_iCurRecord should itself be an iterator.

[dsinclair, 2016-05-18 13:09:06]

https://codereview.chromium.org/1980293004/diff/40001/xfa/fwl/basewidget/fwl_...
File xfa/fwl/basewidget/fwl_editimp.cpp (right):

https://codereview.chromium.org/1980293004/diff/40001/xfa/fwl/basewidget/fwl_...
xfa/fwl/basewidget/fwl_editimp.cpp:1475: m_DoRecords.erase(m_DoRecords.begin() +
m_iCurRecord + 1,
On 2016/05/17 21:24:38, Tom Sepez wrote:
> Maybe m_iCurRecord should itself be an iterator.

Filed pdfium:494 to track.

[dsinclair, 2016-05-18 13:09:11]

The CQ bit was checked by dsinclair@chromium.org

[commit-bot: I haz the power, 2016-05-18 13:09:16]

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

[commit-bot: I haz the power, 2016-05-18 13:09:38]

Description was changed from

==========
Pass objects instead of strings for undo/redo records.

Currently the Undo/Redo records are serialized as byte strings and stored
into a CFX_ByteStringArray. They are deserialized when used.

This CL removes the serialization and stores the objects in a deque of unique
pointers.
==========

to

==========
Pass objects instead of strings for undo/redo records.

Currently the Undo/Redo records are serialized as byte strings and stored
into a CFX_ByteStringArray. They are deserialized when used.

This CL removes the serialization and stores the objects in a deque of unique
pointers.

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

[commit-bot: I haz the power, 2016-05-18 13:09:39]

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

[Lei Zhang, 2016-05-19 23:46:48]

https://codereview.chromium.org/1980293004/diff/40001/xfa/fwl/basewidget/fwl_...
File xfa/fwl/basewidget/fwl_editimp.cpp (right):

https://codereview.chromium.org/1980293004/diff/40001/xfa/fwl/basewidget/fwl_...
xfa/fwl/basewidget/fwl_editimp.cpp:785: return m_iCurRecord < m_DoRecords.size()
- 1;
When I tried to roll DEPS, this failed on the Chromium Windows trybots with:

FAILED: obj/third_party/pdfium/xfa/fwl_editimp.obj 
ninja -t msvc -e environment.x86 -- E:\b\build\slave\cache\cipd\goma/gomacc.exe
"E:\b\depot_tools\win_toolchain\vs_files\95ddda401ec5678f15eeed01d2bee08fcbc5ee97\VC\bin\amd64_x86/cl.exe"
/nologo /showIncludes /FC @obj/third_party/pdfium/xfa/fwl_editimp.obj.rsp /c
../../third_party/pdfium/xfa/fwl/basewidget/fwl_editimp.cpp
/Foobj/third_party/pdfium/xfa/fwl_editimp.obj
/Fd"obj/third_party/pdfium/xfa_cc.pdb"
e:\b\build\slave\win_gn\build\src\third_party\pdfium\xfa\fwl\basewidget\fwl_editimp.cpp(782):
error C2220: warning treated as error - no 'object' file generated
e:\b\build\slave\win_gn\build\src\third_party\pdfium\xfa\fwl\basewidget\fwl_editimp.cpp(782):
warning C4018: '<': signed/unsigned mismatch

https://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng...