Print Preview: Merge NativeLayerStubs for tests

Print Preview: Merge NativeLayerStubs for tests

Move NativeLayerStub to a separate file and use the same stub for both
the PrintPreviewDestinationSearchTests and the PrintPreviewWebUITests.

Also move all print_preview related tests into a subfolder of
chrome/test/data/webui/ with the new native_layer_stub.js file.

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

Review-Url: https://codereview.chromium.org/2893003003
Cr-Commit-Position: refs/heads/master@{#473738}
Committed: https://chromium.googlesource.com/chromium/src/+/7d5b724780d484c91a46232ec2fd6c51e0d15f37

[rbpotter, 2017-05-19 19:32:21]

Description was changed from

==========
Print Preview: Factor out NativeLayerStub

Move NativeLayerStub to a separate file and use the same stub for both
the PrintPreviewDestinationSearchTests and the PrintPreviewWebUITests.

BUG=717296
==========

to

==========
Print Preview: Factor out NativeLayerStub

Move NativeLayerStub to a separate file and use the same stub for both
the PrintPreviewDestinationSearchTests and the PrintPreviewWebUITests.

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

[rbpotter, 2017-05-19 19:36:02]

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

[rbpotter, 2017-05-19 19:36:48]

Description was changed from

==========
Print Preview: Factor out NativeLayerStub

Move NativeLayerStub to a separate file and use the same stub for both
the PrintPreviewDestinationSearchTests and the PrintPreviewWebUITests.

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

to

==========
Print Preview: Factor out NativeLayerStub

Move NativeLayerStub to a separate file and use the same stub for both
the PrintPreviewDestinationSearchTests and the PrintPreviewWebUITests.

Also move all print_preview related tests into a subfolder of
chrome/test/data/webui/ with the new native_layer_stub.js file.

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

[commit-bot: I haz the power, 2017-05-19 19:37:40]

Dry run: CQ is trying da patch.

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

[rbpotter, 2017-05-19 19:39:13]

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

[commit-bot: I haz the power, 2017-05-19 19:53:58]

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

[commit-bot: I haz the power, 2017-05-19 19:54:00]

Dry run: Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

[rbpotter, 2017-05-19 20:34:11]

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

[commit-bot: I haz the power, 2017-05-19 20:35:28]

Dry run: CQ is trying da patch.

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

[rbpotter, 2017-05-19 20:35:38]

Description was changed from

==========
Print Preview: Factor out NativeLayerStub

Move NativeLayerStub to a separate file and use the same stub for both
the PrintPreviewDestinationSearchTests and the PrintPreviewWebUITests.

Also move all print_preview related tests into a subfolder of
chrome/test/data/webui/ with the new native_layer_stub.js file.

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

to

==========
Print Preview: Merge NativeLayerStubs for tests

Move NativeLayerStub to a separate file and use the same stub for both
the PrintPreviewDestinationSearchTests and the PrintPreviewWebUITests.

Also move all print_preview related tests into a subfolder of
chrome/test/data/webui/ with the new native_layer_stub.js file.

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

[commit-bot: I haz the power, 2017-05-19 20:39:22]

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

[commit-bot: I haz the power, 2017-05-19 20:39:23]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dpapad, 2017-05-19 20:59:29]

https://codereview.chromium.org/2893003003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/print_preview/native_layer.js (right):

https://codereview.chromium.org/2893003003/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/native_layer.js:83: /** @private
{print_preview.NativeLayer} */
?print_preview.NativeLayer

https://codereview.chromium.org/2893003003/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/native_layer.js:87: * @return
{!print_preview.NativeLayer} an instance of NativeLayer.
Nit:
"an instance of NativeLayer." -> "The singleton instance."

https://codereview.chromium.org/2893003003/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/native_layer.js:91: if (currentInstance)
This condition should be reversed I think. Also let's compare to null directly.

https://codereview.chromium.org/2893003003/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/BUILD.gn (right):

https://codereview.chromium.org/2893003003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/BUILD.gn:153: sources -= [
"print_preview/print_preview.js" ]
Should we also be excluding print_preview_destination_search_test.js in this
case?

