Print Preview: Remove global onPrivetPrintFailed

Print Preview: Remove global onPrivetPrintFailed

Remove global onPrivetPrintFailed method and replace with a WebUI
listener.

Also rename to onPrintFailed/print-failed since this can also be
called due to an extension printer failure.

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

Review-Url: https://codereview.chromium.org/2935303003
Cr-Commit-Position: refs/heads/master@{#479778}
Committed: https://chromium.googlesource.com/chromium/src/+/30eeb64af965012272dc1ed7be0c0e6db5005871

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

Description was changed from

==========
Print Preview: Remove global onPrivetPrintFailed

Remove global onPrivetPrintFailed method and replace with a WebUI
listener.

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

to

==========
Print Preview: Remove global onPrivetPrintFailed

Remove global onPrivetPrintFailed method and replace with a WebUI
listener.

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

[rbpotter, 2017-06-14 19:05:46]

Description was changed from

==========
Print Preview: Remove global onPrivetPrintFailed

Remove global onPrivetPrintFailed method and replace with a WebUI
listener.

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

to

==========
Print Preview: Remove global onPrivetPrintFailed

Remove global onPrivetPrintFailed method and replace with a WebUI
listener.

Also rename to onPrintFailed/print-failed since this can also be
called due to an extension printer failure.

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

[rbpotter, 2017-06-14 19:05:46]

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

[rbpotter, 2017-06-14 19:05:51]

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

[commit-bot: I haz the power, 2017-06-14 19:06:18]

Dry run: CQ is trying da patch.

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

[rbpotter, 2017-06-14 19:24:11]

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

[commit-bot: I haz the power, 2017-06-14 19:24:41]

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-06-14 20:49:21]

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

[commit-bot: I haz the power, 2017-06-14 20:49:22]

Dry run: This issue passed the CQ dry run.

[dpapad, 2017-06-14 20:52:45]

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

https://codereview.chromium.org/2935303003/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/native_layer.js:153: PRIVET_PRINT_FAILED:
'print_preview.NativeLayer.PRIVET_PRINT_FAILED',
Is this still needed?

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

