Assertion removed in print_preview_pdf_generated.cc.

Removed assertion to check if the path that |pdf_file_save_path_| points to exists. This is due to a race condition, where the file doesn't currently exist, but will exist in the future. Asserting to check if it exists isn't appropriate and causes the test to fail when it shouldn't. The problem manifests itself when using multiple test files.

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284835

[Lei Zhang, 2014-07-21 21:35:17]

Maybe just fix the race condition instead of trying to deal with it?

[ivandavid, 2014-07-21 23:58:28]

https://codereview.chromium.org/410473002/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/print_preview/print_preview_ui.h (right):

https://codereview.chromium.org/410473002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/print_preview/print_preview_ui.h:10: #include
"base/callback.h"
This needed to be included. I attempted to do the same thing as base::FilePath,
however the compiler said I was redefining the class.

[Lei Zhang, 2014-07-22 00:11:30]

https://codereview.chromium.org/410473002/diff/40001/chrome/browser/printing/...
File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right):

https://codereview.chromium.org/410473002/diff/40001/chrome/browser/printing/...
chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:189:
EndLoop();
Actually, do you need this EndLoop() call? It seems weird that on the success
code path, EndLoop() gets called twice.

https://codereview.chromium.org/410473002/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/print_preview/print_preview_ui.h (right):

https://codereview.chromium.org/410473002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/print_preview/print_preview_ui.h:10: #include
"base/callback.h"
On 2014/07/21 23:58:28, ivandavid wrote:
> This needed to be included. I attempted to do the same thing as
base::FilePath,
> however the compiler said I was redefining the class.

You need to #include "base/callback_forward.h" instead.

https://codereview.chromium.org/410473002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/print_preview/print_preview_ui.h:163: void
SetCallbackForTesting(const base::Closure* callback);
This name is really vague. There's thousands of callbacks in Chrome for all
kinds of things. What does this callback do exactly?

https://codereview.chromium.org/410473002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/print_preview/print_preview_ui.h:163: void
SetCallbackForTesting(const base::Closure* callback);
Callbacks are passed by const reference, not via pointer.

[ivandavid, 2014-07-22 02:40:26]

https://codereview.chromium.org/410473002/diff/40001/chrome/browser/printing/...
File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right):

https://codereview.chromium.org/410473002/diff/40001/chrome/browser/printing/...
chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:189:
EndLoop();
On 2014/07/22 00:11:30, Lei Zhang wrote:
> Actually, do you need this EndLoop() call? It seems weird that on the success
> code path, EndLoop() gets called twice.
Oooo. I didn't really think about this. I removed Print() and now I only have
the message loop in NavigateAndPreview (now NavigateAndPrint).

https://codereview.chromium.org/410473002/diff/40001/chrome/browser/printing/...
chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:189:
EndLoop();
On 2014/07/22 00:11:30, Lei Zhang wrote:
> Actually, do you need this EndLoop() call? It seems weird that on the success
> code path, EndLoop() gets called twice.

Done.

https://codereview.chromium.org/410473002/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/print_preview/print_preview_ui.h (right):

https://codereview.chromium.org/410473002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/print_preview/print_preview_ui.h:10: #include
"base/callback.h"
On 2014/07/22 00:11:30, Lei Zhang wrote:
> On 2014/07/21 23:58:28, ivandavid wrote:
> > This needed to be included. I attempted to do the same thing as
> base::FilePath,
> > however the compiler said I was redefining the class.
> 
> You need to #include "base/callback_forward.h" instead.

Done.

https://codereview.chromium.org/410473002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/print_preview/print_preview_ui.h:163: void
SetCallbackForTesting(const base::Closure* callback);
On 2014/07/22 00:11:30, Lei Zhang wrote:
> This name is really vague. There's thousands of callbacks in Chrome for all
> kinds of things. What does this callback do exactly?

Done.

https://codereview.chromium.org/410473002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/print_preview/print_preview_ui.h:163: void
SetCallbackForTesting(const base::Closure* callback);
On 2014/07/22 00:11:30, Lei Zhang wrote:
> Callbacks are passed by const reference, not via pointer.

Done.

[Lei Zhang, 2014-07-22 03:04:39]

https://codereview.chromium.org/410473002/diff/100001/chrome/browser/printing...
File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right):

https://codereview.chromium.org/410473002/diff/100001/chrome/browser/printing...
chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:305: // for
all the settings to be set, then the webpage is printed to a PDF.
... then save the preview to PDF.

https://codereview.chromium.org/410473002/diff/100001/chrome/browser/printing...
chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:530:
ASSERT_FALSE(pdf_file_save_path_.empty());
This can't be false, you just appended to it one line above. Rather,
PrintPreviewObserver(), which is the consumer of |pdf_file_save_path_| should
sanity check this.

https://codereview.chromium.org/410473002/diff/100001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right):

