Print Preview: Finish removing global Javascript functions.

Print Preview: Finish removing global Javascript functions.

Remove remaining global javascript functions from the print preview
native layer and convert to sendWithPromise and web UI events.

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

Review-Url: https://codereview.chromium.org/2969383003
Cr-Commit-Position: refs/heads/master@{#485430}
Committed: https://chromium.googlesource.com/chromium/src/+/c1665b3a3c4c1b59c69bca64af65aeb334a76311

[rbpotter, 2017-07-07 01:01:26]

Description was changed from

==========
Print Preview: Finish removing global Javascript functions.

Remove remaining global javascript functions from the print preview
native layer and convert to sendWithPromise and web UI events.

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

to

==========
Print Preview: Finish removing global Javascript functions.

Remove remaining global javascript functions from the print preview
native layer and convert to sendWithPromise and web UI events.

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

[rbpotter, 2017-07-07 01:01:43]

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

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

Dry run: CQ is trying da patch.

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

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

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

[rbpotter, 2017-07-07 01:15:30]

Description was changed from

==========
Print Preview: Finish removing global Javascript functions.

Remove remaining global javascript functions from the print preview
native layer and convert to sendWithPromise and web UI events.

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

to

==========
Print Preview: Finish removing global Javascript functions.

Remove remaining global javascript functions from the print preview
native layer and convert to sendWithPromise and web UI events.

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

[rbpotter, 2017-07-07 01:15:30]

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

[commit-bot: I haz the power, 2017-07-07 01:15:38]

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-07-07 01:42:48]

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

[commit-bot: I haz the power, 2017-07-07 01:42:49]

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

[dpapad, 2017-07-07 17:28:44]

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

https://codereview.chromium.org/2969383003/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/print_preview.js:352:
this.destinationStore_.onDestinationsReload_.bind(
Two questions:
1) If onDestinationsReload_ has to be accessed from here, then it shouldn't be
private (and compiler should catch this in theory). Did it not complain?
2) Why are we adding the listener here, instead of within destinationStore_
itself? I see that we are re-using the existing listenerTracker, but this
creates an implicit dependency (in the reverse direction) between
print_preview.js and destination_store.js.

For example: Say there is a destinationStore unit test (meaning there is no
PrintPreview instance, just a DestinationStore instance) that fires
'reload-printer-list' and expects something to happen. It will not happen,
because DestinationStore does not register the listener itself, it relies on its
owning object to register listeners for it.

Same question for PreviewArea and PreviewGenerator.

[rbpotter, 2017-07-07 17:40:14]

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

https://codereview.chromium.org/2969383003/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/print_preview.js:352:
this.destinationStore_.onDestinationsReload_.bind(
On 2017/07/07 17:28:43, dpapad wrote:
> Two questions:
> 1) If onDestinationsReload_ has to be accessed from here, then it shouldn't be
> private (and compiler should catch this in theory). Did it not complain?
> 2) Why are we adding the listener here, instead of within destinationStore_
> itself? I see that we are re-using the existing listenerTracker, but this
> creates an implicit dependency (in the reverse direction) between
> print_preview.js and destination_store.js.
> 
> For example: Say there is a destinationStore unit test (meaning there is no
> PrintPreview instance, just a DestinationStore instance) that fires
> 'reload-printer-list' and expects something to happen. It will not happen,
> because DestinationStore does not register the listener itself, it relies on
its
> owning object to register listeners for it.
> 
> Same question for PreviewArea and PreviewGenerator.

1) No, compiler did not complain. Will make it public, as it is also
accessed in line 1011.
2) Listener is added here for the same reason as privet-printer-added above;
DestinationStore (like PreviewGenerator) is not a component so does not have
a listenerTracker or an "exitDocument" function where the listeners can be
removed. Would it be better to add a "removeListeners" public function for
the DestinationStore and PreviewGenerator that PrintPreview and PreviewArea
respectively could call during exitDocument? I think in this case registering
listeners would no longer be dependent on the owning object, although
removing them still would be.

[dpapad, 2017-07-07 18:04:29]

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

https://codereview.chromium.org/2969383003/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/print_preview.js:352:
this.destinationStore_.onDestinationsReload_.bind(
> 1) No, compiler did not complain. Will make it public, as it is also
> accessed in line 1011.

Ok, that's odd. I am wondering if this is related to concatenating files before
sending them to the compiler, since @private does have file-level effect AFAIK.

