Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
345123002:
Add support to extract viewer preference (Closed)
|
Created:
6 years, 6 months ago by Nikhil Modified:
6 years, 4 months ago CC:
pdfium-reviews_googlegroups.com Base URL:
https://pdfium.googlesource.com/pdfium.git@master Visibility:
Public. |
DescriptionAdd support to extract viewer preference
This change adds the support to extract "NumCopies", "PrintPageRange", "Duplex" viewer preferences for printing.
BUG=169120
Patch Set 1 #Patch Set 2 : Added more getters #
Total comments: 4
Patch Set 3 : Review feedback #
Total comments: 19
Patch Set 4 : Review feedback #
Total comments: 7
Patch Set 5 : Revieew feedback (move enum to fpdfsdk) #
Total comments: 3
Patch Set 6 : Review feedback (return proper value) #
Total comments: 2
Patch Set 7 : Review feedback (proper return value in fpdfview) #
Total comments: 3
Patch Set 8 : Rebase and warning fix #
Messages
Total messages: 40 (0 generated)
PTAL. Thanks! If you've any review comments, then please let me know. I'll incorporate them.
On 2014/06/20 14:40:34, Nikhil wrote: > PTAL. Thanks! > > If you've any review comments, then please let me know. I'll incorporate them. According PDF spec FitWindow is not for printing. Not sure that we want to support this in PDF viewer.
On 2014/06/20 18:09:18, Vitaly Buka wrote: > On 2014/06/20 14:40:34, Nikhil wrote: > > PTAL. Thanks! > > > > If you've any review comments, then please let me know. I'll incorporate them. > > According PDF spec FitWindow is not for printing. Not sure that we want to > support this in PDF viewer. Jumped the gun! I saw 'Number of Copies' and 'Print Page Range' options also. How about them?
On 2014/06/20 19:23:57, Nikhil wrote: > On 2014/06/20 18:09:18, Vitaly Buka wrote: > > On 2014/06/20 14:40:34, Nikhil wrote: > > > PTAL. Thanks! > > > > > > If you've any review comments, then please let me know. I'll incorporate > them. > > > > According PDF spec FitWindow is not for printing. Not sure that we want to > > support this in PDF viewer. > > Jumped the gun! I saw 'Number of Copies' and 'Print Page Range' options also. > How about them? Maybe it's OK to put all desired option into a single patch.
PTAL. Thanks!
+jam https://codereview.chromium.org/345123002/diff/20001/core/include/fpdfdoc/fpd... File core/include/fpdfdoc/fpdf_doc.h (right): https://codereview.chromium.org/345123002/diff/20001/core/include/fpdfdoc/fpd... core/include/fpdfdoc/fpdf_doc.h:1664: CFX_ByteString Duplex() const; Duplex should return FX_INT32 and we need some defines like PDFZOOM_XYZ, or even better enum {} print preview still supports only bool value, so maybe FX_BOOL is enough https://codereview.chromium.org/345123002/diff/20001/fpdfsdk/src/fpdfview.cpp File fpdfsdk/src/fpdfview.cpp (right): https://codereview.chromium.org/345123002/diff/20001/fpdfsdk/src/fpdfview.cpp... fpdfsdk/src/fpdfview.cpp:866: please remove this empty line
PTAL. Thanks! https://codereview.chromium.org/345123002/diff/20001/core/include/fpdfdoc/fpd... File core/include/fpdfdoc/fpdf_doc.h (right): https://codereview.chromium.org/345123002/diff/20001/core/include/fpdfdoc/fpd... core/include/fpdfdoc/fpdf_doc.h:1664: CFX_ByteString Duplex() const; Added enum to map possible Duplex values. On 2014/06/24 21:53:46, Vitaly Buka wrote: > Duplex should return FX_INT32 and we need some defines like PDFZOOM_XYZ, or even > better enum {} > > print preview still supports only bool value, so maybe FX_BOOL is enough https://codereview.chromium.org/345123002/diff/20001/fpdfsdk/src/fpdfview.cpp File fpdfsdk/src/fpdfview.cpp (right): https://codereview.chromium.org/345123002/diff/20001/fpdfsdk/src/fpdfview.cpp... fpdfsdk/src/fpdfview.cpp:866: On 2014/06/24 21:53:46, Vitaly Buka wrote: > please remove this empty line Done.
https://codereview.chromium.org/345123002/diff/40001/core/include/fpdfdoc/fpd... File core/include/fpdfdoc/fpdf_doc.h (right): https://codereview.chromium.org/345123002/diff/40001/core/include/fpdfdoc/fpd... core/include/fpdfdoc/fpdf_doc.h:1650: enum DuplexType { Maybe? None -> DuplexUndefined Simplex -> DuplexOff https://codereview.chromium.org/345123002/diff/40001/core/include/fpdfdoc/fpd... core/include/fpdfdoc/fpdf_doc.h:1671: FX_INT32 Duplex() const; this should return DuplexType https://codereview.chromium.org/345123002/diff/40001/core/src/fpdfdoc/doc_vie... File core/src/fpdfdoc/doc_viewerPreferences.cpp (right): https://codereview.chromium.org/345123002/diff/40001/core/src/fpdfdoc/doc_vie... core/src/fpdfdoc/doc_viewerPreferences.cpp:56: DuplexType type; no need "type" variable. https://codereview.chromium.org/345123002/diff/40001/core/src/fpdfdoc/doc_vie... core/src/fpdfdoc/doc_viewerPreferences.cpp:65: type = None; return immediatly https://codereview.chromium.org/345123002/diff/40001/fpdfsdk/include/fpdfview.h File fpdfsdk/include/fpdfview.h (right): https://codereview.chromium.org/345123002/diff/40001/fpdfsdk/include/fpdfview... fpdfsdk/include/fpdfview.h:570: // The number of copies to be printed. please replace tabs with spaces https://codereview.chromium.org/345123002/diff/40001/fpdfsdk/include/fpdfview... fpdfsdk/include/fpdfview.h:579: // The print page range to be used for printing. ditto https://codereview.chromium.org/345123002/diff/40001/fpdfsdk/include/fpdfview... fpdfsdk/include/fpdfview.h:589: // ditto https://codereview.chromium.org/345123002/diff/40001/fpdfsdk/include/fpdfview... fpdfsdk/include/fpdfview.h:590: DLLEXPORT int STDCALL FPDF_VIEWERREF_GetDuplex(FPDF_DOCUMENT document); Please return enum here
PTAL. Thanks! https://codereview.chromium.org/345123002/diff/40001/core/include/fpdfdoc/fpd... File core/include/fpdfdoc/fpdf_doc.h (right): https://codereview.chromium.org/345123002/diff/40001/core/include/fpdfdoc/fpd... core/include/fpdfdoc/fpdf_doc.h:1650: enum DuplexType { On 2014/06/25 18:02:24, Vitaly Buka wrote: > Maybe? > None -> DuplexUndefined > Simplex -> DuplexOff Done. https://codereview.chromium.org/345123002/diff/40001/core/include/fpdfdoc/fpd... core/include/fpdfdoc/fpdf_doc.h:1671: FX_INT32 Duplex() const; On 2014/06/25 18:02:24, Vitaly Buka wrote: > this should return DuplexType Done. https://codereview.chromium.org/345123002/diff/40001/core/src/fpdfdoc/doc_vie... File core/src/fpdfdoc/doc_viewerPreferences.cpp (right): https://codereview.chromium.org/345123002/diff/40001/core/src/fpdfdoc/doc_vie... core/src/fpdfdoc/doc_viewerPreferences.cpp:56: DuplexType type; On 2014/06/25 18:02:24, Vitaly Buka wrote: > no need "type" variable. Done. https://codereview.chromium.org/345123002/diff/40001/core/src/fpdfdoc/doc_vie... core/src/fpdfdoc/doc_viewerPreferences.cpp:65: type = None; On 2014/06/25 18:02:24, Vitaly Buka wrote: > return immediatly Done. https://codereview.chromium.org/345123002/diff/40001/fpdfsdk/include/fpdfview.h File fpdfsdk/include/fpdfview.h (right): https://codereview.chromium.org/345123002/diff/40001/fpdfsdk/include/fpdfview... fpdfsdk/include/fpdfview.h:570: // The number of copies to be printed. It looks like all files here use tabs. Can we use clang-format here? On 2014/06/25 18:02:24, Vitaly Buka wrote: > please replace tabs with spaces https://codereview.chromium.org/345123002/diff/40001/fpdfsdk/include/fpdfview... fpdfsdk/include/fpdfview.h:590: DLLEXPORT int STDCALL FPDF_VIEWERREF_GetDuplex(FPDF_DOCUMENT document); With current implementation, enum DuplexType isn't exposed here. So I added define for FPDF_DUPLEXTYPE similar to FPDF_BOOL. Please let me know if you think this should be changed somehow.
On 2014/06/24 21:53:46, Vitaly Buka wrote: > +jam I'm not familiar with this code, please add one of the Foxit developers to review this.
lgtm Bo can you please review this? Or reassign to someone familiar with this code? https://codereview.chromium.org/345123002/diff/40001/fpdfsdk/include/fpdfview.h File fpdfsdk/include/fpdfview.h (right): https://codereview.chromium.org/345123002/diff/40001/fpdfsdk/include/fpdfview... fpdfsdk/include/fpdfview.h:590: DLLEXPORT int STDCALL FPDF_VIEWERREF_GetDuplex(FPDF_DOCUMENT document); I'd prefer to move enum to some accessible location. Any opinion from Foxit team? On 2014/06/26 10:21:38, Nikhil wrote: > With current implementation, enum DuplexType isn't exposed here. So I added > define for FPDF_DUPLEXTYPE similar to FPDF_BOOL. Please let me know if you think > this should be changed somehow. https://codereview.chromium.org/345123002/diff/100001/fpdfsdk/include/fpdfview.h File fpdfsdk/include/fpdfview.h (right): https://codereview.chromium.org/345123002/diff/100001/fpdfsdk/include/fpdfvie... fpdfsdk/include/fpdfview.h:570: // Return value: my guess it should be allied with "document" from line above. https://codereview.chromium.org/345123002/diff/100001/fpdfsdk/include/fpdfvie... fpdfsdk/include/fpdfview.h:580: // The print page range to be used for printing. ditto
I will take a look at this.
https://codereview.chromium.org/345123002/diff/100001/fpdfsdk/include/fpdfview.h File fpdfsdk/include/fpdfview.h (right): https://codereview.chromium.org/345123002/diff/100001/fpdfsdk/include/fpdfvie... fpdfsdk/include/fpdfview.h:570: // Return value: On 2014/06/26 20:03:37, Vitaly Buka wrote: > my guess it should be allied with "document" from line above. I tried that earlier with spaces, but it looked inconsistent in vi editor and another IDE that I tried. Same with FPDF_DUPLEXTYPE defined above. Files here use tabs instead of spaces. Can't we use clang-format here?
https://codereview.chromium.org/345123002/diff/40001/core/src/fpdfdoc/doc_vie... File core/src/fpdfdoc/doc_viewerPreferences.cpp (right): https://codereview.chromium.org/345123002/diff/40001/core/src/fpdfdoc/doc_vie... core/src/fpdfdoc/doc_viewerPreferences.cpp:52: FX_INT32 CPDF_ViewerPreferences::Duplex() const This function should return a CFX_ByteString like "Simplex". Then leave the ( ? == ?) part to fpdfview.cpp, so you can define the enum there. The design of foxit "core" and "fpdfsdk" was to try to return the raw data from "core" and process it in "fpdfsdk". https://codereview.chromium.org/345123002/diff/40001/fpdfsdk/include/fpdfview.h File fpdfsdk/include/fpdfview.h (right): https://codereview.chromium.org/345123002/diff/40001/fpdfsdk/include/fpdfview... fpdfsdk/include/fpdfview.h:590: DLLEXPORT int STDCALL FPDF_VIEWERREF_GetDuplex(FPDF_DOCUMENT document); I agree. Move enum to fpdfsdk part will be more clear. On 2014/06/26 20:03:37, Vitaly Buka wrote: > I'd prefer to move enum to some accessible location. > > Any opinion from Foxit team? > > On 2014/06/26 10:21:38, Nikhil wrote: > > With current implementation, enum DuplexType isn't exposed here. So I added > > define for FPDF_DUPLEXTYPE similar to FPDF_BOOL. Please let me know if you > think > > this should be changed somehow. > https://codereview.chromium.org/345123002/diff/100001/core/include/fpdfdoc/fp... File core/include/fpdfdoc/fpdf_doc.h (right): https://codereview.chromium.org/345123002/diff/100001/core/include/fpdfdoc/fp... core/include/fpdfdoc/fpdf_doc.h:1651: DuplexUndefined = 0, From PDF32000 standard, p364, The value should be "Simplex" rather than "DuplexOff" https://codereview.chromium.org/345123002/diff/100001/core/src/fpdfdoc/doc_vi... File core/src/fpdfdoc/doc_viewerPreferences.cpp (right): https://codereview.chromium.org/345123002/diff/100001/core/src/fpdfdoc/doc_vi... core/src/fpdfdoc/doc_viewerPreferences.cpp:37: return FALSE; The "NumCopies" entry is optional, and the default value is typically one. I suggest to change to "return 1;" in line 37.
Apologies for the delay in getting back to this. I've incorporated the review comments. PTAL. Thanks! https://codereview.chromium.org/345123002/diff/40001/core/src/fpdfdoc/doc_vie... File core/src/fpdfdoc/doc_viewerPreferences.cpp (right): https://codereview.chromium.org/345123002/diff/40001/core/src/fpdfdoc/doc_vie... core/src/fpdfdoc/doc_viewerPreferences.cpp:52: FX_INT32 CPDF_ViewerPreferences::Duplex() const On 2014/06/29 20:58:16, bo_xu wrote: > This function should return a CFX_ByteString like "Simplex". > > Then leave the ( ? == ?) part to fpdfview.cpp, so you can define the enum there. > > The design of foxit "core" and "fpdfsdk" was to try to return the raw data from > "core" and process it in "fpdfsdk". Done. https://codereview.chromium.org/345123002/diff/40001/fpdfsdk/include/fpdfview.h File fpdfsdk/include/fpdfview.h (right): https://codereview.chromium.org/345123002/diff/40001/fpdfsdk/include/fpdfview... fpdfsdk/include/fpdfview.h:590: DLLEXPORT int STDCALL FPDF_VIEWERREF_GetDuplex(FPDF_DOCUMENT document); On 2014/06/29 20:58:16, bo_xu wrote: > I agree. Move enum to fpdfsdk part will be more clear. > > On 2014/06/26 20:03:37, Vitaly Buka wrote: > > I'd prefer to move enum to some accessible location. > > > > Any opinion from Foxit team? > > > > On 2014/06/26 10:21:38, Nikhil wrote: > > > With current implementation, enum DuplexType isn't exposed here. So I added > > > define for FPDF_DUPLEXTYPE similar to FPDF_BOOL. Please let me know if you > > think > > > this should be changed somehow. > > > Done. https://codereview.chromium.org/345123002/diff/100001/core/include/fpdfdoc/fp... File core/include/fpdfdoc/fpdf_doc.h (right): https://codereview.chromium.org/345123002/diff/100001/core/include/fpdfdoc/fp... core/include/fpdfdoc/fpdf_doc.h:1651: DuplexUndefined = 0, On 2014/06/29 20:58:16, bo_xu wrote: > From PDF32000 standard, p364, The value should be "Simplex" rather than > "DuplexOff" Done. https://codereview.chromium.org/345123002/diff/100001/core/src/fpdfdoc/doc_vi... File core/src/fpdfdoc/doc_viewerPreferences.cpp (right): https://codereview.chromium.org/345123002/diff/100001/core/src/fpdfdoc/doc_vi... core/src/fpdfdoc/doc_viewerPreferences.cpp:37: return FALSE; On 2014/06/29 20:58:16, bo_xu wrote: > The "NumCopies" entry is optional, and the default value is typically one. I > suggest to change to "return 1;" in line 37. Done.
I just have one comment. With that fixed, lgtm. Thanks!
https://codereview.chromium.org/345123002/diff/120001/core/src/fpdfdoc/doc_vi... File core/src/fpdfdoc/doc_viewerPreferences.cpp (right): https://codereview.chromium.org/345123002/diff/120001/core/src/fpdfdoc/doc_vi... core/src/fpdfdoc/doc_viewerPreferences.cpp:37: return TRUE; As the return type is FX_INT32, we should do "return 1;" explicitly here.
Thanks for reviewing, I've added proper return value. PTAL. https://codereview.chromium.org/345123002/diff/120001/core/include/fpdfdoc/fp... File core/include/fpdfdoc/fpdf_doc.h (right): https://codereview.chromium.org/345123002/diff/120001/core/include/fpdfdoc/fp... core/include/fpdfdoc/fpdf_doc.h:1650: Empty line got added by mistake so I removed it. https://codereview.chromium.org/345123002/diff/120001/core/src/fpdfdoc/doc_vi... File core/src/fpdfdoc/doc_viewerPreferences.cpp (right): https://codereview.chromium.org/345123002/diff/120001/core/src/fpdfdoc/doc_vi... core/src/fpdfdoc/doc_viewerPreferences.cpp:37: return TRUE; On 2014/07/07 18:11:19, bo_xu wrote: > As the return type is FX_INT32, we should do "return 1;" explicitly here. Done.
On 2014/07/08 07:54:37, Nikhil wrote: > Thanks for reviewing, I've added proper return value. PTAL. > > https://codereview.chromium.org/345123002/diff/120001/core/include/fpdfdoc/fp... > File core/include/fpdfdoc/fpdf_doc.h (right): > > https://codereview.chromium.org/345123002/diff/120001/core/include/fpdfdoc/fp... > core/include/fpdfdoc/fpdf_doc.h:1650: > Empty line got added by mistake so I removed it. > > https://codereview.chromium.org/345123002/diff/120001/core/src/fpdfdoc/doc_vi... > File core/src/fpdfdoc/doc_viewerPreferences.cpp (right): > > https://codereview.chromium.org/345123002/diff/120001/core/src/fpdfdoc/doc_vi... > core/src/fpdfdoc/doc_viewerPreferences.cpp:37: return TRUE; > On 2014/07/07 18:11:19, bo_xu wrote: > > As the return type is FX_INT32, we should do "return 1;" explicitly here. > > Done. Thanks. Do you have the right to land this patch? Or either Jam or me has to land this for you.
lgtm please fix this one https://codereview.chromium.org/345123002/diff/140001/fpdfsdk/src/fpdfview.cpp File fpdfsdk/src/fpdfview.cpp (right): https://codereview.chromium.org/345123002/diff/140001/fpdfsdk/src/fpdfview.cp... fpdfsdk/src/fpdfview.cpp:872: if (!pDoc) return TRUE; return 1
https://codereview.chromium.org/345123002/diff/140001/fpdfsdk/src/fpdfview.cpp File fpdfsdk/src/fpdfview.cpp (right): https://codereview.chromium.org/345123002/diff/140001/fpdfsdk/src/fpdfview.cp... fpdfsdk/src/fpdfview.cpp:872: if (!pDoc) return TRUE; On 2014/07/08 23:33:13, Vitaly Buka wrote: > return 1 Done.
On 2014/07/08 23:14:42, bo_xu wrote: > Thanks. Do you have the right to land this patch? Or either Jam or me has to > land this for you. I'm not sure what rights are required to land this. Maybe Jam or Vitaly can suggest. But all changes as per review comments are done, so this is good to land. Thanks!
The CQ bit was checked by vitalybuka@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Commit queue rejected this change because it did not recognize the base URL. Please commit your change manually.
On 2014/07/09 08:39:14, I haz the power (commit-bot) wrote: > Commit queue rejected this change because it did not recognize the base URL. > Please commit your change manually. Not sure what the error means. I'm seeing it for the first time. Do I need to rebase? Please let me know if I need to do something at my end to submit this. Thanks!
On 2014/07/09 09:10:04, Nikhil wrote: > On 2014/07/09 08:39:14, I haz the power (commit-bot) wrote: > > Commit queue rejected this change because it did not recognize the base URL. > > Please commit your change manually. > > Not sure what the error means. I'm seeing it for the first time. Do I need to > rebase? Please let me know if I need to do something at my end to submit this. > Thanks! I believe you have to do "git cl land" to commit. That requires permission too. Maybe you can ask jam?
On 2014/07/09 18:28:30, bo_xu wrote: > On 2014/07/09 09:10:04, Nikhil wrote: > > On 2014/07/09 08:39:14, I haz the power (commit-bot) wrote: > > > Commit queue rejected this change because it did not recognize the base URL. > > > Please commit your change manually. > > > > Not sure what the error means. I'm seeing it for the first time. Do I need to > > rebase? Please let me know if I need to do something at my end to submit this. > > Thanks! > > I believe you have to do "git cl land" to commit. That requires permission too. > Maybe you can ask jam? @jam - ping! Could you please advise how to submit this patch?
https://codereview.chromium.org/345123002/diff/160001/fpdfsdk/src/fpdfview.cpp File fpdfsdk/src/fpdfview.cpp (right): https://codereview.chromium.org/345123002/diff/160001/fpdfsdk/src/fpdfview.cp... fpdfsdk/src/fpdfview.cpp:899: } I just realized that this gives warning! Control may reach end of non-void function. Do we worry about warnings? It can be removed by maintaining a local variable, setting its value based on string comparisons, and then returning it.
https://codereview.chromium.org/345123002/diff/160001/fpdfsdk/src/fpdfview.cpp File fpdfsdk/src/fpdfview.cpp (right): https://codereview.chromium.org/345123002/diff/160001/fpdfsdk/src/fpdfview.cp... fpdfsdk/src/fpdfview.cpp:899: } Please just return DuplexUndefined; at the end of function. and you can remove if (FX_BSTRC("None") == duplex) case On 2014/07/11 06:58:42, Nikhil wrote: > I just realized that this gives warning! Control may reach end of non-void > function. Do we worry about warnings? It can be removed by maintaining a local > variable, setting its value based on string comparisons, and then returning it.
Warning is removed. PTAL. Thanks! Also, please advise how to submit this. I don't think I've permissions. This is ready to submit. https://codereview.chromium.org/345123002/diff/160001/fpdfsdk/src/fpdfview.cpp File fpdfsdk/src/fpdfview.cpp (right): https://codereview.chromium.org/345123002/diff/160001/fpdfsdk/src/fpdfview.cp... fpdfsdk/src/fpdfview.cpp:899: } On 2014/07/11 19:31:54, Vitaly Buka wrote: > Please just return DuplexUndefined; at the end of function. > and you can remove if (FX_BSTRC("None") == duplex) case > > On 2014/07/11 06:58:42, Nikhil wrote: > > I just realized that this gives warning! Control may reach end of non-void > > function. Do we worry about warnings? It can be removed by maintaining a local > > variable, setting its value based on string comparisons, and then returning > it. > Done.
I guess Jam can help you commit?
On 2014/07/14 20:28:59, bo_xu wrote: > I guess Jam can help you commit? jam@ - ping! Please suggest on how to submit this.
On 2014/07/14 20:37:04, Nikhil wrote: > On 2014/07/14 20:28:59, bo_xu wrote: > > I guess Jam can help you commit? > > jam@ - ping! Please suggest on how to submit this. Hi Nikhil, I have committed this for you in https://pdfium.googlesource.com/pdfium/+/9114e831cc8ae619ce541719887c5d7c2fc4...
On 2014/07/14 21:44:10, bo_xu wrote: > On 2014/07/14 20:37:04, Nikhil wrote: > > On 2014/07/14 20:28:59, bo_xu wrote: > > > I guess Jam can help you commit? > > > > jam@ - ping! Please suggest on how to submit this. > > Hi Nikhil, I have committed this for you in > https://pdfium.googlesource.com/pdfium/+/9114e831cc8ae619ce541719887c5d7c2fc4... Thanks Bo. In the future, please give attribution to the original author in the cl description (by editing the description on Rietveld before doing "git cl land"). Also, we need to add Nikhil's name and email to the AUTHORS file. I verified that he (as part of Samsung) has already signed the CLA per http://dev.chromium.org/developers/contributing-code/external-contributor-che....
Hi Jam, right, I realized that after I land it. Just wonder how do I edit the description after I do "git cl issue issue#"? Or I don't really link it to the existing issue? (But seems if I don't upload, I can not edit on Rietveld?) > > Thanks Bo. In the future, please give attribution to the original author in the > cl description (by editing the description on Rietveld before doing "git cl > land"). Also, we need to add Nikhil's name and email to the AUTHORS file. I > verified that he (as part of Samsung) has already signed the CLA per > http://dev.chromium.org/developers/contributing-code/external-contributor-che....
On 2014/07/14 22:03:43, bo_xu wrote: > Hi Jam, right, I realized that after I land it. Just wonder how do I edit the > description after I do "git cl issue issue#"? Or I don't really link it to the > existing issue? (But seems if I don't upload, I can not edit on Rietveld?) When you do "git cl land", the tool will fetch the latest description from Rietveld. So anytime before landing, edit the Rietveld description. i.e. this can be before or after you run "git cl issue". > > > > Thanks Bo. In the future, please give attribution to the original author in > the > > cl description (by editing the description on Rietveld before doing "git cl > > land"). Also, we need to add Nikhil's name and email to the AUTHORS file. I > > verified that he (as part of Samsung) has already signed the CLA per > > > http://dev.chromium.org/developers/contributing-code/external-contributor-che....
On 2014/07/14 22:23:42, jam wrote: > On 2014/07/14 22:03:43, bo_xu wrote: > > Hi Jam, right, I realized that after I land it. Just wonder how do I edit the > > description after I do "git cl issue issue#"? Or I don't really link it to the > > existing issue? (But seems if I don't upload, I can not edit on Rietveld?) > > When you do "git cl land", the tool will fetch the latest description from > Rietveld. So anytime before landing, edit the Rietveld description. i.e. this > can be before or after you run "git cl issue". > > > > > > > Thanks Bo. In the future, please give attribution to the original author in > > the > > > cl description (by editing the description on Rietveld before doing "git cl > > > land"). Also, we need to add Nikhil's name and email to the AUTHORS file. I > > > verified that he (as part of Samsung) has already signed the CLA per > > > > > > http://dev.chromium.org/developers/contributing-code/external-contributor-che.... I see. I don't think I have the permission to edit though... Maybe there is some permission issue?
On 2014/07/14 21:52:32, jam wrote: > On 2014/07/14 21:44:10, bo_xu wrote: > > On 2014/07/14 20:37:04, Nikhil wrote: > > > On 2014/07/14 20:28:59, bo_xu wrote: > > > > I guess Jam can help you commit? > > > > > > jam@ - ping! Please suggest on how to submit this. > > > > Hi Nikhil, I have committed this for you in > > > https://pdfium.googlesource.com/pdfium/+/9114e831cc8ae619ce541719887c5d7c2fc4... > Thanks Bo, I can see the change when I do "git pull" in third_party/pdfium. Now do we need to roll these changes into Chrome? Similar to Blink changes. > Thanks Bo. In the future, please give attribution to the original author in the > cl description (by editing the description on Rietveld before doing "git cl > land"). Also, we need to add Nikhil's name and email to the AUTHORS file. I > verified that he (as part of Samsung) has already signed the CLA per > http://dev.chromium.org/developers/contributing-code/external-contributor-che.... Unsure about editing the description now, but if I need to update the AUTHORS file then please let me know.
Closing this review as changes were successfully submitted manually. Thank you all for reviewing this! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