https://codereview.chromium.org/410473002/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:287:
pdf_file_saved_callback.Run();
What happens when you save to PDF outside of your test? Does this work when the
callback is "null" ?

https://codereview.chromium.org/410473002/diff/100001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/print_preview/print_preview_ui.h (right):

https://codereview.chromium.org/410473002/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/print_preview/print_preview_ui.h:163: // Sets
|pdf_file_saved_callback_| to |callback|. The callback is used to end
This doesn't make sense in this context. PrintPreviewUI does not have a
|pdf_file_saved_callback_| variable.

https://codereview.chromium.org/410473002/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/print_preview/print_preview_ui.h:164: // the message
loop in PrintPreviewObserver in print_preview_pdf_generated.cc.
print_preview_pdf_generated.cc also doesn't exist, but more over, rather than
describing the fact that it's used in a specific test, describe how tests should
use this in general.

[Lei Zhang, 2014-07-22 03:11:02]

CC: dbeam just FYI.

[ivandavid, 2014-07-22 19:02:01]

Forgot to add comments last time. Here they are.

https://codereview.chromium.org/410473002/diff/100001/chrome/browser/printing...
File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right):

https://codereview.chromium.org/410473002/diff/100001/chrome/browser/printing...
chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:305: // for
all the settings to be set, then the webpage is printed to a PDF.
On 2014/07/22 03:04:39, Lei Zhang wrote:
> ... then save the preview to PDF.

Done.

https://codereview.chromium.org/410473002/diff/100001/chrome/browser/printing...
chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:530:
ASSERT_FALSE(pdf_file_save_path_.empty());
On 2014/07/22 03:04:39, Lei Zhang wrote:
> This can't be false, you just appended to it one line above. Rather,
> PrintPreviewObserver(), which is the consumer of |pdf_file_save_path_| should
> sanity check this.
I moved this to ManipulatePreviewSettings() when |observer_| is in the
kWaitingForFinalMessage state.

https://codereview.chromium.org/410473002/diff/100001/chrome/browser/printing...
chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:530:
ASSERT_FALSE(pdf_file_save_path_.empty());
On 2014/07/22 03:04:39, Lei Zhang wrote:
> This can't be false, you just appended to it one line above. Rather,
> PrintPreviewObserver(), which is the consumer of |pdf_file_save_path_| should
> sanity check this.

Done.

https://codereview.chromium.org/410473002/diff/100001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right):

https://codereview.chromium.org/410473002/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:287:
pdf_file_saved_callback.Run();
On 2014/07/22 03:04:39, Lei Zhang wrote:
> What happens when you save to PDF outside of your test? Does this work when
the
> callback is "null" ?

Done.

https://codereview.chromium.org/410473002/diff/100001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/print_preview/print_preview_ui.h (right):

https://codereview.chromium.org/410473002/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/print_preview/print_preview_ui.h:163: // Sets
|pdf_file_saved_callback_| to |callback|. The callback is used to end
On 2014/07/22 03:04:39, Lei Zhang wrote:
> This doesn't make sense in this context. PrintPreviewUI does not have a
> |pdf_file_saved_callback_| variable.
Guess that's why I shouldn't copy paste comments :|.

https://codereview.chromium.org/410473002/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/print_preview/print_preview_ui.h:163: // Sets
|pdf_file_saved_callback_| to |callback|. The callback is used to end
On 2014/07/22 03:04:39, Lei Zhang wrote:
> This doesn't make sense in this context. PrintPreviewUI does not have a
> |pdf_file_saved_callback_| variable.

Done.

https://codereview.chromium.org/410473002/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/print_preview/print_preview_ui.h:164: // the message
loop in PrintPreviewObserver in print_preview_pdf_generated.cc.
On 2014/07/22 03:04:39, Lei Zhang wrote:
> print_preview_pdf_generated.cc also doesn't exist, but more over, rather than
> describing the fact that it's used in a specific test, describe how tests
should
> use this in general.

Done.

[Lei Zhang, 2014-07-22 21:25:37]

https://codereview.chromium.org/410473002/diff/160001/chrome/browser/printing...
File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right):

https://codereview.chromium.org/410473002/diff/160001/chrome/browser/printing...
chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:197:
ASSERT_FALSE(pdf_file_save_path_.empty());
This looks really weird, since you wedged it between the comment above and the
closure below.

https://codereview.chromium.org/410473002/diff/160001/chrome/browser/printing...
chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:298:
base::FilePath pdf_file_save_path_;
You can make this const.

https://codereview.chromium.org/410473002/diff/160001/chrome/browser/printing...
chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:302: //
variable to prevent the closure from going out of scope.
Does it really not work if you make it a local variable?

https://codereview.chromium.org/410473002/diff/160001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/print_preview/print_preview_ui.h (right):

