Added basic browser test for window.print() from Chrome apps.

Added basic browser test for window.print() from Chrome apps.

This just tests the success of the call to window.print(). It does not
test whether the call leads to a print preview window and ultimately to
a printout. It will be extended over time to provide additional coverage
of the print functionality.

BUG=NONE

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

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

[dharcourt, 2013-08-13 12:30:09]

Vitaly, could you review this initial window.print()-for-Chrome-apps browser
test when you get a chance?

Antony, could you provide an OWNERS review of platform_app_browsertest.cc?

Thanks!

    - Charles

[asargent_no_longer_on_chrome, 2013-08-13 22:38:54]

lgtm

https://codereview.chromium.org/23054002/diff/1/chrome/test/data/extensions/p...
File chrome/test/data/extensions/platform_apps/print_api/test.js (right):

https://codereview.chromium.org/23054002/diff/1/chrome/test/data/extensions/p...
chrome/test/data/extensions/platform_apps/print_api/test.js:9:
chrome.test.notifyPass();
nit: I don't think you need this final call here - each invocation of
callbackPass should increment the counter of "pending callbacks that need to not
fail for the overall test to succeed", so just exiting out of this scope without
failing should be sufficient (I think).

[dharcourt, 2013-08-13 23:06:30]

https://codereview.chromium.org/23054002/diff/1/chrome/test/data/extensions/p...
File chrome/test/data/extensions/platform_apps/print_api/test.js (right):

https://codereview.chromium.org/23054002/diff/1/chrome/test/data/extensions/p...
chrome/test/data/extensions/platform_apps/print_api/test.js:9:
chrome.test.notifyPass();
Fixed, thanks! I thought I had tried that earlier but it must have been before I
was using callbackPass() correctly because it wasn't working then. Now it is
:-).

On 2013/08/13 22:38:54, Antony Sargent wrote:
> nit: I don't think you need this final call here - each invocation of
> callbackPass should increment the counter of "pending callbacks that need to
not
> fail for the overall test to succeed", so just exiting out of this scope
without
> failing should be sufficient (I think).

[Vitaly Buka (NO REVIEWS), 2013-08-15 03:18:18]

lgtm

[commit-bot: I haz the power, 2013-08-19 17:13:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dharcourt@chromium.org/23054002/19001

[commit-bot: I haz the power, 2013-08-19 17:13:22]

Failed to apply patch for chrome/browser/extensions/platform_app_browsertest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: chrome/browser/extensions/platform_app_browsertest.cc
  |diff --git a/chrome/browser/extensions/platform_app_browsertest.cc
b/chrome/browser/extensions/platform_app_browsertest.cc
  |index
81269887287dcfd86b30ba7cb26a2e5b6e36faa2..c95a5d4cf0f679eb40412d58572dd308804c0e3c
100644
  |--- a/chrome/browser/extensions/platform_app_browsertest.cc
  |+++ b/chrome/browser/extensions/platform_app_browsertest.cc
  --------------------------
  No file to patch.  Skipping patch.
  1 out of 1 hunk ignored

Patch:       chrome/browser/extensions/platform_app_browsertest.cc
Index: chrome/browser/extensions/platform_app_browsertest.cc
diff --git a/chrome/browser/extensions/platform_app_browsertest.cc
b/chrome/browser/extensions/platform_app_browsertest.cc
index
81269887287dcfd86b30ba7cb26a2e5b6e36faa2..c95a5d4cf0f679eb40412d58572dd308804c0e3c
100644
--- a/chrome/browser/extensions/platform_app_browsertest.cc
+++ b/chrome/browser/extensions/platform_app_browsertest.cc
@@ -1023,6 +1023,10 @@ IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest,
MAYBE_WebContentsHasFocus) {
       GetRenderWidgetHostView()->HasFocus());
 }
 
+IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest, WindowDotPrintWorks) {
+  ASSERT_TRUE(RunPlatformAppTest("platform_apps/print_api")) << message_;
+}
+
 
 #if defined(OS_CHROMEOS)

