Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2028063002:
xfa_fm2jscontext method cleanup - pt III (Closed)
Descriptionxfa_fm2jscontext method cleanup - pt III
Cleanup ::Min, ::Mod, ::Round, ::Date2Num, ::DateFmt and ::ISoTime2Num.
Committed: https://pdfium.googlesource.com/pdfium/+/dbdcb81a82cd9e46023a3ee500df75717c1a47b4
Patch Set 1 #
Total comments: 27
Patch Set 2 : #
Messages
Total messages: 12 (3 generated)
dsinclair@chromium.org changed reviewers: + thestig@chromium.org, tsepez@chromium.org
PTAL. https://codereview.chromium.org/2028063002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon... File xfa/fxfa/fm2js/xfa_fm2jscontext.cpp (right): https://codereview.chromium.org/2028063002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon... xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:808: return; Added return due to throw. https://codereview.chromium.org/2028063002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon... xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:1092: return; Added return due to throw. https://codereview.chromium.org/2028063002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon... xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:1317: CXFA_LocaleMgr* pMgr = pDoc->GetLocalMgr(); We just casted this to CXFA_LocalMgr on the old line 1331 so keep it as the original type instead of casting twice.
Still reading. https://codereview.chromium.org/2028063002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon... File xfa/fxfa/fm2js/xfa_fm2jscontext.cpp (right): https://codereview.chromium.org/2028063002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon... xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:857: uCount++; We seem to repeat this a lot. https://codereview.chromium.org/2028063002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon... xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:920: if (FXJSE_Value_IsArray(argTwo.get())) { This look almost identical to the |argOne| block starting at line 892.
https://codereview.chromium.org/2028063002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon... File xfa/fxfa/fm2js/xfa_fm2jscontext.cpp (right): https://codereview.chromium.org/2028063002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon... xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:891: FX_DOUBLE dDividor = 0.0; divisor, not dividor https://codereview.chromium.org/2028063002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon... xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:986: if (FXJSE_Value_IsNull(propertyValue.get())) { This if-else block looks identical to code that deals with dDividor above. https://codereview.chromium.org/2028063002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon... xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:1004: } else if (argc == 2) { Just return in the argc == 1 block, assert argc == 2 here, and de-indent the rest. https://codereview.chromium.org/2028063002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon... xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:1013: if (FXJSE_Value_IsArray(argOne.get())) { More similar looking code here and on line 1033. https://codereview.chromium.org/2028063002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon... xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:1052: if (dPrecision < 0) static_cast<uint8_t>(std::min(std::max(dPrecision, 0), 12)) https://codereview.chromium.org/2028063002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon... xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:1182: if ((argc <= 0) || (argc >= 4)) { extra parenthesis https://codereview.chromium.org/2028063002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon... xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:1268: if (formatStr.IsEmpty()) Is this necessary? https://codereview.chromium.org/2028063002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon... xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:1340: IFX_Locale* pDefLocale = pMgr->GetDefLocale(); Don't bother with the extra local variable? https://codereview.chromium.org/2028063002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon... xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:1344: int32_t mins = hour * 60 + min; I wonder if we have some other time conversion code to consolicate with. Add a TODO for now?
Still reading ... https://codereview.chromium.org/2028063002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon... File xfa/fxfa/fm2js/xfa_fm2jscontext.cpp (right): https://codereview.chromium.org/2028063002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon... xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:891: FX_DOUBLE dDividor = 0.0; On 2016/06/01 20:00:43, Lei Zhang wrote: > divisor, not dividor Either way, initializing dividor to 0.0 seems like a bad idea :0.
I've nothing beyond what Lei said.
https://codereview.chromium.org/2028063002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon... File xfa/fxfa/fm2js/xfa_fm2jscontext.cpp (right): https://codereview.chromium.org/2028063002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon... xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:857: uCount++; On 2016/06/01 19:50:46, Lei Zhang wrote: > We seem to repeat this a lot. Yea, the problem is some bits of it are inside inner for() loops so makes it hard to consolidate. https://codereview.chromium.org/2028063002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon... xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:891: FX_DOUBLE dDividor = 0.0; On 2016/06/01 20:00:43, Lei Zhang (OOO) wrote: > divisor, not dividor Done. https://codereview.chromium.org/2028063002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon... xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:891: FX_DOUBLE dDividor = 0.0; On 2016/06/01 20:07:12, Tom Sepez wrote: > On 2016/06/01 20:00:43, Lei Zhang wrote: > > divisor, not dividor > Either way, initializing dividor to 0.0 seems like a bad idea :0. We do check if it == 0 before usage, but changed to 1.0. It should always be set below to some value anyway. https://codereview.chromium.org/2028063002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon... xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:920: if (FXJSE_Value_IsArray(argTwo.get())) { On 2016/06/01 19:50:46, Lei Zhang (OOO) wrote: > This look almost identical to the |argOne| block starting at line 892. Done. https://codereview.chromium.org/2028063002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon... xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:986: if (FXJSE_Value_IsNull(propertyValue.get())) { On 2016/06/01 20:00:43, Lei Zhang (OOO) wrote: > This if-else block looks identical to code that deals with dDividor above. Done. https://codereview.chromium.org/2028063002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon... xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:1004: } else if (argc == 2) { On 2016/06/01 20:00:42, Lei Zhang (OOO) wrote: > Just return in the argc == 1 block, assert argc == 2 here, and de-indent the > rest. Rewrote it as argc == 1 and argc == 2 duplicated a bunch of code. https://codereview.chromium.org/2028063002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon... xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:1013: if (FXJSE_Value_IsArray(argOne.get())) { On 2016/06/01 20:00:42, Lei Zhang (OOO) wrote: > More similar looking code here and on line 1033. Done. https://codereview.chromium.org/2028063002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon... xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:1052: if (dPrecision < 0) On 2016/06/01 20:00:42, Lei Zhang (OOO) wrote: > static_cast<uint8_t>(std::min(std::max(dPrecision, 0), 12)) Done. https://codereview.chromium.org/2028063002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon... xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:1182: if ((argc <= 0) || (argc >= 4)) { On 2016/06/01 20:00:43, Lei Zhang (OOO) wrote: > extra parenthesis Done. https://codereview.chromium.org/2028063002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon... xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:1268: if (formatStr.IsEmpty()) On 2016/06/01 20:00:43, Lei Zhang (OOO) wrote: > Is this necessary? Wasn't sure, so I left it. I don't think it makes sense though as, "" is the empty string. Removed. https://codereview.chromium.org/2028063002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon... xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:1340: IFX_Locale* pDefLocale = pMgr->GetDefLocale(); On 2016/06/01 20:00:43, Lei Zhang (OOO) wrote: > Don't bother with the extra local variable? Done. https://codereview.chromium.org/2028063002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon... xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:1344: int32_t mins = hour * 60 + min; On 2016/06/01 20:00:43, Lei Zhang (OOO) wrote: > I wonder if we have some other time conversion code to consolicate with. Add a > TODO for now? Done.
The CQ bit was checked by thestig@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2028063002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2028063002/20001
Message was sent while issue was closed.
Description was changed from ========== xfa_fm2jscontext method cleanup - pt III Cleanup ::Min, ::Mod, ::Round, ::Date2Num, ::DateFmt and ::ISoTime2Num. ========== to ========== xfa_fm2jscontext method cleanup - pt III Cleanup ::Min, ::Mod, ::Round, ::Date2Num, ::DateFmt and ::ISoTime2Num. Committed: https://pdfium.googlesource.com/pdfium/+/dbdcb81a82cd9e46023a3ee500df75717c1a... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://pdfium.googlesource.com/pdfium/+/dbdcb81a82cd9e46023a3ee500df75717c1a... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
