Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2472473005:
Move CPDF_Document insert methods from namespace (Closed)
DescriptionMove CPDF_Document insert methods from namespace
Making the insert methods private allows us to use private members, as I
will need on https://codereview.chromium.org/2470803003/
Committed: https://pdfium.googlesource.com/pdfium/+/20ef5b93439a0e28cd612b824831f8dfcf234dfd
Patch Set 1 #
Total comments: 10
Patch Set 2 : Addressing comments #
Messages
Total messages: 19 (13 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...
Description was changed from ========== Move CPDF_Document insert methods from namespace Making the insert methods private allows us to use private members, as I will need on https://codereview.chromium.org/2470803003/ ========== to ========== Move CPDF_Document insert methods from namespace Making the insert methods private allows us to use private members, as I will need on https://codereview.chromium.org/2470803003/ ==========
npm@chromium.org changed reviewers: + dsinclair@chromium.org, thestig@chromium.org, tsepez@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2472473005/diff/1/core/fpdfapi/parser/cpdf_do... File core/fpdfapi/parser/cpdf_document.cpp (right): https://codereview.chromium.org/2472473005/diff/1/core/fpdfapi/parser/cpdf_do... core/fpdfapi/parser/cpdf_document.cpp:643: int CPDF_Document::InsertDeletePDFPage(CPDF_Dictionary* pPages, This method can just return a bool. Nobody cares whether it returns 0 or 1. All the callers only checks for less than 0 or not. https://codereview.chromium.org/2472473005/diff/1/core/fpdfapi/parser/cpdf_do... core/fpdfapi/parser/cpdf_document.cpp:645: CPDF_Dictionary* pPage, Name this "pPageDict" like InsertNewPage's parameter, so it's not easily confused with |pPages| ? https://codereview.chromium.org/2472473005/diff/1/core/fpdfapi/parser/cpdf_do... core/fpdfapi/parser/cpdf_document.cpp:666: nPagesToGo--; Some early continues can make this easier to follow: for (...) { PDF_Dictionary* pKid = pKidList->GetDictAt(i); if (pKid->GetStringFor("Type") == "Page") { if (nPagesToGo != 0) { nPagesToGo--; continue; } // handle 0 case break; } int nPages = pKid->GetIntegerFor("Count"); if (nPagesToGo >= nPages) { nPagesToGo -= nPages; continue; } // handle reached case break; } return true; https://codereview.chromium.org/2472473005/diff/1/core/fpdfapi/parser/cpdf_do... core/fpdfapi/parser/cpdf_document.cpp:688: int CPDF_Document::InsertNewPage(int iPage, This can also return a bool. https://codereview.chromium.org/2472473005/diff/1/core/fpdfapi/parser/cpdf_do... core/fpdfapi/parser/cpdf_document.cpp:690: CFX_ArrayTemplate<uint32_t>& pageList) { Don't pass by non-const ref. It's easy to avoid here.
LGTM given Lei's comments.
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...
https://codereview.chromium.org/2472473005/diff/1/core/fpdfapi/parser/cpdf_do... File core/fpdfapi/parser/cpdf_document.cpp (right): https://codereview.chromium.org/2472473005/diff/1/core/fpdfapi/parser/cpdf_do... core/fpdfapi/parser/cpdf_document.cpp:643: int CPDF_Document::InsertDeletePDFPage(CPDF_Dictionary* pPages, On 2016/11/03 07:01:57, Lei Zhang wrote: > This method can just return a bool. Nobody cares whether it returns 0 or 1. All > the callers only checks for less than 0 or not. Done. https://codereview.chromium.org/2472473005/diff/1/core/fpdfapi/parser/cpdf_do... core/fpdfapi/parser/cpdf_document.cpp:645: CPDF_Dictionary* pPage, On 2016/11/03 07:01:57, Lei Zhang wrote: > Name this "pPageDict" like InsertNewPage's parameter, so it's not easily > confused with |pPages| ? Done. https://codereview.chromium.org/2472473005/diff/1/core/fpdfapi/parser/cpdf_do... core/fpdfapi/parser/cpdf_document.cpp:666: nPagesToGo--; On 2016/11/03 07:01:57, Lei Zhang wrote: > Some early continues can make this easier to follow: > > for (...) { > PDF_Dictionary* pKid = pKidList->GetDictAt(i); > if (pKid->GetStringFor("Type") == "Page") { > if (nPagesToGo != 0) { > nPagesToGo--; > continue; > } > // handle 0 case > break; > } > > int nPages = pKid->GetIntegerFor("Count"); > if (nPagesToGo >= nPages) { > nPagesToGo -= nPages; > continue; > } > // handle reached case > break; > } > return true; Done. https://codereview.chromium.org/2472473005/diff/1/core/fpdfapi/parser/cpdf_do... core/fpdfapi/parser/cpdf_document.cpp:688: int CPDF_Document::InsertNewPage(int iPage, On 2016/11/03 07:01:57, Lei Zhang wrote: > This can also return a bool. Done. https://codereview.chromium.org/2472473005/diff/1/core/fpdfapi/parser/cpdf_do... core/fpdfapi/parser/cpdf_document.cpp:690: CFX_ArrayTemplate<uint32_t>& pageList) { On 2016/11/03 07:01:57, Lei Zhang wrote: > Don't pass by non-const ref. It's easy to avoid here. Well, InsertAt is not const. But the param can be removed altogether.
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 tsepez@chromium.org, thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2472473005/#ps20001 (title: "Addressing 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 ========== Move CPDF_Document insert methods from namespace Making the insert methods private allows us to use private members, as I will need on https://codereview.chromium.org/2470803003/ ========== to ========== Move CPDF_Document insert methods from namespace Making the insert methods private allows us to use private members, as I will need on https://codereview.chromium.org/2470803003/ Committed: https://pdfium.googlesource.com/pdfium/+/20ef5b93439a0e28cd612b824831f8dfcf23... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://pdfium.googlesource.com/pdfium/+/20ef5b93439a0e28cd612b824831f8dfcf23... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