[commit-bot: I haz the power, 2013-08-19 18:22:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dharcourt@chromium.org/23054002/26001

[commit-bot: I haz the power, 2013-08-19 18:23:08]

Failed to apply patch for chrome/browser/extensions/platform_app_browsertest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: chrome/browser/extensions/platform_app_browsertest.cc
  |diff --git a/chrome/browser/extensions/platform_app_browsertest.cc
b/chrome/browser/extensions/platform_app_browsertest.cc
  |index
81269887287dcfd86b30ba7cb26a2e5b6e36faa2..c95a5d4cf0f679eb40412d58572dd308804c0e3c
100644
  |--- a/chrome/browser/extensions/platform_app_browsertest.cc
  |+++ b/chrome/browser/extensions/platform_app_browsertest.cc
  --------------------------
  No file to patch.  Skipping patch.
  1 out of 1 hunk ignored

Patch:       chrome/browser/extensions/platform_app_browsertest.cc
Index: chrome/browser/extensions/platform_app_browsertest.cc
diff --git a/chrome/browser/extensions/platform_app_browsertest.cc
b/chrome/browser/extensions/platform_app_browsertest.cc
index
81269887287dcfd86b30ba7cb26a2e5b6e36faa2..c95a5d4cf0f679eb40412d58572dd308804c0e3c
100644
--- a/chrome/browser/extensions/platform_app_browsertest.cc
+++ b/chrome/browser/extensions/platform_app_browsertest.cc
@@ -1023,6 +1023,10 @@ IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest,
MAYBE_WebContentsHasFocus) {
       GetRenderWidgetHostView()->HasFocus());
 }
 
+IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest, WindowDotPrintWorks) {
+  ASSERT_TRUE(RunPlatformAppTest("platform_apps/print_api")) << message_;
+}
+
 
 #if defined(OS_CHROMEOS)

