Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2419433004:
Clean up fx_basic_util a little (Closed)
DescriptionClean up fx_basic_util a little
- Nit fixes.
- Remove unused methods.
- Replace FX_BOOL with bool.
Committed: https://pdfium.googlesource.com/pdfium/+/aeb44f8c4675141e170329d5093b251fbfad8023
Patch Set 1 #Patch Set 2 : Another method deletion #
Total comments: 8
Patch Set 3 : Address comments #Patch Set 4 : Fix win #
Total comments: 8
Patch Set 5 : Comments #
Messages
Total messages: 35 (25 generated)
The CQ bit was checked by npm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by npm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Clean up fx_basic_util a little - Nit fixes. - Remove unused method. - Replace FX_BOOL with bool. ========== to ========== Clean up fx_basic_util a little - Nit fixes. - Remove unused method. - Replace FX_BOOL with bool. ==========
npm@chromium.org changed reviewers: + dsinclair@chromium.org, tsepez@chromium.org, weili@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Clean up fx_basic_util a little - Nit fixes. - Remove unused method. - Replace FX_BOOL with bool. ========== to ========== Clean up fx_basic_util a little - Nit fixes. - Remove unused methods. - Replace FX_BOOL with bool. ==========
PTAL
https://codereview.chromium.org/2419433004/diff/20001/core/fxcrt/fx_basic_uti... File core/fxcrt/fx_basic_util.cpp (right): https://codereview.chromium.org/2419433004/diff/20001/core/fxcrt/fx_basic_uti... core/fxcrt/fx_basic_util.cpp:197: #if _FXM_PLATFORM_ == _FXM_PLATFORM_WINDOWS_ Can we duplicate the method signature inside the #if's and then change the void* handle into actual types? https://codereview.chromium.org/2419433004/diff/20001/core/fxcrt/fx_basic_uti... core/fxcrt/fx_basic_util.cpp:208: #elif defined(__native_client__) Do we even support building under __native_client__? https://codereview.chromium.org/2419433004/diff/20001/xfa/fgas/font/fgas_stdf... File xfa/fgas/font/fgas_stdfontmgr.cpp (right): https://codereview.chromium.org/2419433004/diff/20001/xfa/fgas/font/fgas_stdf... xfa/fgas/font/fgas_stdfontmgr.cpp:516: return GetNextFile(); return m_FolderPaths.GetSize() != 0 ? GetNextFile() : "";
https://codereview.chromium.org/2419433004/diff/20001/core/fxcrt/fx_stream.h File core/fxcrt/fx_stream.h (right): https://codereview.chromium.org/2419433004/diff/20001/core/fxcrt/fx_stream.h#... core/fxcrt/fx_stream.h:14: bool FX_GetNextFile(void* handle, CFX_ByteString& filename, bool& bFolder); can this be const CFX_BytString& filename? can this be bool* bFolder?
The CQ bit was checked by npm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_no_v8 on master.tryserver.client.pdfium (JOB_FAILED, https://build.chromium.org/p/tryserver.client.pdfium/builders/win_no_v8/build...)
The CQ bit was checked by npm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL https://codereview.chromium.org/2419433004/diff/20001/core/fxcrt/fx_basic_uti... File core/fxcrt/fx_basic_util.cpp (right): https://codereview.chromium.org/2419433004/diff/20001/core/fxcrt/fx_basic_uti... core/fxcrt/fx_basic_util.cpp:197: #if _FXM_PLATFORM_ == _FXM_PLATFORM_WINDOWS_ On 2016/10/12 20:43:23, dsinclair wrote: > Can we duplicate the method signature inside the #if's and then change the void* > handle into actual types? Cleaner: created typedef for file handle. https://codereview.chromium.org/2419433004/diff/20001/core/fxcrt/fx_basic_uti... core/fxcrt/fx_basic_util.cpp:208: #elif defined(__native_client__) On 2016/10/12 20:43:23, dsinclair wrote: > Do we even support building under __native_client__? Deleted... Tom? https://codereview.chromium.org/2419433004/diff/20001/core/fxcrt/fx_stream.h File core/fxcrt/fx_stream.h (right): https://codereview.chromium.org/2419433004/diff/20001/core/fxcrt/fx_stream.h#... core/fxcrt/fx_stream.h:14: bool FX_GetNextFile(void* handle, CFX_ByteString& filename, bool& bFolder); On 2016/10/12 21:02:56, Tom Sepez wrote: > can this be const CFX_BytString& filename? > can this be bool* bFolder? Cannot be const. Changed to CFX_BytString* filename. Done. https://codereview.chromium.org/2419433004/diff/20001/xfa/fgas/font/fgas_stdf... File xfa/fgas/font/fgas_stdfontmgr.cpp (right): https://codereview.chromium.org/2419433004/diff/20001/xfa/fgas/font/fgas_stdf... xfa/fgas/font/fgas_stdfontmgr.cpp:516: return GetNextFile(); On 2016/10/12 20:43:23, dsinclair wrote: > return m_FolderPaths.GetSize() != 0 ? GetNextFile() : ""; Done.
lg w/ nits. Would like to hear from tsepez@ on the removal of the native_client bit before committing. https://codereview.chromium.org/2419433004/diff/60001/core/fxcrt/fx_basic_uti... File core/fxcrt/fx_basic_util.cpp (right): https://codereview.chromium.org/2419433004/diff/60001/core/fxcrt/fx_basic_uti... core/fxcrt/fx_basic_util.cpp:161: return dir; nit: return opendir(path); https://codereview.chromium.org/2419433004/diff/60001/core/fxcrt/fx_stream.h File core/fxcrt/fx_stream.h (right): https://codereview.chromium.org/2419433004/diff/60001/core/fxcrt/fx_stream.h#... core/fxcrt/fx_stream.h:25: typedef DIR FX_FileHandle; Can this if/else move into the if/else above? https://codereview.chromium.org/2419433004/diff/60001/core/fxcrt/fx_stream.h#... core/fxcrt/fx_stream.h:35: #if _FXM_PLATFORM_ == _FXM_PLATFORM_WINDOWS_ Combine with this as well? https://codereview.chromium.org/2419433004/diff/60001/core/fxge/android/fpf_s... File core/fxge/android/fpf_skiafontmgr.cpp (right): https://codereview.chromium.org/2419433004/diff/60001/core/fxge/android/fpf_s... core/fxge/android/fpf_skiafontmgr.cpp:9: #if _FX_OS_ == _FX_ANDROID_ Followup: Can we make this decision in the BUILD.gn file instead of wrapping the whole file in #if?
I don't know about the status of NaCl -- Ideally, we'd nacl the whole thing when running PDFium under chromium. I've not worked on it, and it would be a looooong term effort
The CQ bit was checked by npm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Undoing the nacl remove just in case. PTAL https://codereview.chromium.org/2419433004/diff/60001/core/fxcrt/fx_basic_uti... File core/fxcrt/fx_basic_util.cpp (right): https://codereview.chromium.org/2419433004/diff/60001/core/fxcrt/fx_basic_uti... core/fxcrt/fx_basic_util.cpp:161: return dir; On 2016/10/13 13:45:47, dsinclair wrote: > nit: return opendir(path); Done. https://codereview.chromium.org/2419433004/diff/60001/core/fxcrt/fx_stream.h File core/fxcrt/fx_stream.h (right): https://codereview.chromium.org/2419433004/diff/60001/core/fxcrt/fx_stream.h#... core/fxcrt/fx_stream.h:25: typedef DIR FX_FileHandle; On 2016/10/13 13:45:47, dsinclair wrote: > Can this if/else move into the if/else above? Done. https://codereview.chromium.org/2419433004/diff/60001/core/fxcrt/fx_stream.h#... core/fxcrt/fx_stream.h:35: #if _FXM_PLATFORM_ == _FXM_PLATFORM_WINDOWS_ On 2016/10/13 13:45:47, dsinclair wrote: > Combine with this as well? Done. https://codereview.chromium.org/2419433004/diff/60001/core/fxge/android/fpf_s... File core/fxge/android/fpf_skiafontmgr.cpp (right): https://codereview.chromium.org/2419433004/diff/60001/core/fxge/android/fpf_s... core/fxge/android/fpf_skiafontmgr.cpp:9: #if _FX_OS_ == _FX_ANDROID_ On 2016/10/13 13:45:47, dsinclair wrote: > Followup: Can we make this decision in the BUILD.gn file instead of wrapping the > whole file in #if? Ok
lgtm
The CQ bit was checked by npm@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Clean up fx_basic_util a little - Nit fixes. - Remove unused methods. - Replace FX_BOOL with bool. ========== to ========== Clean up fx_basic_util a little - Nit fixes. - Remove unused methods. - Replace FX_BOOL with bool. Committed: https://pdfium.googlesource.com/pdfium/+/aeb44f8c4675141e170329d5093b251fbfad... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://pdfium.googlesource.com/pdfium/+/aeb44f8c4675141e170329d5093b251fbfad... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
