Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2018733002:
Clean up part of CXFA_Node class (Closed)
DescriptionClean up part of CXFA_Node class
Mainly clean up Script_NodeClass_Xxx() functions. There are no behavior
or API changes. The clean up mainly includes moving static functions into
namespace, remove unnecessary conditions or braces, changing
NULL->nullptr and local FX_BOOL->bool.
Committed: https://pdfium.googlesource.com/pdfium/+/44f8faf9852a37df1edda34f99e1de4c9d222921
Patch Set 1 : #
Total comments: 23
Patch Set 2 : address comments #Patch Set 3 : remove consts #Patch Set 4 : rebase #Patch Set 5 : revert two FX_BOOL->bool #Messages
Total messages: 20 (12 generated)
Description was changed from ========== Clean up part of CXFA_Node class Mainly clean up Script_NodeClass_Xxx() functions. There are no behavior or API change. The clean up mainly include moving static functions into namespace, remove unnecessary conditions or braces, changing NULL->nullptr and local FX_BOOL->bool. ========== to ========== Clean up part of CXFA_Node class Mainly clean up Script_NodeClass_Xxx() functions. There are no behavior or API changes. The clean up mainly includes moving static functions into namespace, remove unnecessary conditions or braces, changing NULL->nullptr and local FX_BOOL->bool. ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
weili@chromium.org changed reviewers: + tsepez@chromium.org
for review, thanks
https://codereview.chromium.org/2018733002/diff/40001/xfa/fxfa/parser/xfa_obj... File xfa/fxfa/parser/xfa_object_imp.cpp (right): https://codereview.chromium.org/2018733002/diff/40001/xfa/fxfa/parser/xfa_obj... xfa/fxfa/parser/xfa_object_imp.cpp:44: XFA_MAPDATABLOCKCALLBACKINFO deleteWideStringCallBack = {XFA_DeleteWideString, nit: const FA_MAPDATABLOCKCALLBACKINFO https://codereview.chromium.org/2018733002/diff/40001/xfa/fxfa/parser/xfa_obj... xfa/fxfa/parser/xfa_object_imp.cpp:48: return (XFA_OBJECTTYPE)XFA_GetElementByID(eElement)->eObjectType; nit: static cast. https://codereview.chromium.org/2018733002/diff/40001/xfa/fxfa/parser/xfa_obj... xfa/fxfa/parser/xfa_object_imp.cpp:55: XFA_MAPDATABLOCKCALLBACKINFO deleteBindItemCallBack = { const XFA_MAPDATABLOCKCALLBACKINFO https://codereview.chromium.org/2018733002/diff/40001/xfa/fxfa/parser/xfa_obj... xfa/fxfa/parser/xfa_object_imp.cpp:132: CXFA_Node* pNext; make pNode local to body of loop https://codereview.chromium.org/2018733002/diff/40001/xfa/fxfa/parser/xfa_obj... xfa/fxfa/parser/xfa_object_imp.cpp:279: bool bFilterChildren = !!(dwTypeFilter & XFA_NODEFILTER_Children); nice. https://codereview.chromium.org/2018733002/diff/40001/xfa/fxfa/parser/xfa_obj... xfa/fxfa/parser/xfa_object_imp.cpp:562: CXFA_Node* pKeep = this is a shadow of the variable on line 543. Let's use a differnt name for this one. https://codereview.chromium.org/2018733002/diff/40001/xfa/fxfa/parser/xfa_obj... xfa/fxfa/parser/xfa_object_imp.cpp:964: std::unique_ptr<IXFA_Parser> pParser(IXFA_Parser::Create(m_pDocument)); Did we get rid of the Release() in IXFA_Parser in favor of a virtual dtor? If it still exists, we'll want to use a std::unique_ptr<IXFA_Parser, ReleaseDeleter<IFXA_Parser>> here. https://codereview.chromium.org/2018733002/diff/40001/xfa/fxfa/parser/xfa_obj... xfa/fxfa/parser/xfa_object_imp.cpp:3831: if (!GetValue(eAttr, XFA_ATTRIBUTETYPE_Boolean, bUseDefault, pValue)) { nit: no {} https://codereview.chromium.org/2018733002/diff/40001/xfa/fxfa/parser/xfa_obj... xfa/fxfa/parser/xfa_object_imp.cpp:3841: if (!GetValue(eAttr, XFA_ATTRIBUTETYPE_Integer, bUseDefault, pValue)) { ditto https://codereview.chromium.org/2018733002/diff/40001/xfa/fxfa/parser/xfa_obj... xfa/fxfa/parser/xfa_object_imp.cpp:3851: if (!GetValue(eAttr, XFA_ATTRIBUTETYPE_Enum, bUseDefault, pValue)) { ditto https://codereview.chromium.org/2018733002/diff/40001/xfa/fxfa/parser/xfa_obj... xfa/fxfa/parser/xfa_object_imp.cpp:4482: if (pExistProperty && (pExistProperty->uFlags & XFA_PROPERTYFLAG_OneOf)) { nit: no {} https://codereview.chromium.org/2018733002/diff/40001/xfa/fxfa/parser/xfa_obj... xfa/fxfa/parser/xfa_object_imp.cpp:4492: if (!pNewNode) { nit: no {}
thanks! https://codereview.chromium.org/2018733002/diff/40001/xfa/fxfa/parser/xfa_obj... File xfa/fxfa/parser/xfa_object_imp.cpp (right): https://codereview.chromium.org/2018733002/diff/40001/xfa/fxfa/parser/xfa_obj... xfa/fxfa/parser/xfa_object_imp.cpp:44: XFA_MAPDATABLOCKCALLBACKINFO deleteWideStringCallBack = {XFA_DeleteWideString, On 2016/05/26 23:55:04, Tom Sepez wrote: > nit: const FA_MAPDATABLOCKCALLBACKINFO Let's defer this change since it requires changing SetUserData() and other functions to accept const pointer. https://codereview.chromium.org/2018733002/diff/40001/xfa/fxfa/parser/xfa_obj... xfa/fxfa/parser/xfa_object_imp.cpp:48: return (XFA_OBJECTTYPE)XFA_GetElementByID(eElement)->eObjectType; On 2016/05/26 23:55:04, Tom Sepez wrote: > nit: static cast. Done. https://codereview.chromium.org/2018733002/diff/40001/xfa/fxfa/parser/xfa_obj... xfa/fxfa/parser/xfa_object_imp.cpp:55: XFA_MAPDATABLOCKCALLBACKINFO deleteBindItemCallBack = { On 2016/05/26 23:55:03, Tom Sepez wrote: > const XFA_MAPDATABLOCKCALLBACKINFO Let's defer this since it requires changing SetObject() and other function/data structure to use const pointer. https://codereview.chromium.org/2018733002/diff/40001/xfa/fxfa/parser/xfa_obj... xfa/fxfa/parser/xfa_object_imp.cpp:132: CXFA_Node* pNext; On 2016/05/26 23:55:03, Tom Sepez wrote: > make pNode local to body of loop pNext? Done. https://codereview.chromium.org/2018733002/diff/40001/xfa/fxfa/parser/xfa_obj... xfa/fxfa/parser/xfa_object_imp.cpp:562: CXFA_Node* pKeep = On 2016/05/26 23:55:03, Tom Sepez wrote: > this is a shadow of the variable on line 543. Let's use a differnt name for > this one. Done. https://codereview.chromium.org/2018733002/diff/40001/xfa/fxfa/parser/xfa_obj... xfa/fxfa/parser/xfa_object_imp.cpp:964: std::unique_ptr<IXFA_Parser> pParser(IXFA_Parser::Create(m_pDocument)); On 2016/05/26 23:55:03, Tom Sepez wrote: > Did we get rid of the Release() in IXFA_Parser in favor of a virtual dtor? If > it still exists, we'll want to use a > std::unique_ptr<IXFA_Parser, ReleaseDeleter<IFXA_Parser>> here. thanks, added for the other two unique_ptrs as well. https://codereview.chromium.org/2018733002/diff/40001/xfa/fxfa/parser/xfa_obj... xfa/fxfa/parser/xfa_object_imp.cpp:3831: if (!GetValue(eAttr, XFA_ATTRIBUTETYPE_Boolean, bUseDefault, pValue)) { On 2016/05/26 23:55:03, Tom Sepez wrote: > nit: no {} Done. https://codereview.chromium.org/2018733002/diff/40001/xfa/fxfa/parser/xfa_obj... xfa/fxfa/parser/xfa_object_imp.cpp:3841: if (!GetValue(eAttr, XFA_ATTRIBUTETYPE_Integer, bUseDefault, pValue)) { On 2016/05/26 23:55:04, Tom Sepez wrote: > ditto Done. https://codereview.chromium.org/2018733002/diff/40001/xfa/fxfa/parser/xfa_obj... xfa/fxfa/parser/xfa_object_imp.cpp:3851: if (!GetValue(eAttr, XFA_ATTRIBUTETYPE_Enum, bUseDefault, pValue)) { On 2016/05/26 23:55:04, Tom Sepez wrote: > ditto Done. https://codereview.chromium.org/2018733002/diff/40001/xfa/fxfa/parser/xfa_obj... xfa/fxfa/parser/xfa_object_imp.cpp:4482: if (pExistProperty && (pExistProperty->uFlags & XFA_PROPERTYFLAG_OneOf)) { On 2016/05/26 23:55:03, Tom Sepez wrote: > nit: no {} Done. https://codereview.chromium.org/2018733002/diff/40001/xfa/fxfa/parser/xfa_obj... xfa/fxfa/parser/xfa_object_imp.cpp:4492: if (!pNewNode) { On 2016/05/26 23:55:03, Tom Sepez wrote: > nit: no {} Done.
lgtm
Patchset #4 (id:100001) has been deleted
Patchset #4 (id:120001) has been deleted
The CQ bit was checked by weili@chromium.org
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/2018733002/#ps140001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2018733002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2018733002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_xfa on master.tryserver.client.pdfium (JOB_FAILED, https://build.chromium.org/p/tryserver.client.pdfium/builders/win_xfa/builds/902)
The CQ bit was checked by weili@chromium.org
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/2018733002/#ps160001 (title: "revert two FX_BOOL->bool")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2018733002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2018733002/160001
Message was sent while issue was closed.
Description was changed from ========== Clean up part of CXFA_Node class Mainly clean up Script_NodeClass_Xxx() functions. There are no behavior or API changes. The clean up mainly includes moving static functions into namespace, remove unnecessary conditions or braces, changing NULL->nullptr and local FX_BOOL->bool. ========== to ========== Clean up part of CXFA_Node class Mainly clean up Script_NodeClass_Xxx() functions. There are no behavior or API changes. The clean up mainly includes moving static functions into namespace, remove unnecessary conditions or braces, changing NULL->nullptr and local FX_BOOL->bool. Committed: https://pdfium.googlesource.com/pdfium/+/44f8faf9852a37df1edda34f99e1de4c9d22... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:160001) as https://pdfium.googlesource.com/pdfium/+/44f8faf9852a37df1edda34f99e1de4c9d22... |
