Print Preview: Use cr.sendWithPromise for getInitialSettings

Print Preview: Use cr.sendWithPromise for getInitialSettings

Start replacing some chrome.send calls with cr.sendWithPromise in print
preview by using cr.sendWithPromise for the getInitialSettings message.
To avoid breaking tests during transition, also:
- Change NativeLayerStub in PrintPreviewWebUITests to inherit from
settings.TestBrowserProxy.
- Change NativeLayer so that it no longer inherits from cr.EventTarget.
- Add an EventTarget member to NativeLayer for dispatching and
receiving the print preview messages that have not yet been migrated
to use cr.sendWithPromise (will be removed when all messages have
been changed).

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

Review-Url: https://codereview.chromium.org/2881213003
Cr-Commit-Position: refs/heads/master@{#473731}
Committed: https://chromium.googlesource.com/chromium/src/+/4adaebf29293acf54c54ac2910f0a3bb2f32683f

[rbpotter, 2017-05-15 21:07:07]

Description was changed from

==========
Substitute one message

Replace chrome.send with cr.sendWithPromise

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

to

==========
Substitute one message

Replace chrome.send with cr.sendWithPromise

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

[rbpotter, 2017-05-15 21:16:15]

Description was changed from

==========
Substitute one message

Replace chrome.send with cr.sendWithPromise

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

to

==========
Substitute one message

Replace chrome.send with cr.sendWithPromise in print preview. Begin
with one (initial settings) message for now.

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

[rbpotter, 2017-05-17 01:07:04]

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

[commit-bot: I haz the power, 2017-05-17 01:08:30]

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-17 01:46:14]

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

[commit-bot: I haz the power, 2017-05-17 01:47:13]

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-05-17 03:20:23]

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

[commit-bot: I haz the power, 2017-05-17 03:20:24]

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

[rbpotter, 2017-05-17 18:47:30]

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

[commit-bot: I haz the power, 2017-05-17 18:48:17]

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-17 18:54:25]

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

[commit-bot: I haz the power, 2017-05-17 18:55:53]

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-05-17 20:42:07]

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

[commit-bot: I haz the power, 2017-05-17 20:42:08]

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

[dpapad, 2017-05-17 23:59:11]

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

[dpapad, 2017-05-17 23:59:13]

https://codereview.chromium.org/2881213003/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/print_preview/native_layer.js (right):

https://codereview.chromium.org/2881213003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/print_preview/native_layer.js:79: this.eventTarget_ =
new cr.EventTarget();
@private {!cr.EventTarget}

https://codereview.chromium.org/2881213003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/print_preview/native_layer.js:153: * @return
{!cr.EventTarget}
Nit:
@return {!cr.EventTarget} The event target for the native layer.

https://codereview.chromium.org/2881213003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/print_preview/native_layer.js:169: * @return
{!Promise<print_preview.NativeInitialSettings>}
Shouldn't this be
!Promise<!print_preview.NativeInitialSettings>

https://codereview.chromium.org/2881213003/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/print_preview/print_preview.js (right):

https://codereview.chromium.org/2881213003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/print_preview/print_preview.js:349: function() {
getInitialSettings() should never fail (see comment in C++ handler), so it is OK
to omit the error handler here.

Also, if a Promise fails for some unexpected reason, and there is no error
handler, an error is shown in the console anyway.

https://codereview.chromium.org/2881213003/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right):

https://codereview.chromium.org/2881213003/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1215: if
(!args->GetString(0, &callback_id) || callback_id.empty()) {
If we get in here, that would be a programmer's error, right? Can we use an
assertion CHECK or DCHECK instead? See example
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/settings/languag....

[rbpotter, 2017-05-18 17:51:53]

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

[commit-bot: I haz the power, 2017-05-18 17:52:46]

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-18 17:56:30]

https://codereview.chromium.org/2881213003/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/print_preview/native_layer.js (right):

