printing::Metafile: Simplify OS_MACOSX-specific code path

printing::Metafile:  Simplify OS_MACOSX-specific code path

  - printing::Metafile
      * move MacRenderPageParams to PdfMetafileCg
      * remove virtual RenderPage()
  - printing::PdfMetafileCg
      * Add MacRenderPageParams
      * RenderPage no longer an override
  - printing::PdfMetafileSkia
      * no longer implment RenderPage()
      * code moved to printing::PrintedDocument::RenderPrintedPage()

This will help isolate BUG=chromium:374359
R=thestig@chromium.org

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

[hal.canary, 2017-04-07 17:38:32]

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

[hal.canary, 2017-04-07 17:38:42]

PTAL

[commit-bot: I haz the power, 2017-04-07 17:38: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-04-07 18:49:12]

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

[commit-bot: I haz the power, 2017-04-07 18:49:13]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Lei Zhang, 2017-04-07 18:53:41]

Looking... you got red Mac trybots.

[Lei Zhang, 2017-04-08 01:24:25]

I wasn't able to eyeball the red bots. Hope you can figure that out.

https://codereview.chromium.org/2802093006/diff/1/printing/pdf_metafile_cg_mac.h
File printing/pdf_metafile_cg_mac.h (right):

https://codereview.chromium.org/2802093006/diff/1/printing/pdf_metafile_cg_ma...
printing/pdf_metafile_cg_mac.h:10: #include "build/build_config.h"
Not needed?

https://codereview.chromium.org/2802093006/diff/1/printing/pdf_metafile_cg_ma...
printing/pdf_metafile_cg_mac.h:14: typedef struct CGContext* CGContextRef;
Can we use "using CGContextRef = struct CGContext*;" ?

https://codereview.chromium.org/2802093006/diff/1/printing/pdf_metafile_cg_ma...
printing/pdf_metafile_cg_mac.h:44: // Renders the given page from src_buffer
into |rect| in the given context.
Refer to |src_buffer| consistently like other variables.