https://codereview.chromium.org/2935303003/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/print_preview.js:344:
cr.addWebUIListener('print-failed',
I think that we need to remove this listener in exitDocument method (see example
where exitDocument is overriden at
https://cs.chromium.org/chromium/src/chrome/browser/resources/print_preview/c...),
to be safe.

This could be a problem during tests, if we ever destroy a PrintPreview instance
and create a new one, where both old and new would have an active listener. You
can find an example of how to delete a WebUI listener at
https://cs.chromium.org/chromium/src/ui/webui/resources/js/web_ui_listener_be...
(unfortunately can't reuse that code, since "behavior" is a Polymer concept.

[rbpotter, 2017-06-14 22:39:21]

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

[commit-bot: I haz the power, 2017-06-14 22:39:52]

Dry run: CQ is trying da patch.

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

[rbpotter, 2017-06-14 22:52:55]

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

https://codereview.chromium.org/2935303003/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/native_layer.js:153: PRIVET_PRINT_FAILED:
'print_preview.NativeLayer.PRIVET_PRINT_FAILED',
On 2017/06/14 20:52:45, dpapad wrote:
> Is this still needed?

No, removed.

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

https://codereview.chromium.org/2935303003/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/print_preview.js:344:
cr.addWebUIListener('print-failed',
On 2017/06/14 20:52:45, dpapad wrote:
> I think that we need to remove this listener in exitDocument method (see
example
> where exitDocument is overriden at
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/print_preview/c...),
> to be safe.
> 
> This could be a problem during tests, if we ever destroy a PrintPreview
instance
> and create a new one, where both old and new would have an active listener.
You
> can find an example of how to delete a WebUI listener at
>
https://cs.chromium.org/chromium/src/ui/webui/resources/js/web_ui_listener_be...
> (unfortunately can't reuse that code, since "behavior" is a Polymer concept.

Done.

[dpapad, 2017-06-15 00:19:18]

This looks almost l-g-t-m, see comment about webui_listener_tracker.js location.

Also once this lands, we can make web_ui_listener_behavior use an instance of
WebUIListenerTracker, to avoid duplicating code.

https://codereview.chromium.org/2935303003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/print_preview/component.js (right):

https://codereview.chromium.org/2935303003/diff/60001/chrome/browser/resource...
chrome/browser/resources/print_preview/component.js:26: * @private
{EventTracker}
Since you are cleaning this up, let's also add the "!".

https://codereview.chromium.org/2935303003/diff/60001/chrome/browser/resource...
chrome/browser/resources/print_preview/component.js:32: * @private
{WebUIListenerTracker}
Same here.

https://codereview.chromium.org/2935303003/diff/60001/chrome/browser/resource...
chrome/browser/resources/print_preview/component.js:56: /** @return
{WebUIListenerTracker} Component's Web UI listener tracker. */
Same here, let's add the "!".

https://codereview.chromium.org/2935303003/diff/60001/ui/webui/resources/js/w...
File ui/webui/resources/js/webui_listener_tracker.js (right):

https://codereview.chromium.org/2935303003/diff/60001/ui/webui/resources/js/w...
ui/webui/resources/js/webui_listener_tracker.js:14: function
WebUIListenerTracker() {
The policy we've been following so far is to move things to ui/webui/resources/
onyl after a 2nd client for that code is located. For example
WebUIListenerBehavior is used by both settings/ and md_user_manager/.

Can we move this under print_preview/ for now?

[dpapad, 2017-06-15 00:21:24]

On 2017/06/15 at 00:19:18, dpapad wrote:
> This looks almost l-g-t-m, see comment about webui_listener_tracker.js
location.
> 
> Also once this lands, we can make web_ui_listener_behavior use an instance of
WebUIListenerTracker, to avoid duplicating code.

Realizing now that if we do this^, we would have two usages of
WebUIListenerTracker,and we would move it back to where it is now in this CL, so
maybe it's better to skip moving it back and forth, and just add the 2nd usage
to not violate the policy. I can do this separately, so ignore previous comment.
Should be fine under this rationale.


> 
>
https://codereview.chromium.org/2935303003/diff/60001/chrome/browser/resource...
> File chrome/browser/resources/print_preview/component.js (right):
> 
>
https://codereview.chromium.org/2935303003/diff/60001/chrome/browser/resource...
> chrome/browser/resources/print_preview/component.js:26: * @private
{EventTracker}
> Since you are cleaning this up, let's also add the "!".
> 
>
https://codereview.chromium.org/2935303003/diff/60001/chrome/browser/resource...
> chrome/browser/resources/print_preview/component.js:32: * @private
{WebUIListenerTracker}
> Same here.
> 
>
https://codereview.chromium.org/2935303003/diff/60001/chrome/browser/resource...
> chrome/browser/resources/print_preview/component.js:56: /** @return
{WebUIListenerTracker} Component's Web UI listener tracker. */
> Same here, let's add the "!".
> 
>
https://codereview.chromium.org/2935303003/diff/60001/ui/webui/resources/js/w...
> File ui/webui/resources/js/webui_listener_tracker.js (right):
> 
>
https://codereview.chromium.org/2935303003/diff/60001/ui/webui/resources/js/w...
> ui/webui/resources/js/webui_listener_tracker.js:14: function
WebUIListenerTracker() {
> The policy we've been following so far is to move things to
ui/webui/resources/ onyl after a 2nd client for that code is located. For
example WebUIListenerBehavior is used by both settings/ and md_user_manager/.
> 
> Can we move this under print_preview/ for now?

[rbpotter, 2017-06-15 01:01:08]

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

[commit-bot: I haz the power, 2017-06-15 01:01:25]

Dry run: CQ is trying da patch.

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

[rbpotter, 2017-06-15 01:03:01]

https://codereview.chromium.org/2935303003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/print_preview/component.js (right):

https://codereview.chromium.org/2935303003/diff/60001/chrome/browser/resource...
chrome/browser/resources/print_preview/component.js:26: * @private
{EventTracker}
On 2017/06/15 00:19:18, dpapad wrote:
> Since you are cleaning this up, let's also add the "!".

Done.

https://codereview.chromium.org/2935303003/diff/60001/chrome/browser/resource...
chrome/browser/resources/print_preview/component.js:32: * @private
{WebUIListenerTracker}
On 2017/06/15 00:19:18, dpapad wrote:
> Same here.

Done.

https://codereview.chromium.org/2935303003/diff/60001/chrome/browser/resource...
chrome/browser/resources/print_preview/component.js:56: /** @return
{WebUIListenerTracker} Component's Web UI listener tracker. */
On 2017/06/15 00:19:18, dpapad wrote:
> Same here, let's add the "!".

Done.

[dpapad, 2017-06-15 01:12:06]

LGTM

[commit-bot: I haz the power, 2017-06-15 02:36:44]

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

[commit-bot: I haz the power, 2017-06-15 02:36:45]

Dry run: This issue passed the CQ dry run.

[rbpotter, 2017-06-15 19:15:44]

The CQ bit was checked by rbpotter@chromium.org

[commit-bot: I haz the power, 2017-06-15 19:16:23]

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-06-15 19:23:25]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1497554144086770,
"parent_rev": "ce3e74bd59411bcd3c795f7f1d87b697463cd2a4", "commit_rev":
"30eeb64af965012272dc1ed7be0c0e6db5005871"}

[commit-bot: I haz the power, 2017-06-15 19:23:37]

Description was changed from

==========
Print Preview: Remove global onPrivetPrintFailed

Remove global onPrivetPrintFailed method and replace with a WebUI
listener.

Also rename to onPrintFailed/print-failed since this can also be
called due to an extension printer failure.

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

to

==========
Print Preview: Remove global onPrivetPrintFailed

Remove global onPrivetPrintFailed method and replace with a WebUI
listener.

Also rename to onPrintFailed/print-failed since this can also be
called due to an extension printer failure.

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

Review-Url: https://codereview.chromium.org/2935303003
Cr-Commit-Position: refs/heads/master@{#479778}
Committed:
https://chromium.googlesource.com/chromium/src/+/30eeb64af965012272dc1ed7be0c...
==========

[commit-bot: I haz the power, 2017-06-15 19:23:40]

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