Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1353093003:
Support linearized loading (Closed)
|
Created:
5 years, 3 months ago by jun_fang Modified:
5 years, 1 month ago CC:
pdfium-reviews_googlegroups.com, kai_jing, steven_wu, 'jeff breidenbach' Base URL:
https://pdfium.googlesource.com/pdfium.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionSupport linearized loading
BUG=446715
R=tsepez@chromium.org
Committed: https://pdfium.googlesource.com/pdfium/+/d946f3011984755b14d7dcfb05d572e870f93f3f
Patch Set 1 #
Total comments: 55
Patch Set 2 : Add embedder tests #Patch Set 3 : Address the comments from code review #Patch Set 4 : #
Total comments: 49
Patch Set 5 : #
Total comments: 2
Patch Set 6 : #
Total comments: 28
Patch Set 7 : #Patch Set 8 : Change pdfium_test and add the third return value to indicate an error #
Total comments: 2
Patch Set 9 : #
Total comments: 1
Patch Set 10 : Add an argument 'must_linearize' in OpenDocument #
Total comments: 7
Patch Set 11 : #
Total comments: 21
Patch Set 12 : #Patch Set 13 : #
Messages
Total messages: 45 (2 generated)
jun_fang@foxitsoftware.com changed reviewers: + thestig@chromium.org, tsepez@chromium.org
I am working on some test cases for this patch. https://codereview.chromium.org/1353093003/diff/1/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/1353093003/diff/1/BUILD.gn#newcode299 BUILD.gn:299: "core/src/fpdfapi/fpdf_parser/parser_int.h" will remove redundant spaces in next patch. https://codereview.chromium.org/1353093003/diff/1/pdfium.gyp File pdfium.gyp (right): https://codereview.chromium.org/1353093003/diff/1/pdfium.gyp#newcode296 pdfium.gyp:296: 'core/src/fpdfapi/fpdf_parser/parser_int.h', will remove redundant spaces in next patch.
https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... File core/src/fpdfapi/fpdf_parser/parser_int.h (right): https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/parser_int.h:5: // Original code copyright 2014 Foxit Software Inc. http://www.foxitsoftware.com 2015
We're going to need a lot more checking on the values in the untruted bit-stream. I assume there's another patch where we use the values loaded into these arrays? https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:16: #include "./parser_int.h" nit: no ./ https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:3025: friend class CPDF_HintTables; nit: we'd be happier if you added the corresponding accessor methods for the fields you want to manipulate by this friending. https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:3103: if (m_pHintTables) { nit: just delete m_pHintables. There's no need to check for nullness, C++ handles that for you. https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:3104: delete m_pHintTables; if CPDF_DataAvail owns these hint tables, then use nonstd::unique_ptr<> and deletion becomes automatic. https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:3742: FX_FILESIZE szHSStart, szHSLength; nit: one per line, combine declaration and initialization below. https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:3757: CPDF_HintTables* pHintTables = new CPDF_HintTables(this, pDict); nit: unique_ptr<> might help here, as well. https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4555: if (!m_pHintTables->CheckPage(iPage, pHints)) { nit: just return m_pHintTabls->CheckPage(...); https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4559: } else { nit: else after return not needed. https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4797: FX_DWORD CPDF_HintTables::GetItemLength(int index, CFX_FileSizeArray& szArray) { nit: const CFX_FileSizeArray& https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4801: return (szArray[index + 1] - szArray[index]); nit: unnecessary () https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4809: CPDF_Object* pStreamLen = pRange ? pRange->GetElementValue(1) : nullptr; nit: prefer early return here rather than duplicating the same test in several ? operators. https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4811: int nStreanLen = pStreamLen ? pStreamLen->GetInteger() : 0; nit: typo: stream vs. strean https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4817: // Item 2: The location of the first page¡¯s page object. nit: typo. https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4820: m_szFirstPageObjOffset = dwFirstObjLoc + nStreanLen; how do we know this doesn't overflow? https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4843: // where in the page¡¯s content stream the object is first referenced. nit: typo https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4847: FX_DWORD dwSharedDenominator = h.GetBits(16); nit: just skipbits for item 13 like above. https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4854: m_dwDeltaNObjsArray.Add(h.GetBits(dwDeltaObjectsBits) + dwObjLeastNum); overflow on addition? https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4892: m_szPageOffsetArray.Add(m_szFirstPageObjOffset); I don't think this can be reached since we returned at 4850 otherwise? https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4896: /** The number of shared objects. */ nit: prefer c++ // style. https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4909: h.SkipBits(m_dwNSharedObjsArray[i] * dwSharedNumeratorBits); again, overflow? https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4912: h.SkipBits(dwDeltaPageLenBits * nPages); overflow? https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4920: CPDF_Array* pRange = m_pLinearizedDict->GetArray(FX_BSTRC("H")); nit: same as above. Let's make a helper function and consolidate. https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4924: int nStreanLen = pStreamLen ? pStreamLen->GetInteger() : 0; nit: typo: stream vs. strean https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4959: dwSharedObjsLen = h.GetBits(dwDeltaGroupLen) + dwGroupLeastLen; nit: overflow. https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:5049: dwIndex = m_dwIdentifierArray[offset + j]; how do we know that the bounds are trustworthy here? https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:5084: const uint8_t* pData = acc.GetData(); nit: don't ned a local here, just bs.Init(acc.GetData()...) below. https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:5087: if (!ReadPageHintTable(bs) || !ReadSharedObjHintTable(bs)) { nit: or just return ReadPageHintTable(bs) && readSharedObjHintTable(bs); https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... File core/src/fpdfapi/fpdf_parser/parser_int.h (right): https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/parser_int.h:32: CPDF_Dictionary* m_pLinearizedDict; nit: maybe blank line here between methods and members.
https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:16: #include "./parser_int.h" On 2015/09/22 00:05:38, Tom Sepez wrote: > nit: no ./ Acknowledged. https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:3103: if (m_pHintTables) { On 2015/09/22 00:05:38, Tom Sepez wrote: > nit: just delete m_pHintables. There's no need to check for nullness, C++ > handles that for you. Acknowledged. https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:3742: FX_FILESIZE szHSStart, szHSLength; On 2015/09/22 00:05:39, Tom Sepez wrote: > nit: one per line, combine declaration and initialization below. Acknowledged. https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:3757: CPDF_HintTables* pHintTables = new CPDF_HintTables(this, pDict); On 2015/09/22 00:05:39, Tom Sepez wrote: > nit: unique_ptr<> might help here, as well. If we define pHintTables and m_pHintTables like below: nonstd::unique_ptr<CPDF_HintTables> pHintTables; nonstd::unique_ptr<CPDF_HintTables> m_pHintTables; There is no way to assign pHintTables to m_pHintTables. https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4555: if (!m_pHintTables->CheckPage(iPage, pHints)) { On 2015/09/22 00:05:38, Tom Sepez wrote: > nit: just return m_pHintTabls->CheckPage(...); Acknowledged. https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4559: } else { On 2015/09/22 00:05:39, Tom Sepez wrote: > nit: else after return not needed. Acknowledged. https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4797: FX_DWORD CPDF_HintTables::GetItemLength(int index, CFX_FileSizeArray& szArray) { On 2015/09/22 00:05:38, Tom Sepez wrote: > nit: const CFX_FileSizeArray& Acknowledged. https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4801: return (szArray[index + 1] - szArray[index]); On 2015/09/22 00:05:39, Tom Sepez wrote: > nit: unnecessary () Acknowledged. https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4809: CPDF_Object* pStreamLen = pRange ? pRange->GetElementValue(1) : nullptr; On 2015/09/22 00:05:39, Tom Sepez wrote: > nit: prefer early return here rather than duplicating the same test in several ? > operators. Acknowledged. https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4811: int nStreanLen = pStreamLen ? pStreamLen->GetInteger() : 0; On 2015/09/22 00:05:39, Tom Sepez wrote: > nit: typo: stream vs. strean Acknowledged. https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4817: // Item 2: The location of the first page¡¯s page object. On 2015/09/22 00:05:38, Tom Sepez wrote: > nit: typo. Acknowledged. https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4820: m_szFirstPageObjOffset = dwFirstObjLoc + nStreanLen; On 2015/09/22 00:05:38, Tom Sepez wrote: > how do we know this doesn't overflow? Acknowledged. https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4843: // where in the page¡¯s content stream the object is first referenced. On 2015/09/22 00:05:39, Tom Sepez wrote: > nit: typo Acknowledged. https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4847: FX_DWORD dwSharedDenominator = h.GetBits(16); On 2015/09/22 00:05:39, Tom Sepez wrote: > nit: just skipbits for item 13 like above. Acknowledged. https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4854: m_dwDeltaNObjsArray.Add(h.GetBits(dwDeltaObjectsBits) + dwObjLeastNum); On 2015/09/22 00:05:38, Tom Sepez wrote: > overflow on addition? Acknowledged. https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4892: m_szPageOffsetArray.Add(m_szFirstPageObjOffset); On 2015/09/22 00:05:38, Tom Sepez wrote: > I don't think this can be reached since we returned at 4850 otherwise? Acknowledged. https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4896: /** The number of shared objects. */ On 2015/09/22 00:05:39, Tom Sepez wrote: > nit: prefer c++ // style. Acknowledged. https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4912: h.SkipBits(dwDeltaPageLenBits * nPages); On 2015/09/22 00:05:39, Tom Sepez wrote: > overflow? Acknowledged. https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4920: CPDF_Array* pRange = m_pLinearizedDict->GetArray(FX_BSTRC("H")); On 2015/09/22 00:05:39, Tom Sepez wrote: > nit: same as above. Let's make a helper function and consolidate. Acknowledged. https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4924: int nStreanLen = pStreamLen ? pStreamLen->GetInteger() : 0; On 2015/09/22 00:05:39, Tom Sepez wrote: > nit: typo: stream vs. strean Acknowledged. https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:5084: const uint8_t* pData = acc.GetData(); On 2015/09/22 00:05:39, Tom Sepez wrote: > nit: don't ned a local here, just bs.Init(acc.GetData()...) below. Acknowledged. https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:5087: if (!ReadPageHintTable(bs) || !ReadSharedObjHintTable(bs)) { On 2015/09/22 00:05:39, Tom Sepez wrote: > nit: or just > > return ReadPageHintTable(bs) && readSharedObjHintTable(bs); Acknowledged. https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... File core/src/fpdfapi/fpdf_parser/parser_int.h (right): https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/parser_int.h:32: CPDF_Dictionary* m_pLinearizedDict; On 2015/09/22 00:05:39, Tom Sepez wrote: > nit: maybe blank line here between methods and members. Acknowledged.
On 2015/09/22 12:42:33, jun_fang wrote: > https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... > File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): > > https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:16: #include > "./parser_int.h" > On 2015/09/22 00:05:38, Tom Sepez wrote: > > nit: no ./ > > Acknowledged. > > https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:3103: if (m_pHintTables) { > On 2015/09/22 00:05:38, Tom Sepez wrote: > > nit: just delete m_pHintables. There's no need to check for nullness, C++ > > handles that for you. > > Acknowledged. > > https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:3742: FX_FILESIZE szHSStart, > szHSLength; > On 2015/09/22 00:05:39, Tom Sepez wrote: > > nit: one per line, combine declaration and initialization below. > > Acknowledged. > > https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:3757: CPDF_HintTables* > pHintTables = new CPDF_HintTables(this, pDict); > On 2015/09/22 00:05:39, Tom Sepez wrote: > > nit: unique_ptr<> might help here, as well. > > If we define pHintTables and m_pHintTables like below: > nonstd::unique_ptr<CPDF_HintTables> pHintTables; > nonstd::unique_ptr<CPDF_HintTables> m_pHintTables; > > There is no way to assign pHintTables to m_pHintTables. > > https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4555: if > (!m_pHintTables->CheckPage(iPage, pHints)) { > On 2015/09/22 00:05:38, Tom Sepez wrote: > > nit: just return m_pHintTabls->CheckPage(...); > > Acknowledged. > > https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4559: } else { > On 2015/09/22 00:05:39, Tom Sepez wrote: > > nit: else after return not needed. > > Acknowledged. > > https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4797: FX_DWORD > CPDF_HintTables::GetItemLength(int index, CFX_FileSizeArray& szArray) { > On 2015/09/22 00:05:38, Tom Sepez wrote: > > nit: const CFX_FileSizeArray& > > Acknowledged. > > https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4801: return (szArray[index > + 1] - szArray[index]); > On 2015/09/22 00:05:39, Tom Sepez wrote: > > nit: unnecessary () > > Acknowledged. > > https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4809: CPDF_Object* > pStreamLen = pRange ? pRange->GetElementValue(1) : nullptr; > On 2015/09/22 00:05:39, Tom Sepez wrote: > > nit: prefer early return here rather than duplicating the same test in several > ? > > operators. > > Acknowledged. > > https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4811: int nStreanLen = > pStreamLen ? pStreamLen->GetInteger() : 0; > On 2015/09/22 00:05:39, Tom Sepez wrote: > > nit: typo: stream vs. strean > > Acknowledged. > > https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4817: // Item 2: The > location of the first page¡¯s page object. > On 2015/09/22 00:05:38, Tom Sepez wrote: > > nit: typo. > > Acknowledged. > > https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4820: m_szFirstPageObjOffset > = dwFirstObjLoc + nStreanLen; > On 2015/09/22 00:05:38, Tom Sepez wrote: > > how do we know this doesn't overflow? > > Acknowledged. > > https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4843: // where in the > page¡¯s content stream the object is first referenced. > On 2015/09/22 00:05:39, Tom Sepez wrote: > > nit: typo > > Acknowledged. > > https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4847: FX_DWORD > dwSharedDenominator = h.GetBits(16); > On 2015/09/22 00:05:39, Tom Sepez wrote: > > nit: just skipbits for item 13 like above. > > Acknowledged. > > https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4854: > m_dwDeltaNObjsArray.Add(h.GetBits(dwDeltaObjectsBits) + dwObjLeastNum); > On 2015/09/22 00:05:38, Tom Sepez wrote: > > overflow on addition? > > Acknowledged. > > https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4892: > m_szPageOffsetArray.Add(m_szFirstPageObjOffset); > On 2015/09/22 00:05:38, Tom Sepez wrote: > > I don't think this can be reached since we returned at 4850 otherwise? > > Acknowledged. > > https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4896: /** The number of > shared objects. */ > On 2015/09/22 00:05:39, Tom Sepez wrote: > > nit: prefer c++ // style. > > Acknowledged. > > https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4912: > h.SkipBits(dwDeltaPageLenBits * nPages); > On 2015/09/22 00:05:39, Tom Sepez wrote: > > overflow? > > Acknowledged. > > https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4920: CPDF_Array* pRange = > m_pLinearizedDict->GetArray(FX_BSTRC("H")); > On 2015/09/22 00:05:39, Tom Sepez wrote: > > nit: same as above. Let's make a helper function and consolidate. > > Acknowledged. > > https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4924: int nStreanLen = > pStreamLen ? pStreamLen->GetInteger() : 0; > On 2015/09/22 00:05:39, Tom Sepez wrote: > > nit: typo: stream vs. strean > > Acknowledged. > > https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:5084: const uint8_t* pData = > acc.GetData(); > On 2015/09/22 00:05:39, Tom Sepez wrote: > > nit: don't ned a local here, just bs.Init(acc.GetData()...) below. > > Acknowledged. > > https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:5087: if > (!ReadPageHintTable(bs) || !ReadSharedObjHintTable(bs)) { > On 2015/09/22 00:05:39, Tom Sepez wrote: > > nit: or just > > > > return ReadPageHintTable(bs) && readSharedObjHintTable(bs); > > Acknowledged. > > https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... > File core/src/fpdfapi/fpdf_parser/parser_int.h (right): > > https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... > core/src/fpdfapi/fpdf_parser/parser_int.h:32: CPDF_Dictionary* > m_pLinearizedDict; > On 2015/09/22 00:05:39, Tom Sepez wrote: > > nit: maybe blank line here between methods and members. > > Acknowledged. Hi Tom, We need to design some unit tests for this patch. It may take me two days to finish them.
https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:3757: CPDF_HintTables* pHintTables = new CPDF_HintTables(this, pDict); On 2015/09/22 12:42:32, jun_fang wrote: > On 2015/09/22 00:05:39, Tom Sepez wrote: > > nit: unique_ptr<> might help here, as well. > > If we define pHintTables and m_pHintTables like below: > nonstd::unique_ptr<CPDF_HintTables> pHintTables; > nonstd::unique_ptr<CPDF_HintTables> m_pHintTables; > > There is no way to assign pHintTables to m_pHintTables. See nonstd::move() (recently added) (a websearch on std::move() will give some background).
On 2015/09/22 15:16:43, Tom Sepez wrote: > https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... > File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): > > https://codereview.chromium.org/1353093003/diff/1/core/src/fpdfapi/fpdf_parse... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:3757: CPDF_HintTables* > pHintTables = new CPDF_HintTables(this, pDict); > On 2015/09/22 12:42:32, jun_fang wrote: > > On 2015/09/22 00:05:39, Tom Sepez wrote: > > > nit: unique_ptr<> might help here, as well. > > > > If we define pHintTables and m_pHintTables like below: > > nonstd::unique_ptr<CPDF_HintTables> pHintTables; > > nonstd::unique_ptr<CPDF_HintTables> m_pHintTables; > > > > There is no way to assign pHintTables to m_pHintTables. > See nonstd::move() (recently added) (a websearch on std::move() will give some > background). Thanks!
> > > There is no way to assign pHintTables to m_pHintTables. > > See nonstd::move() (recently added) (a websearch on std::move() will give some > > background). > > Thanks! Actually, that support isn't quite there, you'll need to use the older way: ptr2.reset(ptr1.release()); I've got a CL to add the newer ptr2 = std::move(ptr1) support later.
Jun, curious where the test file came from -- who authored it? https://codereview.chromium.org/1353093003/diff/60001/core/include/fpdfapi/fp... File core/include/fpdfapi/fpdf_parser.h (right): https://codereview.chromium.org/1353093003/diff/60001/core/include/fpdfapi/fp... core/include/fpdfapi/fpdf_parser.h:846: #define PDF_UNKNOW_LINEARIZED -1 nit: sp. UNKNOWN
https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:3090: m_bSupportHintTable = bSupportHintTable; Why did we lose this initialization? It needs to be null if we are going to delete it in the case where we never make a table. On the other hand, if we are using the unique_ptr class for this member, the class does the init for us (to NULL). https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:3102: delete m_pHintTables; remove when unique_ptr. https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:3763: m_pHintTables = pHintTables.get(); You need to call release(), not get() here, otherwise the table gets deleted when pHintTables goes out of scope. evenutally: m_pHintTables.reset(pHintTables.release()); https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... File core/src/fpdfapi/fpdf_parser/parser_int.h (right): https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/parser_int.h:5: // Original code copyright 2015 Foxit Software Inc. http://www.foxitsoftware.com Nah, no need to update this every time the year changes. https://codereview.chromium.org/1353093003/diff/60001/fpdfsdk/src/fpdf_dataav... File fpdfsdk/src/fpdf_dataavail.cpp (right): https://codereview.chromium.org/1353093003/diff/60001/fpdfsdk/src/fpdf_dataav... fpdfsdk/src/fpdf_dataavail.cpp:89: DLLEXPORT FX_BOOL STDCALL nit: Format seems wrong https://codereview.chromium.org/1353093003/diff/60001/public/fpdf_dataavail.h File public/fpdf_dataavail.h (right): https://codereview.chromium.org/1353093003/diff/60001/public/fpdf_dataavail.h... public/fpdf_dataavail.h:18: #define PDFFORM_NOTAVAIL 0 can we just use a bool here? https://codereview.chromium.org/1353093003/diff/60001/public/fpdf_dataavail.h... public/fpdf_dataavail.h:144: FPDFAvail_IsDocAvail(FPDF_AVAIL avail, FX_DOWNLOADHINTS* hints); nit: did you run git cl format? https://codereview.chromium.org/1353093003/diff/60001/testing/embedder_test.cpp File testing/embedder_test.cpp (right): https://codereview.chromium.org/1353093003/diff/60001/testing/embedder_test.c... testing/embedder_test.cpp:18: #include "../public/fpdf_dataavail.h" nit: alphabetize. https://codereview.chromium.org/1353093003/diff/60001/testing/embedder_test.c... testing/embedder_test.cpp:207: while (true) { strange spinnng here. do you have to yield or check something else? Are we sure that it will eventually return true? associated nit: I'd write while (!FPDFAvail_IsDocAvail(avail_, &hints_)) continue; https://codereview.chromium.org/1353093003/diff/60001/testing/embedder_test.c... testing/embedder_test.cpp:216: while (true) { nit: same here.
https://codereview.chromium.org/1353093003/diff/60001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/1353093003/diff/60001/BUILD.gn#newcode299 BUILD.gn:299: "core/src/fpdfapi/fpdf_parser/parser_int.h" missing trailing comma https://codereview.chromium.org/1353093003/diff/60001/core/include/fpdfapi/fp... File core/include/fpdfapi/fpdf_parser.h (right): https://codereview.chromium.org/1353093003/diff/60001/core/include/fpdfapi/fp... core/include/fpdfapi/fpdf_parser.h:37: class CPDF_HintTables; alphabetical order https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2816: FX_BOOL bSupportHintTable = FALSE); It would be nice to not have default parameters https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:3025: friend class CPDF_HintTables; just put this in the public section https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:3190: if (!IsDataAvail(offset, size, pHints)) { In the old code, if IsValid() returns false, we break out of the switch() and continues to the next for-loop iteration. Now, we continue on to the else if block. Is this intentional? https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4795: FX_BOOL CPDF_HintTables::ReadPageHintTable(CFX_BitStream& h) { Change this to a pointer, i.e. no non-const references please. https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4866: if (i == nFirstPageNum) braces {} to match else statements https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4869: if (i == 1) ditto https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4876: if (i == 0) ditto ditto https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4966: if (i == m_nFirstPageSharedObjs) braces here too https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... File core/src/fpdfapi/fpdf_parser/parser_int.h (right): https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/parser_int.h:5: // Original code copyright 2015 Foxit Software Inc. http://www.foxitsoftware.com On 2015/09/22 19:17:59, Tom Sepez wrote: > Nah, no need to update this every time the year changes. Well, if FoxIt is writing this code in 2015... Also, line 1 says 2014 still. https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/parser_int.h:7: #ifndef _FPDFAPI_PARSER_INT_H_ CORE_SRC_FPDFAPI_FPDF_PARSER_PARSER_INT_H_ https://codereview.chromium.org/1353093003/diff/60001/public/fpdf_dataavail.h File public/fpdf_dataavail.h (right): https://codereview.chromium.org/1353093003/diff/60001/public/fpdf_dataavail.h... public/fpdf_dataavail.h:144: FPDFAvail_IsDocAvail(FPDF_AVAIL avail, FX_DOWNLOADHINTS* hints); On 2015/09/22 19:17:59, Tom Sepez wrote: > nit: did you run git cl format? I believe "git cl format" changed its mind at some point. Sigh. https://codereview.chromium.org/1353093003/diff/60001/public/fpdf_dataavail.h... public/fpdf_dataavail.h:253: * FSDK_UNKNOW_LINEARIZED because there is no enough information to "not enough"
https://codereview.chromium.org/1353093003/diff/60001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/1353093003/diff/60001/BUILD.gn#newcode299 BUILD.gn:299: "core/src/fpdfapi/fpdf_parser/parser_int.h" On 2015/09/22 23:33:42, Lei Zhang wrote: > missing trailing comma Acknowledged. https://codereview.chromium.org/1353093003/diff/60001/core/include/fpdfapi/fp... File core/include/fpdfapi/fpdf_parser.h (right): https://codereview.chromium.org/1353093003/diff/60001/core/include/fpdfapi/fp... core/include/fpdfapi/fpdf_parser.h:37: class CPDF_HintTables; On 2015/09/22 23:33:43, Lei Zhang wrote: > alphabetical order Acknowledged. https://codereview.chromium.org/1353093003/diff/60001/core/include/fpdfapi/fp... core/include/fpdfapi/fpdf_parser.h:846: #define PDF_UNKNOW_LINEARIZED -1 On 2015/09/22 18:58:30, Tom Sepez wrote: > nit: sp. UNKNOWN Acknowledged. https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2816: FX_BOOL bSupportHintTable = FALSE); On 2015/09/22 23:33:43, Lei Zhang wrote: > It would be nice to not have default parameters Acknowledged. https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:3025: friend class CPDF_HintTables; On 2015/09/22 23:33:43, Lei Zhang wrote: > just put this in the public section Acknowledged. https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:3090: m_bSupportHintTable = bSupportHintTable; On 2015/09/22 19:17:59, Tom Sepez wrote: > Why did we lose this initialization? It needs to be null if we are going to > delete it in the case where we never make a table. On the other hand, if we are > using the unique_ptr class for this member, the class does the init for us (to > NULL). Good catch! I tried to use unique_ptr first but didn't find a way to assign one unique_ptr to the others. And then I went back to use CPDF_HintTables* m_pHintTables but forgot to add the init back. https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:3102: delete m_pHintTables; On 2015/09/22 19:17:59, Tom Sepez wrote: > remove when unique_ptr. Acknowledged. https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:3190: if (!IsDataAvail(offset, size, pHints)) { On 2015/09/22 23:33:43, Lei Zhang wrote: > In the old code, if IsValid() returns false, we break out of the switch() and > continues to the next for-loop iteration. Now, we continue on to the else if > block. Is this intentional? Good catch. Will update the code in IsDataAvail(). If IsValid() return false, only check whether size (= m_dwFileLen - offset) is available rather than returning false in the old code. https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:3763: m_pHintTables = pHintTables.get(); On 2015/09/22 19:17:59, Tom Sepez wrote: > You need to call release(), not get() here, otherwise the table gets deleted > when pHintTables goes out of scope. > > evenutally: > m_pHintTables.reset(pHintTables.release()); Acknowledged. https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4795: FX_BOOL CPDF_HintTables::ReadPageHintTable(CFX_BitStream& h) { On 2015/09/22 23:33:43, Lei Zhang wrote: > Change this to a pointer, i.e. no non-const references please. Acknowledged. https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4866: if (i == nFirstPageNum) On 2015/09/22 23:33:43, Lei Zhang wrote: > braces {} to match else statements Acknowledged. I saw that your style was no braces{} for single line in "if", right? https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4869: if (i == 1) On 2015/09/22 23:33:43, Lei Zhang wrote: > ditto Acknowledged. https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4876: if (i == 0) On 2015/09/22 23:33:43, Lei Zhang wrote: > ditto ditto Acknowledged. https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... File core/src/fpdfapi/fpdf_parser/parser_int.h (right): https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/parser_int.h:5: // Original code copyright 2015 Foxit Software Inc. http://www.foxitsoftware.com On 2015/09/22 23:33:43, Lei Zhang wrote: > On 2015/09/22 19:17:59, Tom Sepez wrote: > > Nah, no need to update this every time the year changes. > > Well, if FoxIt is writing this code in 2015... > > Also, line 1 says 2014 still. OK. will keep 2014 here. https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/parser_int.h:7: #ifndef _FPDFAPI_PARSER_INT_H_ On 2015/09/22 23:33:43, Lei Zhang wrote: > CORE_SRC_FPDFAPI_FPDF_PARSER_PARSER_INT_H_ Acknowledged. https://codereview.chromium.org/1353093003/diff/60001/fpdfsdk/src/fpdf_dataav... File fpdfsdk/src/fpdf_dataavail.cpp (right): https://codereview.chromium.org/1353093003/diff/60001/fpdfsdk/src/fpdf_dataav... fpdfsdk/src/fpdf_dataavail.cpp:89: DLLEXPORT FX_BOOL STDCALL On 2015/09/22 19:17:59, Tom Sepez wrote: > nit: Format seems wrong Maybe it was caused by the command "git cl format". https://codereview.chromium.org/1353093003/diff/60001/public/fpdf_dataavail.h File public/fpdf_dataavail.h (right): https://codereview.chromium.org/1353093003/diff/60001/public/fpdf_dataavail.h... public/fpdf_dataavail.h:18: #define PDFFORM_NOTAVAIL 0 On 2015/09/22 19:17:59, Tom Sepez wrote: > can we just use a bool here? It's used to check the return value from CPDF_DataAvail::IsFormAvail(). There are 3 possible values: #define PDFFORM_NOTAVAIL 0 #define PDFFORM_AVAIL 1 #define PDFFORM_NOTEXIST 2 https://codereview.chromium.org/1353093003/diff/60001/public/fpdf_dataavail.h... public/fpdf_dataavail.h:144: FPDFAvail_IsDocAvail(FPDF_AVAIL avail, FX_DOWNLOADHINTS* hints); On 2015/09/22 23:33:43, Lei Zhang wrote: > On 2015/09/22 19:17:59, Tom Sepez wrote: > > nit: did you run git cl format? > > I believe "git cl format" changed its mind at some point. Sigh. you are right. I did run git cl format. This mistake was made by this tool. https://codereview.chromium.org/1353093003/diff/60001/public/fpdf_dataavail.h... public/fpdf_dataavail.h:253: * FSDK_UNKNOW_LINEARIZED because there is no enough information to On 2015/09/22 23:33:43, Lei Zhang wrote: > "not enough" Acknowledged. https://codereview.chromium.org/1353093003/diff/60001/testing/embedder_test.cpp File testing/embedder_test.cpp (right): https://codereview.chromium.org/1353093003/diff/60001/testing/embedder_test.c... testing/embedder_test.cpp:18: #include "../public/fpdf_dataavail.h" On 2015/09/22 19:17:59, Tom Sepez wrote: > nit: alphabetize. Acknowledged. https://codereview.chromium.org/1353093003/diff/60001/testing/embedder_test.c... testing/embedder_test.cpp:207: while (true) { On 2015/09/22 19:17:59, Tom Sepez wrote: > strange spinnng here. do you have to yield or check something else? Are we sure > that it will eventually return true? > associated nit: I'd write > while (!FPDFAvail_IsDocAvail(avail_, &hints_)) > continue; If the return value is false, that means we need to wait for data to be downloaded. https://codereview.chromium.org/1353093003/diff/60001/testing/embedder_test.c... testing/embedder_test.cpp:216: while (true) { On 2015/09/22 19:17:59, Tom Sepez wrote: > nit: same here. Acknowledged.
On 2015/09/23 12:26:59, jun_fang wrote: > https://codereview.chromium.org/1353093003/diff/60001/BUILD.gn > File BUILD.gn (right): > > https://codereview.chromium.org/1353093003/diff/60001/BUILD.gn#newcode299 > BUILD.gn:299: "core/src/fpdfapi/fpdf_parser/parser_int.h" > On 2015/09/22 23:33:42, Lei Zhang wrote: > > missing trailing comma > > Acknowledged. > > https://codereview.chromium.org/1353093003/diff/60001/core/include/fpdfapi/fp... > File core/include/fpdfapi/fpdf_parser.h (right): > > https://codereview.chromium.org/1353093003/diff/60001/core/include/fpdfapi/fp... > core/include/fpdfapi/fpdf_parser.h:37: class CPDF_HintTables; > On 2015/09/22 23:33:43, Lei Zhang wrote: > > alphabetical order > > Acknowledged. > > https://codereview.chromium.org/1353093003/diff/60001/core/include/fpdfapi/fp... > core/include/fpdfapi/fpdf_parser.h:846: #define PDF_UNKNOW_LINEARIZED -1 > On 2015/09/22 18:58:30, Tom Sepez wrote: > > nit: sp. UNKNOWN > > Acknowledged. > > https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... > File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): > > https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2816: FX_BOOL > bSupportHintTable = FALSE); > On 2015/09/22 23:33:43, Lei Zhang wrote: > > It would be nice to not have default parameters > > Acknowledged. > > https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:3025: friend class > CPDF_HintTables; > On 2015/09/22 23:33:43, Lei Zhang wrote: > > just put this in the public section > > Acknowledged. > > https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:3090: m_bSupportHintTable = > bSupportHintTable; > On 2015/09/22 19:17:59, Tom Sepez wrote: > > Why did we lose this initialization? It needs to be null if we are going to > > delete it in the case where we never make a table. On the other hand, if we > are > > using the unique_ptr class for this member, the class does the init for us (to > > NULL). > > Good catch! I tried to use unique_ptr first but didn't find a way to assign one > unique_ptr to the others. And then I went back to use CPDF_HintTables* > m_pHintTables but forgot to add the init back. > > https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:3102: delete m_pHintTables; > On 2015/09/22 19:17:59, Tom Sepez wrote: > > remove when unique_ptr. > > Acknowledged. > > https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:3190: if > (!IsDataAvail(offset, size, pHints)) { > On 2015/09/22 23:33:43, Lei Zhang wrote: > > In the old code, if IsValid() returns false, we break out of the switch() and > > continues to the next for-loop iteration. Now, we continue on to the else if > > block. Is this intentional? > > Good catch. Will update the code in IsDataAvail(). If IsValid() return false, > only check whether size (= m_dwFileLen - offset) is available rather than > returning false in the old code. > > https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:3763: m_pHintTables = > pHintTables.get(); > On 2015/09/22 19:17:59, Tom Sepez wrote: > > You need to call release(), not get() here, otherwise the table gets deleted > > when pHintTables goes out of scope. > > > > evenutally: > > m_pHintTables.reset(pHintTables.release()); > > Acknowledged. > > https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4795: FX_BOOL > CPDF_HintTables::ReadPageHintTable(CFX_BitStream& h) { > On 2015/09/22 23:33:43, Lei Zhang wrote: > > Change this to a pointer, i.e. no non-const references please. > > Acknowledged. > > https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4866: if (i == > nFirstPageNum) > On 2015/09/22 23:33:43, Lei Zhang wrote: > > braces {} to match else statements > > Acknowledged. > > I saw that your style was no braces{} for single line in "if", right? > > https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4869: if (i == 1) > On 2015/09/22 23:33:43, Lei Zhang wrote: > > ditto > > Acknowledged. > > https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4876: if (i == 0) > On 2015/09/22 23:33:43, Lei Zhang wrote: > > ditto ditto > > Acknowledged. > > https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... > File core/src/fpdfapi/fpdf_parser/parser_int.h (right): > > https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... > core/src/fpdfapi/fpdf_parser/parser_int.h:5: // Original code copyright 2015 > Foxit Software Inc. http://www.foxitsoftware.com > On 2015/09/22 23:33:43, Lei Zhang wrote: > > On 2015/09/22 19:17:59, Tom Sepez wrote: > > > Nah, no need to update this every time the year changes. > > > > Well, if FoxIt is writing this code in 2015... > > > > Also, line 1 says 2014 still. > > OK. will keep 2014 here. > > https://codereview.chromium.org/1353093003/diff/60001/core/src/fpdfapi/fpdf_p... > core/src/fpdfapi/fpdf_parser/parser_int.h:7: #ifndef _FPDFAPI_PARSER_INT_H_ > On 2015/09/22 23:33:43, Lei Zhang wrote: > > CORE_SRC_FPDFAPI_FPDF_PARSER_PARSER_INT_H_ > > Acknowledged. > > https://codereview.chromium.org/1353093003/diff/60001/fpdfsdk/src/fpdf_dataav... > File fpdfsdk/src/fpdf_dataavail.cpp (right): > > https://codereview.chromium.org/1353093003/diff/60001/fpdfsdk/src/fpdf_dataav... > fpdfsdk/src/fpdf_dataavail.cpp:89: DLLEXPORT FX_BOOL STDCALL > On 2015/09/22 19:17:59, Tom Sepez wrote: > > nit: Format seems wrong > > Maybe it was caused by the command "git cl format". > > https://codereview.chromium.org/1353093003/diff/60001/public/fpdf_dataavail.h > File public/fpdf_dataavail.h (right): > > https://codereview.chromium.org/1353093003/diff/60001/public/fpdf_dataavail.h... > public/fpdf_dataavail.h:18: #define PDFFORM_NOTAVAIL 0 > On 2015/09/22 19:17:59, Tom Sepez wrote: > > can we just use a bool here? > > It's used to check the return value from CPDF_DataAvail::IsFormAvail(). > There are 3 possible values: > #define PDFFORM_NOTAVAIL 0 > #define PDFFORM_AVAIL 1 > #define PDFFORM_NOTEXIST 2 > > https://codereview.chromium.org/1353093003/diff/60001/public/fpdf_dataavail.h... > public/fpdf_dataavail.h:144: FPDFAvail_IsDocAvail(FPDF_AVAIL avail, > FX_DOWNLOADHINTS* hints); > On 2015/09/22 23:33:43, Lei Zhang wrote: > > On 2015/09/22 19:17:59, Tom Sepez wrote: > > > nit: did you run git cl format? > > > > I believe "git cl format" changed its mind at some point. Sigh. > > you are right. I did run git cl format. This mistake was made by this tool. > > https://codereview.chromium.org/1353093003/diff/60001/public/fpdf_dataavail.h... > public/fpdf_dataavail.h:253: * FSDK_UNKNOW_LINEARIZED because there is > no enough information to > On 2015/09/22 23:33:43, Lei Zhang wrote: > > "not enough" > > Acknowledged. > > https://codereview.chromium.org/1353093003/diff/60001/testing/embedder_test.cpp > File testing/embedder_test.cpp (right): > > https://codereview.chromium.org/1353093003/diff/60001/testing/embedder_test.c... > testing/embedder_test.cpp:18: #include "../public/fpdf_dataavail.h" > On 2015/09/22 19:17:59, Tom Sepez wrote: > > nit: alphabetize. > > Acknowledged. > > https://codereview.chromium.org/1353093003/diff/60001/testing/embedder_test.c... > testing/embedder_test.cpp:207: while (true) { > On 2015/09/22 19:17:59, Tom Sepez wrote: > > strange spinnng here. do you have to yield or check something else? Are we > sure > > that it will eventually return true? > > associated nit: I'd write > > while (!FPDFAvail_IsDocAvail(avail_, &hints_)) > > continue; > > If the return value is false, that means we need to wait for data to be > downloaded. > > https://codereview.chromium.org/1353093003/diff/60001/testing/embedder_test.c... > testing/embedder_test.cpp:216: while (true) { > On 2015/09/22 19:17:59, Tom Sepez wrote: > > nit: same here. > > Acknowledged. Move to https://codereview.chromium.org/1366593002/.
FYI, the bug number for the BUG= line is 446715
https://codereview.chromium.org/1353093003/diff/60001/public/fpdf_dataavail.h File public/fpdf_dataavail.h (right): https://codereview.chromium.org/1353093003/diff/60001/public/fpdf_dataavail.h... public/fpdf_dataavail.h:18: #define PDFFORM_NOTAVAIL 0 On 2015/09/23 12:26:59, jun_fang wrote: > On 2015/09/22 19:17:59, Tom Sepez wrote: > > can we just use a bool here? > > It's used to check the return value from CPDF_DataAvail::IsFormAvail(). > There are 3 possible values: > #define PDFFORM_NOTAVAIL 0 > #define PDFFORM_AVAIL 1 > #define PDFFORM_NOTEXIST 2 So are we duplicating definitions in another header file? That would be wrong. Its fine to include this file and use public constants all the way down ... what happened to the notexist constant? https://codereview.chromium.org/1353093003/diff/60001/testing/embedder_test.cpp File testing/embedder_test.cpp (right): https://codereview.chromium.org/1353093003/diff/60001/testing/embedder_test.c... testing/embedder_test.cpp:207: while (true) { On 2015/09/23 12:26:59, jun_fang wrote: > On 2015/09/22 19:17:59, Tom Sepez wrote: > > strange spinnng here. do you have to yield or check something else? Are we > sure > > that it will eventually return true? > > associated nit: I'd write > > while (!FPDFAvail_IsDocAvail(avail_, &hints_)) > > continue; > > If the return value is false, that means we need to wait for data to be > downloaded. So...do we need an IFX_Pause or some such to check here? https://codereview.chromium.org/1353093003/diff/80001/core/include/fpdfapi/fp... File core/include/fpdfapi/fpdf_parser.h (right): https://codereview.chromium.org/1353093003/diff/80001/core/include/fpdfapi/fp... core/include/fpdfapi/fpdf_parser.h:847: #define PDF_FILE_UNKNOW -1 nit: Still misspelled. UNKNOWN how about PDF_LINEARIZED PDF_NOT_LINEARIZED PDF_LINEARIZATION_UNKNOWN
https://codereview.chromium.org/1353093003/diff/80001/core/include/fpdfapi/fp... File core/include/fpdfapi/fpdf_parser.h (right): https://codereview.chromium.org/1353093003/diff/80001/core/include/fpdfapi/fp... core/include/fpdfapi/fpdf_parser.h:847: #define PDF_FILE_UNKNOW -1 On 2015/09/24 18:24:40, Tom Sepez wrote: > nit: Still misspelled. UNKNOWN > how about > PDF_LINEARIZED > PDF_NOT_LINEARIZED > PDF_LINEARIZATION_UNKNOWN Acknowledged.
Very much looking forward to trying this out.
jun_fang@foxitsoftware.com changed reviewers: + yali_he@foxitsoftware.com
On 2015/10/14 11:45:32, jun_fang wrote: feature_linearized_loading.pdf was manually created by Foxit.
Thanks for the test! https://codereview.chromium.org/1353093003/diff/100001/core/include/fpdfapi/f... File core/include/fpdfapi/fpdf_parser.h (right): https://codereview.chromium.org/1353093003/diff/100001/core/include/fpdfapi/f... core/include/fpdfapi/fpdf_parser.h:10: #include "../../../public/fpdf_macro.h" this should just include public/fpdf_datavail.h, no need for the new file. https://codereview.chromium.org/1353093003/diff/100001/core/src/fpdfapi/fpdf_... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): https://codereview.chromium.org/1353093003/diff/100001/core/src/fpdfapi/fpdf_... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4746: m_nFirstPageSharedObjs = 0; These assignments are usually pointless in a dtor. https://codereview.chromium.org/1353093003/diff/100001/core/src/fpdfapi/fpdf_... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4757: if (index < 0 || index > szArray.GetSize() - 2) szArray.GetSize() - 2 would underflow if it were ever changed to return an unsigned type and the size was 0 or 1. Probably OK for now, but it makes replacing CFX containers with STL ones more difficult. You'd want to assert that szArraySize() >= 2 perhaps. https://codereview.chromium.org/1353093003/diff/100001/core/src/fpdfapi/fpdf_... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_embeddertest.cpp (right): https://codereview.chromium.org/1353093003/diff/100001/core/src/fpdfapi/fpdf_... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_embeddertest.cpp:12: EXPECT_FALSE(OpenDocument("testing/resources/bug_454695.pdf")); Are we sure its OK to fail these documents? https://codereview.chromium.org/1353093003/diff/100001/core/src/fpdfapi/fpdf_... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_embeddertest.cpp:24: EXPECT_TRUE(OpenDocument("testing/resources/feature_linearized_loading.pdf")); How do we know that the linearized loading path was taken? If I accidentally changed feature_linearized_loading.pdf to not do so, this would still pass. https://codereview.chromium.org/1353093003/diff/100001/fpdfsdk/src/fpdf_dataa... File fpdfsdk/src/fpdf_dataavail.cpp (right): https://codereview.chromium.org/1353093003/diff/100001/fpdfsdk/src/fpdf_dataa... fpdfsdk/src/fpdf_dataavail.cpp:92: return true; why are we inverting this return value? https://codereview.chromium.org/1353093003/diff/100001/fpdfsdk/src/fpdf_dataa... fpdfsdk/src/fpdf_dataavail.cpp:127: return true; same here https://codereview.chromium.org/1353093003/diff/100001/public/fpdf_macro.h File public/fpdf_macro.h (right): https://codereview.chromium.org/1353093003/diff/100001/public/fpdf_macro.h#ne... public/fpdf_macro.h:1: // Copyright 2014 PDFium Authors. All rights reserved. This should remain in the fpdf_datavail.h file; why do we need this new file? https://codereview.chromium.org/1353093003/diff/100001/testing/embedder_test.cpp File testing/embedder_test.cpp (right): https://codereview.chromium.org/1353093003/diff/100001/testing/embedder_test.... testing/embedder_test.cpp:223: while (!FPDFAvail_IsDocAvail(avail_, &hints_)) do we need to make this same change in samples/pdfium_test.cc ?
https://codereview.chromium.org/1353093003/diff/100001/core/include/fpdfapi/f... File core/include/fpdfapi/fpdf_parser.h (right): https://codereview.chromium.org/1353093003/diff/100001/core/include/fpdfapi/f... core/include/fpdfapi/fpdf_parser.h:10: #include "../../../public/fpdf_macro.h" On 2015/10/14 16:57:45, Tom Sepez wrote: > this should just include public/fpdf_datavail.h, no need for the new file. Pdfium has two parts, sdk and core. sdk has the dependency on core so sdk includes header files from core. In contrast, core doesn't include any sdk header files in order not to break the rule. For marcos used by sdk and core, currently they are defined twice. One is defined in public for applications to use. The other is defined in core. public/fpdf_datavail.h declares some APIs related to linearization. Usually, it's included by customer's applications to implement linearization functionality. The purpose of including public/fpdf_datavail.h in core/include/fpdfapi/fpdf_parser.h is only to use macros. It's not worthy to include whole header file. I want to define all marcos used by applications, sdk and core in this new head file to make design clear. In this way, we don't need to define macros twice. We don't include the whole header file if only macros will be used. https://codereview.chromium.org/1353093003/diff/100001/core/src/fpdfapi/fpdf_... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): https://codereview.chromium.org/1353093003/diff/100001/core/src/fpdfapi/fpdf_... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4746: m_nFirstPageSharedObjs = 0; On 2015/10/14 16:57:45, Tom Sepez wrote: > These assignments are usually pointless in a dtor. Acknowledged. https://codereview.chromium.org/1353093003/diff/100001/core/src/fpdfapi/fpdf_... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4757: if (index < 0 || index > szArray.GetSize() - 2) On 2015/10/14 16:57:45, Tom Sepez wrote: > szArray.GetSize() - 2 would underflow if it were ever changed to return an > unsigned type and the size was 0 or 1. Probably OK for now, but it makes > replacing CFX containers with STL ones more difficult. You'd want to assert > that szArraySize() >= 2 perhaps. Acknowledged. https://codereview.chromium.org/1353093003/diff/100001/core/src/fpdfapi/fpdf_... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_embeddertest.cpp (right): https://codereview.chromium.org/1353093003/diff/100001/core/src/fpdfapi/fpdf_... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_embeddertest.cpp:12: EXPECT_FALSE(OpenDocument("testing/resources/bug_454695.pdf")); On 2015/10/14 16:57:45, Tom Sepez wrote: > Are we sure its OK to fail these documents? Before this CL, we didn't check whether document_ is nullptr. For bug_454695.pdf, the returned document_ is nullptr. I added a checking in this CL. if document_ is nullptr, it returns false. if (!FPDFAvail_IsLinearized(avail_)) { document_ = FPDF_LoadCustomDocument(&file_access_, nullptr); } else { document_ = FPDFAvail_GetDocument(avail_, nullptr); } https://codereview.chromium.org/1353093003/diff/100001/core/src/fpdfapi/fpdf_... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_embeddertest.cpp:24: EXPECT_TRUE(OpenDocument("testing/resources/feature_linearized_loading.pdf")); On 2015/10/14 16:57:45, Tom Sepez wrote: > How do we know that the linearized loading path was taken? If I accidentally > changed feature_linearized_loading.pdf to not do so, this would still pass. I am not clear about this question. https://codereview.chromium.org/1353093003/diff/100001/fpdfsdk/src/fpdf_dataa... File fpdfsdk/src/fpdf_dataavail.cpp (right): https://codereview.chromium.org/1353093003/diff/100001/fpdfsdk/src/fpdf_dataa... fpdfsdk/src/fpdf_dataavail.cpp:92: return true; On 2015/10/14 16:57:45, Tom Sepez wrote: > why are we inverting this return value? It means data are not available when false is returned. Applications will keep trying when data are not available. If avail or hints is nullptr, we think that the doc is not a linearized doc rather than data not available. https://codereview.chromium.org/1353093003/diff/100001/fpdfsdk/src/fpdf_dataa... fpdfsdk/src/fpdf_dataavail.cpp:127: return true; On 2015/10/14 16:57:45, Tom Sepez wrote: > same here The answer as above. https://codereview.chromium.org/1353093003/diff/100001/public/fpdf_macro.h File public/fpdf_macro.h (right): https://codereview.chromium.org/1353093003/diff/100001/public/fpdf_macro.h#ne... public/fpdf_macro.h:1: // Copyright 2014 PDFium Authors. All rights reserved. On 2015/10/14 16:57:45, Tom Sepez wrote: > This should remain in the fpdf_datavail.h file; why do we need this new file? I replied it for your first comment. https://codereview.chromium.org/1353093003/diff/100001/testing/embedder_test.cpp File testing/embedder_test.cpp (right): https://codereview.chromium.org/1353093003/diff/100001/testing/embedder_test.... testing/embedder_test.cpp:223: while (!FPDFAvail_IsDocAvail(avail_, &hints_)) On 2015/10/14 16:57:45, Tom Sepez wrote: > do we need to make this same change in samples/pdfium_test.cc ? Acknowledged.
https://codereview.chromium.org/1353093003/diff/100001/core/include/fpdfapi/f... File core/include/fpdfapi/fpdf_parser.h (right): https://codereview.chromium.org/1353093003/diff/100001/core/include/fpdfapi/f... core/include/fpdfapi/fpdf_parser.h:10: #include "../../../public/fpdf_macro.h" > We don't include the whole header file if only macros > will be used. Nah, it's much better to keep the declarations near where they are returned to make thing more understandable for the embedders, even at the expense of processing some additional lines when including the whole header. As to the the core/fpdfsdk split, if you want to be really strict about layering, then you'd have to have two definitions, with static_asserts() to ensure they line up, along with a comment about layering. Maybe its better to go back to that model. On the other hand, we already violate this, I'd presumed that public can be included at any level, which is part of the reason I moved it from fpdfsdk/include. $ git grep public/ core core/include/fpdfapi/fpdf_parser.h:#include "../../../public/fpdfview.h" core/include/fpdfapi/fpdf_render.h:#include "../../../public/fpdf_progressive.h" core/src/fpdfapi/fpdf_parser/fpdf_parser_decode_embeddertest.cpp:#include "../../../../public/fpdfview.h"
https://codereview.chromium.org/1353093003/diff/100001/core/src/fpdfapi/fpdf_... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_embeddertest.cpp (right): https://codereview.chromium.org/1353093003/diff/100001/core/src/fpdfapi/fpdf_... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_embeddertest.cpp:24: EXPECT_TRUE(OpenDocument("testing/resources/feature_linearized_loading.pdf")); On 2015/10/15 10:22:28, jun_fang wrote: > On 2015/10/14 16:57:45, Tom Sepez wrote: > > How do we know that the linearized loading path was taken? If I accidentally > > changed feature_linearized_loading.pdf to not do so, this would still pass. > > I am not clear about this question. Ah. Suppose you landed this test, but did not land the corresponding code that implements linearized loading. In that case, this test would still pass. Is there a way that we can make it so the test fails unless linearized loading support is present? If someone were to break the code such that the linearized loading path was never taken, instead always falling back to the non-linearized path, we'd like to detect that in the test.
On 2015/10/15 16:23:30, Tom Sepez wrote: > https://codereview.chromium.org/1353093003/diff/100001/core/src/fpdfapi/fpdf_... > File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_embeddertest.cpp (right): > > https://codereview.chromium.org/1353093003/diff/100001/core/src/fpdfapi/fpdf_... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_embeddertest.cpp:24: > EXPECT_TRUE(OpenDocument("testing/resources/feature_linearized_loading.pdf")); > On 2015/10/15 10:22:28, jun_fang wrote: > > On 2015/10/14 16:57:45, Tom Sepez wrote: > > > How do we know that the linearized loading path was taken? If I accidentally > > > changed feature_linearized_loading.pdf to not do so, this would still pass. > > > > I am not clear about this question. > Ah. Suppose you landed this test, but did not land the corresponding code that > implements linearized loading. In that case, this test would still pass. Is > there a way that we can make it so the test fails unless linearized loading > support is present? If someone were to break the code such that the linearized > loading path was never taken, instead always falling back to the non-linearized > path, we'd like to detect that in the test. Anyways, I'm glad to see we're making progress on this. Let me know when you get the next patch set.
https://codereview.chromium.org/1353093003/diff/100001/core/include/fpdfapi/f... File core/include/fpdfapi/fpdf_parser.h (right): https://codereview.chromium.org/1353093003/diff/100001/core/include/fpdfapi/f... core/include/fpdfapi/fpdf_parser.h:10: #include "../../../public/fpdf_macro.h" On 2015/10/15 16:20:04, Tom Sepez wrote: > > We don't include the whole header file if only macros > > will be used. > Nah, it's much better to keep the declarations near where they are returned to > make thing more understandable for the embedders, even at the expense of > processing some additional lines when including the whole header. > > As to the the core/fpdfsdk split, if you want to be really strict about > layering, then you'd have to have two definitions, with static_asserts() to > ensure they line up, along with a comment about layering. Maybe its better to > go back to that model. > > On the other hand, we already violate this, I'd presumed that public > can be included at any level, which is part of the reason I moved it from > fpdfsdk/include. > > $ git grep public/ core > core/include/fpdfapi/fpdf_parser.h:#include "../../../public/fpdfview.h" > core/include/fpdfapi/fpdf_render.h:#include "../../../public/fpdf_progressive.h" > core/src/fpdfapi/fpdf_parser/fpdf_parser_decode_embeddertest.cpp:#include > "../../../../public/fpdfview.h" > Acknowledged. https://codereview.chromium.org/1353093003/diff/100001/core/src/fpdfapi/fpdf_... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_embeddertest.cpp (right): https://codereview.chromium.org/1353093003/diff/100001/core/src/fpdfapi/fpdf_... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_embeddertest.cpp:24: EXPECT_TRUE(OpenDocument("testing/resources/feature_linearized_loading.pdf")); On 2015/10/15 16:23:30, Tom Sepez wrote: > On 2015/10/15 10:22:28, jun_fang wrote: > > On 2015/10/14 16:57:45, Tom Sepez wrote: > > > How do we know that the linearized loading path was taken? If I accidentally > > > changed feature_linearized_loading.pdf to not do so, this would still pass. > > > > I am not clear about this question. > Ah. Suppose you landed this test, but did not land the corresponding code that > implements linearized loading. In that case, this test would still pass. Is > there a way that we can make it so the test fails unless linearized loading > support is present? If someone were to break the code such that the linearized > loading path was never taken, instead always falling back to the non-linearized > path, we'd like to detect that in the test. You are right. Currently, we don't have a perfect solution to detect. Pdfium always falls back to non-linearized path when it detects that it can't handle a pdf file in the linearized way.
https://codereview.chromium.org/1353093003/diff/100001/core/src/fpdfapi/fpdf_... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): https://codereview.chromium.org/1353093003/diff/100001/core/src/fpdfapi/fpdf_... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:3721: return FALSE; m_docStatus should be set as PDF_DATAAVAIL_DONE before returning FALSE. https://codereview.chromium.org/1353093003/diff/100001/core/src/fpdfapi/fpdf_... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:3724: return FALSE; The same as above. https://codereview.chromium.org/1353093003/diff/100001/core/src/fpdfapi/fpdf_... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4648: return m_pDocument->GetPageCount(); retrun m_pDocument ? m_pDocument->GetPageCount() : 0; https://codereview.chromium.org/1353093003/diff/100001/core/src/fpdfapi/fpdf_... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4759: return szArray[index + 1] - szArray[index]; szArray[index + 1] must be larger than szArray[index]
https://codereview.chromium.org/1353093003/diff/100001/core/src/fpdfapi/fpdf_... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_embeddertest.cpp (right): https://codereview.chromium.org/1353093003/diff/100001/core/src/fpdfapi/fpdf_... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_embeddertest.cpp:24: EXPECT_TRUE(OpenDocument("testing/resources/feature_linearized_loading.pdf")); On 2015/10/19 13:20:30, jun_fang wrote: > On 2015/10/15 16:23:30, Tom Sepez wrote: > > On 2015/10/15 10:22:28, jun_fang wrote: > > > On 2015/10/14 16:57:45, Tom Sepez wrote: > > > > How do we know that the linearized loading path was taken? If I > accidentally > > > > changed feature_linearized_loading.pdf to not do so, this would still > pass. > > > > > > I am not clear about this question. > > Ah. Suppose you landed this test, but did not land the corresponding code > that > > implements linearized loading. In that case, this test would still pass. Is > > there a way that we can make it so the test fails unless linearized loading > > support is present? If someone were to break the code such that the > linearized > > loading path was never taken, instead always falling back to the > non-linearized > > path, we'd like to detect that in the test. > > You are right. Currently, we don't have a perfect solution to detect. > Pdfium always falls back to non-linearized path when it detects that > it can't handle a pdf file in the linearized way. Ok, looking at EmbedderTest::OpenDocument(), we could add an optional argument called must_linearize, which would then return false instead of failing back at line https://code.google.com/p/chromium/codesearch#chromium/src/third_party/pdfium...
https://codereview.chromium.org/1353093003/diff/100001/core/src/fpdfapi/fpdf_... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): https://codereview.chromium.org/1353093003/diff/100001/core/src/fpdfapi/fpdf_... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:3688: return TRUE; m_docStatus should be set as PDF_DATAAVAIL_DONE before returning TRUE.
https://codereview.chromium.org/1353093003/diff/140001/core/src/fpdfapi/fpdf_... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): https://codereview.chromium.org/1353093003/diff/140001/core/src/fpdfapi/fpdf_... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:3190: if (!pHints) { It's checked in APIs. https://codereview.chromium.org/1353093003/diff/140001/core/src/fpdfapi/fpdf_... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4493: if (!m_pDocument || !pHints) { It's checked in API level. https://codereview.chromium.org/1353093003/diff/160001/core/src/fpdfapi/fpdf_... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): https://codereview.chromium.org/1353093003/diff/160001/core/src/fpdfapi/fpdf_... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:3687: m_docStatus = PDF_DATAAVAIL_ERROR; Set doc status to PDF_DATAAVAIL_ERROR and return false. It will trigger the state machine to be run.
On 2015/10/21 11:08:12, jun_fang wrote: > https://codereview.chromium.org/1353093003/diff/140001/core/src/fpdfapi/fpdf_... > File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): > > https://codereview.chromium.org/1353093003/diff/140001/core/src/fpdfapi/fpdf_... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:3190: if (!pHints) { > It's checked in APIs. > > https://codereview.chromium.org/1353093003/diff/140001/core/src/fpdfapi/fpdf_... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4493: if (!m_pDocument || > !pHints) { > It's checked in API level. > > https://codereview.chromium.org/1353093003/diff/160001/core/src/fpdfapi/fpdf_... > File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): > > https://codereview.chromium.org/1353093003/diff/160001/core/src/fpdfapi/fpdf_... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:3687: m_docStatus = > PDF_DATAAVAIL_ERROR; > Set doc status to PDF_DATAAVAIL_ERROR and return false. It will trigger the > state machine to be run. Please review patch 10.
https://codereview.chromium.org/1353093003/diff/180001/public/fpdf_dataavail.h File public/fpdf_dataavail.h (right): https://codereview.chromium.org/1353093003/diff/180001/public/fpdf_dataavail.... public/fpdf_dataavail.h:138: * PDF_DATA_ERROR: A common error is returned. It can't tell Public API change. Can we do what we need to do without breaking compatibliity? https://codereview.chromium.org/1353093003/diff/180001/samples/pdfium_test.cc File samples/pdfium_test.cc (right): https://codereview.chromium.org/1353093003/diff/180001/samples/pdfium_test.cc... samples/pdfium_test.cc:549: nRet = FPDFAvail_IsDocAvail(pdf_avail, &hints); For example, if we had a separate function to check if the reason for the unavailability was due to error, the public API stays stable, e.g. while (!FPDFAvail_IsDocAvail(pdf_avail, &hints) { if (FPDFAvail_Error(fpdf_avail)) return; } https://codereview.chromium.org/1353093003/diff/180001/testing/embedder_test.cpp File testing/embedder_test.cpp (right): https://codereview.chromium.org/1353093003/diff/180001/testing/embedder_test.... testing/embedder_test.cpp:244: } else { I'd imagine you would wait til here to check must_linearize, returing error before hitting the non-linearized path. https://codereview.chromium.org/1353093003/diff/180001/testing/embedder_test.h File testing/embedder_test.h (right): https://codereview.chromium.org/1353093003/diff/180001/testing/embedder_test.... testing/embedder_test.h:84: const bool must_linearize); nit: const is pointless for bool arguments. Also maybe a default value of false to avoid updating all those call sites.
https://codereview.chromium.org/1353093003/diff/180001/public/fpdf_dataavail.h File public/fpdf_dataavail.h (right): https://codereview.chromium.org/1353093003/diff/180001/public/fpdf_dataavail.... public/fpdf_dataavail.h:138: * PDF_DATA_ERROR: A common error is returned. It can't tell On 2015/10/21 17:53:08, Tom Sepez wrote: > Public API change. Can we do what we need to do without breaking compatibliity? This public API was designed for linearization functionality. Before this CL, this functionality wasn't implemented correctly. This is the first time that applications can call this API in the right way. So this API is almost a new API. In the other hand, I define PDF_DATA_ERROR as -1. It follows the legacy logic. Before this CL, it returns true even this are any errors. So there are two possibilities for 'true', data available or error. Both of them are non-zero. https://codereview.chromium.org/1353093003/diff/180001/samples/pdfium_test.cc File samples/pdfium_test.cc (right): https://codereview.chromium.org/1353093003/diff/180001/samples/pdfium_test.cc... samples/pdfium_test.cc:549: nRet = FPDFAvail_IsDocAvail(pdf_avail, &hints); On 2015/10/21 17:53:08, Tom Sepez wrote: > For example, if we had a separate function to check if the reason for the > unavailability was due to error, the public API stays stable, e.g. > > while (!FPDFAvail_IsDocAvail(pdf_avail, &hints) { > if (FPDFAvail_Error(fpdf_avail)) > return; > } Using the example as above, we shall use FPDFAvail_Error(fpdf_avail, &hints) and there should be almost the same code in FPDFAvail_Error as FPDFAvail_IsDocAvail. Because some errors are raised in the process of checking doc. As I mentioned in the previous comment, it returns true when any errors are found in checking whether docs or pages are available (It causes infinite loop if returning false). It really makes readers confused. https://codereview.chromium.org/1353093003/diff/180001/testing/embedder_test.h File testing/embedder_test.h (right): https://codereview.chromium.org/1353093003/diff/180001/testing/embedder_test.... testing/embedder_test.h:84: const bool must_linearize); On 2015/10/21 17:53:08, Tom Sepez wrote: > nit: const is pointless for bool arguments. Also maybe a default value of false > to avoid updating all those call sites. I will remove 'const' and add a default value of false. After that, shall I restore all call sites that I added 'false' for?
On 2015/10/22 00:50:06, jun_fang wrote: > https://codereview.chromium.org/1353093003/diff/180001/public/fpdf_dataavail.h > File public/fpdf_dataavail.h (right): > > https://codereview.chromium.org/1353093003/diff/180001/public/fpdf_dataavail.... > public/fpdf_dataavail.h:138: * PDF_DATA_ERROR: A common error is > returned. It can't tell > On 2015/10/21 17:53:08, Tom Sepez wrote: > > Public API change. Can we do what we need to do without breaking > compatibliity? > > This public API was designed for linearization functionality. > Before this CL, this functionality wasn't implemented correctly. > This is the first time that applications can call this API > in the right way. So this API is almost a new API. > > In the other hand, I define PDF_DATA_ERROR as -1. It follows the > legacy logic. Before this CL, it returns true even this are any > errors. So there are two possibilities for 'true', data available or error. Both > of them are non-zero. > > https://codereview.chromium.org/1353093003/diff/180001/samples/pdfium_test.cc > File samples/pdfium_test.cc (right): > > https://codereview.chromium.org/1353093003/diff/180001/samples/pdfium_test.cc... > samples/pdfium_test.cc:549: nRet = FPDFAvail_IsDocAvail(pdf_avail, &hints); > On 2015/10/21 17:53:08, Tom Sepez wrote: > > For example, if we had a separate function to check if the reason for the > > unavailability was due to error, the public API stays stable, e.g. > > > > while (!FPDFAvail_IsDocAvail(pdf_avail, &hints) { > > if (FPDFAvail_Error(fpdf_avail)) > > return; > > } > > Using the example as above, we shall use > FPDFAvail_Error(fpdf_avail, &hints) and there should be almost > the same code in FPDFAvail_Error as FPDFAvail_IsDocAvail. Because > some errors are raised in the process of checking doc. As I > mentioned in the previous comment, it returns true when any errors > are found in checking whether docs or pages are available (It causes > infinite loop if returning false). It really makes readers confused. > > https://codereview.chromium.org/1353093003/diff/180001/testing/embedder_test.h > File testing/embedder_test.h (right): > > https://codereview.chromium.org/1353093003/diff/180001/testing/embedder_test.... > testing/embedder_test.h:84: const bool must_linearize); > On 2015/10/21 17:53:08, Tom Sepez wrote: > > nit: const is pointless for bool arguments. Also maybe a default value of > false > > to avoid updating all those call sites. > > I will remove 'const' and add a default value of false. After that, shall I > restore all call sites that I added 'false' for? Any suggestion for my comments?
On 2015/10/26 13:37:40, jun_fang wrote: > On 2015/10/22 00:50:06, jun_fang wrote: > > https://codereview.chromium.org/1353093003/diff/180001/public/fpdf_dataavail.h > > File public/fpdf_dataavail.h (right): > > > > > https://codereview.chromium.org/1353093003/diff/180001/public/fpdf_dataavail.... > > public/fpdf_dataavail.h:138: * PDF_DATA_ERROR: A common error is > > returned. It can't tell > > On 2015/10/21 17:53:08, Tom Sepez wrote: > > > Public API change. Can we do what we need to do without breaking > > compatibliity? > > > > This public API was designed for linearization functionality. > > Before this CL, this functionality wasn't implemented correctly. > > This is the first time that applications can call this API > > in the right way. So this API is almost a new API. > > > > In the other hand, I define PDF_DATA_ERROR as -1. It follows the > > legacy logic. Before this CL, it returns true even this are any > > errors. So there are two possibilities for 'true', data available or error. > Both > > of them are non-zero. > > > > https://codereview.chromium.org/1353093003/diff/180001/samples/pdfium_test.cc > > File samples/pdfium_test.cc (right): > > > > > https://codereview.chromium.org/1353093003/diff/180001/samples/pdfium_test.cc... > > samples/pdfium_test.cc:549: nRet = FPDFAvail_IsDocAvail(pdf_avail, &hints); > > On 2015/10/21 17:53:08, Tom Sepez wrote: > > > For example, if we had a separate function to check if the reason for the > > > unavailability was due to error, the public API stays stable, e.g. > > > > > > while (!FPDFAvail_IsDocAvail(pdf_avail, &hints) { > > > if (FPDFAvail_Error(fpdf_avail)) > > > return; > > > } > > > > Using the example as above, we shall use > > FPDFAvail_Error(fpdf_avail, &hints) and there should be almost > > the same code in FPDFAvail_Error as FPDFAvail_IsDocAvail. Because > > some errors are raised in the process of checking doc. As I > > mentioned in the previous comment, it returns true when any errors > > are found in checking whether docs or pages are available (It causes > > infinite loop if returning false). It really makes readers confused. > > > > https://codereview.chromium.org/1353093003/diff/180001/testing/embedder_test.h > > File testing/embedder_test.h (right): > > > > > https://codereview.chromium.org/1353093003/diff/180001/testing/embedder_test.... > > testing/embedder_test.h:84: const bool must_linearize); > > On 2015/10/21 17:53:08, Tom Sepez wrote: > > > nit: const is pointless for bool arguments. Also maybe a default value of > > false > > > to avoid updating all those call sites. > > > > I will remove 'const' and add a default value of false. After that, shall I > > restore all call sites that I added 'false' for? > > Any suggestion for my comments? OK going with your original approach for the public API change, given that's its new. Also, yes, please restore the call sites for the test code.
Hi Tom and Lei, Please help to review patch 11. I raised some comments to remind me to follow up in next patch. https://codereview.chromium.org/1353093003/diff/200001/fpdfsdk/src/fpdf_dataa... File fpdfsdk/src/fpdf_dataavail.cpp (right): https://codereview.chromium.org/1353093003/diff/200001/fpdfsdk/src/fpdf_dataa... fpdfsdk/src/fpdf_dataavail.cpp:137: return PDF_FORM_AVAIL; should return PDF_DATA_ERROR; https://codereview.chromium.org/1353093003/diff/200001/public/fpdf_dataavail.h File public/fpdf_dataavail.h (right): https://codereview.chromium.org/1353093003/diff/200001/public/fpdf_dataavail.... public/fpdf_dataavail.h:20: #define PDF_FORM_NOTAVAIL 0 should add "#define PDF_FORM_ERROR -1" https://codereview.chromium.org/1353093003/diff/200001/public/fpdf_dataavail.... public/fpdf_dataavail.h:145: * generated download hints if any, until the function returns true. true should be changed to PDF_DATA_ERROR or PDF_DATA_AVAIL. https://codereview.chromium.org/1353093003/diff/200001/public/fpdf_dataavail.... public/fpdf_dataavail.h:208: * generated download hints if any, until the function returns true. true should be changed to PDF_DATA_ERROR or PDF_DATA_AVAIL. https://codereview.chromium.org/1353093003/diff/200001/public/fpdf_dataavail.... public/fpdf_dataavail.h:228: * Details: -1 - error, the input parameter not correct, such as hints Use the following marcros to replace integers. PDF_FORM_ERROR PDF_FORM_NOTAVAIL PDF_FORM_AVAIL PDF_FORM_NOTEXIST https://codereview.chromium.org/1353093003/diff/200001/samples/pdfium_test.cc File samples/pdfium_test.cc (right): https://codereview.chromium.org/1353093003/diff/200001/samples/pdfium_test.cc... samples/pdfium_test.cc:455: fprintf(stderr, "Unknown error in checking if doc is available.\n"); format error. https://codereview.chromium.org/1353093003/diff/200001/samples/pdfium_test.cc... samples/pdfium_test.cc:458: if (FPDFAvail_IsFormAvail(pdf_avail, &hints) == PDF_FORM_NOTAVAIL) { should check whether an error is returned. https://codereview.chromium.org/1353093003/diff/200001/samples/pdfium_test.cc... samples/pdfium_test.cc:463: } format error.
https://codereview.chromium.org/1353093003/diff/200001/public/fpdf_dataavail.h File public/fpdf_dataavail.h (right): https://codereview.chromium.org/1353093003/diff/200001/public/fpdf_dataavail.... public/fpdf_dataavail.h:145: * generated download hints if any, until the function returns true. On 2015/10/29 15:31:45, jun_fang wrote: > true should be changed to PDF_DATA_ERROR or PDF_DATA_AVAIL. "true" should be changed to "PDF_DATA_ERROR or PDF_DATA_AVAIL". https://codereview.chromium.org/1353093003/diff/200001/public/fpdf_dataavail.... public/fpdf_dataavail.h:208: * generated download hints if any, until the function returns true. On 2015/10/29 15:31:45, jun_fang wrote: > true should be changed to PDF_DATA_ERROR or PDF_DATA_AVAIL. "true" should be changed to "PDF_DATA_ERROR or PDF_DATA_AVAIL".
One comment, some nits, and why don't you fix all of your own comments before landing? That way the interfaces won't change twice. https://codereview.chromium.org/1353093003/diff/200001/core/src/fpdfapi/fpdf_... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): https://codereview.chromium.org/1353093003/diff/200001/core/src/fpdfapi/fpdf_... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2972: for (int i = 0; i < iSize; ++i) { nit: might as well be int32_t if iSize is as well. what is the return type of GetSize() and can we match it? https://codereview.chromium.org/1353093003/diff/200001/core/src/fpdfapi/fpdf_... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2973: ((CPDF_Object*)m_arrayAcroforms.GetAt(i))->Release(); Why did we change the static cast? Looks like some changes from other CLs may have been lost during your refresh? https://codereview.chromium.org/1353093003/diff/200001/core/src/fpdfapi/fpdf_... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_embeddertest.cpp (right): https://codereview.chromium.org/1353093003/diff/200001/core/src/fpdfapi/fpdf_... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_embeddertest.cpp:11: // Test a dictionary with Hex string rather than expected. nit: expected what? expected size? https://codereview.chromium.org/1353093003/diff/200001/samples/pdfium_test.cc File samples/pdfium_test.cc (right): https://codereview.chromium.org/1353093003/diff/200001/samples/pdfium_test.cc... samples/pdfium_test.cc:455: fprintf(stderr, "Unknown error in checking if doc is available.\n"); nit: run git cl format. https://codereview.chromium.org/1353093003/diff/200001/testing/embedder_test.cpp File testing/embedder_test.cpp (right): https://codereview.chromium.org/1353093003/diff/200001/testing/embedder_test.... testing/embedder_test.cpp:114: if (FPDFAvail_IsLinearized(avail_) == PDF_LINEARIZED || must_linearize) { Actually, we want to test if FPDFAvail_IsLinearized() did its job propery, so don't use must_linearize to force a particular path. https://codereview.chromium.org/1353093003/diff/200001/testing/embedder_test.... testing/embedder_test.cpp:140: document_ = FPDF_LoadCustomDocument(&file_access_, nullptr); However, if we came here, and must_linearize was specified, return error.
https://codereview.chromium.org/1353093003/diff/200001/core/src/fpdfapi/fpdf_... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): https://codereview.chromium.org/1353093003/diff/200001/core/src/fpdfapi/fpdf_... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2973: ((CPDF_Object*)m_arrayAcroforms.GetAt(i))->Release(); On 2015/10/29 16:20:17, Tom Sepez wrote: > Why did we change the static cast? Looks like some changes from other CLs may > have been lost during your refresh? It was introduced in conflicting resolved. I noticed this change in self-review after I submitted this CL. But I don't know why I didn't point it out. https://codereview.chromium.org/1353093003/diff/200001/core/src/fpdfapi/fpdf_... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_embeddertest.cpp (right): https://codereview.chromium.org/1353093003/diff/200001/core/src/fpdfapi/fpdf_... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_embeddertest.cpp:11: // Test a dictionary with Hex string rather than expected. On 2015/10/29 16:20:17, Tom Sepez wrote: > nit: expected what? expected size? I don't know when someone changed bug_454695.pdf as below: % Hex string, not a dict as expected 1 0 obj <feedbeef2dad> endobj It's nothing with size. So we need to update this comment. https://codereview.chromium.org/1353093003/diff/200001/samples/pdfium_test.cc File samples/pdfium_test.cc (right): https://codereview.chromium.org/1353093003/diff/200001/samples/pdfium_test.cc... samples/pdfium_test.cc:455: fprintf(stderr, "Unknown error in checking if doc is available.\n"); On 2015/10/29 16:20:17, Tom Sepez wrote: > nit: run git cl format. Acknowledged. https://codereview.chromium.org/1353093003/diff/200001/testing/embedder_test.cpp File testing/embedder_test.cpp (right): https://codereview.chromium.org/1353093003/diff/200001/testing/embedder_test.... testing/embedder_test.cpp:114: if (FPDFAvail_IsLinearized(avail_) == PDF_LINEARIZED || must_linearize) { On 2015/10/29 16:20:17, Tom Sepez wrote: > Actually, we want to test if FPDFAvail_IsLinearized() did its job propery, so > don't use must_linearize to force a particular path. Acknowledged. https://codereview.chromium.org/1353093003/diff/200001/testing/embedder_test.... testing/embedder_test.cpp:140: document_ = FPDF_LoadCustomDocument(&file_access_, nullptr); On 2015/10/29 16:20:17, Tom Sepez wrote: > However, if we came here, and must_linearize was specified, return error. Acknowledged.
lgtm
Message was sent while issue was closed.
Committed patchset #13 (id:240001) manually as d946f3011984755b14d7dcfb05d572e870f93f3f (presubmit successful).
Message was sent while issue was closed.
On 2015/11/02 05:45:55, jun_fang wrote: > Committed patchset #13 (id:240001) manually as > d946f3011984755b14d7dcfb05d572e870f93f3f (presubmit successful). It looks like this hasn't been merged to the XFA branch yet, is there anything that needs to be done to merge this into XFA?
Message was sent while issue was closed.
On 2015/11/03 17:08:49, dsinclair wrote: > On 2015/11/02 05:45:55, jun_fang wrote: > > Committed patchset #13 (id:240001) manually as > > d946f3011984755b14d7dcfb05d572e870f93f3f (presubmit successful). > > > It looks like this hasn't been merged to the XFA branch yet, is there anything > that needs to be done to merge this into XFA? Ping again. The longer we wait to merge to XFA, the more difficult it gets because the two branches diverges.
Message was sent while issue was closed.
On 2015/11/09 19:57:58, Lei Zhang wrote: > On 2015/11/03 17:08:49, dsinclair wrote: > > On 2015/11/02 05:45:55, jun_fang wrote: > > > Committed patchset #13 (id:240001) manually as > > > d946f3011984755b14d7dcfb05d572e870f93f3f (presubmit successful). > > > > > > It looks like this hasn't been merged to the XFA branch yet, is there anything > > that needs to be done to merge this into XFA? > > Ping again. The longer we wait to merge to XFA, the more difficult it gets > because the two branches diverges. I will merge it to XFA branch today. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