https://codereview.chromium.org/2802093006/diff/1/printing/pdf_metafile_cg_ma...
printing/pdf_metafile_cg_mac.h:47: static bool RenderPage(const void*
src_buffer,
Change this to take: const std::vector& buffer, since all the callers have a
vector to pass in already?

https://codereview.chromium.org/2802093006/diff/1/printing/pdf_metafile_cg_ma...
File printing/pdf_metafile_cg_mac_unittest.cc (right):

https://codereview.chromium.org/2802093006/diff/1/printing/pdf_metafile_cg_ma...
printing/pdf_metafile_cg_mac_unittest.cc:45: EXPECT_GT(size, 0U);
This should be an ASSERT_GT(), BTW.

https://codereview.chromium.org/2802093006/diff/1/printing/printed_document_m...
File printing/printed_document_mac.cc (right):

https://codereview.chromium.org/2802093006/diff/1/printing/printed_document_m...
printing/printed_document_mac.cc:7: #include
<ApplicationServices/ApplicationServices.h>
BTW, I'm clueless about #import vs #include, so maybe ask someone with more Mac
experience if you are not certain.

https://codereview.chromium.org/2802093006/diff/1/printing/printed_document_m...
printing/printed_document_mac.cc:38: struct PdfMetafileCg::RenderPageParams
params;
I think you can omit the struct keyword.

[hal.canary, 2017-04-11 18:35:48]

Description was changed from

==========
printing::Metafile:  Simplify OS_MACOSX-specific code path

  - printing::Metafile
      * move MacRenderPageParams to PdfMetafileCg
      * remove virtual RenderPage()
  - printing::PdfMetafileCg
      * Add MacRenderPageParams
      * RenderPage no longer an override
      * Add InitFromMetafile() function
  - printing::PdfMetafileSkia
      * no longer implment RenderPage()
      * code moved to printing::PrintedDocument::RenderPrintedPage()

This will help isolate BUG=chromium:374359

R=thestig@chromium.org
==========

to

==========
printing::Metafile:  Simplify OS_MACOSX-specific code path

  - printing::Metafile
      * move MacRenderPageParams to PdfMetafileCg
      * remove virtual RenderPage()
  - printing::PdfMetafileCg
      * Add MacRenderPageParams
      * RenderPage no longer an override
  - printing::PdfMetafileSkia
      * no longer implment RenderPage()
      * code moved to printing::PrintedDocument::RenderPrintedPage()

This will help isolate BUG=chromium:374359

R=thestig@chromium.org
==========

[hal.canary, 2017-04-11 19:07:27]

https://codereview.chromium.org/2802093006/diff/1/printing/pdf_metafile_cg_mac.h
File printing/pdf_metafile_cg_mac.h (right):

https://codereview.chromium.org/2802093006/diff/1/printing/pdf_metafile_cg_ma...
printing/pdf_metafile_cg_mac.h:10: #include "build/build_config.h"
On 2017/04/08 01:24:24, Lei Zhang wrote:
> Not needed?

I reverted most of this file.   No longer applicable.

https://codereview.chromium.org/2802093006/diff/1/printing/pdf_metafile_cg_ma...
printing/pdf_metafile_cg_mac.h:14: typedef struct CGContext* CGContextRef;
On 2017/04/08 01:24:24, Lei Zhang wrote:
> Can we use "using CGContextRef = struct CGContext*;" ?

I reverted most of this file.   No longer applicable.

https://codereview.chromium.org/2802093006/diff/1/printing/pdf_metafile_cg_ma...
printing/pdf_metafile_cg_mac.h:44: // Renders the given page from src_buffer
into |rect| in the given context.
On 2017/04/08 01:24:24, Lei Zhang wrote:
> Refer to |src_buffer| consistently like other variables.

Done.

https://codereview.chromium.org/2802093006/diff/1/printing/pdf_metafile_cg_ma...
printing/pdf_metafile_cg_mac.h:47: static bool RenderPage(const void*
src_buffer,
On 2017/04/08 01:24:24, Lei Zhang wrote:
> Change this to take: const std::vector& buffer, since all the callers have a
> vector to pass in already?

Done.

https://codereview.chromium.org/2802093006/diff/1/printing/pdf_metafile_cg_ma...
File printing/pdf_metafile_cg_mac_unittest.cc (right):

https://codereview.chromium.org/2802093006/diff/1/printing/pdf_metafile_cg_ma...
printing/pdf_metafile_cg_mac_unittest.cc:45: EXPECT_GT(size, 0U);
On 2017/04/08 01:24:24, Lei Zhang wrote:
> This should be an ASSERT_GT(), BTW.

I reverted changes to this file.   No longer applicable.

https://codereview.chromium.org/2802093006/diff/1/printing/printed_document_m...
File printing/printed_document_mac.cc (right):

https://codereview.chromium.org/2802093006/diff/1/printing/printed_document_m...
printing/printed_document_mac.cc:7: #include
<ApplicationServices/ApplicationServices.h>
On 2017/04/08 01:24:25, Lei Zhang wrote:
> BTW, I'm clueless about #import vs #include, so maybe ask someone with more
Mac
> experience if you are not certain.

Acknowledged.

https://codereview.chromium.org/2802093006/diff/1/printing/printed_document_m...
printing/printed_document_mac.cc:38: struct PdfMetafileCg::RenderPageParams
params;
On 2017/04/08 01:24:25, Lei Zhang wrote:
> I think you can omit the struct keyword.

Done.

[Lei Zhang, 2017-04-11 20:46:29]

lgtm

[hal.canary, 2017-04-11 22:16:34]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2017-04-11 22:17:14]

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-11 23:18:24]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491948994618500,
"parent_rev": "57078b411bb68d6db438b05bd90e2bd60cb670e4", "commit_rev":
"c843c89f3051e4b01cf032e52c478000f7ded0ee"}

[commit-bot: I haz the power, 2017-04-11 23:19:14]

Description was changed from

==========
printing::Metafile:  Simplify OS_MACOSX-specific code path

  - printing::Metafile
      * move MacRenderPageParams to PdfMetafileCg
      * remove virtual RenderPage()
  - printing::PdfMetafileCg
      * Add MacRenderPageParams
      * RenderPage no longer an override
  - printing::PdfMetafileSkia
      * no longer implment RenderPage()
      * code moved to printing::PrintedDocument::RenderPrintedPage()

This will help isolate BUG=chromium:374359

R=thestig@chromium.org
==========

to

==========
printing::Metafile:  Simplify OS_MACOSX-specific code path

  - printing::Metafile
      * move MacRenderPageParams to PdfMetafileCg
      * remove virtual RenderPage()
  - printing::PdfMetafileCg
      * Add MacRenderPageParams
      * RenderPage no longer an override
  - printing::PdfMetafileSkia
      * no longer implment RenderPage()
      * code moved to printing::PrintedDocument::RenderPrintedPage()

This will help isolate BUG=chromium:374359

R=thestig@chromium.org

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

[commit-bot: I haz the power, 2017-04-11 23:19:17]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/c843c89f3051e4b01cf032e52c47...

[Lei Zhang, 2017-04-26 22:21:27]

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

The reason for reverting is: Likely causing https://crbug.com/712309.