Fix WindowdotPrintShouldBringUpPrintPreview

Fix WindowdotPrintShouldBringUpPrintPreview

Fix this test by changing auto_cancel to false. The delegate was
waiting for the preview to be ready but cancel was being clicked in
javascript before the preview finished so the delegate never stopped.

Also move to base::RunLoop instead of content::MessageLoopRunner
since MessageLoopRunner is deprecated per comment in test_utils.h.

BUG=312897
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2683653005
Cr-Commit-Position: refs/heads/master@{#449430}
Committed: https://chromium.googlesource.com/chromium/src/+/1c04025f853bc892cb4064cc5c7711bd33facbd6

[rbpotter, 2017-02-08 23:41:36]

The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 23:42:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-09 00:28:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-09 00:28:15]

Dry run: This issue passed the CQ dry run.

[rbpotter, 2017-02-09 00:37:46]

Description was changed from

==========
Fix WindowdotPrintShouldBringUpPrintPreview

Fix this test by changing auto_cancel to false. The delegate was
waiting for the preview to be ready but cancel was being clicked in
javascript before the preview finished, causing a time out.

Also move to base::RunLoop instead of MessageLoop since MessageLoop is
deprecated.

BUG=312897
==========

to

==========
Fix WindowdotPrintShouldBringUpPrintPreview

Fix this test by changing auto_cancel to false. The delegate was
waiting for the preview to be ready but cancel was being clicked in
javascript before the preview finished, causing a time out.

Also move to base::RunLoop instead of content::MessageLoopRunner
since MessageLoopRunner is deprecated per comment in test_utils.h.

BUG=312897
==========

[rbpotter, 2017-02-09 00:40:24]

Description was changed from

==========
Fix WindowdotPrintShouldBringUpPrintPreview

Fix this test by changing auto_cancel to false. The delegate was
waiting for the preview to be ready but cancel was being clicked in
javascript before the preview finished, causing a time out.

Also move to base::RunLoop instead of content::MessageLoopRunner
since MessageLoopRunner is deprecated per comment in test_utils.h.

BUG=312897
==========

to

==========
Fix WindowdotPrintShouldBringUpPrintPreview

Fix this test by changing auto_cancel to false. The delegate was
waiting for the preview to be ready but cancel was being clicked in
javascript before the preview finished so the delegate never stopped.

Also move to base::RunLoop instead of content::MessageLoopRunner
since MessageLoopRunner is deprecated per comment in test_utils.h.

BUG=312897
==========

[rbpotter, 2017-02-09 00:40:24]

rbpotter@chromium.org changed reviewers:
+ tapted@chromium.org

[rbpotter, 2017-02-09 00:44:07]

Small fix of a disabled/previously failing printing test in
app_browsertest.

[tapted, 2017-02-09 01:10:02]

https://codereview.chromium.org/2683653005/diff/1/chrome/browser/apps/app_bro...
File chrome/browser/apps/app_browsertest.cc (right):

https://codereview.chromium.org/2683653005/diff/1/chrome/browser/apps/app_bro...
chrome/browser/apps/app_browsertest.cc:45: #include
"content/public/test/test_utils.h"
is one or both of these content..test_utils.h headers now unused with
content::MessageLoopRunner gone?

https://codereview.chromium.org/2683653005/diff/1/chrome/browser/apps/app_bro...
chrome/browser/apps/app_browsertest.cc:156: CHECK(!run_loop_);
This is no longer being cleared after removing `waiting_runner_ = NULL;` - was
that intentional?. I think a neater way to do this is to declare run_loop_ as a
raw pointer.

base::RunLoop* run_loop_ = nullptr;

then this method is

void WaitUntilPreviewIsReady() {
  if (rendered_page_count_ >= total_page_count_)
    return;

  base::RunLoop run_loop;
  base::AutoReset<RunLoop> auto_reset(&run_loop_, &run_loop);
  run_loop.Run();
}


(this restructure is an optional nit though - I don't have strong feelings -
that's just how I'd write it ;)

https://codereview.chromium.org/2683653005/diff/1/chrome/browser/apps/app_bro...
chrome/browser/apps/app_browsertest.cc:1142: ScopedPreviewTestingDelegate
preview_delegate(false);
in the bug you say this was the only test passing |true|, so can the bool
argument just be removed?

[rbpotter, 2017-02-09 01:35:49]

Description was changed from

==========
Fix WindowdotPrintShouldBringUpPrintPreview

Fix this test by changing auto_cancel to false. The delegate was
waiting for the preview to be ready but cancel was being clicked in
javascript before the preview finished so the delegate never stopped.

Also move to base::RunLoop instead of content::MessageLoopRunner
since MessageLoopRunner is deprecated per comment in test_utils.h.

BUG=312897
==========

to

==========
Fix WindowdotPrintShouldBringUpPrintPreview

Fix this test by changing auto_cancel to false. The delegate was
waiting for the preview to be ready but cancel was being clicked in
javascript before the preview finished so the delegate never stopped.

Also move to base::RunLoop instead of content::MessageLoopRunner
since MessageLoopRunner is deprecated per comment in test_utils.h.

BUG=312897
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[rbpotter, 2017-02-09 01:36:11]

The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-09 01:36:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rbpotter, 2017-02-09 01:56:40]

rbpotter@chromium.org changed reviewers:
+ dpapad@chromium.org, vitalybuka@chromium.org

[rbpotter, 2017-02-09 01:56:41]

Addressed comments.

+printing reviewers vitalybuka@ and dpapad@ for
chrome/browser/ui/webui/print_preview and
chrome/browser/resources/print_preview. Is there any
reason we need to keep the auto cancel code path that is now
completely unreferenced? It has been unused for a while as
this test was the only thing that used it and has been
disabled for some time.