https://codereview.chromium.org/2881213003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/print_preview/native_layer.js:79: this.eventTarget_ =
new cr.EventTarget();
On 2017/05/17 23:59:12, dpapad wrote:
> @private {!cr.EventTarget}

Done.

https://codereview.chromium.org/2881213003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/print_preview/native_layer.js:153: * @return
{!cr.EventTarget}
On 2017/05/17 23:59:12, dpapad wrote:
> Nit:
> @return {!cr.EventTarget} The event target for the native layer.

Done.

https://codereview.chromium.org/2881213003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/print_preview/native_layer.js:169: * @return
{!Promise<print_preview.NativeInitialSettings>}
On 2017/05/17 23:59:12, dpapad wrote:
> Shouldn't this be
> !Promise<!print_preview.NativeInitialSettings>

Done.

https://codereview.chromium.org/2881213003/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/print_preview/print_preview.js (right):

https://codereview.chromium.org/2881213003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/print_preview/print_preview.js:349: function() {
On 2017/05/17 23:59:12, dpapad wrote:
> getInitialSettings() should never fail (see comment in C++ handler), so it is
OK
> to omit the error handler here.
> 
> Also, if a Promise fails for some unexpected reason, and there is no error
> handler, an error is shown in the console anyway.

Done.

https://codereview.chromium.org/2881213003/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right):

https://codereview.chromium.org/2881213003/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1215: if
(!args->GetString(0, &callback_id) || callback_id.empty()) {
On 2017/05/17 23:59:12, dpapad wrote:
> If we get in here, that would be a programmer's error, right? Can we use an
> assertion CHECK or DCHECK instead? See example
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/settings/languag....

Done.

[commit-bot: I haz the power, 2017-05-18 18:04:34]

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

[commit-bot: I haz the power, 2017-05-18 18:04:35]

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

[dpapad, 2017-05-18 19:11:26]

https://codereview.chromium.org/2881213003/diff/140001/chrome/test/data/webui...
File chrome/test/data/webui/print_preview.js (right):

https://codereview.chromium.org/2881213003/diff/140001/chrome/test/data/webui...
chrome/test/data/webui/print_preview.js:13: var printPreview = null;
Can this be defined inside PrintPreviewWebUITest constructor as a member var
instead of a global?

https://codereview.chromium.org/2881213003/diff/140001/chrome/test/data/webui...
chrome/test/data/webui/print_preview.js:150: ROOT_PATH +
'ui/webui/resources/js/cr.js',
Nit, within array the indentation should be 2 instead of 4.

https://codereview.chromium.org/2881213003/diff/140001/chrome/test/data/webui...
chrome/test/data/webui/print_preview.js:161: printPreview = new
print_preview.PrintPreview();
It does not have to be in this CL, but eventually we should stop accessing
private member variables. Specifically for NativeLayer,

Approach 1: Using Dependency Injection as folows:

var nativeLayer = new NativeLayerStub();
printPreview = new print_preview.PrintPreview(nativeLayer);

Then instead of
printPreview.nativeLayer_.whenCalled('getInitialSettings')...
we can do
nativeLayer.whenCalled('getInitialSettings')

This way you also don't need to replace the constructor as follows.
print_preview.NativeLayer = NativeLayerStub;


Approach 2: Factory method

// native_layer.js

var instance_ = null;

// Create a static factory method. By default creates a prod NativeLayer.
NativeLayer.getInstance = function() {
 if (!instance_ == null)
   instance_ = new NativeLayer();
 return instance_;
};

NativeLayer.setInstance = function(instance) {
  instance_ = instance;
};

Then you change PrintPreview constructor to call getInstance(). In tests, you
call setInstance() first, before you create a PrintPreview() object.

https://codereview.chromium.org/2881213003/diff/140001/chrome/test/data/webui...
File chrome/test/data/webui/print_preview_destination_search_test.js (right):

https://codereview.chromium.org/2881213003/diff/140001/chrome/test/data/webui...
chrome/test/data/webui/print_preview_destination_search_test.js:48: * @extends
{settings.TestBrowserProxy}
Can you add a TODO to extract NativeLayerStub to a new file and re-use it from
all tests?

https://codereview.chromium.org/2881213003/diff/140001/chrome/test/data/webui...
chrome/test/data/webui/print_preview_destination_search_test.js:152: var
resolver = new PromiseResolver();
Where is resolver.promise resolved? I don't see a call to resolver.resolve()
anymore.

[rbpotter, 2017-05-18 23:35:24]

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

[commit-bot: I haz the power, 2017-05-18 23:36:39]

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-18 23:40:35]

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

