store MetafileSkiaWrapper in plugin instead of canvas

store MetafileSkiaWrapper in plugin instead of canvas, since the former is reference counted, and the latter is on the chopping block to become not reference counted.

Additionally, the metafile is all that is actually needed for printEnd, so it is a bit overspecified to pass a canvas around.

BUG=666154

[reed1, 2016-11-17 20:50:18]

Description was changed from

==========
store MetafileSkiaWrapper in plugin instead of canvas

BUG=
==========

to

==========
store MetafileSkiaWrapper in plugin instead of canvas

BUG=666154
==========

[reed1, 2016-11-17 20:50:22]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 20:51:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[reed1, 2016-11-17 21:02:28]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 21:04:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[reed1, 2016-11-17 21:41:17]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 21:42:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[reed1, 2016-11-17 21:42:44]

Description was changed from

==========
store MetafileSkiaWrapper in plugin instead of canvas

BUG=666154
==========

to

==========
store MetafileSkiaWrapper in plugin instead of canvas, since the former is
reference counted, and the latter is on the chopping block to become not
reference counted.

Additionally, the metafile is all that is actually needed for printEnd, so it is
a bit overspecified to pass a canvas around.

BUG=666154
==========

[reed1, 2016-11-17 21:42:58]

reed@google.com changed reviewers:
+ bbudge@chromium.org, raymes@chromium.org

[commit-bot: I haz the power, 2016-11-17 22:01:03]

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

[commit-bot: I haz the power, 2016-11-17 22:01:04]

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

[reed1, 2016-11-17 22:13:45]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 22:14:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bbudge, 2016-11-17 22:37:00]

LGTM w/comments

https://codereview.chromium.org/2512783002/diff/50001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/2512783002/diff/50001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1802:
metafile_wrapper_.reset();
Would a DCHECK(!metafile_wrapper_) make sense here? I'm assuming it's bad to
hold on to a metafile for a long time.

https://codereview.chromium.org/2512783002/diff/50001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:2040: if (metafile)
How about:
if (wrapper) {
  printing::PdfMetafileSkia* metafile = ... etc.

https://codereview.chromium.org/2512783002/diff/50001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.h (right):

https://codereview.chromium.org/2512783002/diff/50001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.h:843: //
|metafile_wrapper_|
nit: formatting is a little hard to read here.

[commit-bot: I haz the power, 2016-11-17 23:05:41]

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

[commit-bot: I haz the power, 2016-11-17 23:05:42]

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

[Lei Zhang, 2016-11-18 01:53:36]

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

[Lei Zhang, 2016-11-18 01:53:37]

Just some nits. I will help take a look at the red cast* bots.

https://codereview.chromium.org/2512783002/diff/50001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/2512783002/diff/50001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1854: if
(!ranges_.empty())
nit: needs curly braces now.

https://codereview.chromium.org/2512783002/diff/50001/printing/metafile_skia_...
File printing/metafile_skia_wrapper.h (right):

https://codereview.chromium.org/2512783002/diff/50001/printing/metafile_skia_...
printing/metafile_skia_wrapper.h:28: PdfMetafileSkia* getMetafile() const {
return metafile_; }
nit: GetMetafile(), or just metafile() since it's a simple accessor.

[Lei Zhang, 2016-11-18 02:25:53]

https://codereview.chromium.org/2512783002/diff/50001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/2512783002/diff/50001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:97: #include
"printing/metafile_skia_wrapper.h"
BTW, this is already on line 135.

https://codereview.chromium.org/2512783002/diff/50001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.h (right):

https://codereview.chromium.org/2512783002/diff/50001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.h:848:
sk_sp<printing::MetafileSkiaWrapper> metafile_wrapper_;
So this is slightly problematic as-is because it always pulls in printing as a
dependency, and there are build configs where printing is turned off. I'll see
if we can ifdef this...

[Lei Zhang, 2016-11-18 02:38:34]

I put my copy of this CL up at: https://codereview.chromium.org/2518443002/

I started try jobs. Let's see how that goes.

Feel free to grab it and run with it. In which case, lgtm, of course.

[reed1, 2016-11-18 19:00:14]

Abandoning in favor of https://codereview.chromium.org/2515783002