[commit-bot: I haz the power, 2013-08-19 21:51:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dharcourt@chromium.org/23054002/34001

[commit-bot: I haz the power, 2013-08-19 22:00:08]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-08-19 22:21:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dharcourt@chromium.org/23054002/50001

[commit-bot: I haz the power, 2013-08-20 01:03:47]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[dharcourt, 2013-08-28 00:59:56]

Hi Vitaly,

This is a proposal for how to implement these tests with the print preview panel
auto-canceling, something that is required to prevent the tests from timing out.
Please let me know if there's another way you'd prefer to see this auto-cancel
implemented.

Antony, thank for your help with this and thanks for the review.

    - Charles

[Vitaly Buka (NO REVIEWS), 2013-08-28 01:20:26]

https://codereview.chromium.org/23054002/diff/109001/chrome/browser/apps/app_...
File chrome/browser/apps/app_browsertest.cc (right):

https://codereview.chromium.org/23054002/diff/109001/chrome/browser/apps/app_...
chrome/browser/apps/app_browsertest.cc:1033: #if !defined(GOOGLE_CHROME_BUILD)
ifdef could be removed

https://codereview.chromium.org/23054002/diff/109001/chrome/browser/apps/app_...
chrome/browser/apps/app_browsertest.cc:1056:
ASSERT_TRUE(RunPlatformAppTest("platform_apps/print_api")) << message_;
without auto close, where exactly tests timeouts? inside of RunPlatformAppTest
or after

https://codereview.chromium.org/23054002/diff/109001/chrome/browser/apps/app_...
chrome/browser/apps/app_browsertest.cc:1057:
PrintPreviewUI::SetAutoCancelForTesting(false);
Probably you can avoid static methods using if you find UI using code:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

https://codereview.chromium.org/23054002/diff/109001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/print_preview/print_preview_ui.cc (right):

https://codereview.chromium.org/23054002/diff/109001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/print_preview/print_preview_ui.cc:590: static bool
auto_cancel_for_testing_ = false;
auto_cancel_for_testing_ -> g_auto_cancel_for_testing

[dharcourt, 2013-08-28 23:23:57]

Here's a slightly different alternative that avoid public test methods at the
expense of  moving the test outside of any namespace and making it a friend of
PrintPreviewUI.

Vitaly, does that look any better to you? Antony, is it OK to have the test
outside any namespace?

Unfortunately Rietveld got confused by the test move in app_browsertest.cc and
shows more differences there than there really is. You may want to view this
patch relative to the trunk code rather than relative to the previous patch.

Thanks for the review!

    - Charles

https://codereview.chromium.org/23054002/diff/109001/chrome/browser/apps/app_...
File chrome/browser/apps/app_browsertest.cc (right):

https://codereview.chromium.org/23054002/diff/109001/chrome/browser/apps/app_...
chrome/browser/apps/app_browsertest.cc:1033: #if !defined(GOOGLE_CHROME_BUILD)
On 2013/08/28 01:20:26, Vitaly Buka wrote:
> ifdef could be removed

Done.

https://codereview.chromium.org/23054002/diff/109001/chrome/browser/apps/app_...
chrome/browser/apps/app_browsertest.cc:1056:
ASSERT_TRUE(RunPlatformAppTest("platform_apps/print_api")) << message_;
On 2013/08/28 01:20:26, Vitaly Buka wrote:
> without auto close, where exactly tests timeouts? inside of RunPlatformAppTest
> or after

Inside RunPlatformAppTest, which never returns if the print panel is up.

FWIW, the timeouts were not occurring with the initial version of this change
list because the test JavaScript was explicitly calling chrome.test.notifyPass()
in that version, and that worked around the issue. Unfortunately
chrome.test.notifyPass() is not designed to be called this way and that version
of the test was susceptible to false negatives. Fixing that brought up the
timeout issue.

https://codereview.chromium.org/23054002/diff/109001/chrome/browser/apps/app_...
chrome/browser/apps/app_browsertest.cc:1057:
PrintPreviewUI::SetAutoCancelForTesting(false);
On 2013/08/28 01:20:26, Vitaly Buka wrote:
> Probably you can avoid static methods using if you find UI using code:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

Unfortunately I don't think I can use the same method here because the print
preview dialog gets created by JavaScript in the test app called by
RunPlatformAppTest() and so it doesn't exist at the time I want to set it to
auto-cancel. Am I missing something?

I've tried an alternative that uses FRIEND_TEST_ALL_PREFIXES() and private
static methods... is that any better, was the previous version better, or is
there another better way to do this?

https://codereview.chromium.org/23054002/diff/109001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/print_preview/print_preview_ui.cc (right):

https://codereview.chromium.org/23054002/diff/109001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/print_preview/print_preview_ui.cc:590: static bool
auto_cancel_for_testing_ = false;
On 2013/08/28 01:20:26, Vitaly Buka wrote:
> auto_cancel_for_testing_ -> g_auto_cancel_for_testing

Done, thanks.

[Vitaly Buka (NO REVIEWS), 2013-08-29 00:10:24]

https://chromiumcodereview.appspot.com/23054002/diff/109001/chrome/browser/ui...
File chrome/browser/ui/webui/print_preview/print_preview_ui.cc (right):

https://chromiumcodereview.appspot.com/23054002/diff/109001/chrome/browser/ui...
chrome/browser/ui/webui/print_preview/print_preview_ui.cc:523: if
(AutoCancelForTesting()) {
Is possible to avoid js chages and just call OnHidePreviewDialog() here?

[dharcourt, 2013-08-29 04:02:02]

I removed the changes to the print preview JavaScript code, simplifying things.

The only remaining question may be whether to use public "ForTesting" methods in
PrintPreviewUI (as in patch 6) or whether to have those methods private and use
FRIEND_TEST_ALL_PREFIXES in combination with namespace-less tests (as in this
patch). Thoughts?

Thanks for the review,

    - Charles

https://codereview.chromium.org/23054002/diff/109001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/print_preview/print_preview_ui.cc (right):

https://codereview.chromium.org/23054002/diff/109001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/print_preview/print_preview_ui.cc:523: if
(AutoCancelForTesting()) {
On 2013/08/29 00:10:25, Vitaly Buka wrote:
> Is possible to avoid js chages and just call OnHidePreviewDialog() here?

Calling OnHidePreviewDialog() doesn't work (the tests time out), but calling
OnClosePrintPreviewDialog() does :-).

I followed the code and the only significant thing that print_preview.js's
close_() does in the context of the test is OnClosePrintPreviewDialog(), so I
was able to back out all JavaScript changes. Thanks!

[Vitaly Buka (NO REVIEWS), 2013-08-29 04:41:16]

lgtm

Thanks. Regarding FRIEND, i am OK with either approach.

[dharcourt, 2013-08-29 20:47:33]

On 2013/08/29 04:41:16, Vitaly Buka wrote:
> lgtm
> 
> Thanks. Regarding FRIEND, i am OK with either approach.

Great. FYI, I reverted to the non-friend approach as Antony pointer out
correctly this prevents the testing class from having access to anything private
in PrintPreviewUI other than the auto-cancel flag. Non-test classes are
prevented from accessing the auto-cancel flag by presubmit checks than disallow
calling *ForTesting() methods from non-test classes.

I will be committing after this final change, thanks for the review!

    - Charles

[Vitaly Buka (NO REVIEWS), 2013-08-29 20:49:11]

lgtm

[commit-bot: I haz the power, 2013-08-29 20:51:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dharcourt@chromium.org/23054002/90009

[commit-bot: I haz the power, 2013-08-29 22:01:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dharcourt@chromium.org/23054002/140001

[commit-bot: I haz the power, 2013-08-30 01:19:37]

Change committed as 220467

[commit-bot: I haz the power, 2013-08-30 03:29:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dharcourt@chromium.org/23054002/104001

[commit-bot: I haz the power, 2013-08-30 09:10:34]

Change committed as 220542