[rbpotter, 2017-05-18 23:40:57]

https://codereview.chromium.org/2881213003/diff/140001/chrome/test/data/webui...
File chrome/test/data/webui/print_preview.js (right):

https://codereview.chromium.org/2881213003/diff/140001/chrome/test/data/webui...
chrome/test/data/webui/print_preview.js:13: var printPreview = null;
On 2017/05/18 19:11:26, dpapad wrote:
> Can this be defined inside PrintPreviewWebUITest constructor as a member var
> instead of a global?

Done.

https://codereview.chromium.org/2881213003/diff/140001/chrome/test/data/webui...
chrome/test/data/webui/print_preview.js:150: ROOT_PATH +
'ui/webui/resources/js/cr.js',
On 2017/05/18 19:11:26, dpapad wrote:
> Nit, within array the indentation should be 2 instead of 4.

Done.

https://codereview.chromium.org/2881213003/diff/140001/chrome/test/data/webui...
chrome/test/data/webui/print_preview.js:161: printPreview = new
print_preview.PrintPreview();
On 2017/05/18 19:11:26, dpapad wrote:
> It does not have to be in this CL, but eventually we should stop accessing
> private member variables. Specifically for NativeLayer,
> 
> Approach 1: Using Dependency Injection as folows:
> 
> var nativeLayer = new NativeLayerStub();
> printPreview = new print_preview.PrintPreview(nativeLayer);
> 
> Then instead of
> printPreview.nativeLayer_.whenCalled('getInitialSettings')...
> we can do
> nativeLayer.whenCalled('getInitialSettings')
> 
> This way you also don't need to replace the constructor as follows.
> print_preview.NativeLayer = NativeLayerStub;
> 
> 
> Approach 2: Factory method
> 
> // native_layer.js
> 
> var instance_ = null;
> 
> // Create a static factory method. By default creates a prod NativeLayer.
> NativeLayer.getInstance = function() {
>  if (!instance_ == null)
>    instance_ = new NativeLayer();
>  return instance_;
> };
> 
> NativeLayer.setInstance = function(instance) {
>   instance_ = instance;
> };
> 
> Then you change PrintPreview constructor to call getInstance(). In tests, you
> call setInstance() first, before you create a PrintPreview() object.

Will do this in a follow-up CL. Do we want to change to something like
this for all the member variables accessed? In addition to the two
here we access previewGenerator_, printTicketStore_, destinationStore_,
etc in various tests.

https://codereview.chromium.org/2881213003/diff/140001/chrome/test/data/webui...
File chrome/test/data/webui/print_preview_destination_search_test.js (right):

https://codereview.chromium.org/2881213003/diff/140001/chrome/test/data/webui...
chrome/test/data/webui/print_preview_destination_search_test.js:48: * @extends
{settings.TestBrowserProxy}
On 2017/05/18 19:11:26, dpapad wrote:
> Can you add a TODO to extract NativeLayerStub to a new file and re-use it from
> all tests?

Done.

https://codereview.chromium.org/2881213003/diff/140001/chrome/test/data/webui...
chrome/test/data/webui/print_preview_destination_search_test.js:152: var
resolver = new PromiseResolver();
On 2017/05/18 19:11:26, dpapad wrote:
> Where is resolver.promise resolved? I don't see a call to resolver.resolve()
> anymore.