> 2) Listener is added here for the same reason as privet-printer-added above;
> DestinationStore (like PreviewGenerator) is not a component so does not have
> a listenerTracker or an "exitDocument" function where the listeners can be
> removed. Would it be better to add a "removeListeners" public function for
> the DestinationStore and PreviewGenerator that PrintPreview and PreviewArea
> respectively could call during exitDocument? I think in this case registering
> listeners would no longer be dependent on the owning object, although
> removing them still would be.

Thanks for the explanation, it makes sense now. I think what you are proposing
makes the code cleaner.

Mentioning a similar (maybe shorter) alternative: Keep explicitly adding
listeners from the owning object, but minimize the code residing on the owning
objects, as follows:

 - Expose a public addWebUIEventListeners(listenerTracker) from DestinationStore
and PreviewGenerator, passing PrintePreview's and PreviewArea's listenerTracker
respectively.
 - Call that method here, like
this.destinationStore_.addWebUIEventListeners(this.listenerTracker_);
 - Within DestinationStore/PreviewGenerator, register listeners on the
|listenerTracker| that is passed as a parameter, which belongs to the parent.
 - You no longer need to expose a public removeEventListeners(), since they will
be automatically removed on the owning Component's exitDocument().

WDYT?

Also I noticed some potentially existing leaking listeners in
destination_store.js setCloudPrintInterface, see [1] where we basically use an
EventTracker instance, but we never call removeAll(). So, for example if
setCloudPrintInterface() is called twice, we blindly re-register the listeners.
We should either assert that cloudPrintInterface_ is null when the method is
called (if it is only ever called once), or allow multiple calls by adding a
this.tracker_.removeAll() before re-adding the listeners. If that is indeed a
problem, let's leave this for a separate CL.

[1]
https://cs.chromium.org/chromium/src/chrome/browser/resources/print_preview/d...

[rbpotter, 2017-07-07 19:14:40]

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

[commit-bot: I haz the power, 2017-07-07 19:15:03]

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-07-07 20:35:31]

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

[commit-bot: I haz the power, 2017-07-07 20:35:32]

Dry run: This issue passed the CQ dry run.

[rbpotter, 2017-07-07 20:45:10]

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

https://codereview.chromium.org/2969383003/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/print_preview.js:352:
this.destinationStore_.onDestinationsReload_.bind(
On 2017/07/07 18:04:29, dpapad wrote:
> > 1) No, compiler did not complain. Will make it public, as it is also
> > accessed in line 1011.
> 
> Ok, that's odd. I am wondering if this is related to concatenating files
before
> sending them to the compiler, since @private does have file-level effect
AFAIK.
> 
> > 2) Listener is added here for the same reason as privet-printer-added above;
> > DestinationStore (like PreviewGenerator) is not a component so does not have
> > a listenerTracker or an "exitDocument" function where the listeners can be
> > removed. Would it be better to add a "removeListeners" public function for
> > the DestinationStore and PreviewGenerator that PrintPreview and PreviewArea
> > respectively could call during exitDocument? I think in this case
registering
> > listeners would no longer be dependent on the owning object, although
> > removing them still would be.
> 
> Thanks for the explanation, it makes sense now. I think what you are proposing
> makes the code cleaner.
> 
> Mentioning a similar (maybe shorter) alternative: Keep explicitly adding
> listeners from the owning object, but minimize the code residing on the owning
> objects, as follows:
> 
>  - Expose a public addWebUIEventListeners(listenerTracker) from
DestinationStore
> and PreviewGenerator, passing PrintePreview's and PreviewArea's
listenerTracker
> respectively.
>  - Call that method here, like
> this.destinationStore_.addWebUIEventListeners(this.listenerTracker_);
>  - Within DestinationStore/PreviewGenerator, register listeners on the
> |listenerTracker| that is passed as a parameter, which belongs to the parent.
>  - You no longer need to expose a public removeEventListeners(), since they
will
> be automatically removed on the owning Component's exitDocument().
> 
> WDYT?
> 
> Also I noticed some potentially existing leaking listeners in
> destination_store.js setCloudPrintInterface, see [1] where we basically use an
> EventTracker instance, but we never call removeAll(). So, for example if
> setCloudPrintInterface() is called twice, we blindly re-register the
listeners.
> We should either assert that cloudPrintInterface_ is null when the method is
> called (if it is only ever called once), or allow multiple calls by adding a
> this.tracker_.removeAll() before re-adding the listeners. If that is indeed a
> problem, let's leave this for a separate CL.
> 
> [1]
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/print_preview/d...

Done. That does seem a bit cleaner than having to remove in the owning
object, and we can pass in a listener if we need to test these objects
separately for some reason.

