clean up printing::Image and printing::Metafile

clean up printing::Image and printing::Metafile

printing::Image now takes raw data, rather than a metafile. this removes
the need for a virtual on printing::Metafile, Playback().
printing::Image gets move constructor
printing::Image loses dead code.

delete dead code: chrome/browser/printing/printing_layout_browsertest.cc

ServiceUtilityProcessHost takes metafile data, not a MetafilePlayer.  This will
help remove printing::Metafile::SafePlayback().

printing::PdfMetafileCg::OnRenderPage is added to copy data fewer times.

Review-Url: https://codereview.chromium.org/2812263002
Cr-Commit-Position: refs/heads/master@{#464714}
Committed: https://chromium.googlesource.com/chromium/src/+/574b10750dfea3d1103946615a1c8a4a9bff3b5c

[hal.canary, 2017-04-12 13:59:35]

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

[commit-bot: I haz the power, 2017-04-12 13:59:49]

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

[hal.canary, 2017-04-12 13:59:54]

halcanary@google.com changed reviewers:
+ thestig@chromium.org

[hal.canary, 2017-04-12 13:59:55]

PTAL

[hal.canary, 2017-04-12 14:35:36]

Description was changed from

==========
clean up printing::Image and printing::Metafile()

printing::Image now takes raw data, rather than a metafile. this removes
the need for a virtual on printing::Metafile, Playback().

delete dead code: chrome/browser/printing/printing_layout_browsertest.cc

ServiceUtilityProcessHost takes a Emf, not a MetafilePlayer.  This will
help remove printing::Metafile::SafePlayback().
==========

to

==========
clean up printing::Image and printing::Metafile

printing::Image now takes raw data, rather than a metafile. this removes
the need for a virtual on printing::Metafile, Playback().

delete dead code: chrome/browser/printing/printing_layout_browsertest.cc

ServiceUtilityProcessHost takes a Emf, not a MetafilePlayer.  This will
help remove printing::Metafile::SafePlayback().
==========

[commit-bot: I haz the power, 2017-04-12 14:59:04]

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

[commit-bot: I haz the power, 2017-04-12 14:59:05]

Dry run: This issue passed the CQ dry run.

[hal.canary, 2017-04-12 15:59:30]

Description was changed from

==========
clean up printing::Image and printing::Metafile

printing::Image now takes raw data, rather than a metafile. this removes
the need for a virtual on printing::Metafile, Playback().

delete dead code: chrome/browser/printing/printing_layout_browsertest.cc

ServiceUtilityProcessHost takes a Emf, not a MetafilePlayer.  This will
help remove printing::Metafile::SafePlayback().
==========

to

==========
clean up printing::Image and printing::Metafile

printing::Image now takes raw data, rather than a metafile. this removes
the need for a virtual on printing::Metafile, Playback().

delete dead code: chrome/browser/printing/printing_layout_browsertest.cc

ServiceUtilityProcessHost takes metafile data, not a MetafilePlayer.  This will
help remove printing::Metafile::SafePlayback().
==========

[hal.canary, 2017-04-12 16:01:55]

Patchset #2 (id:20001) has been deleted

[hal.canary, 2017-04-12 16:04:12]

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

[commit-bot: I haz the power, 2017-04-12 16:04:40]

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-04-12 16:42:55]

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

[commit-bot: I haz the power, 2017-04-12 16:42:56]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[hal.canary, 2017-04-12 17:57:14]

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

[commit-bot: I haz the power, 2017-04-12 17:58:02]

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-04-12 18:53:01]

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

[commit-bot: I haz the power, 2017-04-12 18:53:02]

Dry run: 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_...)

[Lei Zhang, 2017-04-12 19:33:33]

https://codereview.chromium.org/2812263002/diff/60001/chrome/browser/printing...
File chrome/browser/printing/printing_layout_browsertest.cc (left):

https://codereview.chromium.org/2812263002/diff/60001/chrome/browser/printing...
chrome/browser/printing/printing_layout_browsertest.cc:430:
embedded_test_server()->GetURL("/printing/popup_delayed_print.htm");
Can you file a bug to re-implement these tests and assign it to me? Otherwise
some/all of the test files are going unused.