Done. Rewrote these to be very different for CrOs vs non CrOs,
but hopefully keeping the original intent of running through
the destination setup/capabilities set code paths.

[commit-bot: I haz the power, 2017-05-18 23:41:49]

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-05-18 23:54:10]

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

[commit-bot: I haz the power, 2017-05-18 23:54:11]

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

[dpapad, 2017-05-19 00:10:19]

https://codereview.chromium.org/2881213003/diff/140001/chrome/test/data/webui...
File chrome/test/data/webui/print_preview.js (right):

https://codereview.chromium.org/2881213003/diff/140001/chrome/test/data/webui...
chrome/test/data/webui/print_preview.js:161: printPreview = new
print_preview.PrintPreview();
> Will do this in a follow-up CL. Do we want to change to something like
> this for all the member variables accessed? In addition to the two
> here we access previewGenerator_, printTicketStore_, destinationStore_,
> etc in various tests.

For previewGenerator_, printTicketStore_ and destinationStore_ just exposing a
public getter might be enough. Unlike nativeLayer, I don't think we completely
replace those with stub objects. Even so, let's only do one thing at a time, and
nativeLayer seems of higher importance.

https://codereview.chromium.org/2881213003/diff/140001/chrome/test/data/webui...
chrome/test/data/webui/print_preview.js:611: this.setInitialSettings();
Is getInitialSettisgs() called twice here? Once because of createPrintPreview()
and once because of setInitialSettings()?

If so, and assuming you want to wait on the 2nd time, we need to call

printPreview.nativeLayer_.resetResolver('getInitialSettings') at line 610.

https://codereview.chromium.org/2881213003/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/print_preview/print_preview.js (right):

https://codereview.chromium.org/2881213003/diff/200001/chrome/browser/resourc...
chrome/browser/resources/print_preview/print_preview.js:340:
assert(this.uiState_ == PrintPreviewUiState_.INITIALIZING,
Is there a reason to move some logic out if onInitialSettingsSet_? Why not
keeping the logic where it was before and doing the following?

this.nativeLayer_.getInitialSettings().then(this.onInitialSettings_.bind(this));

https://codereview.chromium.org/2881213003/diff/200001/chrome/test/data/webui...
File chrome/test/data/webui/print_preview_destination_search_test.js (right):

https://codereview.chromium.org/2881213003/diff/200001/chrome/test/data/webui...
chrome/test/data/webui/print_preview_destination_search_test.js:77:
this.methodCalled('setupPrinter');
this.methodCalled('setupPrinter', printerId);

This will allow you to assert that the code being tested passes the expected
printerId to the nativeLayer.

https://codereview.chromium.org/2881213003/diff/200001/chrome/test/data/webui...
chrome/test/data/webui/print_preview_destination_search_test.js:78: if
(this.shouldReject_)
return this.shouldReject_ ?
    Promise.reject(this.setupPrinterResponse_) :
    Promise.resolve(this.setupPrinterResponse_);

https://codereview.chromium.org/2881213003/diff/200001/chrome/test/data/webui...
chrome/test/data/webui/print_preview_destination_search_test.js:279: return
nativeLayer_.whenCalled('setupPrinter', destId).then(
whenCalled() only accepts one parameter (here and elsewhere in this file), see
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/test_bro....

What you could do though is to assert as follows (combined with the suggestion
at line 77)

return nativeLayer_.whenCalled('setupPrinter').then(function(actualDestId) {
  assertEquals(destId, actualDestId);
  ...
});

https://codereview.chromium.org/2881213003/diff/200001/chrome/test/data/webui...
chrome/test/data/webui/print_preview_destination_search_test.js:283:
}.bind(destinationStore_));
Is the bind() call necessary? At first glance I don't think so.