Regardless of the answer to the above, can we remove print_preview.js from line
102 above, and reverse the logic here?

if (enable_print_preview) {
  sources += [...]
}

https://codereview.chromium.org/2893003003/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/print_preview/print_preview.h (right):

https://codereview.chromium.org/2893003003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/print_preview/print_preview.h:5: #ifndef
CHROME_TEST_DATA_WEBUI_PRINT_PREVIEW_PRINT_PREVIEW_H_
Is this file (and the corresponding cc file) necessary at all? Is it used
somewhere?

AFAIU chrome/test/data/webui/print_preview/print_preview.js automatically
generates a corresponding C++ file at
out/<out_foder>/gen/chrome/test/data/webui/print_preview-gen.cc

[rbpotter, 2017-05-20 00:34:53]

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

[commit-bot: I haz the power, 2017-05-20 00:35:29]

Dry run: CQ is trying da patch.

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

[rbpotter, 2017-05-20 00:43:59]

Plan to rebase this once
https://codereview.chromium.org/2894243004/ lands.

https://codereview.chromium.org/2893003003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/print_preview/native_layer.js (right):

https://codereview.chromium.org/2893003003/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/native_layer.js:83: /** @private
{print_preview.NativeLayer} */
On 2017/05/19 20:59:29, dpapad wrote:
> ?print_preview.NativeLayer

Done.

https://codereview.chromium.org/2893003003/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/native_layer.js:87: * @return
{!print_preview.NativeLayer} an instance of NativeLayer.
On 2017/05/19 20:59:29, dpapad wrote:
> Nit:
> "an instance of NativeLayer." -> "The singleton instance."

Done.

https://codereview.chromium.org/2893003003/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/native_layer.js:91: if (currentInstance)
On 2017/05/19 20:59:29, dpapad wrote:
> This condition should be reversed I think. Also let's compare to null
directly.

Done.

https://codereview.chromium.org/2893003003/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/BUILD.gn (right):

https://codereview.chromium.org/2893003003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/BUILD.gn:153: sources -= [
"print_preview/print_preview.js" ]
On 2017/05/19 20:59:29, dpapad wrote:
> Should we also be excluding print_preview_destination_search_test.js in this
> case?
> 
> Regardless of the answer to the above, can we remove print_preview.js from
line
> 102 above, and reverse the logic here?
> 
> if (enable_print_preview) {
>   sources += [...]
> }

Given that the tests crashed when run with --disable-print-preview
it seems like it makes sense to exclude them here. Switched the
logic direction.

https://codereview.chromium.org/2893003003/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/print_preview/print_preview.h (right):

https://codereview.chromium.org/2893003003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/print_preview/print_preview.h:5: #ifndef
CHROME_TEST_DATA_WEBUI_PRINT_PREVIEW_PRINT_PREVIEW_H_
On 2017/05/19 20:59:29, dpapad wrote:
> Is this file (and the corresponding cc file) necessary at all? Is it used
> somewhere?
> 
> AFAIU chrome/test/data/webui/print_preview/print_preview.js automatically
> generates a corresponding C++ file at
> out/<out_foder>/gen/chrome/test/data/webui/print_preview-gen.cc

Done in https://codereview.chromium.org/2894243004/

[dpapad, 2017-05-20 00:56:01]

I'll take a final look once this CL has been rebased.

https://codereview.chromium.org/2893003003/diff/40001/chrome/test/data/webui/...
File
chrome/test/data/webui/print_preview/print_preview_destination_search_test.js
(right):

https://codereview.chromium.org/2893003003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/print_preview/print_preview_destination_search_test.js:160:
var nativeLayerProxy = nativeLayer_;
This intermediate variable seems unnecessary. Can we remove it?

[commit-bot: I haz the power, 2017-05-20 03:48:22]

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

[commit-bot: I haz the power, 2017-05-20 03:48:23]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[rbpotter, 2017-05-22 20:45:34]

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

[commit-bot: I haz the power, 2017-05-22 20:45:57]

Dry run: CQ is trying da patch.

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

