Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(34)

Issue 1959253002: Remove unneeded CPVT classes. (try 2) (Closed)

Created:
4 years, 7 months ago by Lei Zhang
Modified:
3 years, 9 months ago
Reviewers:
CC:
pdfium-reviews_googlegroups.com
Base URL:
https://pdfium.googlesource.com/pdfium@master
Target Ref:
refs/heads/master
Project:
pdfium
Visibility:
Public.

Description

Remove unneeded CPVT classes. (try 2) - CPVT_Size is the same as CFX_PointF - CPVT_FloatRange is unused. - CPVT_ArrayTemplate is just a wrapper for CFX_ArrayTemplate. -- Convert a CFX_ArrayTemplate to std::vector. - Move classes to their own files. Previous version: https://codereview.chromium.org/1919283008/

Patch Set 1 : Previous CL #

Patch Set 2 : Fix crashes from https://crbug.com/608901 #

Patch Set 3 : Move CSection to its own file #

Patch Set 4 : Move CTypeset to its own file #

Patch Set 5 : Anonymous namespace #

Patch Set 6 : Literal conversion to std::vector #

Patch Set 7 : Fix potential out of bound errors and unreachable code, use std::vector more efficiently #

Patch Set 8 : Make m_WordArray private #

Patch Set 9 : Make m_WordArray hold unique_ptrs #

Total comments: 11
Unified diffs Side-by-side diffs Delta from patch set Stats (+954 lines, -1068 lines) Patch
M BUILD.gn View 1 2 3 3 chunks +3 lines, -1 line 0 comments Download
M core/fpdfdoc/cpdf_variabletext.cpp View 1 2 3 4 5 6 7 8 15 chunks +103 lines, -101 lines 3 comments Download
M core/fpdfdoc/cpvt_generateap.cpp View 1 chunk +1 line, -1 line 0 comments Download
M core/fpdfdoc/csection.h View 1 2 3 4 5 6 7 8 4 chunks +18 lines, -11 lines 5 comments Download
A core/fpdfdoc/csection.cpp View 1 2 3 4 5 6 7 8 1 chunk +295 lines, -0 lines 3 comments Download
M core/fpdfdoc/ctypeset.h View 1 2 3 4 5 6 7 8 1 chunk +6 lines, -3 lines 0 comments Download
A core/fpdfdoc/ctypeset.cpp View 1 2 3 4 5 6 7 8 1 chunk +491 lines, -0 lines 0 comments Download
A + core/fpdfdoc/doc_vt.h View 4 chunks +18 lines, -64 lines 0 comments Download
M core/fpdfdoc/doc_vt.cpp View 1 2 3 2 chunks +12 lines, -733 lines 0 comments Download
M core/fpdfdoc/include/cpdf_variabletext.h View 3 chunks +4 lines, -4 lines 0 comments Download
D core/fpdfdoc/pdf_vt.h View 1 chunk +0 lines, -149 lines 0 comments Download
M pdfium.gyp View 1 2 3 3 chunks +3 lines, -1 line 0 comments Download

Messages

Total messages: 7 (6 generated)
dsinclair
4 years, 7 months ago (2016-05-16 14:12:28 UTC) #6
https://codereview.chromium.org/1959253002/diff/180001/core/fpdfdoc/cpdf_vari...
File core/fpdfdoc/cpdf_variabletext.cpp (right):

https://codereview.chromium.org/1959253002/diff/180001/core/fpdfdoc/cpdf_vari...
core/fpdfdoc/cpdf_variabletext.cpp:921:
pSection->ClearRightWords(wordplace.nWordIndex);
ClearRightWords is ... confusing. ClearWordsFrom maybe?

https://codereview.chromium.org/1959253002/diff/180001/core/fpdfdoc/cpdf_vari...
core/fpdfdoc/cpdf_variabletext.cpp:935: if (pSection->IsWordArrayEmpty()) {
Can this use GetSize() == 0? We got rid of IsEmpty on the SectionArray but then
added it on the WordArray. Would be nice if they were consistent.

https://codereview.chromium.org/1959253002/diff/180001/core/fpdfdoc/cpdf_vari...
core/fpdfdoc/cpdf_variabletext.cpp:958: const CPVT_WordInfo* pWord =
pNextSection->GetWord(i);
It's no longer possible for pWord to be nullptr?

https://codereview.chromium.org/1959253002/diff/180001/core/fpdfdoc/csection.cpp
File core/fpdfdoc/csection.cpp (right):

https://codereview.chromium.org/1959253002/diff/180001/core/fpdfdoc/csection....
core/fpdfdoc/csection.cpp:21: void CSection::ResetAll() {
nit: Just Reset()?

https://codereview.chromium.org/1959253002/diff/180001/core/fpdfdoc/csection....
core/fpdfdoc/csection.cpp:26: void CSection::ResetLineArray() {
nit: ResetLines(), Array is an implmentation detail.

https://codereview.chromium.org/1959253002/diff/180001/core/fpdfdoc/csection....
core/fpdfdoc/csection.cpp:30: void CSection::ResetWordArray() {
ditto

https://codereview.chromium.org/1959253002/diff/180001/core/fpdfdoc/csection.h
File core/fpdfdoc/csection.h (right):

https://codereview.chromium.org/1959253002/diff/180001/core/fpdfdoc/csection....
core/fpdfdoc/csection.h:50: void ClearRightWords(int32_t nWordIndex);
ClearWordsBefore and ClearWordsAfter?

https://codereview.chromium.org/1959253002/diff/180001/core/fpdfdoc/csection....
core/fpdfdoc/csection.h:51: void ClearMidWords(int32_t nBeginIndex, int32_t
nEndIndex);
ClearWordsBetween?

https://codereview.chromium.org/1959253002/diff/180001/core/fpdfdoc/csection....
core/fpdfdoc/csection.h:53: CPDF_VariableText* GetVT() { return m_pVT; }
nit: s/GetVT/GetVariableText

https://codereview.chromium.org/1959253002/diff/180001/core/fpdfdoc/csection....
core/fpdfdoc/csection.h:60: CPVT_WordPlace SecPlace;
Can these be private now?

https://codereview.chromium.org/1959253002/diff/180001/core/fpdfdoc/csection....
core/fpdfdoc/csection.h:66: CPDF_VariableText* const m_pVT;
nit: s/m_pVT/m_pVariableText ?

Powered by Google App Engine
This is Rietveld 408576698