https://codereview.chromium.org/2683653005/diff/1/chrome/browser/apps/app_bro...
File chrome/browser/apps/app_browsertest.cc (right):

https://codereview.chromium.org/2683653005/diff/1/chrome/browser/apps/app_bro...
chrome/browser/apps/app_browsertest.cc:45: #include
"content/public/test/test_utils.h"
On 2017/02/09 01:10:02, tapted wrote:
> is one or both of these content..test_utils.h headers now unused with
> content::MessageLoopRunner gone?

Done.

https://codereview.chromium.org/2683653005/diff/1/chrome/browser/apps/app_bro...
chrome/browser/apps/app_browsertest.cc:156: CHECK(!run_loop_);
On 2017/02/09 01:10:02, tapted wrote:
> This is no longer being cleared after removing `waiting_runner_ = NULL;` - was
> that intentional?. I think a neater way to do this is to declare run_loop_ as
a
> raw pointer.
> 
> base::RunLoop* run_loop_ = nullptr;
> 
> then this method is
> 
> void WaitUntilPreviewIsReady() {
>   if (rendered_page_count_ >= total_page_count_)
>     return;
> 
>   base::RunLoop run_loop;
>   base::AutoReset<RunLoop> auto_reset(&run_loop_, &run_loop);
>   run_loop.Run();
> }
> 
> 
> (this restructure is an optional nit though - I don't have strong feelings -
> that's just how I'd write it ;)

Done. Thanks for the suggestion.

https://codereview.chromium.org/2683653005/diff/1/chrome/browser/apps/app_bro...
chrome/browser/apps/app_browsertest.cc:1142: ScopedPreviewTestingDelegate
preview_delegate(false);
On 2017/02/09 01:10:02, tapted wrote:
> in the bug you say this was the only test passing |true|, so can the bool
> argument just be removed?

Done.

[tapted, 2017-02-09 02:03:02]

c/b/apps lgtm - thanks!

https://codereview.chromium.org/2683653005/diff/20001/chrome/browser/apps/app...
File chrome/browser/apps/app_browsertest.cc (right):

https://codereview.chromium.org/2683653005/diff/20001/chrome/browser/apps/app...
chrome/browser/apps/app_browsertest.cc:165: int total_page_count_;
nit: move the = 1 down here (and = 0 below) rather than doing some stuff in the
constructor

[Vitaly Buka (NO REVIEWS), 2017-02-09 02:44:22]

lgtm

[commit-bot: I haz the power, 2017-02-09 05:50:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-09 05:50:31]

Dry run: This issue passed the CQ dry run.

[dpapad, 2017-02-09 18:32:25]

lgtm

[rbpotter, 2017-02-09 19:36:15]

The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-09 19:36:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rbpotter, 2017-02-09 20:11:47]

https://codereview.chromium.org/2683653005/diff/20001/chrome/browser/apps/app...
File chrome/browser/apps/app_browsertest.cc (right):

https://codereview.chromium.org/2683653005/diff/20001/chrome/browser/apps/app...
chrome/browser/apps/app_browsertest.cc:165: int total_page_count_;
On 2017/02/09 02:03:02, tapted wrote:
> nit: move the = 1 down here (and = 0 below) rather than doing some stuff in
the
> constructor

Done.

[commit-bot: I haz the power, 2017-02-09 21:45:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-09 21:45:59]

Dry run: This issue passed the CQ dry run.

[rbpotter, 2017-02-09 21:54:12]

The CQ bit was checked by rbpotter@chromium.org

[rbpotter, 2017-02-09 21:54:12]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org,
dpapad@chromium.org, vitalybuka@chromium.org
Link to the patchset: https://codereview.chromium.org/2683653005/#ps40001
(title: "Move inits")

[commit-bot: I haz the power, 2017-02-09 21:54:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-09 21:59:34]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486677252363970,
"parent_rev": "c466e2e9ccd59d341410371c774662eab4d0a755", "commit_rev":
"1c04025f853bc892cb4064cc5c7711bd33facbd6"}

[commit-bot: I haz the power, 2017-02-09 22:00:09]

Description was changed from

==========
Fix WindowdotPrintShouldBringUpPrintPreview

Fix this test by changing auto_cancel to false. The delegate was
waiting for the preview to be ready but cancel was being clicked in
javascript before the preview finished so the delegate never stopped.

Also move to base::RunLoop instead of content::MessageLoopRunner
since MessageLoopRunner is deprecated per comment in test_utils.h.

BUG=312897
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Fix WindowdotPrintShouldBringUpPrintPreview

Fix this test by changing auto_cancel to false. The delegate was
waiting for the preview to be ready but cancel was being clicked in
javascript before the preview finished so the delegate never stopped.

Also move to base::RunLoop instead of content::MessageLoopRunner
since MessageLoopRunner is deprecated per comment in test_utils.h.

BUG=312897
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2683653005
Cr-Commit-Position: refs/heads/master@{#449430}
Committed:
https://chromium.googlesource.com/chromium/src/+/1c04025f853bc892cb4064cc5c77...
==========

[commit-bot: I haz the power, 2017-02-09 22:00:11]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/1c04025f853bc892cb4064cc5c77...

[Lei Zhang, 2017-02-25 01:33:29]

thestig@chromium.org changed reviewers:
+ thestig@chromium.org

[Lei Zhang, 2017-02-25 01:33:29]

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

https://codereview.chromium.org/2683653005/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/print_preview/print_preview_ui.h:151: // also instructs
the dialog to auto-cancel, which is used for testing only.
Comment's slightly out of date since we remove IsAutoCancelEnabled().