[rbpotter, 2017-05-19 00:47:35]

Patchset #12 (id:220001) has been deleted

[rbpotter, 2017-05-19 01:01:07]

Patchset #12 (id:240001) has been deleted

[rbpotter, 2017-05-19 01:01:14]

Patchset #12 (id:260001) has been deleted

[rbpotter, 2017-05-19 01:18:50]

Description was changed from

==========
Substitute one message

Replace chrome.send with cr.sendWithPromise in print preview. Begin
with one (initial settings) message for now.

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

to

==========
Print Preview: Use cr.sendWithPromise for getInitialSettings

Start replacing chrome.send with cr.sendWithPromise in print preview
by using cr.sendWithPromise for the getInitialSettings message. To 
avoid breaking tests during transition, also:
- Change NativeLayerStub in PrintPreviewWebUITests to inherit from
settings.TestBrowserProxy.
- Change NativeLayer so that it no longer inherits from cr.EventTarget.
- Add an EventTarget member to NativeLayer for dispatching and
receiving the print preview messages that have not yet been migrated
to use cr.sendWithPromise (will be removed when all messages have
been changed).

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

[rbpotter, 2017-05-19 01:19:37]

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

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

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 01:26:59]

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

[rbpotter, 2017-05-19 01:27:01]

+skau@: Please take a look at the changes to
print_preview_destination_search_test.js. Since this CL
changes NativeLayer so that it is no longer an EventTarget
(see description), these tests needed to be modified.

https://codereview.chromium.org/2881213003/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/print_preview/print_preview.js (right):

https://codereview.chromium.org/2881213003/diff/200001/chrome/browser/resourc...
chrome/browser/resources/print_preview/print_preview.js:340:
assert(this.uiState_ == PrintPreviewUiState_.INITIALIZING,
On 2017/05/19 00:10:18, dpapad wrote:
> Is there a reason to move some logic out if onInitialSettingsSet_? Why not
> keeping the logic where it was before and doing the following?
> 
>
this.nativeLayer_.getInitialSettings().then(this.onInitialSettings_.bind(this));

Done.

https://codereview.chromium.org/2881213003/diff/200001/chrome/test/data/webui...
File chrome/test/data/webui/print_preview_destination_search_test.js (right):

https://codereview.chromium.org/2881213003/diff/200001/chrome/test/data/webui...
chrome/test/data/webui/print_preview_destination_search_test.js:77:
this.methodCalled('setupPrinter');
On 2017/05/19 00:10:18, dpapad wrote:
> this.methodCalled('setupPrinter', printerId);
> 
> This will allow you to assert that the code being tested passes the expected
> printerId to the nativeLayer.

Done.

https://codereview.chromium.org/2881213003/diff/200001/chrome/test/data/webui...
chrome/test/data/webui/print_preview_destination_search_test.js:78: if
(this.shouldReject_)
On 2017/05/19 00:10:18, dpapad wrote:
> return this.shouldReject_ ?
>     Promise.reject(this.setupPrinterResponse_) :
>     Promise.resolve(this.setupPrinterResponse_);

Done.

https://codereview.chromium.org/2881213003/diff/200001/chrome/test/data/webui...
chrome/test/data/webui/print_preview_destination_search_test.js:279: return
nativeLayer_.whenCalled('setupPrinter', destId).then(
On 2017/05/19 00:10:18, dpapad wrote:
> whenCalled() only accepts one parameter (here and elsewhere in this file), see
>
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/test_bro....
> 
> What you could do though is to assert as follows (combined with the suggestion
> at line 77)
> 
> return nativeLayer_.whenCalled('setupPrinter').then(function(actualDestId) {
>   assertEquals(destId, actualDestId);
>   ...
> });

Done.

https://codereview.chromium.org/2881213003/diff/200001/chrome/test/data/webui...
chrome/test/data/webui/print_preview_destination_search_test.js:283:
}.bind(destinationStore_));
On 2017/05/19 00:10:18, dpapad wrote:
> Is the bind() call necessary? At first glance I don't think so.