[rbpotter, 2017-05-22 20:47:27]

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

[rbpotter, 2017-05-22 20:47:28]

Rebased.

+skau@ to take a look at changes to
print_preview_destination_search_tests.js.

https://codereview.chromium.org/2893003003/diff/40001/chrome/test/data/webui/...
File
chrome/test/data/webui/print_preview/print_preview_destination_search_test.js
(right):

https://codereview.chromium.org/2893003003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/print_preview/print_preview_destination_search_test.js:160:
var nativeLayerProxy = nativeLayer_;
On 2017/05/20 00:56:01, dpapad wrote:
> This intermediate variable seems unnecessary. Can we remove it?

Done.

[skau, 2017-05-22 20:50:09]

lgtm

[commit-bot: I haz the power, 2017-05-22 22:07:55]

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

[commit-bot: I haz the power, 2017-05-22 22:07:56]

Dry run: This issue passed the CQ dry run.

[dpapad, 2017-05-22 22:08:51]

LGTM

[rbpotter, 2017-05-22 23:08:32]

The CQ bit was checked by rbpotter@chromium.org

[commit-bot: I haz the power, 2017-05-22 23:09:14]

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-05-22 23:15:12]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495494512131040,
"parent_rev": "90880bc17b25c2760518e056cdcd820e7d5f287e", "commit_rev":
"7d5b724780d484c91a46232ec2fd6c51e0d15f37"}

[commit-bot: I haz the power, 2017-05-22 23:15:29]

Description was changed from

==========
Print Preview: Merge NativeLayerStubs for tests

Move NativeLayerStub to a separate file and use the same stub for both
the PrintPreviewDestinationSearchTests and the PrintPreviewWebUITests.

Also move all print_preview related tests into a subfolder of
chrome/test/data/webui/ with the new native_layer_stub.js file.

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

to

==========
Print Preview: Merge NativeLayerStubs for tests

Move NativeLayerStub to a separate file and use the same stub for both
the PrintPreviewDestinationSearchTests and the PrintPreviewWebUITests.

Also move all print_preview related tests into a subfolder of
chrome/test/data/webui/ with the new native_layer_stub.js file.

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

Review-Url: https://codereview.chromium.org/2893003003
Cr-Commit-Position: refs/heads/master@{#473738}
Committed:
https://chromium.googlesource.com/chromium/src/+/7d5b724780d484c91a46232ec2fd...
==========

[commit-bot: I haz the power, 2017-05-22 23:15:30]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/7d5b724780d484c91a46232ec2fd...

[Dan Beam, 2017-06-06 22:00:46]

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

[Dan Beam, 2017-06-06 22:00:47]

https://codereview.chromium.org/2893003003/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/print_preview/native_layer_stub.js (right):

https://codereview.chromium.org/2893003003/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/print_preview/native_layer_stub.js:9: * @extends
{settings.TestBrowserProxy}
can we remove the settings. from TestBrowserProxy now that it's used outside of
settings?

[dpapad, 2017-06-06 22:04:25]

https://codereview.chromium.org/2893003003/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/print_preview/native_layer_stub.js (right):

https://codereview.chromium.org/2893003003/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/print_preview/native_layer_stub.js:9: * @extends
{settings.TestBrowserProxy}
On 2017/06/06 at 22:00:46, Dan Beam wrote:
> can we remove the settings. from TestBrowserProxy now that it's used outside
of settings?

Sure, I'll move it one level up and update clients.

[dpapad, 2017-06-06 22:35:24]

https://codereview.chromium.org/2893003003/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/print_preview/native_layer_stub.js (right):

https://codereview.chromium.org/2893003003/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/print_preview/native_layer_stub.js:9: * @extends
{settings.TestBrowserProxy}
On 2017/06/06 at 22:04:25, dpapad wrote:
> On 2017/06/06 at 22:00:46, Dan Beam wrote:
> > can we remove the settings. from TestBrowserProxy now that it's used outside
of settings?
> 
> Sure, I'll move it one level up and update clients.

See https://codereview.chromium.org/2927653002.