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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 1701883004: Banish CFX_ByteStringArray and CFX_WideStringArray to the XFA side. (Closed)

Created:
4 years, 10 months ago by Tom Sepez
Modified:
4 years, 10 months ago
Reviewers:
Lei Zhang
CC:
pdfium-reviews_googlegroups.com
Base URL:
https://pdfium.googlesource.com/pdfium.git@master
Target Ref:
refs/heads/master
Visibility:
Public.

Description

Banish CFX_ByteArray and CFX_WideArray to the XFA side. Fix IWYU and include paths as we go. R=thestig@chromium.org Committed: https://pdfium.googlesource.com/pdfium/+/ab27768d235985c0789a10ab490be43e262f48f6

Patch Set 1 #

Patch Set 2 : Fix non-xfa build. #

Total comments: 12

Patch Set 3 : Nits #

Unified diffs Side-by-side diffs Delta from patch set Stats (+227 lines, -186 lines) Patch
M core/include/fpdfdoc/fpdf_doc.h View 1 2 1 chunk +0 lines, -2 lines 0 comments Download
M core/include/fxcrt/fx_basic.h View 1 chunk +1 line, -1 line 0 comments Download
M core/include/fxge/fx_font.h View 1 2 6 chunks +13 lines, -10 lines 0 comments Download
M core/include/fxge/fx_ge.h View 1 chunk +2 lines, -2 lines 0 comments Download
M core/src/fpdfapi/fpdf_parser/fpdf_parser_decode.cpp View 5 chunks +9 lines, -9 lines 0 comments Download
M core/src/fpdfdoc/doc_form.cpp View 1 2 1 chunk +0 lines, -11 lines 0 comments Download
M core/src/fpdftext/fpdf_text_int.cpp View 1 2 7 chunks +9 lines, -11 lines 0 comments Download
M core/src/fpdftext/text_int.h View 1 2 1 chunk +1 line, -1 line 0 comments Download
M core/src/fxge/ge/fx_ge_fontmap.cpp View 1 2 7 chunks +14 lines, -10 lines 0 comments Download
M fpdfsdk/include/fpdfxfa/fpdfxfa_doc.h View 2 chunks +3 lines, -1 line 0 comments Download
M fpdfsdk/include/fsdk_mgr.h View 1 chunk +1 line, -1 line 0 comments Download
M fpdfsdk/include/pdfwindow/PWL_Wnd.h View 1 2 2 chunks +3 lines, -1 line 0 comments Download
M fpdfsdk/src/fpdfformfill.cpp View 1 2 7 chunks +39 lines, -29 lines 0 comments Download
M fpdfsdk/src/fsdk_mgr.cpp View 1 chunk +3 lines, -6 lines 0 comments Download
M fpdfsdk/src/javascript/PublicMethods.cpp View 1 chunk +4 lines, -4 lines 0 comments Download
M fpdfsdk/src/pdfwindow/PWL_Edit.cpp View 1 2 2 chunks +3 lines, -5 lines 0 comments Download
M xfa/include/fwl/basewidget/fwl_edit.h View 4 chunks +14 lines, -7 lines 0 comments Download
M xfa/include/fwl/lightwidget/edit.h View 2 chunks +12 lines, -6 lines 0 comments Download
M xfa/include/fxfa/fxfa.h View 3 chunks +4 lines, -2 lines 0 comments Download
M xfa/src/fwl/src/basewidget/fwl_editimp.cpp View 2 chunks +2 lines, -2 lines 0 comments Download
M xfa/src/fwl/src/basewidget/include/fwl_editimp.h View 5 chunks +12 lines, -5 lines 0 comments Download
M xfa/src/fwl/src/lightwidget/edit.cpp View 1 chunk +1 line, -1 line 0 comments Download
M xfa/src/fxfa/src/app/xfa_fftextedit.h View 1 2 2 chunks +48 lines, -40 lines 0 comments Download
M xfa/src/fxfa/src/app/xfa_fftextedit.cpp View 2 chunks +6 lines, -4 lines 0 comments Download
M xfa/src/fxfa/src/app/xfa_ffwidget.h View 3 chunks +7 lines, -4 lines 0 comments Download
M xfa/src/fxfa/src/app/xfa_ffwidgethandler.h View 3 chunks +12 lines, -4 lines 0 comments Download
M xfa/src/fxfa/src/app/xfa_ffwidgethandler.cpp View 1 chunk +4 lines, -7 lines 0 comments Download