RE: setCloudPrintInterface, this should only ever be called once. The
use-cloud-print event is triggered by SendCloudPrintEnable [1], which
is only called in response to getInitialSettings, which happens at
initialization. So I think we should be able to assert both in
DestinationStore and InvitationStore, which has a similar issue, that
cloudPrintInterface_ is null before adding the listeners.

[1]
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/print_preview/pr...

[dpapad, 2017-07-07 21:12:41]

https://codereview.chromium.org/2969383003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/print_preview/data/destination_store.js (right):

https://codereview.chromium.org/2969383003/diff/40001/chrome/browser/resource...
chrome/browser/resources/print_preview/data/destination_store.js:548: *
Registers the destination store for the WebUI events it responds to and
Nit: Slightly confused by the syntax in this comment. Maybe rephrase/simplify?

https://codereview.chromium.org/2969383003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/print_preview/preview_generator.js (right):

https://codereview.chromium.org/2969383003/diff/40001/chrome/browser/resource...
chrome/browser/resources/print_preview/preview_generator.js:171:
addWebUIEventListeners: function(listenerTracker) {
Not for this CL: Just wondering, would it make sense if we take this approach
one step further and

1) pass the |listenerTracker| in the constructor
2) Call addWebUIEventListeners() within the constructor itself
3) Make addWebUIEventListeners private.

This is similar to what the previous code was doing with EventTracker, except
that it was not injected via the constructor. It also eliminates the misuse of
this API, since calling the public addWebUIEventListeners() could be
accidentally called twice, whereas the constructor can't be called twice.

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

https://codereview.chromium.org/2969383003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1183: bool
success = args->GetString(0, &callback_id);
Nit (optional): Do we need the temp |success| variable?

CHECK(args->GetString(0, &callback_id));
...
CHECK(args->GetBoolean(1, &add_account));

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

https://codereview.chromium.org/2969383003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/print_preview/print_preview_destination_search_test.js:127:
Mock4JS.clearMocksToVerify();
Can we delete this now? Or is it still used?

https://codereview.chromium.org/2969383003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/print_preview/print_preview_destination_search_test.js:146:
if (cr.isChromeOS) {
Same question here.

[rbpotter, 2017-07-07 23:58:27]

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

[commit-bot: I haz the power, 2017-07-07 23:58:36]

Dry run: CQ is trying da patch.

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

[rbpotter, 2017-07-07 23:58:40]

https://codereview.chromium.org/2969383003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/print_preview/data/destination_store.js (right):

https://codereview.chromium.org/2969383003/diff/40001/chrome/browser/resource...
chrome/browser/resources/print_preview/data/destination_store.js:548: *
Registers the destination store for the WebUI events it responds to and
On 2017/07/07 21:12:41, dpapad wrote:
> Nit: Slightly confused by the syntax in this comment. Maybe rephrase/simplify?

Done.

https://codereview.chromium.org/2969383003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/print_preview/preview_generator.js (right):

https://codereview.chromium.org/2969383003/diff/40001/chrome/browser/resource...
chrome/browser/resources/print_preview/preview_generator.js:171:
addWebUIEventListeners: function(listenerTracker) {
On 2017/07/07 21:12:41, dpapad wrote:
> Not for this CL: Just wondering, would it make sense if we take this approach
> one step further and
> 
> 1) pass the |listenerTracker| in the constructor
> 2) Call addWebUIEventListeners() within the constructor itself
> 3) Make addWebUIEventListeners private.
> 
> This is similar to what the previous code was doing with EventTracker, except
> that it was not injected via the constructor. It also eliminates the misuse of
> this API, since calling the public addWebUIEventListeners() could be
> accidentally called twice, whereas the constructor can't be called twice.

I think that would make sense. Will change in follow-up CL.

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

https://codereview.chromium.org/2969383003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1183: bool
success = args->GetString(0, &callback_id);
On 2017/07/07 21:12:41, dpapad wrote:
> Nit (optional): Do we need the temp |success| variable?
> 
> CHECK(args->GetString(0, &callback_id));
> ...
> CHECK(args->GetBoolean(1, &add_account));

Done.

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

https://codereview.chromium.org/2969383003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/print_preview/print_preview_destination_search_test.js:127:
Mock4JS.clearMocksToVerify();
On 2017/07/07 21:12:41, dpapad wrote:
> Can we delete this now? Or is it still used?

Done. Appears there are no other mocks.

Also removed teardown().