Done.

[dpapad, 2017-05-19 02:07:06]

Mostly nit comments.

Also in the CL description it is implied that all chrome.send() messages will be
migrated to cr.sendWithPromise(). This is not always possible. Specifically some
messages do not have a 1:1 associating between request and response. Example:

// in JS
chrome.send('startMultiStepTask')

// in C++
CallJavascriptFunction('step1Completed')
... async code here
CallJavascriptFunction('step2Completed')
...
CallJavascriptFunction('step3Completed')
...

Those can't be migrated to cr.sendWithPromise(), and I think there are some such
cases in print preview, but not 100% sure.

https://codereview.chromium.org/2881213003/diff/280001/chrome/browser/resourc...
File chrome/browser/resources/print_preview/print_preview.js (right):

https://codereview.chromium.org/2881213003/diff/280001/chrome/browser/resourc...
chrome/browser/resources/print_preview/print_preview.js:646: 'state: ' +
this.uiState_);
Can you revert this line completely? It seems that "state: " can fit in previous
line (as it was before).

https://codereview.chromium.org/2881213003/diff/280001/chrome/test/data/webui...
File chrome/test/data/webui/print_preview.js (right):

https://codereview.chromium.org/2881213003/diff/280001/chrome/test/data/webui...
chrome/test/data/webui/print_preview.js:331:
recentList.childNodes.item(0).querySelector(
Indentation seems off. It is neither 4 spaces from previous line, nor aligned
with the parameter in previous line.

https://codereview.chromium.org/2881213003/diff/280001/chrome/test/data/webui...
chrome/test/data/webui/print_preview.js:380: recentList.childNodes.item(0).
Indentation looks off here. Shouldn't it be

assertEquals('FooName',
             recentList.childNodes.item(0).
                 querySelector('.destination-list-item-name').
...

https://codereview.chromium.org/2881213003/diff/280001/chrome/test/data/webui...
File chrome/test/data/webui/print_preview_destination_search_test.js (right):

https://codereview.chromium.org/2881213003/diff/280001/chrome/test/data/webui...
chrome/test/data/webui/print_preview_destination_search_test.js:250: return
Promise.all([nativeLayer_.whenCalled('setupPrinter').then(
Instead of adding a then() in the individual promise, and then another then() in
the aggregate promise, can we do the  following?

return Promise.all([
  nativeLayer_.whenCalled('setupPrinter'), waiter
]).then(function(results) {
  assertEquals(destId, results[0]);

  // after setup succeeds and event arrives, the destination should
  // be selected.
  assertNotEquals(null, destinationStore_.selectedDestination);
  ...
});

[rbpotter, 2017-05-19 02:08:56]

Description was changed from

==========
Print Preview: Use cr.sendWithPromise for getInitialSettings

Start replacing chrome.send with cr.sendWithPromise in print preview
by using cr.sendWithPromise for the getInitialSettings message. To 
avoid breaking tests during transition, also:
- Change NativeLayerStub in PrintPreviewWebUITests to inherit from
settings.TestBrowserProxy.
- Change NativeLayer so that it no longer inherits from cr.EventTarget.
- Add an EventTarget member to NativeLayer for dispatching and
receiving the print preview messages that have not yet been migrated
to use cr.sendWithPromise (will be removed when all messages have
been changed).

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

to

==========
Print Preview: Use cr.sendWithPromise for getInitialSettings

Start replacing some chrome.send calls with cr.sendWithPromise in print preview
by using cr.sendWithPromise for the getInitialSettings message.
To avoid breaking tests during transition, also:
- Change NativeLayerStub in PrintPreviewWebUITests to inherit from
settings.TestBrowserProxy.
- Change NativeLayer so that it no longer inherits from cr.EventTarget.
- Add an EventTarget member to NativeLayer for dispatching and
receiving the print preview messages that have not yet been migrated
to use cr.sendWithPromise (will be removed when all messages have
been changed).

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

[rbpotter, 2017-05-19 02:09:08]

Description was changed from

==========
Print Preview: Use cr.sendWithPromise for getInitialSettings

Start replacing some chrome.send calls with cr.sendWithPromise in print preview
by using cr.sendWithPromise for the getInitialSettings message.
To avoid breaking tests during transition, also:
- Change NativeLayerStub in PrintPreviewWebUITests to inherit from
settings.TestBrowserProxy.
- Change NativeLayer so that it no longer inherits from cr.EventTarget.
- Add an EventTarget member to NativeLayer for dispatching and
receiving the print preview messages that have not yet been migrated
to use cr.sendWithPromise (will be removed when all messages have
been changed).

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

to

==========
Print Preview: Use cr.sendWithPromise for getInitialSettings

Start replacing some chrome.send calls with cr.sendWithPromise in print
preview by using cr.sendWithPromise for the getInitialSettings message.
To avoid breaking tests during transition, also:
- Change NativeLayerStub in PrintPreviewWebUITests to inherit from
settings.TestBrowserProxy.
- Change NativeLayer so that it no longer inherits from cr.EventTarget.
- Add an EventTarget member to NativeLayer for dispatching and
receiving the print preview messages that have not yet been migrated
to use cr.sendWithPromise (will be removed when all messages have
been changed).

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

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

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

[rbpotter, 2017-05-19 02:25:39]

https://codereview.chromium.org/2881213003/diff/280001/chrome/browser/resourc...
File chrome/browser/resources/print_preview/print_preview.js (right):

https://codereview.chromium.org/2881213003/diff/280001/chrome/browser/resourc...
chrome/browser/resources/print_preview/print_preview.js:646: 'state: ' +
this.uiState_);
On 2017/05/19 02:07:06, dpapad wrote:
> Can you revert this line completely? It seems that "state: " can fit in
previous
> line (as it was before).

Done.

https://codereview.chromium.org/2881213003/diff/280001/chrome/test/data/webui...
File chrome/test/data/webui/print_preview.js (right):

https://codereview.chromium.org/2881213003/diff/280001/chrome/test/data/webui...
chrome/test/data/webui/print_preview.js:331:
recentList.childNodes.item(0).querySelector(
On 2017/05/19 02:07:06, dpapad wrote:
> Indentation seems off. It is neither 4 spaces from previous line, nor aligned
> with the parameter in previous line.

Done.

https://codereview.chromium.org/2881213003/diff/280001/chrome/test/data/webui...
chrome/test/data/webui/print_preview.js:380: recentList.childNodes.item(0).
On 2017/05/19 02:07:06, dpapad wrote:
> Indentation looks off here. Shouldn't it be
> 
> assertEquals('FooName',
>              recentList.childNodes.item(0).
>                  querySelector('.destination-list-item-name').
> ...
>             
>   

Done.

https://codereview.chromium.org/2881213003/diff/280001/chrome/test/data/webui...
File chrome/test/data/webui/print_preview_destination_search_test.js (right):

https://codereview.chromium.org/2881213003/diff/280001/chrome/test/data/webui...
chrome/test/data/webui/print_preview_destination_search_test.js:250: return
Promise.all([nativeLayer_.whenCalled('setupPrinter').then(
On 2017/05/19 02:07:06, dpapad wrote:
> Instead of adding a then() in the individual promise, and then another then()
in
> the aggregate promise, can we do the  following?
> 
> return Promise.all([
>   nativeLayer_.whenCalled('setupPrinter'), waiter
> ]).then(function(results) {
>   assertEquals(destId, results[0]);
> 
>   // after setup succeeds and event arrives, the destination should
>   // be selected.
>   assertNotEquals(null, destinationStore_.selectedDestination);
>   ...
> });

Done.

[commit-bot: I haz the power, 2017-05-19 02:26:35]

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-05-19 05:19:36]

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

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

Dry run: This issue passed the CQ dry run.

[dpapad, 2017-05-19 17:22:50]

LGTM

[rbpotter, 2017-05-19 21:08:17]

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

[commit-bot: I haz the power, 2017-05-19 21:08:57]

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-05-19 23:00:30]

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

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

Dry run: This issue passed the CQ dry run.

[skau, 2017-05-22 17:10:42]

lgtm

[rbpotter, 2017-05-22 17:21:43]

The CQ bit was checked by rbpotter@chromium.org

[rbpotter, 2017-05-22 17:21:44]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/2881213003/#ps320001
(title: "Rebase")

[commit-bot: I haz the power, 2017-05-22 17:22:10]

CQ is trying da patch.

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

[rbpotter, 2017-05-22 18:10:50]

The CQ bit was unchecked by rbpotter@chromium.org

[rbpotter, 2017-05-22 18:43:01]

The CQ bit was checked by rbpotter@chromium.org

[commit-bot: I haz the power, 2017-05-22 18:43:54]

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 18:52:37]

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

[commit-bot: I haz the power, 2017-05-22 18:52:39]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[rbpotter, 2017-05-22 19:29:39]

The CQ bit was checked by rbpotter@chromium.org

[rbpotter, 2017-05-22 19:29:39]

The patchset sent to the CQ was uploaded after l-g-t-m from skau@chromium.org,
dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/2881213003/#ps360001
(title: "Revert extra change from rebase")

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

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 21:51:25]

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

[commit-bot: I haz the power, 2017-05-22 21:51:26]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[rbpotter, 2017-05-22 22:53:46]

The CQ bit was checked by rbpotter@chromium.org

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

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 22:59:45]

CQ is committing da patch.

Bot data: {"patchset_id": 360001, "attempt_start_ts": 1495493626629640,
"parent_rev": "1ba1aa2f95523e8402a9480b876491f802a97b48", "commit_rev":
"4adaebf29293acf54c54ac2910f0a3bb2f32683f"}

[commit-bot: I haz the power, 2017-05-22 23:00:04]

Description was changed from

==========
Print Preview: Use cr.sendWithPromise for getInitialSettings

Start replacing some chrome.send calls with cr.sendWithPromise in print
preview by using cr.sendWithPromise for the getInitialSettings message.
To avoid breaking tests during transition, also:
- Change NativeLayerStub in PrintPreviewWebUITests to inherit from
settings.TestBrowserProxy.
- Change NativeLayer so that it no longer inherits from cr.EventTarget.
- Add an EventTarget member to NativeLayer for dispatching and
receiving the print preview messages that have not yet been migrated
to use cr.sendWithPromise (will be removed when all messages have
been changed).

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

to

==========
Print Preview: Use cr.sendWithPromise for getInitialSettings

Start replacing some chrome.send calls with cr.sendWithPromise in print
preview by using cr.sendWithPromise for the getInitialSettings message.
To avoid breaking tests during transition, also:
- Change NativeLayerStub in PrintPreviewWebUITests to inherit from
settings.TestBrowserProxy.
- Change NativeLayer so that it no longer inherits from cr.EventTarget.
- Add an EventTarget member to NativeLayer for dispatching and
receiving the print preview messages that have not yet been migrated
to use cr.sendWithPromise (will be removed when all messages have
been changed).

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

Review-Url: https://codereview.chromium.org/2881213003
Cr-Commit-Position: refs/heads/master@{#473731}
Committed:
https://chromium.googlesource.com/chromium/src/+/4adaebf29293acf54c54ac2910f0...
==========

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

Committed patchset #16 (id:360001) as
https://chromium.googlesource.com/chromium/src/+/4adaebf29293acf54c54ac2910f0...