Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
6 years, 5 months ago by ivandavid Modified:
6 years, 5 months ago Reviewers:
Lei Zhang, Dan Beam CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdded GetPDFPageSizeByIndex to expose FPDF_GetPageSizeByIndex. The function outputs the width and height, in points, of a given PDF document page.
BUG=388517
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282300
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fixed merge problem that accidentally deleted code. Refactored GetPDFPageSizeByIndex. #Patch Set 3 : Rewrote GetPDFPageSizeByIndex. Added comments about how the function works. #Patch Set 4 : Redid comments to conform to the style of existing comment. Rename parameters for same reason. #
Total comments: 7
Patch Set 5 : Fixed typo in comments. Moved all the comments over to pdf_engine.h. #
Total comments: 5
Patch Set 6 : Fixed comments and a typo. #
Total comments: 6
Created: 6 years, 5 months ago
Messages
Total messages: 20 (0 generated)
see comments on original CL: https://codereview.chromium.org/335583004/#msg53
Please change the CL description to what this CL actually does. https://codereview.chromium.org/376083002/diff/1/pdf/libpdf.map File pdf/libpdf.map (right): https://codereview.chromium.org/376083002/diff/1/pdf/libpdf.map#newcode8 pdf/libpdf.map:8: GetPDFPageSizeByIndex; As I mentioned in the browser tests CL, this file no longer exists. Please sync your tree. https://codereview.chromium.org/376083002/diff/1/pdf/pdfium/pdfium_engine.cc File pdf/pdfium/pdfium_engine.cc (left): https://codereview.chromium.org/376083002/diff/1/pdf/pdfium/pdfium_engine.cc#... pdf/pdfium/pdfium_engine.cc:139: PP_BrowserFont_Trusted_Weight WeightToBrowserFontTrustedWeight(int weight) { Why is this code deleted? https://codereview.chromium.org/376083002/diff/1/pdf/pdfium/pdfium_engine.cc File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/376083002/diff/1/pdf/pdfium/pdfium_engine.cc#... pdf/pdfium/pdfium_engine.cc:3394: bool success = false; Can you try following my suggested pattern? There's no need to introduce a variable here just yet. Just return false if |doc| is NULL. Declare |success| on the first real use of |success|. https://codereview.chromium.org/376083002/diff/1/pdf/pdfium/pdfium_engine.h File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/376083002/diff/1/pdf/pdfium/pdfium_engine.h#n... pdf/pdfium/pdfium_engine.h:618: // If false returned, the query failed; if true, it succeeded. Please document this properly.
I added comments to document the behavior of the function. I also renamed the parameters and I rewrote the function in the style you guys suggested.
Can you rewrite the CL description to avoid using the past tense? "Added a function to pdf engine exports." -> "Add a function to pdf engine exports." or better yet, "Export a function from the PDF plugin to get the dimensions of a PDF page." Also note the subject line is not part of the commit message. So the actual commit message starts with "The function was added". https://codereview.chromium.org/376083002/diff/60001/pdf/pdf.cc File pdf/pdf.cc (right): https://codereview.chromium.org/376083002/diff/60001/pdf/pdf.cc#newcode205 pdf/pdf.cc:205: // |pdf_buffer_size| is the size of the pdf_buffer in bytes. the pdf_buffer -> |pdf_buffer|
lgtm https://codereview.chromium.org/376083002/diff/60001/pdf/pdf.cc File pdf/pdf.cc (right): https://codereview.chromium.org/376083002/diff/60001/pdf/pdf.cc#newcode210 pdf/pdf.cc:210: // Returns false if the document or the page number are not valid. ^ I'd say put this doc comment on the interface https://codereview.chromium.org/376083002/diff/60001/pdf/pdf_engine.h File pdf/pdf_engine.h (right): https://codereview.chromium.org/376083002/diff/60001/pdf/pdf_engine.h#newcode317 pdf/pdf_engine.h:317: // See the definition of GetPDFPageSizeByIndex in pdf.cc for details. you should make the .cc file always adhere to the doc in the .h rather than telling the .h reader to see the implementation
I fixed the typo and I moved all the comments over to pdf_engine.h PDFEngineExports to keep everything consistent. That way if anybody does something similar, they won't have to choose between putting the comments in one place or another. https://codereview.chromium.org/376083002/diff/60001/pdf/pdf.cc File pdf/pdf.cc (right): https://codereview.chromium.org/376083002/diff/60001/pdf/pdf.cc#newcode205 pdf/pdf.cc:205: // |pdf_buffer_size| is the size of the pdf_buffer in bytes. On 2014/07/09 21:59:51, Lei Zhang wrote: > the pdf_buffer -> |pdf_buffer| Done. https://codereview.chromium.org/376083002/diff/60001/pdf/pdf.cc#newcode210 pdf/pdf.cc:210: // Returns false if the document or the page number are not valid. On 2014/07/09 22:18:21, Dan Beam wrote: > ^ I'd say put this doc comment on the interface OK. I put the comment in pdf_engine.h PDFEngineExports. I also moved over all the other comments, since it would be inconsistent. The other comments is what I went off of when writing this one. I also removed the comment that said where to look for the code and I did this for all of the PDFEngineExports functions to be consistent, since some of them had it and some of them didn't. https://codereview.chromium.org/376083002/diff/60001/pdf/pdf_engine.h File pdf/pdf_engine.h (right): https://codereview.chromium.org/376083002/diff/60001/pdf/pdf_engine.h#newcode317 pdf/pdf_engine.h:317: // See the definition of GetPDFPageSizeByIndex in pdf.cc for details. On 2014/07/09 22:18:21, Dan Beam wrote: > you should make the .cc file always adhere to the doc in the .h rather than > telling the .h reader to see the implementation Done. https://codereview.chromium.org/376083002/diff/60001/pdf/pdfium/pdfium_engine.cc File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/376083002/diff/60001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.cc:207: // Do not attempt to map fonts if pepper is not initialized (for privet local Accidentally deleted this code. I restored it. Same goes for line 3211.
lgtm with a proper commit message in the description field.
not lgtm until i understand what's going on in pdfium_engine.h https://codereview.chromium.org/376083002/diff/110001/pdf/pdf_engine.h File pdf/pdf_engine.h (right): https://codereview.chromium.org/376083002/diff/110001/pdf/pdf_engine.h#newcod... pdf/pdf_engine.h:302: // |dc| is the device context to render into. remove from here down: https://codereview.chromium.org/376083002/diff/110001/pdf/pdf_engine.h#newcod... pdf/pdf_engine.h:323: // the output bound. ^ to here https://codereview.chromium.org/376083002/diff/110001/pdf/pdf_engine.h#newcod... pdf/pdf_engine.h:330: #endif // OS_WIN nit: \n https://codereview.chromium.org/376083002/diff/110001/pdf/pdf_engine.h#newcod... pdf/pdf_engine.h:337: // |bitmap_width| is the width of the output bitmap. I don't see |bitmap_width|, |bitmap_height|, |dpi|, or |autorotate| in RenderPDFPageToBitmap(...) https://codereview.chromium.org/376083002/diff/110001/pdf/pdf_engine.h#newcod... pdf/pdf_engine.h:347: void* bitmap_buffer) = 0; nit: \n
You guys are confusing the exported function with the internal implementation. e.g. RenderPDFPageToDC in pdf.cc is the exported function, and PDFiumEngineExports::RenderPDFPageToDC is the internal implementation. They have different signatures, so copy + pasting the comments over doesn't make sense. I would just put the comments back as they were. Otherwise, the exported function has no comments. There's no corresponding .h file to these exported functions.
Everything is back to how it was. I changed pdf_buffer to |pdf_buffer| and I shifted the comments to make them fit within the 80 line limit. The content is the same, its just moved.
Still lgtm
lgtm https://codereview.chromium.org/376083002/diff/90005/pdf/pdf.cc File pdf/pdf.cc (right): https://codereview.chromium.org/376083002/diff/90005/pdf/pdf.cc#newcode121 pdf/pdf.cc:121: // value is -1, the dpi from the DC will be used. revert https://codereview.chromium.org/376083002/diff/90005/pdf/pdf.cc#newcode124 pdf/pdf.cc:124: // page. revert https://codereview.chromium.org/376083002/diff/90005/pdf/pdf.cc#newcode128 pdf/pdf.cc:128: // the bounds will be clipped. revert https://codereview.chromium.org/376083002/diff/90005/pdf/pdf.cc#newcode136 pdf/pdf.cc:136: // preserved while scaling. revert https://codereview.chromium.org/376083002/diff/90005/pdf/pdf.cc#newcode211 pdf/pdf.cc:211: // Returns false if the document or the page number are not valid. this is also a local pattern (i've never seen), which is good enough to keep this way https://codereview.chromium.org/376083002/diff/90005/pdf/pdf_engine.h File pdf/pdf_engine.h (right): https://codereview.chromium.org/376083002/diff/90005/pdf/pdf_engine.h#newcode317 pdf/pdf_engine.h:317: // See the definition of GetPDFPageSizeByIndex in pdf.cc for details. btw, I didn't know ^ the rest of the file follows this pattern
The CQ bit was checked by ivandavid@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ivandavid@chromium.org/376083002/90005
On 2014/07/10 02:49:44, ivandavid wrote: > The CQ bit was checked by mailto:ivandavid@chromium.org Generally you should address reviewers' follow up comments or at least give a reason why you wont be before clicking "[X] Commit".
On 2014/07/10 02:59:06, Dan Beam wrote: > On 2014/07/10 02:49:44, ivandavid wrote: > > The CQ bit was checked by mailto:ivandavid@chromium.org > > Generally you should address reviewers' follow up comments or at least give a > reason why you wont be before clicking "[X] Commit". Oops. Sorry, I actually don't think those changes should be reverted because they were over the 80 chars per line limit, so I just shifted everything to fit.
On 2014/07/10 03:03:51, ivandavid wrote: > On 2014/07/10 02:59:06, Dan Beam wrote: > > On 2014/07/10 02:49:44, ivandavid wrote: > > > The CQ bit was checked by mailto:ivandavid@chromium.org > > > > Generally you should address reviewers' follow up comments or at least give a > > reason why you wont be before clicking "[X] Commit". > > Oops. Sorry, I actually don't think those changes should be reverted because > they were over the 80 chars per line limit, so I just shifted everything to fit. you mentioned you use vim, so try set tw=80 in your .vimrc or in a running editor. also, here's a useful mapping of "W" to re-wrap the ambient paragraph: map W <Esc>gq}<Return> in general, if you'd like to simply revert parts of a file, you can do fancy things like: git checkout -p master -- pdf/pdf.cc and it'll interactively ask you whether you'd like to discard hunks (parts of a diff) and you can choose "y" to revert that hunk in your index (so it's staged to be committed in your local git repo). #sillydevtricks
btw: the only real reason i ask you revert these is that it messes with the history (it looks like you wrote these comments instead of the original author)
Message was sent while issue was closed.
Change committed as 282300 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