https://codereview.chromium.org/410473002/diff/160001/chrome/browser/ui/webui...
chrome/browser/ui/webui/print_preview/print_preview_ui.h:163: // Calls
|handler->SetPdfSavedClosureForTesting()|. Allows for tests waiting
Since PrintPreviewUI doesn't actually do anything interesting with |closure|,
just right "Passes |closure| to
PrintPreviewHandler::SetPdfSavedClosureForTesting()."

Then in the PrintPreviewHandler::SetPdfSavedClosureForTesting() comments,
describe what it actually does.

[ivandavid, 2014-07-22 21:46:08]

https://codereview.chromium.org/410473002/diff/160001/chrome/browser/printing...
File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right):

https://codereview.chromium.org/410473002/diff/160001/chrome/browser/printing...
chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:197:
ASSERT_FALSE(pdf_file_save_path_.empty());
On 2014/07/22 21:25:37, Lei Zhang wrote:
> This looks really weird, since you wedged it between the comment above and the
> closure below.
I moved it above the comment.

https://codereview.chromium.org/410473002/diff/160001/chrome/browser/printing...
chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:197:
ASSERT_FALSE(pdf_file_save_path_.empty());
On 2014/07/22 21:25:37, Lei Zhang wrote:
> This looks really weird, since you wedged it between the comment above and the
> closure below.

Done.

https://codereview.chromium.org/410473002/diff/160001/chrome/browser/printing...
chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:298:
base::FilePath pdf_file_save_path_;
On 2014/07/22 21:25:37, Lei Zhang wrote:
> You can make this const.

Done.

https://codereview.chromium.org/410473002/diff/160001/chrome/browser/printing...
chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:302: //
variable to prevent the closure from going out of scope.
On 2014/07/22 21:25:37, Lei Zhang wrote:
> Does it really not work if you make it a local variable?

Done.

https://codereview.chromium.org/410473002/diff/160001/chrome/browser/printing...
chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:302: //
variable to prevent the closure from going out of scope.
On 2014/07/22 21:25:37, Lei Zhang wrote:
> Does it really not work if you make it a local variable?
Oops. It does, it didn't when I used the old implementation, but now it does
since I am no longer using that implementation.

https://codereview.chromium.org/410473002/diff/160001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/print_preview/print_preview_ui.h (right):

https://codereview.chromium.org/410473002/diff/160001/chrome/browser/ui/webui...
chrome/browser/ui/webui/print_preview/print_preview_ui.h:163: // Calls
|handler->SetPdfSavedClosureForTesting()|. Allows for tests waiting
On 2014/07/22 21:25:37, Lei Zhang wrote:
> Since PrintPreviewUI doesn't actually do anything interesting with |closure|,
> just right "Passes |closure| to
> PrintPreviewHandler::SetPdfSavedClosureForTesting()."
> 
> Then in the PrintPreviewHandler::SetPdfSavedClosureForTesting() comments,
> describe what it actually does.

Done.

[Lei Zhang, 2014-07-22 22:02:26]

LGTM with a small code shuffle:

https://codereview.chromium.org/410473002/diff/160001/chrome/browser/printing...
File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right):

https://codereview.chromium.org/410473002/diff/160001/chrome/browser/printing...
chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:197:
ASSERT_FALSE(pdf_file_save_path_.empty());
On 2014/07/22 21:46:07, ivandavid wrote:
> On 2014/07/22 21:25:37, Lei Zhang wrote:
> > This looks really weird, since you wedged it between the comment above and
the
> > closure below.
> I moved it above the comment.

Why not put it right before |pdf_file_save_path_| is actually used. Right now
you have:

- pdf_file_save_path_ code
- end_loop_closure code
- pdf_file_save_path_ code

[ivandavid, 2014-07-22 22:07:44]

The CQ bit was checked by ivandavid@chromium.org

[ivandavid, 2014-07-22 22:08:10]

https://codereview.chromium.org/410473002/diff/160001/chrome/browser/printing...
File chrome/browser/printing/print_preview_pdf_generated_browsertest.cc (right):

https://codereview.chromium.org/410473002/diff/160001/chrome/browser/printing...
chrome/browser/printing/print_preview_pdf_generated_browsertest.cc:197:
ASSERT_FALSE(pdf_file_save_path_.empty());
On 2014/07/22 22:02:26, Lei Zhang wrote:
> On 2014/07/22 21:46:07, ivandavid wrote:
> > On 2014/07/22 21:25:37, Lei Zhang wrote:
> > > This looks really weird, since you wedged it between the comment above and
> the
> > > closure below.
> > I moved it above the comment.
> 
> Why not put it right before |pdf_file_save_path_| is actually used. Right now
> you have:
> 
> - pdf_file_save_path_ code
> - end_loop_closure code
> - pdf_file_save_path_ code

Done.

[commit-bot: I haz the power, 2014-07-22 22:08:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ivandavid@chromium.org/410473002/220001

[commit-bot: I haz the power, 2014-07-23 02:04:30]

Change committed as 284835