Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6321017:
Refactor win printing workflow. (Closed)
|
Created:
9 years, 11 months ago by kmadhusu Modified:
9 years, 7 months ago CC:
chromium-reviews, darin-cc_chromium.org, brettw-cc_chromium.org, Stephen White Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRefactor win printing workflow.
As a preparation for print preview workflow, refactored PrintPage() function.
Created the following functions:
1. RenderPage(): Renders the page for printing. Handles Alpha blend transparency.
2. CopyMetafileDataToSharedMem(): Copy the metafile data to shared memory.
BUG=none
TEST=printing works after code changes.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=72542
Patch Set 1 #
Total comments: 15
Patch Set 2 : Fixed comments. #Patch Set 3 : Removed the commented code. #
Total comments: 8
Patch Set 4 : '' #Patch Set 5 : '' #Patch Set 6 : '' #Patch Set 7 : Refactored CopyMetafileDataToSharedMem() function. #
Total comments: 9
Patch Set 8 : Fixed comments. #
Total comments: 5
Patch Set 9 : '' #
Messages
Total messages: 13 (0 generated)
http://codereview.chromium.org/6321017/diff/1/chrome/renderer/print_web_view_... File chrome/renderer/print_web_view_helper_win.cc (right): http://codereview.chromium.org/6321017/diff/1/chrome/renderer/print_web_view_... chrome/renderer/print_web_view_helper_win.cc:140: NOTREACHED() << "Printing page " << page_number << "failed."; nit: "failed" -> " failed." http://codereview.chromium.org/6321017/diff/1/chrome/renderer/print_web_view_... chrome/renderer/print_web_view_helper_win.cc:168: NOTREACHED() << "Printing page " << page_number << "failed"; And here http://codereview.chromium.org/6321017/diff/1/chrome/renderer/print_web_view_... chrome/renderer/print_web_view_helper_win.cc:183: printing::NativeMetafile* metafile2(new printing::NativeMetafile()); scoped_ptr<> http://codereview.chromium.org/6321017/diff/1/chrome/renderer/print_web_view_... chrome/renderer/print_web_view_helper_win.cc:222: metafile->reset(metafile2); to go with the scoped_ptr<>: metafile->reset(metafile2->release()); http://codereview.chromium.org/6321017/diff/1/chrome/renderer/print_web_view_... chrome/renderer/print_web_view_helper_win.cc:236: return false; The old code Sends the DuplicateSection call even if it is too big. Is it a no-op without metafile_data_handle or something? http://codereview.chromium.org/6321017/diff/1/chrome/renderer/print_web_view_... chrome/renderer/print_web_view_helper_win.cc:243: return false; You're missing the CloseEmf() here. This comment and the previous makes me think that maybe the old control flow structure was for the best. http://codereview.chromium.org/6321017/diff/1/chrome/renderer/print_web_view_... chrome/renderer/print_web_view_helper_win.cc:248: shared_buf.GiveToProcess(base::GetCurrentProcessHandle(), Will this cause the shared mem handle to leak? You could pass in the the shared_buf instead of creating it in this method.
Thanks for the comments. Made code changes accordingly. Please review. http://codereview.chromium.org/6321017/diff/1/chrome/renderer/print_web_view_... File chrome/renderer/print_web_view_helper_win.cc (right): http://codereview.chromium.org/6321017/diff/1/chrome/renderer/print_web_view_... chrome/renderer/print_web_view_helper_win.cc:105: page_params.metafile_data_handle = base::SharedMemory::NULLHandle(); Removed this code because metafile_data_handle is already initialized to INVALID_HANDLE_VALUE. http://codereview.chromium.org/6321017/diff/1/chrome/renderer/print_web_view_... chrome/renderer/print_web_view_helper_win.cc:140: NOTREACHED() << "Printing page " << page_number << "failed."; On 2011/01/21 21:53:28, vandebo wrote: > nit: "failed" -> " failed." Done. http://codereview.chromium.org/6321017/diff/1/chrome/renderer/print_web_view_... chrome/renderer/print_web_view_helper_win.cc:168: NOTREACHED() << "Printing page " << page_number << "failed"; On 2011/01/21 21:53:28, vandebo wrote: > And here Done. http://codereview.chromium.org/6321017/diff/1/chrome/renderer/print_web_view_... chrome/renderer/print_web_view_helper_win.cc:183: printing::NativeMetafile* metafile2(new printing::NativeMetafile()); On 2011/01/21 21:53:28, vandebo wrote: > scoped_ptr<> Done. http://codereview.chromium.org/6321017/diff/1/chrome/renderer/print_web_view_... chrome/renderer/print_web_view_helper_win.cc:222: metafile->reset(metafile2); On 2011/01/21 21:53:28, vandebo wrote: > to go with the scoped_ptr<>: > metafile->reset(metafile2->release()); Done. http://codereview.chromium.org/6321017/diff/1/chrome/renderer/print_web_view_... chrome/renderer/print_web_view_helper_win.cc:236: return false; On 2011/01/21 21:53:28, vandebo wrote: > The old code Sends the DuplicateSection call even if it is too big. Is it a > no-op without metafile_data_handle or something? metafile_data_handle is initialized to "INVALID_HANDLE_VALUE". Even though the |buf_size| is too big, we resend the "INVALID_HANDLE_VALUE" to browser process which handles this case. http://codereview.chromium.org/6321017/diff/1/chrome/renderer/print_web_view_... chrome/renderer/print_web_view_helper_win.cc:243: return false; On 2011/01/21 21:53:28, vandebo wrote: > You're missing the CloseEmf() here. This comment and the previous makes me > think that maybe the old control flow structure was for the best. Reverted to the old control flow. http://codereview.chromium.org/6321017/diff/1/chrome/renderer/print_web_view_... chrome/renderer/print_web_view_helper_win.cc:248: shared_buf.GiveToProcess(base::GetCurrentProcessHandle(), On 2011/01/21 21:53:28, vandebo wrote: > Will this cause the shared mem handle to leak? You could pass in the the > shared_buf instead of creating it in this method. No. I confirmed this with brettw@chromium.org.
http://codereview.chromium.org/6321017/diff/8001/chrome/renderer/print_web_vi... File chrome/renderer/print_web_view_helper_win.cc (left): http://codereview.chromium.org/6321017/diff/8001/chrome/renderer/print_web_vi... chrome/renderer/print_web_view_helper_win.cc:198: DCHECK_GT(buf_size, 128u); nit: You've lost this DCHECK http://codereview.chromium.org/6321017/diff/8001/chrome/renderer/print_web_vi... chrome/renderer/print_web_view_helper_win.cc:201: page_params.metafile_data_handle = NULL; This will lead to a different control flow than you have for the >350MB case. http://codereview.chromium.org/6321017/diff/8001/chrome/renderer/print_web_vi... File chrome/renderer/print_web_view_helper_win.cc (right): http://codereview.chromium.org/6321017/diff/8001/chrome/renderer/print_web_vi... chrome/renderer/print_web_view_helper_win.cc:221: (*metafile)->CloseEmf(); nit: Not neded. metafile->reset will destruct *metafile, which calls CloseEmf(). http://codereview.chromium.org/6321017/diff/8001/chrome/renderer/print_web_vi... chrome/renderer/print_web_view_helper_win.cc:227: scoped_ptr<printing::NativeMetafile>& metafile, Pass the raw pointer, not a scoped_ptr<>&.
Made code changes as per comments. Please review. http://codereview.chromium.org/6321017/diff/8001/chrome/renderer/print_web_vi... File chrome/renderer/print_web_view_helper_win.cc (left): http://codereview.chromium.org/6321017/diff/8001/chrome/renderer/print_web_vi... chrome/renderer/print_web_view_helper_win.cc:198: DCHECK_GT(buf_size, 128u); On 2011/01/24 17:39:18, vandebo wrote: > nit: You've lost this DCHECK Fixed. http://codereview.chromium.org/6321017/diff/8001/chrome/renderer/print_web_vi... chrome/renderer/print_web_view_helper_win.cc:201: page_params.metafile_data_handle = NULL; On 2011/01/24 17:39:18, vandebo wrote: > This will lead to a different control flow than you have for the >350MB case. Added "page_params.metafile_data_handle = NULL" statement. http://codereview.chromium.org/6321017/diff/8001/chrome/renderer/print_web_vi... File chrome/renderer/print_web_view_helper_win.cc (right): http://codereview.chromium.org/6321017/diff/8001/chrome/renderer/print_web_vi... chrome/renderer/print_web_view_helper_win.cc:221: (*metafile)->CloseEmf(); On 2011/01/24 17:39:18, vandebo wrote: > nit: Not neded. metafile->reset will destruct *metafile, which calls > CloseEmf(). Fixed. http://codereview.chromium.org/6321017/diff/8001/chrome/renderer/print_web_vi... chrome/renderer/print_web_view_helper_win.cc:227: scoped_ptr<printing::NativeMetafile>& metafile, On 2011/01/24 17:39:18, vandebo wrote: > Pass the raw pointer, not a scoped_ptr<>&. Done.
Added a boolean variable in CopyMetafileDataToSharedMem() function to keep track of return value. Modified the function return statement accordingly. Please review my changes. Thanks.
Refactored nested-if-else clause in CopyMetafileDataToSharedMem() function. Please review.
http://codereview.chromium.org/6321017/diff/24001/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper_win.cc (left): http://codereview.chromium.org/6321017/diff/24001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper_win.cc:237: if (!is_preview_) { Do you need to add a placeholder here to say what we are going to do in the preview case? http://codereview.chromium.org/6321017/diff/24001/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper_win.cc (right): http://codereview.chromium.org/6321017/diff/24001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper_win.cc:123: (*metafile).CloseEmf(); Isn't this the same as metafile->CloseEmf()? http://codereview.chromium.org/6321017/diff/24001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper_win.cc:247: if (buf_size > 350*1024*1024) { Change > to >= to preserve the original code. http://codereview.chromium.org/6321017/diff/24001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper_win.cc:259: if (metafile->GetData(shared_buf.memory(), buf_size)) { If you change this to: if (!metafile->GetData()) then you can get rid of |ret_val| altogether.
Made changes as per comments. Please review. Thanks. http://codereview.chromium.org/6321017/diff/24001/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper_win.cc (left): http://codereview.chromium.org/6321017/diff/24001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper_win.cc:237: if (!is_preview_) { On 2011/01/25 03:02:54, Lei Zhang wrote: > Do you need to add a placeholder here to say what we are going to do in the > preview case? No. For preview workflow, we are not going to call this function. http://codereview.chromium.org/6321017/diff/24001/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper_win.cc (right): http://codereview.chromium.org/6321017/diff/24001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper_win.cc:123: (*metafile).CloseEmf(); On 2011/01/25 03:02:54, Lei Zhang wrote: > Isn't this the same as metafile->CloseEmf()? Fixed. http://codereview.chromium.org/6321017/diff/24001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper_win.cc:247: if (buf_size > 350*1024*1024) { On 2011/01/25 03:02:54, Lei Zhang wrote: > Change > to >= to preserve the original code. Done. http://codereview.chromium.org/6321017/diff/24001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper_win.cc:259: if (metafile->GetData(shared_buf.memory(), buf_size)) { On 2011/01/25 03:02:54, Lei Zhang wrote: > If you change this to: > > if (!metafile->GetData()) > > then you can get rid of |ret_val| altogether. Done.
LGTM. No need for another LG after nits. http://codereview.chromium.org/6321017/diff/28001/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper.h (right): http://codereview.chromium.org/6321017/diff/28001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.h:191: #if !defined(OS_LINUX) nit: Sorry, didn't notice this until now. This should be #if defined(OS_WIN) || #if defined(OS_MAXOSX). See "Platform Specific Code" in https://sites.google.com/a/chromium.org/dev/developers/coding-style http://codereview.chromium.org/6321017/diff/28001/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper_win.cc (right): http://codereview.chromium.org/6321017/diff/28001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper_win.cc:71: scoped_ptr<printing::NativeMetafile> metafile(new printing::NativeMetafile()); nit: extra ()
Thanks for the comments. http://codereview.chromium.org/6321017/diff/28001/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper.h (right): http://codereview.chromium.org/6321017/diff/28001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.h:191: #if !defined(OS_LINUX) On 2011/01/25 18:13:15, vandebo wrote: > nit: Sorry, didn't notice this until now. This should be #if defined(OS_WIN) || > #if defined(OS_MAXOSX). See "Platform Specific Code" in > https://sites.google.com/a/chromium.org/dev/developers/coding-style Done. http://codereview.chromium.org/6321017/diff/28001/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper_win.cc (right): http://codereview.chromium.org/6321017/diff/28001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper_win.cc:71: scoped_ptr<printing::NativeMetafile> metafile(new printing::NativeMetafile()); On 2011/01/25 18:13:15, vandebo wrote: > nit: extra () Done. http://codereview.chromium.org/6321017/diff/28001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper_win.cc:195: new printing::NativeMetafile()); Removed extra "()".
http://codereview.chromium.org/6321017/diff/24001/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper_win.cc (left): http://codereview.chromium.org/6321017/diff/24001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper_win.cc:237: if (!is_preview_) { On 2011/01/25 17:39:18, kmadhusu wrote: > On 2011/01/25 03:02:54, Lei Zhang wrote: > > Do you need to add a placeholder here to say what we are going to do in the > > preview case? > > No. For preview workflow, we are not going to call this function. If you're never going to call this function for print preview, then why have this check?
Please ignore the last comment. I was looking at an old revision of the code. LGTM with green trybots. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
