Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
705823002:
Add pdf layer code for PDFium XFA module (Closed)
DescriptionAdd pdf layer code for PDFium XFA module
BUG=62400
Patch Set 1 #Patch Set 2 : Remove pdfium_engine.h #
Total comments: 19
Patch Set 3 : Format and whitespace #Patch Set 4 : Indent #
Total comments: 22
Patch Set 5 : Naming, comment #
Total comments: 13
Patch Set 6 : Clean up GYP/GN #Patch Set 7 : Change verion to 2 #Messages
Total messages: 33 (4 generated)
tsepez@chromium.org changed reviewers: + thestig@chromium.org
This needs to match the chromium style guide, not the PDFium style. Its a matter of no tabs, lines < 80 chars, and single spaces separating tokens.
On 2014/11/05 22:37:09, Tom Sepez wrote: > This needs to match the chromium style guide, not the PDFium style. Its a > matter of no tabs, lines < 80 chars, and single spaces separating tokens. Sure, I will make the changes.
tsepez@chromium.org changed reviewers: + tsepez@chromium.org
https://codereview.chromium.org/705823002/diff/20001/pdf/pdf.gyp File pdf/pdf.gyp (right): https://codereview.chromium.org/705823002/diff/20001/pdf/pdf.gyp#newcode74 pdf/pdf.gyp:74: 'defines': ['_TEST_XFA_'], nit: the #define should be PDF_USE_XFA for consistency. https://codereview.chromium.org/705823002/diff/20001/pdf/pdfium/pdfium_engine.h File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/705823002/diff/20001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.h:487: //static int Form_GetCurDocumentIndex(FPDF_FORMFILLINFO* pThis); nit: completely remove the // commented out code.
https://codereview.chromium.org/705823002/diff/20001/pdf/pdf.gyp File pdf/pdf.gyp (right): https://codereview.chromium.org/705823002/diff/20001/pdf/pdf.gyp#newcode5 pdf/pdf.gyp:5: 'pdf_use_xfa%': 0, You need to make the corresponding change in pdf/BUILD.gn https://codereview.chromium.org/705823002/diff/20001/pdf/pdfium/pdfium_engine.cc File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/705823002/diff/20001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.cc:682: if (!clientData) return; style: return on the next line please. https://codereview.chromium.org/705823002/diff/20001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.cc:844: base::UTF16ToUTF8(reinterpret_cast<const base::char16*>(wsURL)); Would it be easier to construct the message with FPDF_WIDESTRINGs first and call base::UTF16ToUTF8() once afterwards? https://codereview.chromium.org/705823002/diff/20001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.cc:942: url_str = "NULL"; It's already initialized to "NULL" https://codereview.chromium.org/705823002/diff/20001/pdf/pdfium/pdfium_engine.h File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/705823002/diff/20001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.h:30: #define XFA_TESTFILE(filename) "E:/"#filename Do we need this as part of the CL? https://codereview.chromium.org/705823002/diff/20001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.h:485: FPDF_WIDESTRING to, FPDF_WIDESTRING subject, FPDF_WIDESTRING cc, FPDF_WIDESTRING bcc, FPDF_WIDESTRING message); Please try to stick with the Google C++ style guide here: - no tabs - 2 space indent - 80 chars per line -- when a line is too long and continues on the next line, 4 space indent - Function parameter names are named_like_this. https://codereview.chromium.org/705823002/diff/20001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.h:487: //static int Form_GetCurDocumentIndex(FPDF_FORMFILLINFO* pThis); Can we add the commented out bits in a later CL, or uncomment them and just provide a dummy implementation until they are ready?
https://codereview.chromium.org/705823002/diff/20001/pdf/pdfium/pdfium_engine.h File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/705823002/diff/20001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.h:30: #define XFA_TESTFILE(filename) "E:/"#filename On 2014/11/05 22:45:09, Lei Zhang wrote: > Do we need this as part of the CL? This is used in pdfium_engine.cc. But I am not quite sure if the code there does make sense due to sandbox. Or I can just leave a arbitrary filepath in pdfium_engine.cc, say, line 950, as a placeholder?
https://codereview.chromium.org/705823002/diff/20001/pdf/pdfium/pdfium_engine.h File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/705823002/diff/20001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.h:30: #define XFA_TESTFILE(filename) "E:/"#filename On 2014/11/05 22:51:46, Bo Xu wrote: > On 2014/11/05 22:45:09, Lei Zhang wrote: > > Do we need this as part of the CL? > > This is used in pdfium_engine.cc. But I am not quite sure if the code there does > make sense due to sandbox. Or I can just leave a arbitrary filepath in > pdfium_engine.cc, say, line 950, as a placeholder? Let's just move this into the .cc file. It doesn't need to be in the header. It normally wouldn't make sense to have, but because it's behind a test #define, keep it for now to make debugging/development easier.
No warnings for long line, tab or space line ending in presubmit. Please review again. https://codereview.chromium.org/705823002/diff/20001/pdf/pdf.gyp File pdf/pdf.gyp (right): https://codereview.chromium.org/705823002/diff/20001/pdf/pdf.gyp#newcode5 pdf/pdf.gyp:5: 'pdf_use_xfa%': 0, On 2014/11/05 22:45:09, Lei Zhang wrote: > You need to make the corresponding change in pdf/BUILD.gn Done. https://codereview.chromium.org/705823002/diff/20001/pdf/pdf.gyp#newcode74 pdf/pdf.gyp:74: 'defines': ['_TEST_XFA_'], On 2014/11/05 22:44:47, Tom Sepez wrote: > nit: the #define should be PDF_USE_XFA for consistency. Done. https://codereview.chromium.org/705823002/diff/20001/pdf/pdfium/pdfium_engine.cc File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/705823002/diff/20001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.cc:682: if (!clientData) return; On 2014/11/05 22:45:09, Lei Zhang wrote: > style: return on the next line please. Done. https://codereview.chromium.org/705823002/diff/20001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.cc:844: base::UTF16ToUTF8(reinterpret_cast<const base::char16*>(wsURL)); On 2014/11/05 22:45:09, Lei Zhang wrote: > Would it be easier to construct the message with FPDF_WIDESTRINGs first and call > base::UTF16ToUTF8() once afterwards? Hmm, seems FPDF_WIDESTRING are const unsigned short*, and it is not trivial to combine them here. May need overload + operator in pdfsdk to concatenate FPDF_WIDESTRING. https://codereview.chromium.org/705823002/diff/20001/pdf/pdfium/pdfium_engine.h File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/705823002/diff/20001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.h:30: #define XFA_TESTFILE(filename) "E:/"#filename On 2014/11/05 22:58:04, Lei Zhang wrote: > On 2014/11/05 22:51:46, Bo Xu wrote: > > On 2014/11/05 22:45:09, Lei Zhang wrote: > > > Do we need this as part of the CL? > > > > This is used in pdfium_engine.cc. But I am not quite sure if the code there > does > > make sense due to sandbox. Or I can just leave a arbitrary filepath in > > pdfium_engine.cc, say, line 950, as a placeholder? > > Let's just move this into the .cc file. It doesn't need to be in the header. It > normally wouldn't make sense to have, but because it's behind a test #define, > keep it for now to make debugging/development easier. Done. https://codereview.chromium.org/705823002/diff/20001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.h:485: FPDF_WIDESTRING to, FPDF_WIDESTRING subject, FPDF_WIDESTRING cc, FPDF_WIDESTRING bcc, FPDF_WIDESTRING message); On 2014/11/05 22:45:09, Lei Zhang wrote: > Please try to stick with the Google C++ style guide here: > - no tabs > - 2 space indent > - 80 chars per line > -- when a line is too long and continues on the next line, 4 space indent > - Function parameter names are named_like_this. Done. https://codereview.chromium.org/705823002/diff/20001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.h:487: //static int Form_GetCurDocumentIndex(FPDF_FORMFILLINFO* pThis); On 2014/11/05 22:44:47, Tom Sepez wrote: > nit: completely remove the // commented out code. Done. https://codereview.chromium.org/705823002/diff/20001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.h:487: //static int Form_GetCurDocumentIndex(FPDF_FORMFILLINFO* pThis); On 2014/11/05 22:45:09, Lei Zhang wrote: > Can we add the commented out bits in a later CL, or uncomment them and just > provide a dummy implementation until they are ready? We can add them in a later CL when needed. The callback needs to be implemented in formfill module in pdfsdk, which is not done for some of the XFA action.
https://codereview.chromium.org/705823002/diff/60001/pdf/BUILD.gn File pdf/BUILD.gn (right): https://codereview.chromium.org/705823002/diff/60001/pdf/BUILD.gn#newcode69 pdf/BUILD.gn:69: if(pdf_use_xfa == 1) { nit: space after if https://codereview.chromium.org/705823002/diff/60001/pdf/BUILD.gn#newcode70 pdf/BUILD.gn:70: defines += [ "_PDF_USE_XFA_" ] You need to start with: defines = [] after sources = [ ... ] and also change the "defines =" below to "defines +=" https://codereview.chromium.org/705823002/diff/60001/pdf/pdf.gyp File pdf/pdf.gyp (right): https://codereview.chromium.org/705823002/diff/60001/pdf/pdf.gyp#newcode74 pdf/pdf.gyp:74: 'defines': ['_PDF_USE_XFA_'], Can we rename this to "PDF_USE_XFA" ? We don't put leading/trailing underscores in any other #defines. https://codereview.chromium.org/705823002/diff/60001/pdf/pdfium/pdfium_engine.cc File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/705823002/diff/60001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.cc:659: FPDFDOC_ExitFormFillEnviroument(form_); Can you explain why we have to do this after calling FPDF_CloseDocument() now? https://codereview.chromium.org/705823002/diff/60001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.cc:671: #if defined(WIN32) Please add a comment to mention this is just here for testing, so we don't forget to remove it later. https://codereview.chromium.org/705823002/diff/60001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.cc:677: typedef struct _FPDF_FILE { You can just declare struct FPDF_FILE { .. }; directly. https://codereview.chromium.org/705823002/diff/60001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.cc:685: fclose(((FPDF_FILE*)clientData)->file); Maybe just declare a new pointer and only cast |clientData| once? https://codereview.chromium.org/705823002/diff/60001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.cc:717: //Write data nit: space after // https://codereview.chromium.org/705823002/diff/60001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.cc:828: //platform = new char[3]; There's still some commented out code here. https://codereview.chromium.org/705823002/diff/60001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.cc:849: FPDF_BSTR* respone) { typo: response ? https://codereview.chromium.org/705823002/diff/60001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.cc:861: std::string javascript = "alert(\"Post:" variable is not used for anything? https://codereview.chromium.org/705823002/diff/60001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.cc:895: FPDF_WIDESTRING URL) { URL should be lower case. https://codereview.chromium.org/705823002/diff/60001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.cc:918: std::string url_str = "NULL"; This variable is being set but not used for anything. https://codereview.chromium.org/705823002/diff/60001/pdf/pdfium/pdfium_engine.h File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/705823002/diff/60001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.h:477: static void Form_EmailTo(FPDF_FORMFILLINFO* pThis, We also don't do Hungarian notation, so maybe make this |engine| and static_cast() it to |engine_ptr| in the .cc file? https://codereview.chromium.org/705823002/diff/60001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.h:478: FPDF_FILEHANDLER* fileHandler, fileHandler -> file_handler https://codereview.chromium.org/705823002/diff/60001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.h:538: #endif // _PDF_USE_XFA_ nit: 2 spaces before //
I have change the variable naming to match the code in pdfium_engine.cc Please review again https://codereview.chromium.org/705823002/diff/60001/pdf/BUILD.gn File pdf/BUILD.gn (right): https://codereview.chromium.org/705823002/diff/60001/pdf/BUILD.gn#newcode70 pdf/BUILD.gn:70: defines += [ "_PDF_USE_XFA_" ] On 2014/11/06 04:08:08, Lei Zhang wrote: > You need to start with: > > defines = [] > > after sources = [ ... ] > > and also change the "defines =" below to "defines +=" Done. https://codereview.chromium.org/705823002/diff/60001/pdf/pdf.gyp File pdf/pdf.gyp (right): https://codereview.chromium.org/705823002/diff/60001/pdf/pdf.gyp#newcode74 pdf/pdf.gyp:74: 'defines': ['_PDF_USE_XFA_'], On 2014/11/06 04:08:08, Lei Zhang wrote: > Can we rename this to "PDF_USE_XFA" ? We don't put leading/trailing underscores > in any other #defines. Done. https://codereview.chromium.org/705823002/diff/60001/pdf/pdfium/pdfium_engine.cc File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/705823002/diff/60001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.cc:659: FPDFDOC_ExitFormFillEnviroument(form_); On 2014/11/06 04:08:08, Lei Zhang wrote: > Can you explain why we have to do this after calling FPDF_CloseDocument() now? XFA document is independent from PDF even it can be used in PDF. With XFA module added in, the formfiller in fpdfsdk manages the entire form related resources. So when pdf document is closed, there are still form environment of XFA that needs to be closed. Thus the order changes. https://codereview.chromium.org/705823002/diff/60001/pdf/pdfium/pdfium_engine.h File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/705823002/diff/60001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.h:477: static void Form_EmailTo(FPDF_FORMFILLINFO* pThis, On 2014/11/06 04:08:08, Lei Zhang wrote: > We also don't do Hungarian notation, so maybe make this |engine| and > static_cast() it to |engine_ptr| in the .cc file? I have matched the naming to other parts in the code as |param|
https://codereview.chromium.org/705823002/diff/80001/pdf/pdfium/pdfium_engine.cc File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/705823002/diff/80001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.cc:600: FPDF_FORMFILLINFO::version = 1; New fields are added to this structure, but the version stayed unchanged. This seems wrong; the version should be bumped (to 2) and checked at some point in the pdfium library. We ought to prevent a mismatch between the chrome side and the pdfium side, since it might call into arbitrary memory.
https://codereview.chromium.org/705823002/diff/80001/pdf/pdfium/pdfium_engine.cc File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/705823002/diff/80001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.cc:601: FPDF_FORMFILLINFO::m_pJsPlatform = this; Note that in going from the pre-xfa version of struct to the xfa version of the struct in fpdfsdk/include/fpdfformfill.hfpdfsdk/include/fpdfformfill.h, IPDF_JSPLATFORM* m_pJsPlatform; had its offset displaced by the new methods added above it. This should be moved back to its original offset if you want to make it simple for a v1 library to run against a v2 client.
https://codereview.chromium.org/705823002/diff/80001/pdf/pdfium/pdfium_engine.cc File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/705823002/diff/80001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.cc:600: FPDF_FORMFILLINFO::version = 1; On 2014/11/06 17:45:45, Tom Sepez wrote: > New fields are added to this structure, but the version stayed unchanged. This > seems wrong; the version should be bumped (to 2) and checked at some point in > the pdfium library. We ought to prevent a mismatch between the chrome side and > the pdfium side, since it might call into arbitrary memory. Since we are branching out xfa, the version in xfa branch should be changed to 2. So shall I add a #ifdef PDF_USE_XFA to determine if we want to set version to 1 or 2 here? https://codereview.chromium.org/705823002/diff/80001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.cc:601: FPDF_FORMFILLINFO::m_pJsPlatform = this; On 2014/11/06 18:10:05, Tom Sepez wrote: > Note that in going from the pre-xfa version of struct to the xfa version of the > struct in fpdfsdk/include/fpdfformfill.hfpdfsdk/include/fpdfformfill.h, > IPDF_JSPLATFORM* m_pJsPlatform; > had its offset displaced by the new methods added above it. This should be > moved back to its original offset if you want to make it simple for a v1 library > to run against a v2 client. If I understand correctly, I should move IPDF_JSPLATFORM* m_pJsPlatform; in the middle of the struct definition where it was? I will make that change in xfa branch.
> Since we are branching out xfa, the version in xfa branch should be changed to > 2. So shall I add a #ifdef PDF_USE_XFA to determine if we want to set version to > 1 or 2 here? If you wanted to do this really elegantly, you could add a FPDF_FORMFILLINFO_CURRENT_VERSION #define constant to fpdfsdk/include/fpdfformfill.h, defining it as 1 on the mainline branch, and defining it as 2 on the XFA branch. Then the code on the chromium side can do #if FPDF_FORMFILLINFO_CURRENT_VERSION >= 2 ... #endif in place of the current PDF_USE_XFA test. They you can remove the PDF_USE_XFA macro entirely from the gyp/GN. Then, on the pdfium side, there's one place where you test == 1, and instead just test against 2, on the XFA branch. > pdf/pdfium/pdfium_engine.cc:601: FPDF_FORMFILLINFO::m_pJsPlatform = this; > On 2014/11/06 18:10:05, Tom Sepez wrote: > > Note that in going from the pre-xfa version of struct to the xfa version of > the > > struct in fpdfsdk/include/fpdfformfill.hfpdfsdk/include/fpdfformfill.h, > > IPDF_JSPLATFORM* m_pJsPlatform; > > had its offset displaced by the new methods added above it. This should be > > moved back to its original offset if you want to make it simple for a v1 > library > > to run against a v2 client. > > If I understand correctly, I should move > IPDF_JSPLATFORM* m_pJsPlatform; > in the middle of the struct definition where it was? I will make that change in > xfa branch. Exactly. Having kept the struct the same, you might then add a comment // Version 2. before the first new item.
https://codereview.chromium.org/705823002/diff/60001/pdf/BUILD.gn File pdf/BUILD.gn (right): https://codereview.chromium.org/705823002/diff/60001/pdf/BUILD.gn#newcode70 pdf/BUILD.gn:70: defines += [ "_PDF_USE_XFA_" ] On 2014/11/06 05:43:51, Bo Xu wrote: > On 2014/11/06 04:08:08, Lei Zhang wrote: > > You need to start with: > > > > defines = [] > > > > after sources = [ ... ] > > > > and also change the "defines =" below to "defines +=" > > Done. You need to write it like: defines = [] if (condition1) { defines += [d1] } if (condition2) { ... } As is, "defines" isn't defined on non-Windows platforms to start, and defines += [ "PDF_USE_XFA" ] won't work. https://codereview.chromium.org/705823002/diff/60001/pdf/pdfium/pdfium_engine.cc File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/705823002/diff/60001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.cc:659: FPDFDOC_ExitFormFillEnviroument(form_); On 2014/11/06 05:43:51, Bo Xu wrote: > On 2014/11/06 04:08:08, Lei Zhang wrote: > > Can you explain why we have to do this after calling FPDF_CloseDocument() now? > > XFA document is independent from PDF even it can be used in PDF. With XFA module > added in, the formfiller in fpdfsdk manages the entire form related resources. > So when pdf document is closed, there are still form environment of XFA that > needs to be closed. Thus the order changes. I just want to make sure it works with or without XFA. https://codereview.chromium.org/705823002/diff/80001/pdf/pdfium/pdfium_engine.cc File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/705823002/diff/80001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.cc:679: FPDF_FILEHANDLER fileHandler; nit: file_handler https://codereview.chromium.org/705823002/diff/80001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.cc:683: void Sample_Release(FPDF_LPVOID cilent_data) { cilent -> client https://codereview.chromium.org/705823002/diff/80001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.cc:695: long cur_pos = ftell(file_wrapper->file); ftell() and fseek() can fail and return -1 https://codereview.chromium.org/705823002/diff/80001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.cc:788: int page_index = -1; you don't need this variable, just return the GetMostVisiblePage() return value. https://codereview.chromium.org/705823002/diff/80001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.cc:805: *right = page_view_rect.width() + page_view_rect.x(); pp::Rect has right() and bottom() methods, but no left() / top() https://codereview.chromium.org/705823002/diff/80001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.cc:809: std::string javascript = "alert(\"PageViewRect:" another unused variable, more below https://codereview.chromium.org/705823002/diff/80001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.cc:834: return 3; why doesn't this return |platform_flag| ? https://codereview.chromium.org/705823002/diff/80001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.cc:942: } else { you don't need the else block since you always return in the if block. https://codereview.chromium.org/705823002/diff/80001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.cc:1667: control == FPDF_FORMFIELD_COMBOBOX || control == FPDF_FORMFIELD_XFA); nit: 4 space indent on line continuations.
> They you can remove the PDF_USE_XFA macro entirely from the gyp/GN. (But double-check with Lei on this.)
On 2014/11/06 19:23:28, Tom Sepez wrote: > > They you can remove the PDF_USE_XFA macro entirely from the gyp/GN. > (But double-check with Lei on this.) Are there going to be more XFA related changes in pdf/ in the near future? I don't have enough of the big picture to know whether this should stay behind a build flag or not. s/They/Then/ ? Also, I'm adding BUG=62400 to the CL description.
> s/They/Then/ ? s/They/Then/.
For the sake of moving the project forward, I'm OK with keeping PDFIUM_USE_XFA and hard-coded |2|s.
On 2014/11/06 19:57:07, Tom Sepez wrote: > For the sake of moving the project forward, I'm OK with keeping PDFIUM_USE_XFA > and hard-coded |2|s. Alternatively, we could just put the PDFIUM_USE_XFA #define into fpdfview.h on the branch, and not need to muck with the gyp/GN. I find I'm hopping from branch to branch, and its one less GYP DEFINE to deal with.
On 2014/11/06 19:29:24, Lei Zhang wrote: > On 2014/11/06 19:23:28, Tom Sepez wrote: > > > They you can remove the PDF_USE_XFA macro entirely from the gyp/GN. > > (But double-check with Lei on this.) > > Are there going to be more XFA related changes in pdf/ in the near future? I > don't have enough of the big picture to know whether this should stay behind a > build flag or not. > > s/They/Then/ ? > > Also, I'm adding BUG=62400 to the CL description. Currently the callback functions are mostly like placeholders. They don't fully support the form fill actions and need to be completed on chromium side. Then, if needed, more callback functions need to be added to FPDF_FORMFILLINFO struct and chromium side.
Updated based on previous comments. Now put the PDF_USE_XFA define to pdfium xfa branch
lgtm
lgtm
The CQ bit was checked by tsepez@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/705823002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2014/11/07 01:02:51, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium.linux > (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) Seems I am not in the contributor list. I will ask the company to submit the CLA form tomorrow.
No recent movement; I'm going to pull the patch and land it myself.
On 2014/11/12 22:04:01, Tom Sepez wrote: > No recent movement; I'm going to pull the patch and land it myself. See https://codereview.chromium.org/705823002/
On 2014/11/12 22:05:04, Tom Sepez wrote: > On 2014/11/12 22:04:01, Tom Sepez wrote: > > No recent movement; I'm going to pull the patch and land it myself. > See https://codereview.chromium.org/705823002/ No, see https://codereview.chromium.org/722863002/. |
