Print Preview: Handling pending print to pdf requests.

Print Preview: Handling pending print to pdf requests.

BUG=91888
TEST=Open a large doc in print preview. Select "Print To PDF" and click "Print" immediately. The file dialog should be shown regardless of the pdf file being ready for saving.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=100260

[dpapad, 2011-09-02 18:36:32]

Still working on adding some UI while the metafile is generated. For the moment
the preview tab is hidden once the user selects a filepath for saving.

Also the suggested filename on the save dialog is just "Print" (sometimes). This
will be fixed once http://codereview.chromium.org/7818010/ lands.

http://codereview.chromium.org/7792085/diff/10/chrome/browser/ui/webui/print_...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/7792085/diff/10/chrome/browser/ui/webui/print_...
chrome/browser/ui/webui/print_preview_handler.cc:586: } else if (print_to_pdf) {
I would prefer to move this big else if block to a method named
HandlePrintToPdf(), is that ok?

[kmadhusu, 2011-09-02 21:47:03]

http://codereview.chromium.org/7792085/diff/10/chrome/browser/resources/print...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/7792085/diff/10/chrome/browser/resources/print...
chrome/browser/resources/print_preview/print_preview.js:439:
chrome.send('hidePreview');
Create a helper function to set isTabHidden = true and to send "hidePreview"
message.

http://codereview.chromium.org/7792085/diff/10/chrome/browser/resources/print...
chrome/browser/resources/print_preview/print_preview.js:540: isTabHidden = true;
Call that helper function here.

http://codereview.chromium.org/7792085/diff/10/chrome/browser/resources/print...
chrome/browser/resources/print_preview/print_preview.js:986:
console.log("updatePrintPreview " + previewResponseId);
remove console.log

http://codereview.chromium.org/7792085/diff/10/chrome/browser/ui/webui/print_...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/7792085/diff/10/chrome/browser/ui/webui/print_...
chrome/browser/ui/webui/print_preview_handler.cc:586: } else if (print_to_pdf) {
On 2011/09/02 18:36:33, dpapad wrote:
> I would prefer to move this big else if block to a method named
> HandlePrintToPdf(), is that ok?

sounds good to me.

http://codereview.chromium.org/7792085/diff/10/chrome/browser/ui/webui/print_...
chrome/browser/ui/webui/print_preview_handler.cc:593: FilePath& temp_path =
pending_print_to_pdf_path_;
Is this temp_path necessary?

http://codereview.chromium.org/7792085/diff/10/chrome/browser/ui/webui/print_...
chrome/browser/ui/webui/print_preview_handler.cc:965: is_pending ? delete
preview_tab_wrapper() :
I prefer to have Y ? A : B  in an assignment statement(X = Y ? A : B;) but not
as a independent statement.

Can you make this as if (Y) then A else B?

http://codereview.chromium.org/7792085/diff/10/chrome/browser/ui/webui/print_...
File chrome/browser/ui/webui/print_preview_handler.h (right):

http://codereview.chromium.org/7792085/diff/10/chrome/browser/ui/webui/print_...
chrome/browser/ui/webui/print_preview_handler.h:188: FilePath
pending_print_to_pdf_path_;
Document this variable.

[dpapad, 2011-09-07 00:46:19]

Please review again. After the latest patch the preview tab is not hidden while
the pdf file is generated, instead a message is displayed to the user (the tab
is automatically closed when the file has been generated).

http://codereview.chromium.org/7792085/diff/10/chrome/browser/resources/print...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/7792085/diff/10/chrome/browser/resources/print...
chrome/browser/resources/print_preview/print_preview.js:439:
chrome.send('hidePreview');
On 2011/09/02 21:47:03, kmadhusu wrote:
> Create a helper function to set isTabHidden = true and to send "hidePreview"
> message.

This is only performed once here (as of latest patch), so I think it is not
necessary to create a helper function.

http://codereview.chromium.org/7792085/diff/10/chrome/browser/resources/print...
chrome/browser/resources/print_preview/print_preview.js:540: isTabHidden = true;
On 2011/09/02 21:47:03, kmadhusu wrote:
> Call that helper function here.

The preview tab is not hidden anymore here, instead a message is displayed.

http://codereview.chromium.org/7792085/diff/10/chrome/browser/resources/print...
chrome/browser/resources/print_preview/print_preview.js:986:
console.log("updatePrintPreview " + previewResponseId);
On 2011/09/02 21:47:03, kmadhusu wrote:
> remove console.log

Done.

http://codereview.chromium.org/7792085/diff/10/chrome/browser/ui/webui/print_...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/7792085/diff/10/chrome/browser/ui/webui/print_...
chrome/browser/ui/webui/print_preview_handler.cc:586: } else if (print_to_pdf) {
On 2011/09/02 21:47:03, kmadhusu wrote:
> On 2011/09/02 18:36:33, dpapad wrote:
> > I would prefer to move this big else if block to a method named
> > HandlePrintToPdf(), is that ok?
> 
> sounds good to me.

Done.

http://codereview.chromium.org/7792085/diff/10/chrome/browser/ui/webui/print_...
chrome/browser/ui/webui/print_preview_handler.cc:593: FilePath& temp_path =
pending_print_to_pdf_path_;
On 2011/09/02 21:47:03, kmadhusu wrote:
> Is this temp_path necessary?

Done. Changed pending_print_to_pdf_path_ to be a scoped_ptr.

http://codereview.chromium.org/7792085/diff/10/chrome/browser/ui/webui/print_...
chrome/browser/ui/webui/print_preview_handler.cc:965: is_pending ? delete
preview_tab_wrapper() :
On 2011/09/02 21:47:03, kmadhusu wrote:
> I prefer to have Y ? A : B  in an assignment statement(X = Y ? A : B;) but not
> as a independent statement.
> 
> Can you make this as if (Y) then A else B?

No need anymore, I am not hiding the preview tab, so I can always call
ActivateInitiatorTabAndClosePreviewTab(). Also, I removed the is_pending
parameter.

http://codereview.chromium.org/7792085/diff/10/chrome/browser/ui/webui/print_...
File chrome/browser/ui/webui/print_preview_handler.h (right):

http://codereview.chromium.org/7792085/diff/10/chrome/browser/ui/webui/print_...
chrome/browser/ui/webui/print_preview_handler.h:188: FilePath
pending_print_to_pdf_path_;
On 2011/09/02 21:47:03, kmadhusu wrote:
> Document this variable.

Done.

[kmadhusu, 2011-09-07 20:42:57]

http://codereview.chromium.org/7792085/diff/16004/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7792085/diff/16004/chrome/app/generated_resour...
chrome/app/generated_resources.grd:6042: Printing to PDF in progress
Printing to PDF in progress...

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.html (right):

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.html:59: <div
id="dancing-dots-text" class="hidden">
Do you still need this id?

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:78: var
hasPendingPrintToPdfRequest = false;
This variable is not required. You can reuse  "hasPendingPrintDocumentRequest"
to achieve the same thing. Please correct me if I am wrong.

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:441: if (printToPDF) {
You need to disable the input controls as soon as the user clicks print button.
With this CL, you are disabling the controls only after the user selected the
file name.

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:552:
disableInputElementsInSidebar();
Move the disableInput..() function call to line #442.

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:973: *last_saved_path_ =
path.DirName();
I think you need the save the path name only after we complete the save task. In
this function, the user has just selected that path. Preview workflow can fail
before generating the complete preview document.

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:976:
print_preview_ui->OnFileSelectionCompleted();
You can remove the changes in print_preview_ui.h and .cc and call
web_ui_->CallJavascript......(); from here.

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:981:
pending_print_to_pdf_path_.reset(new FilePath(path));
How about renaming pending_print_to_pdf_path_ => print_to_pdf_path_ and to store
the user selected path irrespective of whether the data exists or not. By doing
that you need not pass "path" parameter in PostPrintToPdfTask()?

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:991:
printing::COMPLETE_PREVIEW_DOCUMENT_INDEX, &data);
Add a dcheck to validate data.

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.h (right):

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.h:25: class StringValue;
Do a forward declaration for DictionaryValue and move #include "base/values.h"
to .cc file.

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.h:90: void HandlePrintToPdf(const
DictionaryValue* settings);
Why do you need a pointer here? Pass it as a reference.

[vandebo (ex-Chrome), 2011-09-07 21:38:19]

http://codereview.chromium.org/7792085/diff/16004/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7792085/diff/16004/chrome/app/generated_resour...
chrome/app/generated_resources.grd:6042: Printing to PDF in progress
Are you supposed to send new strings for UI review?

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview_animations.js (right):

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview_animations.js:105: if
(hasPendingPrintToPdfRequest)
It's not clear to me if this is needed.  As soon as we have the PDF we're going
to close the tab.

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:632: void
PrintPreviewHandler::HandlePrintToPdf(const DictionaryValue* settings) {
Will this method be called exactly once?  If not, the UMA stuff needs to go into
one of the if conditions so that it only gets called once.

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:639:
PostPrintToPdfTask(*pending_print_to_pdf_path_.get());
Can you explain how we would get to this point, I don't see an additional call
in the JS code.

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:981:
pending_print_to_pdf_path_.reset(new FilePath(path));
On 2011/09/07 20:42:57, kmadhusu wrote:
> How about renaming pending_print_to_pdf_path_ => print_to_pdf_path_ and to
store
> the user selected path irrespective of whether the data exists or not. By
doing
> that you need not pass "path" parameter in PostPrintToPdfTask()?

Then you could also free print_to_pdf_path_ in PostPrintToPdfTask as well.

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.h (right):

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.h:14: #include "base/values.h"
Declare only.

[arv (Not doing code reviews), 2011-09-07 21:53:51]

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.html (right):

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.html:63: <div
id="custom-message" class="hidden"></div>
Please use hidden instead.

<div id="custom-message" hidden></div>

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:842:
$('custom-message').innerHTML = errorMessage;
Add a var so you do not have to call $ multiple times

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:842:
$('custom-message').innerHTML = errorMessage;
Do you really need innerHTML here? Are you escaping things correctly?

Maybe use textContent instead?

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:843:
$('custom-message').classList.remove('hidden');
$('custom-message').hidden = false;

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:844:
$('custom-message-with-dots').innerHTML = '';
textContent = ''

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview_animations.js (right):

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview_animations.js:86:
$('dancing-dots-text').innerHTML;
No, don't use innerHTML like this. There is no point serializing
#dancing-dots-text to HTML here. Do something like:

var customMessageWithDots = $('custom-message-with-dots');
var textNode = document.createTextNode(message);
customMessageWithDots.insertBefore(textNode, customMessageWithDots.firstChild);

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview_animations.js:87:
$('custom-message-with-dots').classList.remove('hidden');
hidden = false

[dpapad, 2011-09-07 23:42:39]

Addressed kamdhusu's and vandebo's comments. I will address arv's comment
shortly.

http://codereview.chromium.org/7792085/diff/16004/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7792085/diff/16004/chrome/app/generated_resour...
chrome/app/generated_resources.grd:6042: Printing to PDF in progress
On 2011/09/07 20:42:57, kmadhusu wrote:
> Printing to PDF in progress... 

The dots are added in code (because they are being animated, ust like "Loading
preview" string).

http://codereview.chromium.org/7792085/diff/16004/chrome/app/generated_resour...
chrome/app/generated_resources.grd:6042: Printing to PDF in progress
On 2011/09/07 21:38:19, vandebo wrote:
> Are you supposed to send new strings for UI review?

The UI behavior affected by this CL in general has to be approved by the UI
leads. I was thinking that I could ask Marcin to play with this use case on
Canary after it lands and let me know if there is any further actions to take.

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.html (right):

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.html:59: <div
id="dancing-dots-text" class="hidden">
On 2011/09/07 20:42:57, kmadhusu wrote:
> Do you still need this id?

Yes, it is being used in print_preview_animations.js (showCustomMessage()) in
order to populate 'custom-message-with-dots' element.

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:78: var
hasPendingPrintToPdfRequest = false;
On 2011/09/07 20:42:57, kmadhusu wrote:
> This variable is not required. You can reuse  "hasPendingPrintDocumentRequest"
> to achieve the same thing. Please correct me if I am wrong. 

I am using this variable so that I can keep displaying the "Printing to Pdf"
message, when the first page arrives (if necessary), otherwise the page would be
displayed.
It seems that you are right, but want to try some cases to make sure it is safe
to do this. Will address this along with my next batch of comments (addressing
also arv's comments).

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:441: if (printToPDF) {
On 2011/09/07 20:42:57, kmadhusu wrote:
> You need to disable the input controls as soon as the user clicks print
button.
> With this CL, you are disabling the controls only after the user selected the
> file name.

See my next comment.

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:552:
disableInputElementsInSidebar();
On 2011/09/07 20:42:57, kmadhusu wrote:
> Move the disableInput..() function call to line #442.

If I disable the controls at line 442, and then the user hits cancel in the file
dialog, I need to re-enable them (this also adds UI clutter/flickering). The
file dialog is modal so when it is shown the controls are not reachable (no
point in disabling them). I believe it is better to keep it as it is now. They
only need to be disabled after the user clicks "save" in the dialog and before
the tab is closed. Let me know if you have any objections.

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview_animations.js (right):

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview_animations.js:105: if
(hasPendingPrintToPdfRequest)
On 2011/09/07 21:38:19, vandebo wrote:
> It's not clear to me if this is needed.  As soon as we have the PDF we're
going
> to close the tab.

This is addressing a corner case, when the user has requested to print to pdf
while metafile is not ready (and therefore the message is shown), and then the
first page comes back. Normally this would hide the overlay layer and display
the first page, but in this case we want to keep displaying "Printing to PDF in
progress". Note that this function is called way before we have the PDF metafile
ready.

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:632: void
PrintPreviewHandler::HandlePrintToPdf(const DictionaryValue* settings) {
On 2011/09/07 21:38:19, vandebo wrote:
> Will this method be called exactly once?  If not, the UMA stuff needs to go
into
> one of the if conditions so that it only gets called once.

Good catch. This function will be called twice in the case of pending print to
pdf requests. Only the second condition is guaranteed to be called once. Moved
UMA code there.

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:639:
PostPrintToPdfTask(*pending_print_to_pdf_path_.get());
On 2011/09/07 21:38:19, vandebo wrote:
> Can you explain how we would get to this point, I don't see an additional call
> in the JS code.).

In chrome/browser/resources/print_preview/print_header.js::onPrintButtonClicked
requestToPrintDocument() is being called. Then, in
print_preview.js::updatePrintPreview requestToPrintDocument() is called for the
2nd time (if there is a pending print request).

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:973: *last_saved_path_ =
path.DirName();
On 2011/09/07 20:42:57, kmadhusu wrote:
> I think you need the save the path name only after we complete the save task.
In
> this function, the user has just selected that path. Preview workflow can fail
> before generating the complete preview document. 

At this point the user has demonstrated that he wants to save to |path|. If the
preview fails he will most probably reopen the page and try to save to the same
path, why have him selecting it again?

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:976:
print_preview_ui->OnFileSelectionCompleted();
On 2011/09/07 20:42:57, kmadhusu wrote:
> You can remove the changes in print_preview_ui.h and .cc and call
> web_ui_->CallJavascript......(); from here.

Done.

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:981:
pending_print_to_pdf_path_.reset(new FilePath(path));
On 2011/09/07 20:42:57, kmadhusu wrote:
> How about renaming pending_print_to_pdf_path_ => print_to_pdf_path_ and to
store
> the user selected path irrespective of whether the data exists or not. By
doing
> that you need not pass "path" parameter in PostPrintToPdfTask()?

Done.

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:981:
pending_print_to_pdf_path_.reset(new FilePath(path));
On 2011/09/07 21:38:19, vandebo wrote:
> On 2011/09/07 20:42:57, kmadhusu wrote:
> > How about renaming pending_print_to_pdf_path_ => print_to_pdf_path_ and to
> store
> > the user selected path irrespective of whether the data exists or not. By
> doing
> > that you need not pass "path" parameter in PostPrintToPdfTask()?
> 
> Then you could also free print_to_pdf_path_ in PostPrintToPdfTask as well.

Done. print_to_pdf_path_ is a scoped_ptr already. Is there a benefit from
releasing the pointer manually?

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:991:
printing::COMPLETE_PREVIEW_DOCUMENT_INDEX, &data);
On 2011/09/07 20:42:57, kmadhusu wrote:
> Add a dcheck to validate data.

Done.

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.h (right):

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.h:14: #include "base/values.h"
On 2011/09/07 21:38:19, vandebo wrote:
> Declare only.

Done.

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.h:25: class StringValue;
On 2011/09/07 20:42:57, kmadhusu wrote:
> Do a forward declaration for DictionaryValue and move #include "base/values.h"
> to .cc file.

Thanks for catching! Done.

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.h:90: void HandlePrintToPdf(const
DictionaryValue* settings);
On 2011/09/07 20:42:57, kmadhusu wrote:
> Why do you need a pointer here? Pass it as a reference.

Done.

[kmadhusu, 2011-09-08 00:04:33]

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:552:
disableInputElementsInSidebar();
On 2011/09/07 23:42:39, dpapad wrote:
> On 2011/09/07 20:42:57, kmadhusu wrote:
> > Move the disableInput..() function call to line #442.
> 
> If I disable the controls at line 442, and then the user hits cancel in the
file
> dialog, I need to re-enable them (this also adds UI clutter/flickering). The
> file dialog is modal so when it is shown the controls are not reachable (no
> point in disabling them). I believe it is better to keep it as it is now. They
> only need to be disabled after the user clicks "save" in the dialog and before
> the tab is closed. Let me know if you have any objections.

I am not sure about the time it takes to show the "Save As.." dialog after the
print button is clicked. If that is instant, this is fine. If not, we need to
fix this. I dont want the user to change the settings after the print button is
clicked and before the file dialog is displayed. 

If the file selection is cancelled, we can re-enable the controls in 
fileSelectionCancelled() function. Why do you think the re-enable of controls
will cause a flickering? It should not be a problem.

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:973: *last_saved_path_ =
path.DirName();
On 2011/09/07 23:42:39, dpapad wrote:
> On 2011/09/07 20:42:57, kmadhusu wrote:
> > I think you need the save the path name only after we complete the save
task.
> In
> > this function, the user has just selected that path. Preview workflow can
fail
> > before generating the complete preview document. 
> 
> At this point the user has demonstrated that he wants to save to |path|. If
the
> preview fails he will most probably reopen the page and try to save to the
same
> path, why have him selecting it again?

ok.. leave the code as it is.

http://codereview.chromium.org/7792085/diff/24010/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview_animations.js (right):

http://codereview.chromium.org/7792085/diff/24010/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview_animations.js:102: * Hides
the |overlay-layer| and any messages curretnly displayed.
typo: currently

[dpapad, 2011-09-08 00:11:51]

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:552:
disableInputElementsInSidebar();
On 2011/09/08 00:04:33, kmadhusu wrote:
> On 2011/09/07 23:42:39, dpapad wrote:
> > On 2011/09/07 20:42:57, kmadhusu wrote:
> > > Move the disableInput..() function call to line #442.
> > 
> > If I disable the controls at line 442, and then the user hits cancel in the
> file
> > dialog, I need to re-enable them (this also adds UI clutter/flickering). The
> > file dialog is modal so when it is shown the controls are not reachable (no
> > point in disabling them). I believe it is better to keep it as it is now.
They
> > only need to be disabled after the user clicks "save" in the dialog and
before
> > the tab is closed. Let me know if you have any objections.
> 
> I am not sure about the time it takes to show the "Save As.." dialog after the
> print button is clicked. If that is instant, this is fine. If not, we need to
> fix this. I dont want the user to change the settings after the print button
is
> clicked and before the file dialog is displayed. 
> 
> If the file selection is cancelled, we can re-enable the controls in 
> fileSelectionCancelled() function. Why do you think the re-enable of controls
> will cause a flickering? It should not be a problem.

The main point of this CL is to make opening the file dialog instant and hiding
implementation details from the user. It is instant as far as my eye can tell.
Pls try locally and confirm.

When disabling the controls they become grayed out (and vice versa when
enabling) which in the past has been perceived as flickering from the UI leads
and from other users, so it is best to avoid it if not absolutely necessary.

http://codereview.chromium.org/7792085/diff/24010/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview_animations.js (right):

http://codereview.chromium.org/7792085/diff/24010/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview_animations.js:102: * Hides
the |overlay-layer| and any messages curretnly displayed.
On 2011/09/08 00:04:33, kmadhusu wrote:
> typo: currently

Done.

[vandebo (ex-Chrome), 2011-09-08 00:22:04]

C++ LGTM

[dpapad, 2011-09-08 03:29:20]

Addressed arv's comments and kmadhusu's pending comment.

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.html (right):

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.html:63: <div
id="custom-message" class="hidden"></div>
On 2011/09/07 21:53:52, arv wrote:
> Please use hidden instead.
> 
> <div id="custom-message" hidden></div>
> 

Done.

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:78: var
hasPendingPrintToPdfRequest = false;
On 2011/09/07 23:42:39, dpapad wrote:
> On 2011/09/07 20:42:57, kmadhusu wrote:
> > This variable is not required. You can reuse 
"hasPendingPrintDocumentRequest"
> > to achieve the same thing. Please correct me if I am wrong. 
> 
> I am using this variable so that I can keep displaying the "Printing to Pdf"
> message, when the first page arrives (if necessary), otherwise the page would
be
> displayed.
> It seems that you are right, but want to try some cases to make sure it is
safe
> to do this. Will address this along with my next batch of comments (addressing
> also arv's comments).

Done.

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:842:
$('custom-message').innerHTML = errorMessage;
On 2011/09/07 21:53:52, arv wrote:
> Add a var so you do not have to call $ multiple times

Done.

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:842:
$('custom-message').innerHTML = errorMessage;
On 2011/09/07 21:53:52, arv wrote:
> Do you really need innerHTML here? Are you escaping things correctly?
> 
> Maybe use textContent instead?

Done.

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:843:
$('custom-message').classList.remove('hidden');
On 2011/09/07 21:53:52, arv wrote:
> $('custom-message').hidden = false;

Done.

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:844:
$('custom-message-with-dots').innerHTML = '';
On 2011/09/07 21:53:52, arv wrote:
> textContent = ''

In this case innerHTML is needed since the contents have html tags (span).

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview_animations.js (right):

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview_animations.js:86:
$('dancing-dots-text').innerHTML;
On 2011/09/07 21:53:52, arv wrote:
> No, don't use innerHTML like this. There is no point serializing
> #dancing-dots-text to HTML here. Do something like:
> 
> var customMessageWithDots = $('custom-message-with-dots');
> var textNode = document.createTextNode(message);
> customMessageWithDots.insertBefore(textNode,
customMessageWithDots.firstChild);

This results in a larger spacing between the dancing dots and the displayed
message, so I would prefer to keep this as is. There was a similar bug a long
time ago about this (#82965).

http://codereview.chromium.org/7792085/diff/16004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview_animations.js:87:
$('custom-message-with-dots').classList.remove('hidden');
On 2011/09/07 21:53:52, arv wrote:
> hidden = false

Done.

[kmadhusu, 2011-09-08 18:01:47]

http://codereview.chromium.org/7792085/diff/22021/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/7792085/diff/22021/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:611:
HandlePrintToPdf(*settings.get());
HandlePrintToPdf(*settings);

http://codereview.chromium.org/7792085/diff/22021/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:996:
*print_to_pdf_path_.get());
"*print_to_pdf_path_" is sufficient. ".get()" is not required.

[dpapad, 2011-09-08 19:11:33]

Addressed comments.

http://codereview.chromium.org/7792085/diff/22021/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/7792085/diff/22021/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:611:
HandlePrintToPdf(*settings.get());
On 2011/09/08 18:01:47, kmadhusu wrote:
> HandlePrintToPdf(*settings);

Done.

http://codereview.chromium.org/7792085/diff/22021/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:996:
*print_to_pdf_path_.get());
On 2011/09/08 18:01:47, kmadhusu wrote:
> "*print_to_pdf_path_" is sufficient. ".get()" is not required.

Done.

[kmadhusu, 2011-09-08 20:53:17]

LGTM