Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
467343003:
Defer request to print a PDF when the user initiates the entire frame and the PDF hasn't loaded. (Closed)
DescriptionDefer request to print a PDF when the user initiates the entire frame and the PDF hasn't loaded.
Original fix:
https://codereview.chromium.org/427723004/
The original fix was reverted, however it actually shouldn't have been because it was mostly correct. This fix is the same as the old one, with an additional change. The call to DidStopLoading() in Instance::DocumentLoadComplete was moved to an earlier part of the function, causing DidStopLoading to be called before the print preview request.
BUG=376969
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290010
Patch Set 1 #
Total comments: 10
Patch Set 2 : Removed callback header and fixed grammar issues in comment. #Patch Set 3 : Added call to DidStopLoading() in InvokePrintingForInstance to progess printing process. #Patch Set 4 : Added DCHECK to RequestPrintPreview(). #
Total comments: 6
Patch Set 5 : Removed ppb_pdf_impl.cc and PRINT_PREVIEW_USER_INITIATED_CONTEXT_NODE modifications. #Patch Set 6 : Moved DidStopLoading call in DocumentLoadComplete. #
Total comments: 6
Patch Set 7 : Fixed DCHECK argument. #
Messages
Total messages: 21 (0 generated)
https://codereview.chromium.org/467343003/diff/1/chrome/renderer/printing/pri... File chrome/renderer/printing/print_web_view_helper.cc (left): https://codereview.chromium.org/467343003/diff/1/chrome/renderer/printing/pri... chrome/renderer/printing/print_web_view_helper.cc:1727: case PRINT_PREVIEW_USER_INITIATED_ENTIRE_FRAME: { This is the only case where we should handle the race condition. Handling this condition in PRINT_PREVIEW_USER_INITIATED_SELECTION will break sheets and doesn't seem appropriate as the only problem was with PRINT_PREVIEW_USER_INITIATED_ENTIRE_FRAME. https://codereview.chromium.org/467343003/diff/1/chrome/renderer/printing/pri... File chrome/renderer/printing/print_web_view_helper.h (right): https://codereview.chromium.org/467343003/diff/1/chrome/renderer/printing/pri... chrome/renderer/printing/print_web_view_helper.h:472: PrintPreviewRequestType on_stop_loading_type_; I am using an enum this time because using base::Closure made the code less readable and was overly complicated.
How this is different in regard of Spreadsheets issue? https://codereview.chromium.org/467343003/diff/1/chrome/renderer/printing/pri... File chrome/renderer/printing/print_web_view_helper.cc (left): https://codereview.chromium.org/467343003/diff/1/chrome/renderer/printing/pri... chrome/renderer/printing/print_web_view_helper.cc:1727: case PRINT_PREVIEW_USER_INITIATED_ENTIRE_FRAME: { No, it is not. Try to 1. open huge pdf (for example pdf reference) 2. before it finish loading, click PRINT icon in bottom right corner 3. It would get into PRINT_PREVIEW_USER_INITIATED_CONTEXT_NODE and open empty preview On 2014/08/13 20:58:55, ivandavid wrote: > This is the only case where we should handle the race condition. Handling this > condition in PRINT_PREVIEW_USER_INITIATED_SELECTION will break sheets and > doesn't seem appropriate as the only problem was with > PRINT_PREVIEW_USER_INITIATED_ENTIRE_FRAME. https://codereview.chromium.org/467343003/diff/1/chrome/renderer/printing/pri... File chrome/renderer/printing/print_web_view_helper.cc (right): https://codereview.chromium.org/467343003/diff/1/chrome/renderer/printing/pri... chrome/renderer/printing/print_web_view_helper.cc:1752: params.selection_only = has_selection; could you please put DCHECK(!GetPlugin(print_preview_context_.source_frame())); here https://codereview.chromium.org/467343003/diff/1/chrome/renderer/printing/pri... File chrome/renderer/printing/print_web_view_helper.h (right): https://codereview.chromium.org/467343003/diff/1/chrome/renderer/printing/pri... chrome/renderer/printing/print_web_view_helper.h:472: PrintPreviewRequestType on_stop_loading_type_; Actually I like more Closure version. On 2014/08/13 20:58:55, ivandavid wrote: > I am using an enum this time because using base::Closure made the code less > readable and was overly complicated.
https://codereview.chromium.org/467343003/diff/1/chrome/renderer/printing/pri... File chrome/renderer/printing/print_web_view_helper.cc (left): https://codereview.chromium.org/467343003/diff/1/chrome/renderer/printing/pri... chrome/renderer/printing/print_web_view_helper.cc:1727: case PRINT_PREVIEW_USER_INITIATED_ENTIRE_FRAME: { On 2014/08/13 23:39:45, Vitaly Buka wrote: > No, it is not. > > Try to > 1. open huge pdf (for example pdf reference) > 2. before it finish loading, click PRINT icon in bottom right corner > 3. It would get into PRINT_PREVIEW_USER_INITIATED_CONTEXT_NODE and open empty > preview > > > > On 2014/08/13 20:58:55, ivandavid wrote: > > This is the only case where we should handle the race condition. Handling this > > condition in PRINT_PREVIEW_USER_INITIATED_SELECTION will break sheets and > > doesn't seem appropriate as the only problem was with > > PRINT_PREVIEW_USER_INITIATED_ENTIRE_FRAME. > Done. https://codereview.chromium.org/467343003/diff/1/chrome/renderer/printing/pri... chrome/renderer/printing/print_web_view_helper.cc:1727: case PRINT_PREVIEW_USER_INITIATED_ENTIRE_FRAME: { On 2014/08/13 23:39:45, Vitaly Buka wrote: > No, it is not. > > Try to > 1. open huge pdf (for example pdf reference) > 2. before it finish loading, click PRINT icon in bottom right corner > 3. It would get into PRINT_PREVIEW_USER_INITIATED_CONTEXT_NODE and open empty > preview > > > > On 2014/08/13 20:58:55, ivandavid wrote: > > This is the only case where we should handle the race condition. Handling this > > condition in PRINT_PREVIEW_USER_INITIATED_SELECTION will break sheets and > > doesn't seem appropriate as the only problem was with > > PRINT_PREVIEW_USER_INITIATED_ENTIRE_FRAME. > Oh OK. I figured why the PRINT_PREVIEW_USER_INITIATED_CONTEXT_NODE code path was causing sheets to not work. See the comment in ppb_pdf_impl.cc https://codereview.chromium.org/467343003/diff/1/chrome/renderer/printing/pri... File chrome/renderer/printing/print_web_view_helper.h (right): https://codereview.chromium.org/467343003/diff/1/chrome/renderer/printing/pri... chrome/renderer/printing/print_web_view_helper.h:472: PrintPreviewRequestType on_stop_loading_type_; On 2014/08/13 23:39:45, Vitaly Buka wrote: > Actually I like more Closure version. > > On 2014/08/13 20:58:55, ivandavid wrote: > > I am using an enum this time because using base::Closure made the code less > > readable and was overly complicated. > Ok. I re-implemented it. https://codereview.chromium.org/467343003/diff/1/chrome/renderer/printing/pri... chrome/renderer/printing/print_web_view_helper.h:472: PrintPreviewRequestType on_stop_loading_type_; On 2014/08/13 23:39:45, Vitaly Buka wrote: > Actually I like more Closure version. > > On 2014/08/13 20:58:55, ivandavid wrote: > > I am using an enum this time because using base::Closure made the code less > > readable and was overly complicated. > Ok. I reimplemented it. https://codereview.chromium.org/467343003/diff/1/chrome/renderer/printing/pri... chrome/renderer/printing/print_web_view_helper.h:472: PrintPreviewRequestType on_stop_loading_type_; On 2014/08/13 23:39:45, Vitaly Buka wrote: > Actually I like more Closure version. > > On 2014/08/13 20:58:55, ivandavid wrote: > > I am using an enum this time because using base::Closure made the code less > > readable and was overly complicated. > Done. https://codereview.chromium.org/467343003/diff/80001/chrome/renderer/pepper/p... File chrome/renderer/pepper/ppb_pdf_impl.cc (right): https://codereview.chromium.org/467343003/diff/80001/chrome/renderer/pepper/p... chrome/renderer/pepper/ppb_pdf_impl.cc:467: DidStopLoading(instance_id); Is this an appropriate place to place DidStopLoading()? It fixes the sheets problem, however it might not be a good idea to put it here. I tried looking for references to DidStartLoading, but I couldn't really find one.
https://codereview.chromium.org/467343003/diff/80001/chrome/renderer/pepper/p... File chrome/renderer/pepper/ppb_pdf_impl.cc (right): https://codereview.chromium.org/467343003/diff/80001/chrome/renderer/pepper/p... chrome/renderer/pepper/ppb_pdf_impl.cc:467: DidStopLoading(instance_id); It does not look like a right place let's remove this and _NODE related change and fix that in a separate CL On 2014/08/14 03:38:13, ivandavid wrote: > Is this an appropriate place to place DidStopLoading()? It fixes the sheets > problem, however it might not be a good idea to put it here. I tried looking for > references to DidStartLoading, but I couldn't really find one. https://codereview.chromium.org/467343003/diff/80001/chrome/renderer/printing... File chrome/renderer/printing/print_web_view_helper.cc (right): https://codereview.chromium.org/467343003/diff/80001/chrome/renderer/printing... chrome/renderer/printing/print_web_view_helper.cc:823: if (!on_stop_loading_closure_.is_null()) on_stop_loading_closure_.reset() after run https://codereview.chromium.org/467343003/diff/80001/chrome/renderer/printing... chrome/renderer/printing/print_web_view_helper.cc:1703: DCHECK(!GetPlugin(print_preview_context_.source_frame())); this DCHECK makes no sense if it was correct, all your changes irrelevant. only PRINT_PREVIEW_USER_INITIATED_SELECTION needs this DCHECK
https://codereview.chromium.org/467343003/diff/80001/chrome/renderer/printing... File chrome/renderer/printing/print_web_view_helper.cc (right): https://codereview.chromium.org/467343003/diff/80001/chrome/renderer/printing... chrome/renderer/printing/print_web_view_helper.cc:823: if (!on_stop_loading_closure_.is_null()) On 2014/08/14 05:19:43, Vitaly Buka wrote: > on_stop_loading_closure_.reset() after run Done. https://codereview.chromium.org/467343003/diff/80001/chrome/renderer/printing... chrome/renderer/printing/print_web_view_helper.cc:1703: DCHECK(!GetPlugin(print_preview_context_.source_frame())); On 2014/08/14 05:19:43, Vitaly Buka wrote: > this DCHECK makes no sense > if it was correct, all your changes irrelevant. > > only PRINT_PREVIEW_USER_INITIATED_SELECTION needs this DCHECK Done. https://codereview.chromium.org/467343003/diff/120001/pdf/instance.cc File pdf/instance.cc (right): https://codereview.chromium.org/467343003/diff/120001/pdf/instance.cc#newcode... pdf/instance.cc:1503: if (did_call_start_loading_) { I know you said to create a different CL for all the _NODE code. However, I think I actually figured out the problem. The problem was that DidStopLoading() wasn't being called before the request for print preview. Putting the call to DidStopLoading here forces that to happen. I feel that its OK to move DidStopLoading up here, because all its doing is changing a boolean value. The only difference is now, when somebody prints a Google Drive Sheet a new tab will appear and the print preview screen will immediately pop up rather than forcing the user to click the print button again.
lgtm https://codereview.chromium.org/467343003/diff/120001/chrome/renderer/printin... File chrome/renderer/printing/print_web_view_helper.cc (right): https://codereview.chromium.org/467343003/diff/120001/chrome/renderer/printin... chrome/renderer/printing/print_web_view_helper.cc:1749: DCHECK(GetPlugin(print_preview_context_.source_frame())); DCHECK(!GetPlugin PRINT_PREVIEW_USER_INITIATED_SELECTION is expected for HTML pages only. https://codereview.chromium.org/467343003/diff/120001/pdf/instance.cc File pdf/instance.cc (right): https://codereview.chromium.org/467343003/diff/120001/pdf/instance.cc#newcode... pdf/instance.cc:1503: if (did_call_start_loading_) { Seems reasonable. On 2014/08/15 00:35:23, ivandavid wrote: > I know you said to create a different CL for all the _NODE code. However, I > think I actually figured out the problem. The problem was that DidStopLoading() > wasn't being called before the request for print preview. Putting the call to > DidStopLoading here forces that to happen. I feel that its OK to move > DidStopLoading up here, because all its doing is changing a boolean value. The > only difference is now, when somebody prints a Google Drive Sheet a new tab will > appear and the print preview screen will immediately pop up rather than forcing > the user to click the print button again.
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/467343003/120001
lgtm https://codereview.chromium.org/467343003/diff/120001/pdf/instance.cc File pdf/instance.cc (right): https://codereview.chromium.org/467343003/diff/120001/pdf/instance.cc#newcode... pdf/instance.cc:1503: if (did_call_start_loading_) { On 2014/08/15 00:35:23, ivandavid wrote: > I know you said to create a different CL for all the _NODE code. However, I > think I actually figured out the problem. The problem was that DidStopLoading() > wasn't being called before the request for print preview. Putting the call to > DidStopLoading here forces that to happen. I feel that its OK to move > DidStopLoading up here, because all its doing is changing a boolean value. The > only difference is now, when somebody prints a Google Drive Sheet a new tab will > appear and the print preview screen will immediately pop up rather than forcing > the user to click the print button again. This looks correct. Before this change, there's 2 places that call DidStartLoading(). One for when |full_| is true and one for when |full_| is false. However, We only call DidStopLoading() when |full_| is true. This moves the DidStopLoading() call to before the "if (!full_)" check below to balance the number of calls.
The CQ bit was unchecked by ivandavid@chromium.org
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/467343003/120001
The CQ bit was unchecked by vitalybuka@chromium.org
https://codereview.chromium.org/467343003/diff/120001/chrome/renderer/printin... File chrome/renderer/printing/print_web_view_helper.cc (right): https://codereview.chromium.org/467343003/diff/120001/chrome/renderer/printin... chrome/renderer/printing/print_web_view_helper.cc:1749: DCHECK(GetPlugin(print_preview_context_.source_frame())); Please fix. This should be !GetPlugin On 2014/08/15 00:58:36, Vitaly Buka wrote: > DCHECK(!GetPlugin > > PRINT_PREVIEW_USER_INITIATED_SELECTION is expected for HTML pages only.
The CQ bit was checked by ivandavid@chromium.org
The CQ bit was unchecked by ivandavid@chromium.org
https://codereview.chromium.org/467343003/diff/120001/chrome/renderer/printin... File chrome/renderer/printing/print_web_view_helper.cc (right): https://codereview.chromium.org/467343003/diff/120001/chrome/renderer/printin... chrome/renderer/printing/print_web_view_helper.cc:1749: DCHECK(GetPlugin(print_preview_context_.source_frame())); On 2014/08/15 04:16:26, Vitaly Buka wrote: > Please fix. > This should be !GetPlugin > > On 2014/08/15 00:58:36, Vitaly Buka wrote: > > DCHECK(!GetPlugin > > > > PRINT_PREVIEW_USER_INITIATED_SELECTION is expected for HTML pages only. > Done.
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/467343003/140001
Message was sent while issue was closed.
Committed patchset #7 (140001) as 290010 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
