Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1825433002:
Cleanup public/fpdf_dataavail.h documentation. (Closed)
DescriptionCleanup public/fpdf_dataavail.h documentation.
This CL re-writes the documentation in the public/fpdf_dataavail.h to clean up spelling, grammar and formatting.
R=tsepez@chromium.org
Committed: https://pdfium.googlesource.com/pdfium/+/ea10a0dcae1b593fa9d85ae867c19be5c34ca8ed
Patch Set 1 #
Total comments: 6
Patch Set 2 : #
Total comments: 8
Patch Set 3 : #Messages
Total messages: 16 (5 generated)
Description was changed from ========== Cleanup public/ documentation. This CL re-writes some of the documentation in the public/ folder to clean up spelling, grammar and formatting. ========== to ========== Cleanup public/ documentation. [DO NOT COMMIT] This CL re-writes some of the documentation in the public/ folder to clean up spelling, grammar and formatting. ==========
dsinclair@chromium.org changed reviewers: + tsepez@chromium.org
Tom, what do you think of this iteration for the public/ documentation cleanup?
I think you're on the right track. One item for discussion is that given a precompiled PDFium libray, one could build an embedder using only a C89-compliant compiler, since the API is C, not C++. So do we insist on /* comments */ for maximum compatibility?
On 2016/03/22 04:54:06, Tom Sepez wrote: > I think you're on the right track. One item for discussion is that given a > precompiled PDFium libray, one could build an embedder using only a > C89-compliant compiler, since the API is C, not C++. So do we insist on /* > comments */ for maximum compatibility? We don't use /* */ comments now in public/. If you look at fpdfview.h it's already using // so we don't support C89 strict now. I'd say we target C99 (if not C11 to sync with our use of c++11).
Agreed on // comment style. https://codereview.chromium.org/1825433002/diff/1/public/fpdf_dataavail.h File public/fpdf_dataavail.h (right): https://codereview.chromium.org/1825433002/diff/1/public/fpdf_dataavail.h#new... public/fpdf_dataavail.h:42: // |pThis| - pointer to the interface structure. nit: I don't think we need the vertical bars in a table like this, only when |pThis| appears as part of a sentence. https://codereview.chromium.org/1825433002/diff/1/public/fpdf_dataavail.h#new... public/fpdf_dataavail.h:67: // |avail| - handle to document availability provider to be destroyed. I like the "as returned by" part, which clearly indicates how the two functions are related. https://codereview.chromium.org/1825433002/diff/1/public/fpdf_dataavail.h#new... public/fpdf_dataavail.h:98: // * PDF_DATA_ERROR: A common error is returned. Data availability unknown. nit: not sure why there's a * here.
https://codereview.chromium.org/1825433002/diff/1/public/fpdf_dataavail.h File public/fpdf_dataavail.h (right): https://codereview.chromium.org/1825433002/diff/1/public/fpdf_dataavail.h#new... public/fpdf_dataavail.h:42: // |pThis| - pointer to the interface structure. On 2016/03/22 15:51:35, Tom Sepez wrote: > nit: I don't think we need the vertical bars in a table like this, only when > |pThis| appears as part of a sentence. Ack. https://codereview.chromium.org/1825433002/diff/1/public/fpdf_dataavail.h#new... public/fpdf_dataavail.h:67: // |avail| - handle to document availability provider to be destroyed. On 2016/03/22 15:51:34, Tom Sepez wrote: > I like the "as returned by" part, which clearly indicates how the two functions > are related. So, my problem with 'as returned by' is that, what happens if/when we add a second method that can return FPDFAvail_Create. Do we update all the comments, leave them outdated? It seems like higher level documentation which should be tying the two methods together instead of the header docs. https://codereview.chromium.org/1825433002/diff/1/public/fpdf_dataavail.h#new... public/fpdf_dataavail.h:98: // * PDF_DATA_ERROR: A common error is returned. Data availability unknown. On 2016/03/22 15:51:35, Tom Sepez wrote: > nit: not sure why there's a * here. Attempting to show a list, * is a markdown list marker. Anything better I could use for list representation?
> So, my problem with 'as returned by' is that, what happens if/when we add a > second method that can return FPDFAvail_Create. Do we update all the comments, > leave them outdated? It seems like higher level documentation which should be > tying the two methods together instead of the header docs. > Except that the higher level doc is from FX and becoming dated. > https://codereview.chromium.org/1825433002/diff/1/public/fpdf_dataavail.h#new... > public/fpdf_dataavail.h:98: // * PDF_DATA_ERROR: A common error is returned. > Data availability unknown. > On 2016/03/22 15:51:35, Tom Sepez wrote: > > nit: not sure why there's a * here. > > Attempting to show a list, * is a markdown list marker. Anything better I could > use for list representation? Maybe blank spaces?
On 2016/03/22 16:28:11, Tom Sepez wrote: > > So, my problem with 'as returned by' is that, what happens if/when we add a > > second method that can return FPDFAvail_Create. Do we update all the comments, > > leave them outdated? It seems like higher level documentation which should be > > tying the two methods together instead of the header docs. > > > Except that the higher level doc is from FX and becoming dated. > That's on my list, heh. Once I get public/ cleaned up I want to start putting some markdown documentation into docs/ that explain how to use the API. Killing off the link to the Foxit docs once they're flushed out a bit.
Description was changed from ========== Cleanup public/ documentation. [DO NOT COMMIT] This CL re-writes some of the documentation in the public/ folder to clean up spelling, grammar and formatting. ========== to ========== Cleanup public/ documentation. This CL re-writes some of the documentation in the public/ folder to clean up spelling, grammar and formatting. ==========
Description was changed from ========== Cleanup public/ documentation. This CL re-writes some of the documentation in the public/ folder to clean up spelling, grammar and formatting. ========== to ========== Cleanup public/fpdf_dataavail.h documentation. This CL re-writes the documentation in the public/fpdf_dataavail.h to clean up spelling, grammar and formatting. ==========
PTAL. I think this one is ready for review.
lgtm https://codereview.chromium.org/1825433002/diff/20001/public/fpdf_dataavail.h File public/fpdf_dataavail.h (right): https://codereview.chromium.org/1825433002/diff/20001/public/fpdf_dataavail.h... public/fpdf_dataavail.h:36: // Reports if the specified data section is available. A section is nit: maybe add "at the present time." since this could change as stuff comes over the wire vs. having a perpetual black hole in the file. https://codereview.chromium.org/1825433002/diff/20001/public/fpdf_dataavail.h... public/fpdf_dataavail.h:54: // Create a document availability provider. nit: wish we could say what an availabilty provider represents or does. https://codereview.chromium.org/1825433002/diff/20001/public/fpdf_dataavail.h... public/fpdf_dataavail.h:59: // Returns a handle to the document availability provider. NULL otherwise. nit: I prefer ", or NULL on error." to otherwise. I think its more precise. "Otherwise on tuesday ... " https://codereview.chromium.org/1825433002/diff/20001/public/fpdf_dataavail.h... public/fpdf_dataavail.h:86: // duplicate sections. nit: overlapping sections.
https://codereview.chromium.org/1825433002/diff/20001/public/fpdf_dataavail.h File public/fpdf_dataavail.h (right): https://codereview.chromium.org/1825433002/diff/20001/public/fpdf_dataavail.h... public/fpdf_dataavail.h:36: // Reports if the specified data section is available. A section is On 2016/03/23 20:10:58, Tom Sepez wrote: > nit: maybe add "at the present time." since this could change as stuff comes > over the wire vs. having a perpetual black hole in the file. Changed to 'currently available' https://codereview.chromium.org/1825433002/diff/20001/public/fpdf_dataavail.h... public/fpdf_dataavail.h:54: // Create a document availability provider. On 2016/03/23 20:10:58, Tom Sepez wrote: > nit: wish we could say what an availabilty provider represents or does. Yea, will have to come back to that one. My understanding is pretty shaky on what it actually does. Will flush it out once I've played with it some. https://codereview.chromium.org/1825433002/diff/20001/public/fpdf_dataavail.h... public/fpdf_dataavail.h:59: // Returns a handle to the document availability provider. NULL otherwise. On 2016/03/23 20:10:58, Tom Sepez wrote: > nit: I prefer ", or NULL on error." to otherwise. I think its more precise. > "Otherwise on tuesday ... " Done. https://codereview.chromium.org/1825433002/diff/20001/public/fpdf_dataavail.h... public/fpdf_dataavail.h:86: // duplicate sections. On 2016/03/23 20:10:58, Tom Sepez wrote: > nit: overlapping sections. Done.
Description was changed from ========== Cleanup public/fpdf_dataavail.h documentation. This CL re-writes the documentation in the public/fpdf_dataavail.h to clean up spelling, grammar and formatting. ========== to ========== Cleanup public/fpdf_dataavail.h documentation. This CL re-writes the documentation in the public/fpdf_dataavail.h to clean up spelling, grammar and formatting. R=tsepez@chromium.org Committed: https://pdfium.googlesource.com/pdfium/+/ea10a0dcae1b593fa9d85ae867c19be5c34c... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) manually as ea10a0dcae1b593fa9d85ae867c19be5c34ca8ed (presubmit successful). |