https://codereview.chromium.org/2812263002/diff/60001/chrome/service/service_...
File chrome/service/service_utility_process_host.h (right):

https://codereview.chromium.org/2812263002/diff/60001/chrome/service/service_...
chrome/service/service_utility_process_host.h:50: virtual bool
OnRenderPDFPagesToMetafilePageDone(std::vector<char>& emf_data,
Why is |emf_data| non-const?

https://codereview.chromium.org/2812263002/diff/60001/chrome/service/service_...
chrome/service/service_utility_process_host.h:52: return false;
The impl in the header is causing win_clang bot to complain.

https://codereview.chromium.org/2812263002/diff/60001/printing/emf_win.h
File printing/emf_win.h (right):

https://codereview.chromium.org/2812263002/diff/60001/printing/emf_win.h#newc...
printing/emf_win.h:81: // "Plays" the EMF buffer in a HDC. It is the same effect
as calling the
nit: Can you move this down to keep all the overrides together?

https://codereview.chromium.org/2812263002/diff/60001/printing/image.h
File printing/image.h (right):

https://codereview.chromium.org/2812263002/diff/60001/printing/image.h#newcode67
printing/image.h:67: bool LoadPng(const std::string& compressed);
This is dead code, BTW.

[hal.canary, 2017-04-13 20:55:14]

Description was changed from

==========
clean up printing::Image and printing::Metafile

printing::Image now takes raw data, rather than a metafile. this removes
the need for a virtual on printing::Metafile, Playback().

delete dead code: chrome/browser/printing/printing_layout_browsertest.cc

ServiceUtilityProcessHost takes metafile data, not a MetafilePlayer.  This will
help remove printing::Metafile::SafePlayback().
==========

to

==========
clean up printing::Image and printing::Metafile

printing::Image now takes raw data, rather than a metafile. this removes
the need for a virtual on printing::Metafile, Playback().
printing::Image gets move constructor
printing::Image loses dead code.

delete dead code: chrome/browser/printing/printing_layout_browsertest.cc

ServiceUtilityProcessHost takes metafile data, not a MetafilePlayer.  This will
help remove printing::Metafile::SafePlayback().

PdfMetafileCg::OnRenderPage is added to copy data fewer times.
==========

[hal.canary, 2017-04-13 20:55:33]

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

[hal.canary, 2017-04-13 20:55:38]

https://codereview.chromium.org/2812263002/diff/60001/chrome/browser/printing...
File chrome/browser/printing/printing_layout_browsertest.cc (left):

https://codereview.chromium.org/2812263002/diff/60001/chrome/browser/printing...
chrome/browser/printing/printing_layout_browsertest.cc:430:
embedded_test_server()->GetURL("/printing/popup_delayed_print.htm");
On 2017/04/12 19:33:33, Lei Zhang wrote:
> Can you file a bug to re-implement these tests and assign it to me? Otherwise
> some/all of the test files are going unused.

Done.

https://codereview.chromium.org/2812263002/diff/60001/chrome/service/service_...
File chrome/service/service_utility_process_host.h (right):

https://codereview.chromium.org/2812263002/diff/60001/chrome/service/service_...
chrome/service/service_utility_process_host.h:50: virtual bool
OnRenderPDFPagesToMetafilePageDone(std::vector<char>& emf_data,
oversight on my part.

https://codereview.chromium.org/2812263002/diff/60001/chrome/service/service_...
chrome/service/service_utility_process_host.h:52: return false;
On 2017/04/12 19:33:33, Lei Zhang wrote:
> The impl in the header is causing win_clang bot to complain.

Done.

https://codereview.chromium.org/2812263002/diff/60001/printing/emf_win.h
File printing/emf_win.h (right):

https://codereview.chromium.org/2812263002/diff/60001/printing/emf_win.h#newc...
printing/emf_win.h:81: // "Plays" the EMF buffer in a HDC. It is the same effect
as calling the
On 2017/04/12 19:33:33, Lei Zhang wrote:
> nit: Can you move this down to keep all the overrides together?

Done.

https://codereview.chromium.org/2812263002/diff/60001/printing/image.h
File printing/image.h (right):

https://codereview.chromium.org/2812263002/diff/60001/printing/image.h#newcode67
printing/image.h:67: bool LoadPng(const std::string& compressed);
On 2017/04/12 19:33:33, Lei Zhang wrote:
> This is dead code, BTW.

I'll clean up some of this dead code....and I just refactored PdfMetafileCg
again!

[commit-bot: I haz the power, 2017-04-13 20:55:58]

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

[Lei Zhang, 2017-04-13 21:21:20]

https://codereview.chromium.org/2812263002/diff/60001/printing/image.h
File printing/image.h (right):

https://codereview.chromium.org/2812263002/diff/60001/printing/image.h#newcode67
printing/image.h:67: bool LoadPng(const std::string& compressed);
On 2017/04/13 20:55:38, hal.canary wrote:
> On 2017/04/12 19:33:33, Lei Zhang wrote:
> > This is dead code, BTW.
> 
> I'll clean up some of this dead code....and I just refactored PdfMetafileCg
> again!  

There's certainly a lot of cruft here.

https://codereview.chromium.org/2812263002/diff/80001/chrome/service/service_...
File chrome/service/service_utility_process_host.cc (right):

https://codereview.chromium.org/2812263002/diff/80001/chrome/service/service_...
chrome/service/service_utility_process_host.cc:432: const std::vector<char>&,
We usually write out the parameter name. Comment it out if it's unused. e.g.

const std::vector<char>& /* emf_data */,

https://codereview.chromium.org/2812263002/diff/100001/components/printing/te...
File components/printing/test/mock_printer.cc (right):

https://codereview.chromium.org/2812263002/diff/100001/components/printing/te...
components/printing/test/mock_printer.cc:55: : source_size_(source_size),
image_(std::move(image)) {
I'm not 100% sure, but maybe |image| should just be passed in by value in this
case? See https://www.chromium.org/rvalue-references item 8.

https://codereview.chromium.org/2812263002/diff/100001/components/printing/te...
components/printing/test/mock_printer.cc:213: MockPrinterPage* page_data = new
MockPrinterPage(
Do we even need the |paga_data| variable?

[commit-bot: I haz the power, 2017-04-13 21:57:22]

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

[commit-bot: I haz the power, 2017-04-13 21:57:22]

Dry run: This issue passed the CQ dry run.

[hal.canary, 2017-04-13 23:57:26]

https://codereview.chromium.org/2812263002/diff/100001/components/printing/te...
File components/printing/test/mock_printer.cc (right):

https://codereview.chromium.org/2812263002/diff/100001/components/printing/te...
components/printing/test/mock_printer.cc:55: : source_size_(source_size),
image_(std::move(image)) {
On 2017/04/13 21:21:20, Lei Zhang wrote:
> I'm not 100% sure, but maybe |image| should just be passed in by value in this
> case? See https://www.chromium.org/rvalue-references item 8.

you are right.  good catch.

https://codereview.chromium.org/2812263002/diff/100001/components/printing/te...
components/printing/test/mock_printer.cc:213: MockPrinterPage* page_data = new
MockPrinterPage(
On 2017/04/13 21:21:20, Lei Zhang wrote:
> Do we even need the |paga_data| variable?

nope.  Let's fix that.

[hal.canary, 2017-04-13 23:57:42]

Description was changed from

==========
clean up printing::Image and printing::Metafile

printing::Image now takes raw data, rather than a metafile. this removes
the need for a virtual on printing::Metafile, Playback().
printing::Image gets move constructor
printing::Image loses dead code.

delete dead code: chrome/browser/printing/printing_layout_browsertest.cc

ServiceUtilityProcessHost takes metafile data, not a MetafilePlayer.  This will
help remove printing::Metafile::SafePlayback().

PdfMetafileCg::OnRenderPage is added to copy data fewer times.
==========

to

==========
clean up printing::Image and printing::Metafile

printing::Image now takes raw data, rather than a metafile. this removes
the need for a virtual on printing::Metafile, Playback().
printing::Image gets move constructor
printing::Image loses dead code.

delete dead code: chrome/browser/printing/printing_layout_browsertest.cc

ServiceUtilityProcessHost takes metafile data, not a MetafilePlayer.  This will
help remove printing::Metafile::SafePlayback().

printing::PdfMetafileCg::OnRenderPage is added to copy data fewer times.
==========

[hal.canary, 2017-04-13 23:57:56]

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

[commit-bot: I haz the power, 2017-04-13 23:58:20]

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

[Lei Zhang, 2017-04-14 00:04:34]

lgtm with green bots.

https://codereview.chromium.org/2812263002/diff/100001/components/printing/te...
File components/printing/test/mock_printer.cc (right):

https://codereview.chromium.org/2812263002/diff/100001/components/printing/te...
components/printing/test/mock_printer.cc:55: : source_size_(source_size),
image_(std::move(image)) {
On 2017/04/13 23:57:25, hal.canary wrote:
> On 2017/04/13 21:21:20, Lei Zhang wrote:
> > I'm not 100% sure, but maybe |image| should just be passed in by value in
this
> > case? See https://www.chromium.org/rvalue-references item 8.
> 
> you are right.  good catch.

It's a bit counter-intuitive after years of only passing by const-ref or
pointer, and only passing by value when it's POD.

https://codereview.chromium.org/2812263002/diff/120001/components/printing/te...
File components/printing/test/mock_printer.cc (right):

https://codereview.chromium.org/2812263002/diff/120001/components/printing/te...
components/printing/test/mock_printer.cc:54: printing::Image image)
Need to change components/printing/test/mock_printer.h now.

[commit-bot: I haz the power, 2017-04-14 00:15:24]

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

[commit-bot: I haz the power, 2017-04-14 00:15:25]

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

[hal.canary, 2017-04-14 01:07:43]

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

[commit-bot: I haz the power, 2017-04-14 01:08:07]

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-04-14 02:06:51]

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

[commit-bot: I haz the power, 2017-04-14 02:06:52]

Dry run: This issue passed the CQ dry run.

[hal.canary, 2017-04-14 11:40:28]

The CQ bit was checked by halcanary@google.com

[hal.canary, 2017-04-14 11:40:28]

The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/2812263002/#ps140001
(title: "definition matches declaration")

[commit-bot: I haz the power, 2017-04-14 11:40:51]

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-04-14 11:44:41]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1492170028068930,
"parent_rev": "f0abffcea31f665a2b1cc1979a955cf308c4dcb4", "commit_rev":
"574b10750dfea3d1103946615a1c8a4a9bff3b5c"}

[commit-bot: I haz the power, 2017-04-14 11:45:35]

Description was changed from

==========
clean up printing::Image and printing::Metafile

printing::Image now takes raw data, rather than a metafile. this removes
the need for a virtual on printing::Metafile, Playback().
printing::Image gets move constructor
printing::Image loses dead code.

delete dead code: chrome/browser/printing/printing_layout_browsertest.cc

ServiceUtilityProcessHost takes metafile data, not a MetafilePlayer.  This will
help remove printing::Metafile::SafePlayback().

printing::PdfMetafileCg::OnRenderPage is added to copy data fewer times.
==========

to

==========
clean up printing::Image and printing::Metafile

printing::Image now takes raw data, rather than a metafile. this removes
the need for a virtual on printing::Metafile, Playback().
printing::Image gets move constructor
printing::Image loses dead code.

delete dead code: chrome/browser/printing/printing_layout_browsertest.cc

ServiceUtilityProcessHost takes metafile data, not a MetafilePlayer.  This will
help remove printing::Metafile::SafePlayback().

printing::PdfMetafileCg::OnRenderPage is added to copy data fewer times.

Review-Url: https://codereview.chromium.org/2812263002
Cr-Commit-Position: refs/heads/master@{#464714}
Committed:
https://chromium.googlesource.com/chromium/src/+/574b10750dfea3d1103946615a1c...
==========

[commit-bot: I haz the power, 2017-04-14 11:45:36]

Committed patchset #7 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/574b10750dfea3d1103946615a1c...

[Lei Zhang, 2017-04-26 20:36:18]

A revert of this CL (patchset #7 id:140001) has been created in
https://codereview.chromium.org/2835193007/ by thestig@chromium.org.

The reason for reverting is: Reverting so https://crrev.com/463828 can be
cleanly reverted.

r463828 is likely causing https://crbug.com/712309

.