Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1826113003:
Cleanup fpdf_edit.h and fpdf_doc.h documentation. (Closed)
DescriptionCleanup fpdf_edit.h and fpdf_doc.h documentation.
This CL cleans up the documentation in public/fpdf_doc.h and public/fpdf_edit.h.
Committed: https://pdfium.googlesource.com/pdfium/+/5f597db5596f302a5ae31447e5c3eaf4320dcef1
Patch Set 1 #
Total comments: 26
Patch Set 2 : #
Total comments: 2
Messages
Total messages: 15 (6 generated)
dsinclair@chromium.org changed reviewers: + tsepez@chromium.org
PTAL.
https://codereview.chromium.org/1826113003/diff/1/public/fpdf_doc.h File public/fpdf_doc.h (right): https://codereview.chromium.org/1826113003/diff/1/public/fpdf_doc.h#newcode27 public/fpdf_doc.h:27: #ifndef _FS_DEF_STRUCTURE_QUADPOINTSF_ not needed, FS_QUADPOINTSF only defined here, top-level guard will do. https://codereview.chromium.org/1826113003/diff/1/public/fpdf_doc.h#newcode43 public/fpdf_doc.h:43: // Get the first child of |bookmark|, or the first top level bookmark item. nit: if I were really picky, I'd say that top-level is hyphenated. https://codereview.chromium.org/1826113003/diff/1/public/fpdf_doc.h#newcode60 public/fpdf_doc.h:60: // bookmark at this level. nit: , or NULL https://codereview.chromium.org/1826113003/diff/1/public/fpdf_doc.h#newcode70 public/fpdf_doc.h:70: // Returns the number of bytes in the title, including trailing zeros. The s/trailing zeros/the terminating NUL character/ https://codereview.chromium.org/1826113003/diff/1/public/fpdf_doc.h#newcode75 public/fpdf_doc.h:75: // string is terminated by two bytes of zeros. If |buflen| is less then the terminated by a UTF16 NUL character (U+0000, two bytes of zeros). https://codereview.chromium.org/1826113003/diff/1/public/fpdf_doc.h#newcode76 public/fpdf_doc.h:76: // returned length, or |buffer| is NULL, |buffer| will not be modified. s/returned/required/ https://codereview.chromium.org/1826113003/diff/1/public/fpdf_doc.h#newcode84 public/fpdf_doc.h:84: // title - the UTF-16LE encoded Unicode title with which to search. nit: for which ? https://codereview.chromium.org/1826113003/diff/1/public/fpdf_doc.h#newcode86 public/fpdf_doc.h:86: // Returns the handle to the bookmark, NULL if |title| can't be found. nit:, or NULL, several other places. https://codereview.chromium.org/1826113003/diff/1/public/fpdf_doc.h#newcode127 public/fpdf_doc.h:127: // action - handle to the action. |action| must be a |PDFACTION_GOTO| or Document what happens if it's not down below. https://codereview.chromium.org/1826113003/diff/1/public/fpdf_doc.h#newcode145 public/fpdf_doc.h:145: // Returns the number of bytes in the file path, including the trailing zero. s/trailing UTF16 NUL character/ https://codereview.chromium.org/1826113003/diff/1/public/fpdf_edit.h File public/fpdf_edit.h (right): https://codereview.chromium.org/1826113003/diff/1/public/fpdf_edit.h#newcode22 public/fpdf_edit.h:22: // TODO(dsinclair): What are these for? Nah, let's not check in this TODO https://codereview.chromium.org/1826113003/diff/1/public/fpdf_edit.h#newcode188 public/fpdf_edit.h:188: // Returns TRUE on success. nit: false otherwise just to be pedantic. Several places. https://codereview.chromium.org/1826113003/diff/1/public/fpdf_edit.h#newcode234 public/fpdf_edit.h:234: // TODO(dsinclair): Does this have the same cache clearing requirements as nit: let's not commit this todo either.
https://codereview.chromium.org/1826113003/diff/1/public/fpdf_doc.h File public/fpdf_doc.h (right): https://codereview.chromium.org/1826113003/diff/1/public/fpdf_doc.h#newcode27 public/fpdf_doc.h:27: #ifndef _FS_DEF_STRUCTURE_QUADPOINTSF_ On 2016/03/24 18:32:14, Tom Sepez wrote: > not needed, FS_QUADPOINTSF only defined here, top-level guard will do. Done. https://codereview.chromium.org/1826113003/diff/1/public/fpdf_doc.h#newcode43 public/fpdf_doc.h:43: // Get the first child of |bookmark|, or the first top level bookmark item. On 2016/03/24 18:32:15, Tom Sepez wrote: > nit: if I were really picky, I'd say that top-level is hyphenated. Done. https://codereview.chromium.org/1826113003/diff/1/public/fpdf_doc.h#newcode60 public/fpdf_doc.h:60: // bookmark at this level. On 2016/03/24 18:32:15, Tom Sepez wrote: > nit: , or NULL Done. https://codereview.chromium.org/1826113003/diff/1/public/fpdf_doc.h#newcode70 public/fpdf_doc.h:70: // Returns the number of bytes in the title, including trailing zeros. The On 2016/03/24 18:32:14, Tom Sepez wrote: > s/trailing zeros/the terminating NUL character/ Done. https://codereview.chromium.org/1826113003/diff/1/public/fpdf_doc.h#newcode75 public/fpdf_doc.h:75: // string is terminated by two bytes of zeros. If |buflen| is less then the On 2016/03/24 18:32:14, Tom Sepez wrote: > terminated by a UTF16 NUL character (U+0000, two bytes of zeros). Done. https://codereview.chromium.org/1826113003/diff/1/public/fpdf_doc.h#newcode76 public/fpdf_doc.h:76: // returned length, or |buffer| is NULL, |buffer| will not be modified. On 2016/03/24 18:32:14, Tom Sepez wrote: > s/returned/required/ Done. https://codereview.chromium.org/1826113003/diff/1/public/fpdf_doc.h#newcode84 public/fpdf_doc.h:84: // title - the UTF-16LE encoded Unicode title with which to search. On 2016/03/24 18:32:14, Tom Sepez wrote: > nit: for which ? Done. So much work to not say' title to search for.' heh. https://codereview.chromium.org/1826113003/diff/1/public/fpdf_doc.h#newcode86 public/fpdf_doc.h:86: // Returns the handle to the bookmark, NULL if |title| can't be found. On 2016/03/24 18:32:14, Tom Sepez wrote: > nit:, or NULL, several other places. Done. https://codereview.chromium.org/1826113003/diff/1/public/fpdf_doc.h#newcode127 public/fpdf_doc.h:127: // action - handle to the action. |action| must be a |PDFACTION_GOTO| or On 2016/03/24 18:32:14, Tom Sepez wrote: > Document what happens if it's not down below. I don't know, added to my list of TODO items to figure out about the API. https://codereview.chromium.org/1826113003/diff/1/public/fpdf_doc.h#newcode145 public/fpdf_doc.h:145: // Returns the number of bytes in the file path, including the trailing zero. On 2016/03/24 18:32:14, Tom Sepez wrote: > s/trailing UTF16 NUL character/ Done. https://codereview.chromium.org/1826113003/diff/1/public/fpdf_edit.h File public/fpdf_edit.h (right): https://codereview.chromium.org/1826113003/diff/1/public/fpdf_edit.h#newcode22 public/fpdf_edit.h:22: // TODO(dsinclair): What are these for? On 2016/03/24 18:32:15, Tom Sepez wrote: > Nah, let's not check in this TODO Done. https://codereview.chromium.org/1826113003/diff/1/public/fpdf_edit.h#newcode188 public/fpdf_edit.h:188: // Returns TRUE on success. On 2016/03/24 18:32:15, Tom Sepez wrote: > nit: false otherwise just to be pedantic. Several places. Acknowledged. https://codereview.chromium.org/1826113003/diff/1/public/fpdf_edit.h#newcode234 public/fpdf_edit.h:234: // TODO(dsinclair): Does this have the same cache clearing requirements as On 2016/03/24 18:32:15, Tom Sepez wrote: > nit: let's not commit this todo either. Done.
The CQ bit was checked by dsinclair@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1826113003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1826113003/20001
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-pdfium-committers". Note that this has nothing to do with OWNERS files.
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/1826113003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1826113003/20001
Message was sent while issue was closed.
Description was changed from ========== Cleanup fpdf_edit.h and fpdf_doc.h documentation. This CL cleans up the documentation in public/fpdf_doc.h and public/fpdf_edit.h. ========== to ========== Cleanup fpdf_edit.h and fpdf_doc.h documentation. This CL cleans up the documentation in public/fpdf_doc.h and public/fpdf_edit.h. Committed: https://pdfium.googlesource.com/pdfium/+/5f597db5596f302a5ae31447e5c3eaf4320d... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://pdfium.googlesource.com/pdfium/+/5f597db5596f302a5ae31447e5c3eaf4320d...
Message was sent while issue was closed.
thestig@chromium.org changed reviewers: + thestig@chromium.org
Message was sent while issue was closed.
There's a typo. Fix is in https://pdfium-review.googlesource.com/c/6416/ https://codereview.chromium.org/1826113003/diff/20001/public/fpdf_doc.h File public/fpdf_doc.h (left): https://codereview.chromium.org/1826113003/diff/20001/public/fpdf_doc.h#oldco... public/fpdf_doc.h:155: // The file path is UTF-8 encoded. The return value is the number of UTF-8 https://codereview.chromium.org/1826113003/diff/20001/public/fpdf_doc.h File public/fpdf_doc.h (right): https://codereview.chromium.org/1826113003/diff/20001/public/fpdf_doc.h#newco... public/fpdf_doc.h:143: // Regardless of the platform, the |buffer| is always in UTF-16LE encoding. UTF-16LE? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
