Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2323793003:
Refactor CPDF_Document (Closed)
DescriptionRefactor CPDF_Document by creating new methods
- Methods GetPagesDict, ProcessNonbCJK, CalculateFlags, and
CalculateEncodingDict created to reduce duplicated code.
- Code nits
Committed: https://pdfium.googlesource.com/pdfium/+/01b67ed9b441cd485997bc08482def1f2ab265db
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Try fix windows bot #
Total comments: 11
Patch Set 4 : Comments #
Total comments: 6
Patch Set 5 : Comments #
Messages
Total messages: 34 (25 generated)
The CQ bit was checked by npm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
npm@chromium.org changed reviewers: + dsinclair@chromium.org, thestig@chromium.org, weili@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android on master.tryserver.client.pdfium (JOB_FAILED, https://build.chromium.org/p/tryserver.client.pdfium/builders/android/builds/690)
Can you split this CL into two to make it easier to read? 1- delete unused code 2- rest.
The CQ bit was checked by npm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win on master.tryserver.client.pdfium (JOB_FAILED, https://build.chromium.org/p/tryserver.client.pdfium/builders/win/builds/2002)
The CQ bit was checked by npm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Delete unused methods and refactor CPDF_Document - Unused methods were deleted. - Methods GetPagesDict, ProcessNonbCJK, CalculateFlags, and CalculateEncodingDict created to reduce duplicated code. - Code nits ========== to ========== Refactor CPDF_Document by creating new methods - Methods GetPagesDict, ProcessNonbCJK, CalculateFlags, and CalculateEncodingDict created to reduce duplicated code. - Code nits ==========
On 2016/09/08 20:25:27, dsinclair wrote: > Can you split this CL into two to make it easier to read? 1- delete unused code > 2- rest. Done. PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2323793003/diff/40001/core/fpdfapi/fpdf_parse... File core/fpdfapi/fpdf_parser/cpdf_document.cpp (right): https://codereview.chromium.org/2323793003/diff/40001/core/fpdfapi/fpdf_parse... core/fpdfapi/fpdf_parser/cpdf_document.cpp:506: return pRoot->GetDictBy("Pages"); return pRoot ? pRoot->GetDictBy("Pages") : nullptr; https://codereview.chromium.org/2323793003/diff/40001/core/fpdfapi/fpdf_parse... core/fpdfapi/fpdf_parser/cpdf_document.cpp:731: if (i < FX_ArraySize(g_FX_CharsetUnicodes)) { if (i >= FX_ArraySize(g_FX_CharsetUnicodes) return i; https://codereview.chromium.org/2323793003/diff/40001/core/fpdfapi/fpdf_parse... core/fpdfapi/fpdf_parser/cpdf_document.cpp:933: pLogFont->lfCharSet == SYMBOL_CHARSET); I think this needs a !bCJK && pLogFont->lfCharSet == SYMBOL_CHARSET ? In the case of bCJK being true, the old code would always be NONSYMBOL. The new could could be SYMBOL depending on lfCharSet only. https://codereview.chromium.org/2323793003/diff/40001/core/fpdfapi/fpdf_parse... core/fpdfapi/fpdf_parser/cpdf_document.cpp:934: int italicangle, ascend, descend, capheight, bbox[4]; nit: one per line int italicangle; int ascend; ... https://codereview.chromium.org/2323793003/diff/40001/core/fpdfapi/fpdf_parse... File core/fpdfapi/fpdf_parser/include/cpdf_document.h (right): https://codereview.chromium.org/2323793003/diff/40001/core/fpdfapi/fpdf_parse... core/fpdfapi/fpdf_parser/include/cpdf_document.h:51: CPDF_Dictionary* GetPagesDict() const; Put this in the private section?
PTAL https://codereview.chromium.org/2323793003/diff/40001/core/fpdfapi/fpdf_parse... File core/fpdfapi/fpdf_parser/cpdf_document.cpp (right): https://codereview.chromium.org/2323793003/diff/40001/core/fpdfapi/fpdf_parse... core/fpdfapi/fpdf_parser/cpdf_document.cpp:506: return pRoot->GetDictBy("Pages"); On 2016/09/09 00:21:06, dsinclair wrote: > return pRoot ? pRoot->GetDictBy("Pages") : nullptr; Done. https://codereview.chromium.org/2323793003/diff/40001/core/fpdfapi/fpdf_parse... core/fpdfapi/fpdf_parser/cpdf_document.cpp:731: if (i < FX_ArraySize(g_FX_CharsetUnicodes)) { On 2016/09/09 00:21:06, dsinclair wrote: > if (i >= FX_ArraySize(g_FX_CharsetUnicodes) > return i; Done. https://codereview.chromium.org/2323793003/diff/40001/core/fpdfapi/fpdf_parse... core/fpdfapi/fpdf_parser/cpdf_document.cpp:933: pLogFont->lfCharSet == SYMBOL_CHARSET); On 2016/09/09 00:21:06, dsinclair wrote: > I think this needs a !bCJK && pLogFont->lfCharSet == SYMBOL_CHARSET ? > > In the case of bCJK being true, the old code would always be NONSYMBOL. The new > could could be SYMBOL depending on lfCharSet only. By definition of bCJK, if lfCharSet==SYMBOL_CHARSET, then bCJK is false. https://codereview.chromium.org/2323793003/diff/40001/core/fpdfapi/fpdf_parse... core/fpdfapi/fpdf_parser/cpdf_document.cpp:934: int italicangle, ascend, descend, capheight, bbox[4]; On 2016/09/09 00:21:06, dsinclair wrote: > nit: one per line > > int italicangle; > int ascend; > ... Done. https://codereview.chromium.org/2323793003/diff/40001/core/fpdfapi/fpdf_parse... File core/fpdfapi/fpdf_parser/include/cpdf_document.h (left): https://codereview.chromium.org/2323793003/diff/40001/core/fpdfapi/fpdf_parse... core/fpdfapi/fpdf_parser/include/cpdf_document.h:63: FX_BOOL IsFormStream(uint32_t objnum, FX_BOOL& bForm) const; This slipped from delete CL, should have gone there. https://codereview.chromium.org/2323793003/diff/40001/core/fpdfapi/fpdf_parse... File core/fpdfapi/fpdf_parser/include/cpdf_document.h (right): https://codereview.chromium.org/2323793003/diff/40001/core/fpdfapi/fpdf_parse... core/fpdfapi/fpdf_parser/include/cpdf_document.h:51: CPDF_Dictionary* GetPagesDict() const; On 2016/09/09 00:21:06, dsinclair wrote: > Put this in the private section? Done, but method no longer used in InsertNewPage.
The CQ bit was checked by npm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2323793003/diff/60001/core/fpdfapi/fpdf_parse... File core/fpdfapi/fpdf_parser/cpdf_document.cpp (right): https://codereview.chromium.org/2323793003/diff/60001/core/fpdfapi/fpdf_parse... core/fpdfapi/fpdf_parser/cpdf_document.cpp:723: size_t CPDF_Document::CalculateEncodingDict(int charset, Can this be static or go in an anonymous namespace? https://codereview.chromium.org/2323793003/diff/60001/core/fpdfapi/fpdf_parse... core/fpdfapi/fpdf_parser/cpdf_document.cpp:933: int italicangle; Declare these further down / combine with when they get initializd? https://codereview.chromium.org/2323793003/diff/60001/core/fxge/ge/cfx_font.cpp File core/fxge/ge/cfx_font.cpp (right): https://codereview.chromium.org/2323793003/diff/60001/core/fxge/ge/cfx_font.c... core/fxge/ge/cfx_font.cpp:453: bool ret = FXFT_Is_Face_Italic(m_Face) == FXFT_STYLE_FLAG_ITALIC; This can be: if (FXFT_Is_Face_Italic(m_Face) == FXFT_STYLE_FLAG_ITALIC) return true; CFX_ByteString str(FXFT_Get_Face_Style_Name(m_Face)); str.MakeLower(); return str.Find("italic") != -1;
https://codereview.chromium.org/2323793003/diff/60001/core/fpdfapi/fpdf_parse... File core/fpdfapi/fpdf_parser/cpdf_document.cpp (right): https://codereview.chromium.org/2323793003/diff/60001/core/fpdfapi/fpdf_parse... core/fpdfapi/fpdf_parser/cpdf_document.cpp:723: size_t CPDF_Document::CalculateEncodingDict(int charset, On 2016/09/09 21:27:48, Lei Zhang (OOO) wrote: > Can this be static or go in an anonymous namespace? AddIndirectObject is not static. If added in the namespace, a CPDF_Document would have to be added as a parameter and the method would be called CalculateEncodingDict(this,...) https://codereview.chromium.org/2323793003/diff/60001/core/fpdfapi/fpdf_parse... core/fpdfapi/fpdf_parser/cpdf_document.cpp:933: int italicangle; On 2016/09/09 21:27:48, Lei Zhang (OOO) wrote: > Declare these further down / combine with when they get initializd? Done. https://codereview.chromium.org/2323793003/diff/60001/core/fxge/ge/cfx_font.cpp File core/fxge/ge/cfx_font.cpp (right): https://codereview.chromium.org/2323793003/diff/60001/core/fxge/ge/cfx_font.c... core/fxge/ge/cfx_font.cpp:453: bool ret = FXFT_Is_Face_Italic(m_Face) == FXFT_STYLE_FLAG_ITALIC; On 2016/09/09 21:27:49, Lei Zhang (OOO) wrote: > This can be: > > if (FXFT_Is_Face_Italic(m_Face) == FXFT_STYLE_FLAG_ITALIC) > return true; > > CFX_ByteString str(FXFT_Get_Face_Style_Name(m_Face)); > str.MakeLower(); > return str.Find("italic") != -1; Done.
The CQ bit was checked by npm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by npm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2323793003/#ps80001 (title: "Comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Refactor CPDF_Document by creating new methods - Methods GetPagesDict, ProcessNonbCJK, CalculateFlags, and CalculateEncodingDict created to reduce duplicated code. - Code nits ========== to ========== Refactor CPDF_Document by creating new methods - Methods GetPagesDict, ProcessNonbCJK, CalculateFlags, and CalculateEncodingDict created to reduce duplicated code. - Code nits Committed: https://pdfium.googlesource.com/pdfium/+/01b67ed9b441cd485997bc08482def1f2ab2... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://pdfium.googlesource.com/pdfium/+/01b67ed9b441cd485997bc08482def1f2ab2... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