Messages

Total messages: 8 (4 generated)
Tom Sepez
Lei, pls review.
4 years, 10 months ago (2016-02-17 00:44:19 UTC) #4
Lei Zhang
lgtm https://codereview.chromium.org/1701883004/diff/20001/core/include/fxge/fx_font.h File core/include/fxge/fx_font.h (right): https://codereview.chromium.org/1701883004/diff/20001/core/include/fxge/fx_font.h#newcode16 core/include/fxge/fx_font.h:16: #include "third_party/base/stl_util.h" move GetFaceSize() impl to .cpp file ...
4 years, 10 months ago (2016-02-17 02:17:21 UTC) #5
Tom Sepez
Committed patchset #3 (id:40001) manually as ab27768d235985c0789a10ab490be43e262f48f6 (presubmit successful).
4 years, 10 months ago (2016-02-17 18:07:25 UTC) #7
Tom Sepez
4 years, 10 months ago (2016-02-17 18:07:47 UTC) #8
Message was sent while issue was closed. 
https://codereview.chromium.org/1701883004/diff/20001/core/include/fxge/fx_fo...
File core/include/fxge/fx_font.h (right):

https://codereview.chromium.org/1701883004/diff/20001/core/include/fxge/fx_fo...
core/include/fxge/fx_font.h:16: #include "third_party/base/stl_util.h"
On 2016/02/17 02:17:21, Lei Zhang wrote:
> move GetFaceSize() impl to .cpp file instead?

Done.

https://codereview.chromium.org/1701883004/diff/20001/core/src/fpdfdoc/doc_fo...
File core/src/fpdfdoc/doc_form.cpp (right):

https://codereview.chromium.org/1701883004/diff/20001/core/src/fpdfdoc/doc_fo...
core/src/fpdfdoc/doc_form.cpp:667: void CPDF_InterForm::GetAllFieldNames(
On 2016/02/17 02:17:21, Lei Zhang wrote:
> Dead code, BTW.

And its gone.

https://codereview.chromium.org/1701883004/diff/20001/fpdfsdk/include/pdfwind...
File fpdfsdk/include/pdfwindow/PWL_Wnd.h (right):

https://codereview.chromium.org/1701883004/diff/20001/fpdfsdk/include/pdfwind...
fpdfsdk/include/pdfwindow/PWL_Wnd.h:174: std::vector<CFX_ByteString>& sSuggest)
= 0;
On 2016/02/17 02:17:21, Lei Zhang wrote:
> IWYU

Included.

https://codereview.chromium.org/1701883004/diff/20001/fpdfsdk/src/fpdfformfil...
File fpdfsdk/src/fpdfformfill.cpp (right):

https://codereview.chromium.org/1701883004/diff/20001/fpdfsdk/src/fpdfformfil...
fpdfsdk/src/fpdfformfill.cpp:635: if (!stringHandle || !bsText || size <= 0)
On 2016/02/17 02:17:21, Lei Zhang wrote:
> |size| can't be less than 0?

Done.

https://codereview.chromium.org/1701883004/diff/20001/xfa/src/fxfa/src/app/xf...
File xfa/src/fxfa/src/app/xfa_fftextedit.h (right):

https://codereview.chromium.org/1701883004/diff/20001/xfa/src/fxfa/src/app/xf...
xfa/src/fxfa/src/app/xfa_fftextedit.h:49: int32_t OnProcessMessage(CFWL_Message*
pMessage) override;
On 2016/02/17 02:17:21, Lei Zhang wrote:
> Group these with the overridden methods above?

Different class defined these, added comment about it.

https://codereview.chromium.org/1701883004/diff/20001/xfa/src/fxfa/src/app/xf...
xfa/src/fxfa/src/app/xfa_fftextedit.h:56: FX_BOOL CommitData() override;
On 2016/02/17 02:17:21, Lei Zhang wrote:
> Ditto?

nah.  Prefer having only one protected label.

Powered by Google App Engine
This is Rietveld 408576698