https://codereview.chromium.org/2969383003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/print_preview/print_preview_destination_search_test.js:146:
if (cr.isChromeOS) {
On 2017/07/07 21:12:41, dpapad wrote:
> Same question here.

This is still used to distinguish ChromeOS (which triggers a
setupPrinter call) vs non CrOS (triggers a
getLocalDestinationCapabilities call).

[rbpotter, 2017-07-08 00:25:39]

https://codereview.chromium.org/2969383003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/print_preview/preview_generator.js (right):

https://codereview.chromium.org/2969383003/diff/40001/chrome/browser/resource...
chrome/browser/resources/print_preview/preview_generator.js:171:
addWebUIEventListeners: function(listenerTracker) {
On 2017/07/07 23:58:39, rbpotter wrote:
> On 2017/07/07 21:12:41, dpapad wrote:
> > Not for this CL: Just wondering, would it make sense if we take this
approach
> > one step further and
> > 
> > 1) pass the |listenerTracker| in the constructor
> > 2) Call addWebUIEventListeners() within the constructor itself
> > 3) Make addWebUIEventListeners private.
> > 
> > This is similar to what the previous code was doing with EventTracker,
except
> > that it was not injected via the constructor. It also eliminates the misuse
of
> > this API, since calling the public addWebUIEventListeners() could be
> > accidentally called twice, whereas the constructor can't be called twice.
> 
> I think that would make sense. Will change in follow-up CL.

Follow up for this and other issues at
https://codereview.chromium.org/2975663003/

[dpapad, 2017-07-08 00:40:03]

LGTM with nit.

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

https://codereview.chromium.org/2969383003/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/print_preview/native_layer_stub.js:105:
pageRanges.forEach(function(range) {
Nit: This logic can be simplified.
1) Use Array#push() to grow the array instead of keeping track of |totalPages|
2) Use Array#reduce()

var pages = pageRanges.reduce(function(soFar, range) {
  for (var page = range.from; page <= range.to; page++) {
     soFar.push(page);
  }
  return soFar;
}, []);

[commit-bot: I haz the power, 2017-07-08 01:19:02]

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

[commit-bot: I haz the power, 2017-07-08 01:19:02]

Dry run: This issue passed the CQ dry run.

[rbpotter, 2017-07-08 03:09:33]

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

[commit-bot: I haz the power, 2017-07-08 03:09:36]

Dry run: CQ is trying da patch.

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

[rbpotter, 2017-07-08 03:10:52]

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

https://codereview.chromium.org/2969383003/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/print_preview/native_layer_stub.js:105:
pageRanges.forEach(function(range) {
On 2017/07/08 00:40:03, dpapad wrote:
> Nit: This logic can be simplified.
> 1) Use Array#push() to grow the array instead of keeping track of |totalPages|
> 2) Use Array#reduce()
> 
> var pages = pageRanges.reduce(function(soFar, range) {
>   for (var page = range.from; page <= range.to; page++) {
>      soFar.push(page);
>   }
>   return soFar;
> }, []);

Done.

[commit-bot: I haz the power, 2017-07-08 04:24:26]

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

[commit-bot: I haz the power, 2017-07-08 04:24:27]

Dry run: This issue passed the CQ dry run.

[rbpotter, 2017-07-10 21:13:22]

The CQ bit was checked by rbpotter@chromium.org

[rbpotter, 2017-07-10 21:13:22]

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/2969383003/#ps80001
(title: "Fix test")

[commit-bot: I haz the power, 2017-07-10 21:13:32]

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-07-10 22:41:35]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1499721202179670,
"parent_rev": "a6fb014f8fb2ae32d8908a28e1e8b7b57194dde3", "commit_rev":
"72f9d23b8d927f2ed78a77b0204f6ba1a6f13c0d"}

[commit-bot: I haz the power, 2017-07-10 22:41:54]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1499721202179670,
"parent_rev": "09ac01b33652a775acb4f1368028c7c825d8f423", "commit_rev":
"c1665b3a3c4c1b59c69bca64af65aeb334a76311"}

[commit-bot: I haz the power, 2017-07-10 22:42:08]

Description was changed from

==========
Print Preview: Finish removing global Javascript functions.

Remove remaining global javascript functions from the print preview
native layer and convert to sendWithPromise and web UI events.

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

to

==========
Print Preview: Finish removing global Javascript functions.

Remove remaining global javascript functions from the print preview
native layer and convert to sendWithPromise and web UI events.

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

Review-Url: https://codereview.chromium.org/2969383003
Cr-Commit-Position: refs/heads/master@{#485430}
Committed:
https://chromium.googlesource.com/chromium/src/+/c1665b3a3c4c1b59c69bca64af65...
==========

[commit-bot: I haz the power, 2017-07-10 22:42:09]

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