1345710 - [Mortar] [Windows] Implement Emf, PDFEngineExports and PdfPrintHelperWin

Status: RESOLVED FIXED

(Core :: Printing: Output, enhancement)

Description

[Farmer Tseng[:fatseng](inactive)]

I would like to implement Emf, PDFEngineExports and PdfPrintHelperWin three classes.
They would be used in plugin process and chrome process to deal with PDF printing.

Comment 1

[Farmer Tseng[:fatseng](inactive)]

Attachment: Bug 1345710 - part1: implement a WindowsEMF class to support Windows Enhance Metafile

Review commit: https://reviewboard.mozilla.org/r/118470/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118470/

Comment 2

[Farmer Tseng[:fatseng](inactive)]

Attachment: Bug 1345710 - part2: implement a PDFiumEngineShim class to link to PDFium symbols

Review commit: https://reviewboard.mozilla.org/r/118472/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118472/

Comment 3

[Farmer Tseng[:fatseng](inactive)]

Attachment: Bug 1345710 - part3: implement PDFViaEMFPrintHelper to convert PDF to EMF

Review commit: https://reviewboard.mozilla.org/r/118474/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118474/

Comment 4

[Farmer Tseng[:fatseng](inactive)]

Attachment: Bug 1345710 - fix nsWindowsGfx.cpp build break

Review commit: https://reviewboard.mozilla.org/r/118476/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118476/

Comment 5

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8845293 [details]
Bug 1345710 - part1: implement a WindowsEMF class to support Windows Enhance Metafile

I call Windows APIs in Emf class, this class help create/store EMF and playback EMF on printer DC.

Comment 6

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8845294 [details]
Bug 1345710 - part2: implement a PDFiumEngineShim class to link to PDFium symbols

I implement PDFEngineExports class to find function symbols from pdfium library and export them.

Comment 7

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8845295 [details]
Bug 1345710 - part3: implement PDFViaEMFPrintHelper to convert PDF to EMF

Utilize PDFEngineExports and Emf classes in PdfPrintHelperWin. It loads PDF file and converts to EMF.

Comment 8

[Jonathan Watt [:jwatt]]

Comment on attachment 8845293 [details]
Bug 1345710 - part1: implement a WindowsEMF class to support Windows Enhance Metafile

https://reviewboard.mozilla.org/r/118470/#review121904

Maybe move the files to the widget/windows/... directory. And probably rename the file to match the name of the class.

::: gfx/thebes/PrintEmfWindows.h:16
(Diff revision 1)
> +
> +namespace mozilla {
> +namespace gfx {
> +
> +/**
> + * Windows Enhance Metafile target.

Please add a link to some external documentation. Even a link to the wikipedia page would be good.

Also please add a description of roughly what this is for and how it works. Something like "Provides a Windows HDC that can be drawn to to generate an EMF file. This is useful when we want to send an EMF file to the native Windows print spooler."

::: gfx/thebes/PrintEmfWindows.h:18
(Diff revision 1)
> +namespace gfx {
> +
> +/**
> + * Windows Enhance Metafile target.
> + */
> +class Emf

It would be good to indicate that objects of this type represent a file. Could we call it EMFFile?

::: gfx/thebes/PrintEmfWindows.h:34
(Diff revision 1)
> +  // Metafile methods.
> +  bool Init();
> +
> +  // Generates a new metafile that will record every GDI command, and will
> +  // be saved to |metafile_path|.
> +  bool InitToFile(const wchar_t* metafile_path);

I think it would be better to make this an optional argument of the Init method.

::: gfx/thebes/PrintEmfWindows.h:37
(Diff revision 1)
> +  // Generates a new metafile that will record every GDI command, and will
> +  // be saved to |metafile_path|.
> +  bool InitToFile(const wchar_t* metafile_path);
> +
> +  // Initializes the Emf with the data in |metafile_path|.
> +  bool InitFromFile(const wchar_t* metafile_path);

And call this InitFromFileContents.

::: gfx/thebes/PrintEmfWindows.h:40
(Diff revision 1)
> +
> +  // Initializes the Emf with the data in |metafile_path|.
> +  bool InitFromFile(const wchar_t* metafile_path);
> +
> +  //bool FinishPage();
> +  bool FinishDocument();

Can you add a comment giving a hint what this does (why it is needed)?

::: gfx/thebes/PrintEmfWindows.h:42
(Diff revision 1)
> +  bool InitFromFile(const wchar_t* metafile_path);
> +
> +  //bool FinishPage();
> +  bool FinishDocument();
> +
> +  HDC GetDC();

Can you add a comment noting where the returned DC's output is stored. For example, in the file at the path provided by Init if a path is provided, or in memory if there was no path provided? There may be memory issues in the latter case, so it would be good to call that out.

::: gfx/thebes/PrintEmfWindows.h:44
(Diff revision 1)
> +  //bool FinishPage();
> +  bool FinishDocument();
> +
> +  HDC GetDC();
> +
> +  bool Playback(HDC hdc, const RECT* rect);

Please document why this exists (what it is used to achieve).

Comment 9

[Jonathan Watt [:jwatt]]

Comment on attachment 8845294 [details]
Bug 1345710 - part2: implement a PDFiumEngineShim class to link to PDFium symbols

https://reviewboard.mozilla.org/r/118472/#review121950

::: widget/windows/PDFEngineExports.h:17
(Diff revision 1)
> +#include <windows.h>
> +
> +namespace mozilla {
> +namespace widget {
> +
> +// Page rendering flags. They can be combined with bit-wise OR.

I'm worried that in the future we may update the PDFium source in our tree, but fail to keep these defines etc. in sync. Can you open a bug to replace these defines with an appropriate include once the PDFium source has landed in the tree, and add a comment here about needing to fix that bug.

::: widget/windows/PDFEngineExports.h:105
(Diff revision 1)
> +                                    int flags);
> +typedef void (*FPDF_RenderPage_Close_Pfn) (FPDF_PAGE page);
> +
> +
> +/**
> + * PDF Engine.

I think having "Exports" in the name is a bit confusing. Maybe rename this to "PDFiumEngine". Also please expand on the comment here. :) Something like:

  /**
   * This class exposes an interface to the PDFium library and takes care of loading and linking to the appropriate PDFium symbols.
   */

Comment 10

[Farmer Tseng[:fatseng](inactive)]

(In reply to Jonathan Watt [:jwatt] from comment #9)
> Comment on attachment 8845294 [details]
> Bug 1345710 - part2: implement PDFEngineExports to export pdfium functions
> 
> https://reviewboard.mozilla.org/r/118472/#review121950
> 
> ::: widget/windows/PDFEngineExports.h:17
> (Diff revision 1)
> > +#include <windows.h>
> > +
> > +namespace mozilla {
> > +namespace widget {
> > +
> > +// Page rendering flags. They can be combined with bit-wise OR.
> 
> I'm worried that in the future we may update the PDFium source in our tree,
> but fail to keep these defines etc. in sync. Can you open a bug to replace
> these defines with an appropriate include once the PDFium source has landed
> in the tree, and add a comment here about needing to fix that bug.
> 
Create Bug 1349139 to follow up.

> ::: widget/windows/PDFEngineExports.h:105
> (Diff revision 1)
> > +                                    int flags);
> > +typedef void (*FPDF_RenderPage_Close_Pfn) (FPDF_PAGE page);
> > +
> > +
> > +/**
> > + * PDF Engine.
> 
> I think having "Exports" in the name is a bit confusing. Maybe rename this
> to "PDFiumEngine". Also please expand on the comment here. :) Something like:

There are already "PDFiumEngine" and "PDFiumEngineExports" two classes name in PDFium source.
Per discussed with :jwatt, we would like to rename it to PDFiumEngineShim.

Comment 11

[Farmer Tseng[:fatseng](inactive)]

Attachment: Bug 1345710 - fix nsWindowDbg build break

Review commit: https://reviewboard.mozilla.org/r/123020/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/123020/

Comment 12

[Farmer Tseng[:fatseng](inactive)]

Attachment: Bug 1345710 - fix redefinition warning in Unified_cpp_widget_windows3

Review commit: https://reviewboard.mozilla.org/r/123022/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/123022/

Comment 13

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8845293 [details]
Bug 1345710 - part1: implement a WindowsEMF class to support Windows Enhance Metafile

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118470/diff/1-2/

Comment 14

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8845294 [details]
Bug 1345710 - part2: implement a PDFiumEngineShim class to link to PDFium symbols

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118472/diff/1-2/

Comment 15

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8845295 [details]
Bug 1345710 - part3: implement PDFViaEMFPrintHelper to convert PDF to EMF

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118474/diff/1-2/

Comment 16

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8845296 [details]
Bug 1345710 - fix nsWindowsGfx.cpp build break

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118476/diff/1-2/

Comment 17

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8845296 [details]
Bug 1345710 - fix nsWindowsGfx.cpp build break

Hit nsWindowGfx.cpp build break.
It leaks #include "WinCompositorWidget.h"

1:34.38 c:/mozilla-source/gecko-0215/widget/windows/nsWindowGfx.cpp(252): error C2027: use of undefined type 'mozilla::widget::CompositorWidgetDelegate'
 1:34.38 c:\mozilla-source\gecko-0215\obj-i686-pc-mingw32\dist\include\mozilla/widget/CompositorWidget.h(46): note: see declaration of 'mozilla::widget::CompositorWidgetDelegate'
 1:34.38 c:/mozilla-source/gecko-0215/widget/windows/nsWindowGfx.cpp(252): error C2227: left of '->GetTransparentDC' must point to class/struct/union/generic type
 1:34.38 c:/mozilla-source/gecko-0215/widget/windows/nsWindowGfx.cpp(326): error C2027: use of undefined type 'mozilla::widget::WinCompositorWidget'
 1:34.38 c:\mozilla-source\gecko-0215\obj-i686-pc-mingw32\dist\include\mozilla/widget/CompositorWidget.h(36): note: see declaration of 'mozilla::widget::WinCompositorWidget'
 1:34.38 c:/mozilla-source/gecko-0215/widget/windows/nsWindowGfx.cpp(326): error C2039: 'EnsureTransparentSurface': is not a member of 'RefPtr<mozilla::widget::WinCompositorWidget>'
 1:34.38 c:\mozilla-source\gecko-0215\widget\windows\nsWindow.h(647): note: see declaration of 'RefPtr<mozilla::widget::WinCompositorWidget>'
 1:34.38 c:/mozilla-source/gecko-0215/widget/windows/nsWindowGfx.cpp(391): error C2027: use of undefined type 'mozilla::widget::WinCompositorWidget'
 1:34.38 c:\mozilla-source\gecko-0215\obj-i686-pc-mingw32\dist\include\mozilla/widget/CompositorWidget.h(36): note: see declaration of 'mozilla::widget::WinCompositorWidget'
 1:34.38 c:/mozilla-source/gecko-0215/widget/windows/nsWindowGfx.cpp(391): error C2039: 'RedrawTransparentWindow': is not a member of 'RefPtr<mozilla::widget::WinCompositorWidget>'
 1:34.38 c:\mozilla-source\gecko-0215\widget\windows\nsWindow.h(647): note: see declaration of 'RefPtr<mozilla::widget::WinCompositorWidget>'

Comment 18

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8850440 [details]
Bug 1345710 - fix nsWindowDbg build break

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123020/diff/1-2/

Comment 19

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8850441 [details]
Bug 1345710 -  fix redefinition warning in Unified_cpp_widget_windows3

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123022/diff/1-2/

Comment 20

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8850440 [details]
Bug 1345710 - fix nsWindowDbg build break

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123020/diff/2-3/

Comment 21

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8850441 [details]
Bug 1345710 -  fix redefinition warning in Unified_cpp_widget_windows3

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123022/diff/2-3/

Comment 22

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8850440 [details]
Bug 1345710 - fix nsWindowDbg build break

Replace "LogLevel" by "mozilla::LogLevel" to fix build break.

Build failed log:
0:44.26 c:/mozilla-source/gecko-0215/widget/windows/nsWindowDbg.cpp(43): error C2653: 'LogLevel': is not a class or namespace name
0:44.27 c:/mozilla-source/gecko-0215/widget/windows/nsWindowDbg.cpp(43): error C2065: 'Info': undeclared identifier
0:44.29 c:/mozilla-source/gecko-0215/widget/windows/nsWindowDbg.cpp(54): error C2653: 'LogLevel': is not a class or namespace name
0:44.30 c:/mozilla-source/gecko-0215/widget/windows/nsWindowDbg.cpp(54): error C2065: 'Error': undeclared identifier

Comment 23

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8850441 [details]
Bug 1345710 -  fix redefinition warning in Unified_cpp_widget_windows3

There are some redefintion warning as following.
In order to fix these warning,I reordered including sequence.
"WinMessages.h" should be included after "winuser.h".

1:50.12 Unified_cpp_widget_windows3.cpp
 1:50.12 Warning: C4005 in C:\Program Files (x86)\Windows Kits\10\include\10.0.14393.0\um\winuser.h: 'WM_GETOBJECT': macro redefinition
 1:50.12 C:\Program Files (x86)\Windows Kits\10\include\10.0.14393.0\um\winuser.h(2032): warning C4005: 'WM_GETOBJECT': macro redefinition
 1:50.12 c:\mozilla-source\gecko-0215\obj-i686-pc-mingw32\dist\include\mozilla/widget/WinMessages.h(72): note: see previous definition of 'WM_GETOBJECT'
 1:50.12 Warning: C4005 in C:\Program Files (x86)\Windows Kits\10\include\10.0.14393.0\um\winuser.h: 'MAPVK_VK_TO_VSC': macro redefinition
 1:50.12 C:\Program Files (x86)\Windows Kits\10\include\10.0.14393.0\um\winuser.h(6663): warning C4005: 'MAPVK_VK_TO_VSC': macro redefinition
 1:50.12 c:\mozilla-source\gecko-0215\obj-i686-pc-mingw32\dist\include\mozilla/widget/WinMessages.h(100): note: see previous definition of 'MAPVK_VK_TO_VSC'
 1:50.12 Warning: C4005 in C:\Program Files (x86)\Windows Kits\10\include\10.0.14393.0\um\winuser.h: 'MAPVK_VSC_TO_VK': macro redefinition
 1:50.12 C:\Program Files (x86)\Windows Kits\10\include\10.0.14393.0\um\winuser.h(6664): warning C4005: 'MAPVK_VSC_TO_VK': macro redefinition
 1:50.12 c:\mozilla-source\gecko-0215\obj-i686-pc-mingw32\dist\include\mozilla/widget/WinMessages.h(101): note: see previous definition of 'MAPVK_VSC_TO_VK'
 1:50.12 Warning: C4005 in C:\Program Files (x86)\Windows Kits\10\include\10.0.14393.0\um\winuser.h: 'MAPVK_VK_TO_CHAR': macro redefinition
 1:50.12 C:\Program Files (x86)\Windows Kits\10\include\10.0.14393.0\um\winuser.h(6665): warning C4005: 'MAPVK_VK_TO_CHAR': macro redefinition
 1:50.12 c:\mozilla-source\gecko-0215\obj-i686-pc-mingw32\dist\include\mozilla/widget/WinMessages.h(102): note: see previous definition of 'MAPVK_VK_TO_CHAR'
 1:50.12 Warning: C4005 in C:\Program Files (x86)\Windows Kits\10\include\10.0.14393.0\um\winuser.h: 'MAPVK_VSC_TO_VK_EX': macro redefinition
 1:50.12 C:\Program Files (x86)\Windows Kits\10\include\10.0.14393.0\um\winuser.h(6666): warning C4005: 'MAPVK_VSC_TO_VK_EX': macro redefinition
 1:50.12 c:\mozilla-source\gecko-0215\obj-i686-pc-mingw32\dist\include\mozilla/widget/WinMessages.h(103): note: see previous definition of 'MAPVK_VSC_TO_VK_EX'
 1:50.12 Warning: C4005 in C:\Program Files (x86)\Windows Kits\10\include\10.0.14393.0\um\winuser.h: 'MAPVK_VK_TO_VSC_EX': macro redefinition
 1:50.12 C:\Program Files (x86)\Windows Kits\10\include\10.0.14393.0\um\winuser.h(6669): warning C4005: 'MAPVK_VK_TO_VSC_EX': macro redefinition
 1:50.12 c:\mozilla-source\gecko-0215\obj-i686-pc-mingw32\dist\include\mozilla/widget/WinMessages.h(104): note: see previous definition of 'MAPVK_VK_TO_VSC_EX'

Comment 24

[Peter Chang[:pchang]]

Comment on attachment 8845296 [details]
Bug 1345710 - fix nsWindowsGfx.cpp build break

https://reviewboard.mozilla.org/r/118476/#review125700

Comment 25

[Jonathan Watt [:jwatt]]

Comment on attachment 8845293 [details]
Bug 1345710 - part1: implement a WindowsEMF class to support Windows Enhance Metafile

https://reviewboard.mozilla.org/r/118470/#review127014

::: widget/windows/WindowsEMF.h:15
(Diff revision 2)
> +#include <windows.h>
> +
> +namespace mozilla {
> +namespace widget {
> +
> +/*

Please make the opening line of all your comments start with "/**" instead of just "/*". The double "*" is necessary to get Doxygen to pick up on this as a documentation comment.

This applies here, for the functions below, and in your other patches.

::: widget/windows/WindowsEMF.h:34
(Diff revision 2)
> +  WindowsEMF();
> +  ~WindowsEMF();
> +
> +  /*
> +   * Generates a metafile device context that will record every GDI commands and
> +   * compile it in a EMF data stream. It can be stored in memory or saved to a file.

Actually I don't think we should need to care whether the DC is initialized here. I think this comment should be "Initialized the object with the path of a file where the EMF data stream should be stored. Callers are then expected to call GetDC to draw output before going on to call FinishDocument to generate the EMF output."

::: widget/windows/WindowsEMF.h:36
(Diff revision 2)
> +
> +  /*
> +   * Generates a metafile device context that will record every GDI commands and
> +   * compile it in a EMF data stream. It can be stored in memory or saved to a file.
> +   */
> +  bool Init(const wchar_t* metafile_path = nullptr);

Call this InitForDrawing.

And please use the Mozilla's standard parameter naming convention. So the parameter name would be aMetafilePath.

Please fix the parameter names in the same way elsewhere.

::: widget/windows/WindowsEMF.h:39
(Diff revision 2)
> +   * compile it in a EMF data stream. It can be stored in memory or saved to a file.
> +   */
> +  bool Init(const wchar_t* metafile_path = nullptr);
> +
> +  /* Initializes the EMF with the data from |metafile_path|. */
> +  bool InitFromFileContents(const wchar_t* metafile_path);

I think this comment should be "Initialize the object with an existing EMF file. Consumers cannot use GetDC to obtain an HDC to modify the file. They can only use Playback().

::: widget/windows/WindowsEMF.h:41
(Diff revision 2)
> +  bool Init(const wchar_t* metafile_path = nullptr);
> +
> +  /* Initializes the EMF with the data from |metafile_path|. */
> +  bool InitFromFileContents(const wchar_t* metafile_path);
> +
> +  /* This funciton could get Metafile DC which is generated by Init function. */

It doesn't really matter that it was created under Init. Just say "If this object was initiaziled using InitForDrawing then this function returns an HDC that can be drawn to to generate the EMF output. Otherwise it returns null. After finishing with the HDC, consumers must call CloseDocument() to finish writing the EMF output.".

::: widget/windows/WindowsEMF.h:49
(Diff revision 2)
> +  /*
> +   * Closes an enhanced-metafile device context and stores a handle that identifies
> +   * an enhanced-format metafile. It can be used to draw a picture stored in an EMF
> +   * to Printer DC or others device context.
> +   */
> +  bool FinishDocument();

I think the comment should be "Called to generate the EMF output once a consumer has finished drawing to the HDC returned by GetDC(). If GetDC() has been called then this function must be called before calling Playback(). Once called consumers must stop using the HDC returned by GetDC."

::: widget/windows/WindowsEMF.h:53
(Diff revision 2)
> +   */
> +  bool FinishDocument();
> +
> +  /*
> +   * Play drawing commands which are stored in EMF on specific device context,
> +   * like print DC, display DC.

I think this should just be "Play the EMF file's drawing commands onto the given DC."

::: widget/windows/WindowsEMF.h:62
(Diff revision 2)
> +  /*
> +   * If calling Init function with specific file path, Close function stores
> +   * the metafile on a disk. Otherwise, it deletes the metafile which is stored
> +   * in memory.
> +   */
> +  void Close();

Call this ReleaseEMFHandle(). And in retrospect I think this comment is misleading. Just make it "Releases this object's handle to the EMF file. If the EMF output is in memory it is deleted. If it is on disk it is not."

Comment 26

[Jonathan Watt [:jwatt]]

Comment on attachment 8845294 [details]
Bug 1345710 - part2: implement a PDFiumEngineShim class to link to PDFium symbols

https://reviewboard.mozilla.org/r/118472/#review127024

Comment 27

[Jonathan Watt [:jwatt]]

Comment on attachment 8845295 [details]
Bug 1345710 - part3: implement PDFViaEMFPrintHelper to convert PDF to EMF

https://reviewboard.mozilla.org/r/118474/#review127026

::: widget/windows/PDFPrintHelperWin.h:18
(Diff revision 2)
> +
> +/* include windows.h for the HDC definitions that we need. */
> +#include <windows.h>
> +
> +/**
> + * Convert PDF to EMF.

"Helper to convert..."

::: widget/windows/PDFPrintHelperWin.h:20
(Diff revision 2)
> +#include <windows.h>
> +
> +/**
> + * Convert PDF to EMF.
> + */
> +class PdfPrintHelperWin

Please call this PDFPrintHelperWin.

::: widget/windows/PDFPrintHelperWin.h:24
(Diff revision 2)
> + */
> +class PdfPrintHelperWin
> +{
> +public:
> +  // Input PDF file and printing width, height, it helps convert to EMF
> +  PdfPrintHelperWin(nsIFile *aFile, int width, int height);

aPageHeight and aPageWidth, (assuming these are page sizes)

::: widget/windows/PDFPrintHelperWin.h:27
(Diff revision 2)
> +public:
> +  // Input PDF file and printing width, height, it helps convert to EMF
> +  PdfPrintHelperWin(nsIFile *aFile, int width, int height);
> +  ~PdfPrintHelperWin();
> +
> +  // Initializes PDFium engine, load document.

It seems like the document should be passed to this method rather than being passed to the constructor. It's a bit weird to have an InitDocument function that you pass a library to instead of a document.

::: widget/windows/PDFPrintHelperWin.h:28
(Diff revision 2)
> +  // Input PDF file and printing width, height, it helps convert to EMF
> +  PdfPrintHelperWin(nsIFile *aFile, int width, int height);
> +  ~PdfPrintHelperWin();
> +
> +  // Initializes PDFium engine, load document.
> +  NS_IMETHOD InitDocument(PRLibrary* Library);

aPDFiumLibrary

::: widget/windows/PDFPrintHelperWin.h:34
(Diff revision 2)
> +
> +  // Close document and destory PDFium engine.
> +  void CloseDocument();
> +
> +  // Get page counts.
> +  void GetPageCount(int &page_num);

aPageNum

::: widget/windows/PDFPrintHelperWin.h:37
(Diff revision 2)
> +
> +  // Get page counts.
> +  void GetPageCount(int &page_num);
> +
> +  // Print EnhMetaFile
> +  bool PlayEnhMetaFile(HDC printer_dc, const RECT* rect, unsigned int page_indx);

aPrinterDC, aRect, aPageIndex

::: widget/windows/PDFPrintHelperWin.h:42
(Diff revision 2)
> +  bool PlayEnhMetaFile(HDC printer_dc, const RECT* rect, unsigned int page_indx);
> +
> +private:
> +
> +  nsCOMPtr<nsIFile> mPDFFile;
> +  int mPrint_Page_Width;

mPrintPageWidth and mPrintPageHeight, please.

::: widget/windows/PDFPrintHelperWin.h:46
(Diff revision 2)
> +  nsCOMPtr<nsIFile> mPDFFile;
> +  int mPrint_Page_Width;
> +  int mPrint_Page_Height;
> +
> +  mozilla::UniquePtr<PDFiumEngineShim> mPDFiumEngine;
> +  FPDF_DOCUMENT     mpDoc;

The lowercase "p" here is weird. I think this should be mPDF or mPDFDoc.

::: widget/windows/PDFPrintHelperWin.h:47
(Diff revision 2)
> +  int mPrint_Page_Width;
> +  int mPrint_Page_Height;
> +
> +  mozilla::UniquePtr<PDFiumEngineShim> mPDFiumEngine;
> +  FPDF_DOCUMENT     mpDoc;
> +  int               mTotal_page_num;

mPageCount, I think.

::: widget/windows/PDFPrintHelperWin.h:48
(Diff revision 2)
> +  int mPrint_Page_Height;
> +
> +  mozilla::UniquePtr<PDFiumEngineShim> mPDFiumEngine;
> +  FPDF_DOCUMENT     mpDoc;
> +  int               mTotal_page_num;
> +  mozilla::UniquePtr<char[]> mPDFbuffer;

mPDFBuffer

::: widget/windows/PDFPrintHelperWin.cpp:14
(Diff revision 2)
> +#include "nsFileStreams.h"
> +
> +PdfPrintHelperWin::PdfPrintHelperWin(nsIFile *aFile, int width, int height)
> +{
> +  mPDFFile            = aFile;
> +  mPrint_Page_Width   = width;

Use the initializer list.

PdfPrintHelperWin::PdfPrintHelperWin(nsIFile *aFile, int width, int height)
  : mFile(aFile)
  , mPageWidth(aPageWidth)
  , etc.

::: widget/windows/PDFPrintHelperWin.cpp:48
(Diff revision 2)
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  nsresult rv = NS_OK;
> +  int64_t size = 0;
> +  RefPtr<nsFileInputStream> nsIInputStream = new nsFileInputStream;

nsIInputStream looks like a class name. Just call this inputStream.

::: widget/windows/PDFPrintHelperWin.cpp:60
(Diff revision 2)
> +    return rv;
> +  }
> +
> +  nsIInputStream->GetSize(&size);
> +  mPDFbuffer.reset(nullptr);
> +  mPDFbuffer = MakeUnique<char[]>(size);

MakeUniqueFallible, and return an error if that returns nullptr.

::: widget/windows/PDFPrintHelperWin.cpp:67
(Diff revision 2)
> +  uint32_t bytesRead = 0;
> +  nsIInputStream->Read(mPDFbuffer.get(), size, &bytesRead);
> +
> +  nsIInputStream->Close();
> +  if (!mPDFiumEngine) {
> +    mPDFiumEngine = MakeUnique <PDFiumEngineShim>();

No space between "MakeUnique" and "<"

::: widget/windows/PDFPrintHelperWin.cpp:92
(Diff revision 2)
> +
> +  return rv;
> +}
> +
> +void
> +PdfPrintHelperWin::GetPageCount(int &page_num)

Inline this function in the header.

::: widget/windows/PDFPrintHelperWin.cpp:110
(Diff revision 2)
> +
> +  if (static_cast<int>(page_indx) >= mTotal_page_num) {
> +    return false;
> +  }
> +
> +  FPDF_PAGE pdfPage = nullptr;

You can assign to this at the same time as you declare it.

::: widget/windows/PDFPrintHelperWin.cpp:124
(Diff revision 2)
> +  if (!result) {
> +    mPDFiumEngine->ClosePage(pdfPage);
> +    return result;
> +  }
> +
> +  int dc_width = ::GetDeviceCaps(metafile.GetDC(), HORZRES);

Again, no underscores. dcWidth, dcHeight.

::: widget/windows/PDFPrintHelperWin.cpp:127
(Diff revision 2)
> +  }
> +
> +  int dc_width = ::GetDeviceCaps(metafile.GetDC(), HORZRES);
> +  int dc_height = ::GetDeviceCaps(metafile.GetDC(), VERTRES);
> +
> +  float scale_factor = 1.0;

scaleFactor

Comment 28

[Jonathan Watt [:jwatt]]

Comment on attachment 8850440 [details]
Bug 1345710 - fix nsWindowDbg build break

https://reviewboard.mozilla.org/r/123020/#review127032

Can you not add |using namespace mozilla;| at the top of this file to fix this?

Comment 29

[Jonathan Watt [:jwatt]]

(In reply to Jonathan Watt [:jwatt] from comment #28)
> Can you not add |using namespace mozilla;| at the top of this file to fix
> this?

If so I think both of the patches for build bustage should go in a separate bug to keep them from cluttering this one. I'll give you speedy review on that.

Comment 30

[Jonathan Watt [:jwatt]]

Comment on attachment 8850441 [details]
Bug 1345710 -  fix redefinition warning in Unified_cpp_widget_windows3

https://reviewboard.mozilla.org/r/123022/#review127036

Nowadays we require Visual Studio 2015 to build, so we no longer need these defines in WinMessages.h. Please file a separate bug to remove them.

Comment 31

[Farmer Tseng[:fatseng](inactive)]

(In reply to Jonathan Watt [:jwatt] from comment #29)
> (In reply to Jonathan Watt [:jwatt] from comment #28)
> > Can you not add |using namespace mozilla;| at the top of this file to fix
> > this?
> 
> If so I think both of the patches for build bustage should go in a separate
> bug to keep them from cluttering this one. I'll give you speedy review on
> that.

Create Bug 1352339 to follow up.

Comment 32

[Farmer Tseng[:fatseng](inactive)]

(In reply to Jonathan Watt [:jwatt] from comment #30)
> Comment on attachment 8850441 [details]
> Bug 1345710 -  fix redefinition warning in Unified_cpp_widget_windows3
> 
> https://reviewboard.mozilla.org/r/123022/#review127036
> 
> Nowadays we require Visual Studio 2015 to build, so we no longer need these
> defines in WinMessages.h. Please file a separate bug to remove them.


Create Bug 1352368 to follow up.

Comment 33

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8845293 [details]
Bug 1345710 - part1: implement a WindowsEMF class to support Windows Enhance Metafile

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118470/diff/2-3/

Comment 34

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8845294 [details]
Bug 1345710 - part2: implement a PDFiumEngineShim class to link to PDFium symbols

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118472/diff/2-3/

Comment 35

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8845295 [details]
Bug 1345710 - part3: implement PDFViaEMFPrintHelper to convert PDF to EMF

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118474/diff/2-3/

Comment 36

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8845293 [details]
Bug 1345710 - part1: implement a WindowsEMF class to support Windows Enhance Metafile

https://reviewboard.mozilla.org/r/118470/#review129376

::: widget/windows/WindowsEMF.h:15
(Diff revision 2)
> +#include <windows.h>
> +
> +namespace mozilla {
> +namespace widget {
> +
> +/*

Fixed them

Comment 37

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8845295 [details]
Bug 1345710 - part3: implement PDFViaEMFPrintHelper to convert PDF to EMF

https://reviewboard.mozilla.org/r/118474/#review129378

::: widget/windows/PDFPrintHelperWin.h:46
(Diff revision 2)
> +  nsCOMPtr<nsIFile> mPDFFile;
> +  int mPrint_Page_Width;
> +  int mPrint_Page_Height;
> +
> +  mozilla::UniquePtr<PDFiumEngineShim> mPDFiumEngine;
> +  FPDF_DOCUMENT     mpDoc;

Fixed it

::: widget/windows/PDFPrintHelperWin.h:47
(Diff revision 2)
> +  int mPrint_Page_Width;
> +  int mPrint_Page_Height;
> +
> +  mozilla::UniquePtr<PDFiumEngineShim> mPDFiumEngine;
> +  FPDF_DOCUMENT     mpDoc;
> +  int               mTotal_page_num;

Change to mPageCount

::: widget/windows/PDFPrintHelperWin.cpp:60
(Diff revision 2)
> +    return rv;
> +  }
> +
> +  nsIInputStream->GetSize(&size);
> +  mPDFbuffer.reset(nullptr);
> +  mPDFbuffer = MakeUnique<char[]>(size);

Thanks for suggestion, fixed it.

Comment 38

[Jonathan Watt [:jwatt]]

Comment on attachment 8845295 [details]
Bug 1345710 - part3: implement PDFViaEMFPrintHelper to convert PDF to EMF

https://reviewboard.mozilla.org/r/118474/#review129384

::: widget/windows/PDFPrintHelperWin.h:20
(Diff revision 3)
> +
> +namespace mozilla {
> +namespace widget {
> +
> +/**
> + * This class works with WindowsEMF and PDFiumEngineShim.

The fact that it works with these classes in an implementation detail. I don't think you need to mention that here since it's not part of the class's API (and the class is small enough it's easy to see that for anyone who's interested in the details).

::: widget/windows/PDFPrintHelperWin.h:21
(Diff revision 3)
> +namespace mozilla {
> +namespace widget {
> +
> +/**
> + * This class works with WindowsEMF and PDFiumEngineShim.
> + * It helps convert specific PDF page to EMF and play the EMF onto the given DC.

Please make this "This class help draw a PDF file to a given Windows DC. To do that it first converts the PDF file to EMF." And maybe add a link to some documentation of what "EMF" is (the wikipedia article or something).

::: widget/windows/PDFPrintHelperWin.h:23
(Diff revision 3)
> +
> +/**
> + * This class works with WindowsEMF and PDFiumEngineShim.
> + * It helps convert specific PDF page to EMF and play the EMF onto the given DC.
> + */
> +class PDFPrintHelperWin

I'm now thinking this should probably be called PDFViaEMFPrintHelper. In the future we may want an XPS helper on Windows too. What do you think?

::: widget/windows/PDFPrintHelperWin.h:29
(Diff revision 3)
> +{
> +public:
> +  PDFPrintHelperWin();
> +  ~PDFPrintHelperWin();
> +
> +  /** Initializes PDFium engine and load specific PDF file into engine. */

"and loads the specified PDF file" I think would be better.

::: widget/windows/PDFPrintHelperWin.h:33
(Diff revision 3)
> +
> +  /** Initializes PDFium engine and load specific PDF file into engine. */
> +  NS_IMETHOD InitDocument(PRLibrary* aPDFiumLibrary, nsIFile *aFile);
> +
> +  /** Releases document buffer and PDFium engine. */
> +  void CloseDocument();

The comment says this does more than just closing the document, so its name shouldn't suggest that's all it does. Maybe "Finish" is a better name.

::: widget/windows/PDFPrintHelperWin.h:35
(Diff revision 3)
> +  NS_IMETHOD InitDocument(PRLibrary* aPDFiumLibrary, nsIFile *aFile);
> +
> +  /** Releases document buffer and PDFium engine. */
> +  void CloseDocument();
> +
> +  /** Get page counts. */

Please don't add comments that just repeat what the name of the function says. They don't add anything useful.

::: widget/windows/PDFPrintHelperWin.h:39
(Diff revision 3)
> +
> +  /** Get page counts. */
> +  int GetPageCount() { return mPageCount; }
> +
> +  /** Convert specific PDF page to EMF and play the EMF onto the given DC */
> +  bool PlayEnhMetaFile(HDC aPrinterDC, unsigned int aPageIndex,

I think "Play" isn't the best verb here. Probably "Draw" is better.

And I think we should use "EMF" instead of "EnhMetaFile" (using "EMF", "enhanced meta file" and "metafile" in different places is confusing - for consistency we should just use one term, despite what the inconsistent MS docs do).

Finally, looking at the implementation this function draws a single page, not the whole document.

So actually I think "DrawPage" would be a better name.

::: widget/windows/PDFPrintHelperWin.cpp:35
(Diff revision 3)
> +
> +  if (mPDFBuffer) {
> +    mPDFBuffer = nullptr;
> +  }
> +
> +  if (mPDFiumEngine) {

Can you make this the second |if| block, please? It's logic is closely linked to the first |if| block so having the |if (mPDFBuffer)| block between the two isn't ideal.

::: widget/windows/PDFPrintHelperWin.cpp:47
(Diff revision 3)
> +{
> +  if (mPDFDoc) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  if (aFile) {

Presumably you meant this to be |if (!aFile)|?

Also, can't we just MOZ_ASSERT that the arguents are non-null, or do you think there is a risk that we may for some reason pass null legitimately?

::: widget/windows/PDFPrintHelperWin.cpp:55
(Diff revision 3)
> +
> +  mPDFFile = aFile;
> +  nsresult rv = NS_OK;
> +  int64_t size = 0;
> +  RefPtr<nsFileInputStream> inputStream = new nsFileInputStream;
> +  if (inputStream == nullptr) {

As the style guide requires, please make this:

if (\!inputStream) \{

Actually, that said, you don't need this check since we just crash when small allocations like this fail. Just remove this check.

::: widget/windows/PDFPrintHelperWin.cpp:58
(Diff revision 3)
> +  int64_t size = 0;
> +  RefPtr<nsFileInputStream> inputStream = new nsFileInputStream;
> +  if (inputStream == nullptr) {
> +      return NS_ERROR_OUT_OF_MEMORY;
> +  }
> +  rv = inputStream->Init(mPDFFile, -1, -1, 0);

I think you should just make this the line where rv is declared. This is C++ code, not C code.

::: widget/windows/PDFPrintHelperWin.cpp:64
(Diff revision 3)
> +
> +  if (NS_FAILED(rv)) {
> +    return rv;
> +  }
> +
> +  inputStream->GetSize(&size);

Similarly, declare |size| down here just before you use it.

::: widget/windows/PDFPrintHelperWin.cpp:75
(Diff revision 3)
> +  }
> +
> +  uint32_t bytesRead = 0;
> +  inputStream->Read(mPDFBuffer.get(), size, &bytesRead);
> +
> +  inputStream->Close();

The line breaking here is a bit wierd. Actions on |inputStream| seem to belong to the group of lines above, not the lines it appears attached to below. In other words the line break should be after this line, not before it.

::: widget/windows/PDFPrintHelperWin.cpp:76
(Diff revision 3)
> +
> +  uint32_t bytesRead = 0;
> +  inputStream->Read(mPDFBuffer.get(), size, &bytesRead);
> +
> +  inputStream->Close();
> +  if (!mPDFiumEngine) {

So this looks like you may infact intend to allow an instance of this class to be reinitialized with a new document. Is that true? If so that information needs to be added to the comment that documents this function in the header file. (We should also possibly rethink the API presented by the function.) Please clarify.

::: widget/windows/PDFPrintHelperWin.cpp:83
(Diff revision 3)
> +    if (!mPDFiumEngine->Init_FindFunctionSymbols(aPDFiumLibrary)) {
> +      return NS_ERROR_FAILURE;
> +    }
> +  }
> +
> +  mPDFiumEngine->InitLibrary();

Again, it seems like the line break should be after this line, not before it.

::: widget/windows/PDFPrintHelperWin.cpp:91
(Diff revision 3)
> +    mPDFiumEngine->DestroyLibrary();
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  mPageCount = mPDFiumEngine->GetPageCount(mPDFDoc);
> +  if (!mPageCount) {

mPageCount isn't a pointer, so I think we should use < 1

::: widget/windows/PDFPrintHelperWin.cpp:94
(Diff revision 3)
> +
> +  mPageCount = mPDFiumEngine->GetPageCount(mPDFDoc);
> +  if (!mPageCount) {
> +    mPDFiumEngine->CloseDocument(mPDFDoc);
> +    mPDFiumEngine->DestroyLibrary();
> +    mPDFDoc = nullptr;

Don't you need to null out mPDFiumEngine here too?

Comment 39

[u459114]

Comment on attachment 8845295 [details]
Bug 1345710 - part3: implement PDFViaEMFPrintHelper to convert PDF to EMF

https://reviewboard.mozilla.org/r/118474/#review132338

::: widget/windows/PDFPrintHelperWin.cpp:33
(Diff revision 3)
> +    mPDFDoc = nullptr;
> +  }
> +
> +  if (mPDFBuffer) {
> +    mPDFBuffer = nullptr;
> +  }

You don't need to do this mPDFBuffer will be freed automatically.

::: widget/windows/PDFPrintHelperWin.cpp:36
(Diff revision 3)
> +  if (mPDFBuffer) {
> +    mPDFBuffer = nullptr;
> +  }
> +
> +  if (mPDFiumEngine) {
> +    mPDFiumEngine = nullptr;

You don't need to do this mPDFiumEngine will be freed automatically.

::: widget/windows/PDFPrintHelperWin.cpp:41
(Diff revision 3)
> +    mPDFiumEngine = nullptr;
> +  }
> +}
> +
> +nsresult
> +PDFPrintHelperWin::InitDocument(PRLibrary* aPDFiumLibrary, nsIFile *aFile)

Suggest
1. Add Init(PRLibrary* aPDFiumLibrary)
   Put all mPDFiumEngine initialization code into that function
2. Add OpenDocument(nsIFile *aFile)

::: widget/windows/PDFPrintHelperWin.cpp:46
(Diff revision 3)
> +PDFPrintHelperWin::InitDocument(PRLibrary* aPDFiumLibrary, nsIFile *aFile)
> +{
> +  if (mPDFDoc) {
> +    return NS_ERROR_FAILURE;
> +  }
> +

Here, assertion is better than if statement I think
MOZ_ASSERT(!mPDFDoc)

::: widget/windows/PDFPrintHelperWin.cpp:65
(Diff revision 3)
> +  if (NS_FAILED(rv)) {
> +    return rv;
> +  }
> +
> +  inputStream->GetSize(&size);
> +  mPDFBuffer.reset(nullptr);

If you free mPDFBuffer in the end of InitDocument, you don' need to reset here. Instead, using assertion is more premise.
MOZ_ASSERT(!mPDFBuffer)

And, most of time, I would choose Vector instead of native char array

::: widget/windows/PDFPrintHelperWin.cpp:77
(Diff revision 3)
> +  uint32_t bytesRead = 0;
> +  inputStream->Read(mPDFBuffer.get(), size, &bytesRead);
> +
> +  inputStream->Close();
> +  if (!mPDFiumEngine) {
> +    mPDFiumEngine = MakeUnique<PDFiumEngineShim>();

You do not allow mPDFDoc be set in the beginning of this function, but allow mPDFiumEngine be set here. It make me confuse. Please treat them with the same principle

::: widget/windows/PDFPrintHelperWin.cpp:85
(Diff revision 3)
> +    }
> +  }
> +
> +  mPDFiumEngine->InitLibrary();
> +  mPDFDoc = mPDFiumEngine->LoadMemDocument(mPDFBuffer.get(), size, nullptr);
> +  if (!mPDFDoc) {

Can we free mPDFBuffer rihgt after calling mPDFiumEngine->LoadMemDocument?

::: widget/windows/PDFPrintHelperWin.cpp:96
(Diff revision 3)
> +  if (!mPageCount) {
> +    mPDFiumEngine->CloseDocument(mPDFDoc);
> +    mPDFiumEngine->DestroyLibrary();
> +    mPDFDoc = nullptr;
> +    return NS_ERROR_FAILURE;
> +  }

You may more codes in this if statement into a private member function: CleanUp. and use it in both here and destructor.

::: widget/windows/PDFPrintHelperWin.cpp:105
(Diff revision 3)
> +
> +bool
> +PDFPrintHelperWin::PlayEnhMetaFile(HDC aPrinterDC, unsigned int aPageIndex,
> +                                   int aPageWidth, int aPageHeight)
> +{
> +  if (!mPDFDoc) {

Add an assertion here. You probably should add mInitiated, and set it as true at the end of InitDocument. And MOZ_ASSERT(mInitiated) at the beginning of most public functions.

::: widget/windows/PDFPrintHelperWin.cpp:117
(Diff revision 3)
> +
> +  FPDF_PAGE pdfPage = mPDFiumEngine->LoadPage(mPDFDoc, aPageIndex);
> +  if (!pdfPage) {
> +    return false;
> +  }
> +

if (!mPDFiumEngine->LoadPage(mPDFDoc, aPageIndex)) {
  return false;
}

::: widget/windows/PDFPrintHelperWin.cpp:137
(Diff revision 3)
> +      static_cast<float>(dcWidth) / static_cast <float>(aPageWidth);
> +    float yFactor =
> +      static_cast<float>(dcHeight) / static_cast <float>(aPageHeight);
> +    scaleFactor = std::min(xFactor, yFactor);
> +  }
> +

you may move #125 ~ #136 to
static
float ComputeScaleFactor(...

::: widget/windows/PDFPrintHelperWin.cpp:154
(Diff revision 3)
> +                         0, 0, aPageWidth, aPageHeight,
> +                         0, FPDF_ANNOT | FPDF_PRINTING | FPDF_NO_CATCH);
> +  result = metafile.FinishDocument();
> +  mPDFiumEngine->ClosePage(pdfPage);
> +  if (!result) {
> +    return result;

metafile.ReleaseEMFHandle() ?

::: widget/windows/PDFPrintHelperWin.cpp:162
(Diff revision 3)
> +  RECT printRect;
> +  printRect.left = 0;
> +  printRect.right = aPageWidth;
> +  printRect.top = 0;
> +  printRect.bottom = aPageHeight;
> +

RECT printRect = {0, aPageWidth, 0, aPageHeight}

::: widget/windows/PDFPrintHelperWin.cpp:171
(Diff revision 3)
> +  return result;
> +}
> +
> +void
> +PDFPrintHelperWin::CloseDocument()
> +{

You have InitDocument and CloseDocument, which are not paired. How about OpenDocument and CloseDocument?

::: widget/windows/PDFPrintHelperWin.cpp:172
(Diff revision 3)
> +}
> +
> +void
> +PDFPrintHelperWin::CloseDocument()
> +{
> +  if (mPDFDoc) {

MOZ_ASSERT(mPDFDoc)

::: widget/windows/PDFPrintHelperWin.cpp:175
(Diff revision 3)
> +PDFPrintHelperWin::CloseDocument()
> +{
> +  if (mPDFDoc) {
> +    mPDFiumEngine->CloseDocument(mPDFDoc);
> +    mPDFiumEngine->DestroyLibrary();
> +    mPDFDoc = nullptr;

Free mPDFBuffer as well?

Comment 40

[Farmer Tseng[:fatseng](inactive)]

(In reply to C.J. Ku[:cjku](UTC+8) from comment #39)
> Comment on attachment 8845295 [details]
> Bug 1345710 - part3: implement PDFPrintHelperWin to convert PDF to EMF
> 
> https://reviewboard.mozilla.org/r/118474/#review132338
> 
> ::: widget/windows/PDFPrintHelperWin.cpp:41
> (Diff revision 3)
> > +    mPDFiumEngine = nullptr;
> > +  }
> > +}
> > +
> > +nsresult
> > +PDFPrintHelperWin::InitDocument(PRLibrary* aPDFiumLibrary, nsIFile *aFile)
> 
> Suggest
> 1. Add Init(PRLibrary* aPDFiumLibrary)
>    Put all mPDFiumEngine initialization code into that function
> 2. Add OpenDocument(nsIFile *aFile)
> 
Sounds better than mine.

> ::: widget/windows/PDFPrintHelperWin.cpp:85
> (Diff revision 3)
> > +    }
> > +  }
> > +
> > +  mPDFiumEngine->InitLibrary();
> > +  mPDFDoc = mPDFiumEngine->LoadMemDocument(mPDFBuffer.get(), size, nullptr);
> > +  if (!mPDFDoc) {
> 
> Can we free mPDFBuffer rihgt after calling mPDFiumEngine->LoadMemDocument?
No, we have to keep buffer until finish coverting.

> ::: widget/windows/PDFPrintHelperWin.cpp:117
> (Diff revision 3)
> > +
> > +  FPDF_PAGE pdfPage = mPDFiumEngine->LoadPage(mPDFDoc, aPageIndex);
> > +  if (!pdfPage) {
> > +    return false;
> > +  }
> > +
> 
> if (!mPDFiumEngine->LoadPage(mPDFDoc, aPageIndex)) {
>   return false;
> }
> 
It needs to keep object which is returned from the call.


> ::: widget/windows/PDFPrintHelperWin.cpp:154
> (Diff revision 3)
> > +                         0, 0, aPageWidth, aPageHeight,
> > +                         0, FPDF_ANNOT | FPDF_PRINTING | FPDF_NO_CATCH);
> > +  result = metafile.FinishDocument();
> > +  mPDFiumEngine->ClosePage(pdfPage);
> > +  if (!result) {
> > +    return result;
> 
> metafile.ReleaseEMFHandle() ?
> 
It doesn't need it because ReleaseEMFHandle() would be called in destructor.

> 
> ::: widget/windows/PDFPrintHelperWin.cpp:175
> (Diff revision 3)
> > +PDFPrintHelperWin::CloseDocument()
> > +{
> > +  if (mPDFDoc) {
> > +    mPDFiumEngine->CloseDocument(mPDFDoc);
> > +    mPDFiumEngine->DestroyLibrary();
> > +    mPDFDoc = nullptr;
> 
> Free mPDFBuffer as well?
I would add it. Thanks.

Comment 41

[Farmer Tseng[:fatseng](inactive)]

(In reply to Jonathan Watt [:jwatt] from comment #38)
> Comment on attachment 8845295 [details]
> Bug 1345710 - part3: implement PDFPrintHelperWin to convert PDF to EMF
> 
> https://reviewboard.mozilla.org/r/118474/#review129384
> 
> ::: widget/windows/PDFPrintHelperWin.h:23
> (Diff revision 3)
> > +
> > +/**
> > + * This class works with WindowsEMF and PDFiumEngineShim.
> > + * It helps convert specific PDF page to EMF and play the EMF onto the given DC.
> > + */
> > +class PDFPrintHelperWin
> 
> I'm now thinking this should probably be called PDFViaEMFPrintHelper. In the
> future we may want an XPS helper on Windows too. What do you think?
> 
Sounds good. I didn't consider that we may have XPS helper in the future.

> ::: widget/windows/PDFPrintHelperWin.h:33
> (Diff revision 3)
> > +
> > + /** Initializes PDFium engine and load specific PDF file into engine. */
> > + NS_IMETHOD InitDocument(PRLibrary* aPDFiumLibrary, nsIFile *aFile);
> > +
> > + /** Releases document buffer and PDFium engine. */
> > + void CloseDocument();
> 
> The comment says this does more than just closing the document, so its name
> shouldn't suggest that's all it does. Maybe "Finish" is a better name.
> 
I would keep this function name.
Per cjku suggestion, They would have a paired function, say OpenDocument/CloseDocument.
And, I would move the code of releasing PDFium engine in the destructor.

> ::: widget/windows/PDFPrintHelperWin.cpp:47
> (Diff revision 3)
> > +{
> > + if (mPDFDoc) {
> > + return NS_ERROR_FAILURE;
> > + }
> > +
> > + if (aFile) {
> 
> Presumably you meant this to be |if (!aFile)|?
> 
> Also, can't we just MOZ_ASSERT that the arguents are non-null, or do you
> think there is a risk that we may for some reason pass null legitimately?
> 
Yes, |if (!aFile)| is correct.

> 
> ::: widget/windows/PDFPrintHelperWin.cpp:76
> (Diff revision 3)
> > +
> > + uint32_t bytesRead = 0;
> > + inputStream->Read(mPDFBuffer.get(), size, &bytesRead);
> > +
> > + inputStream->Close();
> > + if (!mPDFiumEngine) {
> 
> So this looks like you may infact intend to allow an instance of this class
> to be reinitialized with a new document. Is that true? If so that
> information needs to be added to the comment that documents this function in
> the header file. (We should also possibly rethink the API presented by the
> function.) Please clarify.
>
Yes, I intend to allow an instance of this class to be reinitialized with a new document.
So, I would like to have a paired function, OpenDcument/CloseDocument, and an Init() to initialize mPDFiumEngine.


> ::: widget/windows/PDFPrintHelperWin.cpp:94
> (Diff revision 3)
> > +
> > + mPageCount = mPDFiumEngine->GetPageCount(mPDFDoc);
> > + if (!mPageCount) {
> > + mPDFiumEngine->CloseDocument(mPDFDoc);
> > + mPDFiumEngine->DestroyLibrary();
> > + mPDFDoc = nullptr;
> 
> Don't you need to null out mPDFiumEngine here too?
It doesn't need to set mPDFiumEngine as null here because mPDFiumEngine could be used for next document.

Comment 42

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8845295 [details]
Bug 1345710 - part3: implement PDFViaEMFPrintHelper to convert PDF to EMF

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118474/diff/3-4/

Comment 43

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8845295 [details]
Bug 1345710 - part3: implement PDFViaEMFPrintHelper to convert PDF to EMF

Hi CJ

Comment 44

[Farmer Tseng[:fatseng](inactive)]

Hi CJ,
Please help review it in advance.
I replaced UniquePtr<char[]> by vector in my local PC, it works but didn't upload for review yet.
I would like to discuss with you about vector on Monday.

Comment 45

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8845295 [details]
Bug 1345710 - part3: implement PDFViaEMFPrintHelper to convert PDF to EMF

https://reviewboard.mozilla.org/r/118474/#review132932

Comment 46

[u459114]

Comment on attachment 8845295 [details]
Bug 1345710 - part3: implement PDFViaEMFPrintHelper to convert PDF to EMF

https://reviewboard.mozilla.org/r/118474/#review132946

::: widget/windows/PDFViaEMFPrintHelper.h:10
(Diff revision 4)
> +
> +#ifndef PDFVIAEMFPRINTHELPER_H_
> +#define PDFVIAEMFPRINTHELPER_H_
> +
> +#include "nsCOMPtr.h"
> +#include "mozilla/dom/File.h"

I don't see a reason that we need to include "File.h" here.

Forward declaration nsIFile here:
class nsIFile;

And inlude "nsIFrame.h" in PDFViaEMFPrintHelper.cpp

::: widget/windows/PDFViaEMFPrintHelper.cpp:30
(Diff revision 4)
> +  return scaleFactor;
> +}
> +
> +PDFViaEMFPrintHelper::PDFViaEMFPrintHelper(PRLibrary* aPDFiumLibrary)
> +  : mPDFiumLibrary(aPDFiumLibrary)
> +  , mPDFiumEngine(nullptr)

mPDFiumEngine and mPDFBuffer are UniquePtr, you don't need to give init value here. please delete these two line.

::: widget/windows/PDFViaEMFPrintHelper.cpp:34
(Diff revision 4)
> +  : mPDFiumLibrary(aPDFiumLibrary)
> +  , mPDFiumEngine(nullptr)
> +  , mPDFDoc(nullptr)
> +  , mPDFBuffer(nullptr)
> +{
> +}

MOZ_ASSERT(mPDFiumLibrary)

::: widget/windows/PDFViaEMFPrintHelper.cpp:53
(Diff revision 4)
> +  if (mPDFiumEngine) {
> +    return true;
> +  }
> +
> +  mPDFiumEngine = MakeUnique<PDFiumEngineShim>();
> +  if (!mPDFiumEngine->Init_FindFunctionSymbols(mPDFiumLibrary)) {

Init_FindFunctionSymbols can be lazy initiate function just like this one

::: widget/windows/PDFViaEMFPrintHelper.cpp:88
(Diff revision 4)
> +  }
> +
> +  uint32_t bytesRead = 0;
> +  inputStream->Read(mPDFBuffer.get(), size, &bytesRead);
> +  inputStream->Close();
> +

Move #77 ~ #88 to
bool PDFViaEMFPrintHelper::AllocatePDFBuffer(nsFileInputStream *aStream) {
  int64_t size = 0;
  aStream->GetSize(&size);
  // What if size == 0?
  //....
}

::: widget/windows/PDFViaEMFPrintHelper.cpp:100
(Diff revision 4)
> +  if (mPDFiumEngine->GetPageCount(mPDFDoc) < 1) {
> +    CloseDocument();
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  return rv;

Suggest:
remove rv, and return NS_OK here.

::: widget/windows/PDFViaEMFPrintHelper.cpp:113
(Diff revision 4)
> +  NS_ENSURE_TRUE_VOID(static_cast<int>(aPageIndex) <
> +                      mPDFiumEngine->GetPageCount(mPDFDoc));
> +
> +  FPDF_PAGE pdfPage = mPDFiumEngine->LoadPage(mPDFDoc, aPageIndex);
> +  NS_ENSURE_TRUE_VOID(pdfPage);
> +

Move #111~#112 to #128 if possible
And I think you may return bool instead of void here

::: widget/windows/PDFViaEMFPrintHelper.cpp:116
(Diff revision 4)
> +  FPDF_PAGE pdfPage = mPDFiumEngine->LoadPage(mPDFDoc, aPageIndex);
> +  NS_ENSURE_TRUE_VOID(pdfPage);
> +
> +  int dcWidth = ::GetDeviceCaps(aDC, HORZRES);
> +  int dcHeight = ::GetDeviceCaps(aDC, VERTRES);
> +  float scaleFactor =ComputeScaleFactor(dcWidth,dcHeight,

Space after "="

::: widget/windows/PDFViaEMFPrintHelper.cpp:119
(Diff revision 4)
> +  int dcWidth = ::GetDeviceCaps(aDC, HORZRES);
> +  int dcHeight = ::GetDeviceCaps(aDC, VERTRES);
> +  float scaleFactor =ComputeScaleFactor(dcWidth,dcHeight,
> +                                        aPageWidth, aPageHeight);
> +
> +  ::SetGraphicsMode(aDC, GM_ADVANCED);

I do not use windows SDK for a long time. As I remember, we should keep the status of HDC before modification, and restore it later. For exp
int mode = ::GetGraphicsMode(aDC);
::SetGraphicsMode(aDC, GM_ADVANCED);
..
::SetGraphicsMode(aDC, mode);

::: widget/windows/PDFViaEMFPrintHelper.cpp:145
(Diff revision 4)
> +  bool result = metafile.InitForDrawing();
> +  NS_ENSURE_TRUE(result, false);
> +
> +  RenderPageToEMF(metafile.GetDC(), aPageIndex, aPageWidth, aPageHeight);
> +
> +  result = metafile.FinishDocument();;

double ;

::: widget/windows/PDFViaEMFPrintHelper.cpp:146
(Diff revision 4)
> +  NS_ENSURE_TRUE(result, false);
> +
> +  RenderPageToEMF(metafile.GetDC(), aPageIndex, aPageWidth, aPageHeight);
> +
> +  result = metafile.FinishDocument();;
> +  NS_ENSURE_TRUE(result, false);

metafile.ReleaseEMFHandle() before early return

Comment 47

[u459114]

(In reply to Farmer Tseng[:fatseng] from comment #44)
> Hi CJ,
> Please help review it in advance.
> I replaced UniquePtr<char[]> by vector in my local PC, it works but didn't
> upload for review yet.
> I would like to discuss with you about vector on Monday.

Sure

Comment 48

[u459114]

And, can we have gtest for this module?

Comment 49

[Farmer Tseng[:fatseng](inactive)]

(In reply to C.J. Ku[:cjku](UTC+8) from comment #48)
> And, can we have gtest for this module?
I will try it.

Comment 50

[u459114]

Comment on attachment 8845295 [details]
Bug 1345710 - part3: implement PDFViaEMFPrintHelper to convert PDF to EMF

https://reviewboard.mozilla.org/r/118474/#review133250

::: widget/windows/PDFViaEMFPrintHelper.cpp:173
(Diff revision 4)
> +  return result;
> +}
> +
> +void
> +PDFViaEMFPrintHelper::CloseDocument()
> +{

NS_ASSERTION(mPDFDoc, "OpenDocument and CloseDocument should be called in pair.");

Comment 51

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8845295 [details]
Bug 1345710 - part3: implement PDFViaEMFPrintHelper to convert PDF to EMF

https://reviewboard.mozilla.org/r/118474/#review133310

::: widget/windows/PDFViaEMFPrintHelper.cpp:88
(Diff revision 4)
> +  }
> +
> +  uint32_t bytesRead = 0;
> +  inputStream->Read(mPDFBuffer.get(), size, &bytesRead);
> +  inputStream->Close();
> +

I will move to 
bool
PDFViaEMFPrintHelper::LoadPDFDataToBuffer(nsFileInputStream *aStream)

::: widget/windows/PDFViaEMFPrintHelper.cpp:119
(Diff revision 4)
> +  int dcWidth = ::GetDeviceCaps(aDC, HORZRES);
> +  int dcHeight = ::GetDeviceCaps(aDC, VERTRES);
> +  float scaleFactor =ComputeScaleFactor(dcWidth,dcHeight,
> +                                        aPageWidth, aPageHeight);
> +
> +  ::SetGraphicsMode(aDC, GM_ADVANCED);

I would like to use SaveDC/RestoreDC to keep status.

::: widget/windows/PDFViaEMFPrintHelper.cpp:146
(Diff revision 4)
> +  NS_ENSURE_TRUE(result, false);
> +
> +  RenderPageToEMF(metafile.GetDC(), aPageIndex, aPageWidth, aPageHeight);
> +
> +  result = metafile.FinishDocument();;
> +  NS_ENSURE_TRUE(result, false);

If FinishDocument() return false, it means there is no EMF handle.

Comment 52

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8845295 [details]
Bug 1345710 - part3: implement PDFViaEMFPrintHelper to convert PDF to EMF

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118474/diff/4-5/

Comment 53

[u459114]

Comment on attachment 8845295 [details]
Bug 1345710 - part3: implement PDFViaEMFPrintHelper to convert PDF to EMF

https://reviewboard.mozilla.org/r/118474/#review133322

::: widget/windows/PDFViaEMFPrintHelper.cpp:53
(Diff revision 5)
> +  if (mPDFiumEngine) {
> +    return true;
> +  }
> +
> +  mPDFiumEngine = MakeUnique<PDFiumEngineShim>(mPDFiumLibrary);
> +  return true;

Init will never return false. You may change return type to void

::: widget/windows/PDFViaEMFPrintHelper.cpp:68
(Diff revision 5)
> +  if (!mPDFData.resize(size)) {
> +    return false;
> +  }
> +
> +  uint32_t bytesRead = 0;
> +  aStream->Read(mPDFData.begin(), size, &bytesRead);

Handle the return value of nsFileInputStream::Read

MOZ_ASSERT(size == bytesRead)

::: widget/windows/PDFViaEMFPrintHelper.cpp:69
(Diff revision 5)
> +    return false;
> +  }
> +
> +  uint32_t bytesRead = 0;
> +  aStream->Read(mPDFData.begin(), size, &bytesRead);
> +  aStream->Close();

You don't need manually call Close

::: widget/windows/PDFViaEMFPrintHelper.cpp:88
(Diff revision 5)
> +
> +  RefPtr<nsFileInputStream> inputStream = new nsFileInputStream();
> +  nsresult rv = inputStream->Init(aFile, -1, -1, 0);
> +  if (NS_FAILED(rv)) {
> +    return rv;
> +  }

You can create inputStream in LoadPDFDataToBuffer.
And use UniquePtr instead of RefPtr

::: widget/windows/PDFViaEMFPrintHelper.cpp:154
(Diff revision 5)
> +  bool result = metafile.InitForDrawing();
> +  NS_ENSURE_TRUE(result, false);
> +
> +  result = RenderPageToEMF(metafile.GetDC(), aPageIndex,
> +                           aPageWidth, aPageHeight);
> +  result = metafile.FinishDocument();

The retrun value of RenderPageToEMF will be overwrittend by metafile.FinishDocument.

::: widget/windows/PDFViaEMFPrintHelper.cpp:174
(Diff revision 5)
> +  bool result = metafile.InitForDrawing(aFilePath);
> +  NS_ENSURE_TRUE(result, false);
> +
> +  result = RenderPageToEMF(metafile.GetDC(), aPageIndex,
> +                           aPageWidth, aPageHeight);
> +  result = metafile.FinishDocument();

The same. The return value of RenderPageToEMF will be overwritten

::: widget/windows/PDFiumEngineShim.cpp:14
(Diff revision 5)
>  namespace mozilla {
>  namespace widget {
>  
> -PDFiumEngineShim::PDFiumEngineShim()
> -  : FPDF_InitLibrary(nullptr)
> +PDFiumEngineShim::PDFiumEngineShim(PRLibrary* aLibrary)
> +  : mPRLibrary(aLibrary)
> +  , FPDF_InitLibrary(nullptr)

Please move this change back to part2

Comment 54

[u459114]

Comment on attachment 8845295 [details]
Bug 1345710 - part3: implement PDFViaEMFPrintHelper to convert PDF to EMF

https://reviewboard.mozilla.org/r/118474/#review133328

::: widget/windows/PDFViaEMFPrintHelper.cpp:16
(Diff revision 5)
> +#include "mozilla/dom/File.h"
> +
> +static
> +float ComputeScaleFactor(int aDCWidth, int aDCHeight,
> +                         int aPageWidth, int aPageHeight)
> +{

thread panic if aPageWidth == 0 or aPageHeight == 0

::: widget/windows/PDFViaEMFPrintHelper.cpp:109
(Diff revision 5)
> +}
> +
> +bool
> +PDFViaEMFPrintHelper::RenderPageToEMF(HDC aDC, unsigned int aPageIndex,
> +                                      int aPageWidth, int aPageHeight)
> +{

MOZ_ASSERT(aDC)

::: widget/windows/PDFViaEMFPrintHelper.cpp:109
(Diff revision 5)
> +}
> +
> +bool
> +PDFViaEMFPrintHelper::RenderPageToEMF(HDC aDC, unsigned int aPageIndex,
> +                                      int aPageWidth, int aPageHeight)
> +{

RenderPageToDC might be a better name. EMF is not used in this function at all.

::: widget/windows/PDFViaEMFPrintHelper.cpp:140
(Diff revision 5)
> +  mPDFiumEngine->RenderPage(aDC, pdfPage,
> +                            0, 0, aPageWidth, aPageHeight,
> +                            0, FPDF_ANNOT | FPDF_PRINTING | FPDF_NO_CATCH);
> +  mPDFiumEngine->ClosePage(pdfPage);
> +  ::RestoreDC(aDC, savedState);
> +

if (pdfPage) {
  mPDFiumEngine->RenderPage(...);
  mPDFiumEngine->ClosePage(...);
}

::RestoreDC(aDC, savedState);

return pdfPage ? true : false;

::: widget/windows/PDFViaEMFPrintHelper.cpp:147
(Diff revision 5)
> +}
> +
> +bool
> +PDFViaEMFPrintHelper::DrawPage(HDC aPrinterDC, unsigned int aPageIndex,
> +                               int aPageWidth, int aPageHeight)
> +{

What if aPrinterDC == 0?

Comment 55

[u459114]

Comment on attachment 8845293 [details]
Bug 1345710 - part1: implement a WindowsEMF class to support Windows Enhance Metafile

https://reviewboard.mozilla.org/r/118470/#review133368

::: widget/windows/WindowsEMF.h:71
(Diff revision 3)
> +  /**
> +   * Releases this object's handle to the EMF file. If the EMF output is in
> +   * memory it is deleted. If it is on disk it is not.
> +   */
> +  void ReleaseEMFHandle();
> +

Move ReleaseEMFHandle & FinishDocument to private section

::: widget/windows/WindowsEMF.h:73
(Diff revision 3)
> +   * memory it is deleted. If it is on disk it is not.
> +   */
> +  void ReleaseEMFHandle();
> +
> +private:
> +

delete copy constructor

::: widget/windows/WindowsEMF.cpp:26
(Diff revision 3)
> +  ReleaseEMFHandle();
> +}
> +
> +bool
> +WindowsEMF::InitForDrawing(const wchar_t* aMetafilePath /* = nullptr */)
> +{

MOZ_ASSERT(!mEmf)

::: widget/windows/WindowsEMF.cpp:33
(Diff revision 3)
> +  return !!mDC;
> +}
> +
> +bool
> +WindowsEMF::InitFromFileContents(const wchar_t* aMetafilePath)
> +{

MOZ_ASSERT(!mDC)

::: widget/windows/WindowsEMF.cpp:61
(Diff revision 3)
> +
> +HDC
> +WindowsEMF::GetDC()
> +{
> +  return mDC;
> +}

move trivial function to the header file

Comment 56

[u459114]

Comment on attachment 8845293 [details]
Bug 1345710 - part1: implement a WindowsEMF class to support Windows Enhance Metafile

https://reviewboard.mozilla.org/r/118470/#review133378

::: widget/windows/WindowsEMF.cpp:33
(Diff revision 3)
> +  return !!mDC;
> +}
> +
> +bool
> +WindowsEMF::InitFromFileContents(const wchar_t* aMetafilePath)
> +{

MOZ_ASSERT(aMetafilePath)?

Comment 57

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8845293 [details]
Bug 1345710 - part1: implement a WindowsEMF class to support Windows Enhance Metafile

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118470/diff/3-4/

Comment 58

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8845294 [details]
Bug 1345710 - part2: implement a PDFiumEngineShim class to link to PDFium symbols

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118472/diff/3-4/

Comment 59

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8845295 [details]
Bug 1345710 - part3: implement PDFViaEMFPrintHelper to convert PDF to EMF

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118474/diff/5-6/

Comment 60

[u459114]

Comment on attachment 8845293 [details]
Bug 1345710 - part1: implement a WindowsEMF class to support Windows Enhance Metafile

https://reviewboard.mozilla.org/r/118470/#review133598

::: widget/windows/WindowsEMF.cpp:28
(Diff revision 4)
> +
> +bool
> +WindowsEMF::InitForDrawing(const wchar_t* aMetafilePath /* = nullptr */)
> +{
> +  MOZ_ASSERT(!mDC);
> +  MOZ_ASSERT(!mEmf);

MOZ_ASSERT(!mDC && !mEmf, "InitForDrawing and InitFromFileContents is designed to be used at most once.");

Comment 61

[u459114]

Comment on attachment 8845294 [details]
Bug 1345710 - part2: implement a PDFiumEngineShim class to link to PDFium symbols

https://reviewboard.mozilla.org/r/118472/#review133602

::: widget/windows/PDFiumEngineShim.cpp:126
(Diff revision 4)
> +  }
> +
> +  if (FPDF_LoadMemDocument) {
> +    return FPDF_LoadMemDocument(aDataBuf, aSize, aPassword);
> +  }
> +  return nullptr;

return FPDF_LoadMemDocument ? FPDF_LoadMemDocument(aDataBuf, aSize, aPassword) : nullptr;

Comment 62

[u459114]

Comment on attachment 8845295 [details]
Bug 1345710 - part3: implement PDFViaEMFPrintHelper to convert PDF to EMF

https://reviewboard.mozilla.org/r/118474/#review133604

::: widget/windows/PDFViaEMFPrintHelper.cpp:19
(Diff revision 6)
> +static
> +float ComputeScaleFactor(int aDCWidth, int aDCHeight,
> +                         int aPageWidth, int aPageHeight)
> +{
> +  MOZ_ASSERT(aPageWidth != 0);
> +  MOZ_ASSERT(aPageHeight != 0);

MOZ_ASSERT(aPageWidth !=0 && aPageWidth != 0);


assertion can not prevent thread panic. Please see my comment in RenderPageToDC

::: widget/windows/PDFViaEMFPrintHelper.cpp:64
(Diff revision 6)
> +{
> +  RefPtr<nsFileInputStream> inputStream = new nsFileInputStream();
> +  nsresult rv = inputStream->Init(aFile, -1, -1, 0);
> +  if (NS_FAILED(rv)) {
> +    return false;
> +  }

if (NS_FAILED(inputStream->Init(aFile, -1, -1, 0))) {
  return false;
}

::: widget/windows/PDFViaEMFPrintHelper.cpp:112
(Diff revision 6)
> +{
> +  MOZ_ASSERT(aDC);
> +  NS_ENSURE_TRUE(mPDFDoc, false);
> +  NS_ENSURE_TRUE(static_cast<int>(aPageIndex) <
> +                 mPDFiumEngine->GetPageCount(mPDFDoc), false);
> +

if (aPageWidth == 0 || aPageHeight == 0) {
  return false;
}

::: widget/windows/PDFViaEMFPrintHelper.cpp:172
(Diff revision 6)
> +
> +  result = RenderPageToDC(metafile.GetDC(), aPageIndex,
> +                           aPageWidth, aPageHeight);
> +  NS_ENSURE_TRUE(result, false);
> +  result = metafile.SaveToFile();
> +  return result;

return metafile.SaveToFile();

Comment 63

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8845293 [details]
Bug 1345710 - part1: implement a WindowsEMF class to support Windows Enhance Metafile

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118470/diff/4-5/

Comment 64

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8845294 [details]
Bug 1345710 - part2: implement a PDFiumEngineShim class to link to PDFium symbols

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118472/diff/4-5/

Comment 65

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8845295 [details]
Bug 1345710 - part3: implement PDFViaEMFPrintHelper to convert PDF to EMF

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118474/diff/6-7/

Comment 66

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8845295 [details]
Bug 1345710 - part3: implement PDFViaEMFPrintHelper to convert PDF to EMF

Hi Jwatt, cjku helped review patch in advance. Please help review it.

Comment 67

[u459114]

Comment on attachment 8845295 [details]
Bug 1345710 - part3: implement PDFViaEMFPrintHelper to convert PDF to EMF

https://reviewboard.mozilla.org/r/118474/#review133742

::: widget/windows/PDFViaEMFPrintHelper.cpp:140
(Diff revision 7)
> +                              0, FPDF_ANNOT | FPDF_PRINTING | FPDF_NO_CATCH);
> +    mPDFiumEngine->ClosePage(pdfPage);
> +  }
> +  ::RestoreDC(aDC, savedState);
> +
> +  return pdfPage ? true : false;

return !!pdfPage;

Comment 68

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8845295 [details]
Bug 1345710 - part3: implement PDFViaEMFPrintHelper to convert PDF to EMF

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118474/diff/7-8/

Comment 69

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8845294 [details]
Bug 1345710 - part2: implement a PDFiumEngineShim class to link to PDFium symbols

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118472/diff/5-6/

Comment 70

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8845295 [details]
Bug 1345710 - part3: implement PDFViaEMFPrintHelper to convert PDF to EMF

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118474/diff/8-9/

Comment 71

[Jonathan Watt [:jwatt]]

Comment on attachment 8845294 [details]
Bug 1345710 - part2: implement a PDFiumEngineShim class to link to PDFium symbols

https://reviewboard.mozilla.org/r/118472/#review135444

Generally looks pretty good.

::: widget/windows/PDFiumEngineShim.h:117
(Diff revision 6)
> +public:
> +
> +  PDFiumEngineShim(PRLibrary* aLibrary);
> +  ~PDFiumEngineShim();
> +
> +  void DestroyLibrary();

Given that initialization is implicit in LoadMemDocument, and that destruction is implicit in CloseDocument, should this method be private like the initialization methods?

Also I do wonder whether we should be unloading PDFium when we close a document. If people print one document during a browsing session there's a good chance they will print another, and loading and unloading the lib for each document seems poor. Can you at least file a follow-up bug to consider this and make it block the bug on shipping? We should definitely revisit this when we're further along with the implementation.

::: widget/windows/PDFiumEngineShim.h:143
(Diff revision 6)
> +  void InitLibrary();
> +
> +  PRLibrary*  mPRLibrary;
> +  bool        mInitiated;
> +
> +  FPDF_InitLibrary_Pfn FPDF_InitLibrary;

I think this should be prefixed by "m" like all other members. So "mFPDF_InitLibrary". Same with the others.

::: widget/windows/PDFiumEngineShim.cpp:118
(Diff revision 6)
> +                                  int aSize,
> +                                  FPDF_BYTESTRING aPassword)
> +{
> +  if (!mInitiated) {
> +    if (!FindFunctionSymbols()) {
> +      return nullptr;

This seems a little unncessarily split. Can you combine FindFunctionSymbols and InitLibrary into a single Init()/InitSymbolsAndLibrary() function? Or is there some reason that you want the split?

And also rename mInitiated to mInitialized for consistency with the noun we generally use in Mozilla.

::: widget/windows/PDFiumEngineShim.cpp:129
(Diff revision 6)
> +}
> +
> +void
> +PDFiumEngineShim::CloseDocument(FPDF_DOCUMENT aDocument)
> +{
> +  if (mInitiated) {

It seems like a programming error if anyone calls this without LoadMemDocument having been successfully called first. Probably just make this a MOZ_ASSERT.

::: widget/windows/PDFiumEngineShim.cpp:131
(Diff revision 6)
> +void
> +PDFiumEngineShim::CloseDocument(FPDF_DOCUMENT aDocument)
> +{
> +  if (mInitiated) {
> +    FPDF_CloseDocument(aDocument);
> +    DestroyLibrary();

I'm not sure it would be expected that CloseDocument would also call DestroyLibrary under the hood, but okay.

::: widget/windows/PDFiumEngineShim.cpp:139
(Diff revision 6)
> +}
> +
> +int
> +PDFiumEngineShim::GetPageCount(FPDF_DOCUMENT aDocument)
> +{
> +  return FPDF_GetPageCount ?

Again this seems like a programming error if FPDF_GetPageCount isn't set and should just be a MOZ_ASSERT. In fact is seems like we should really be asserting that mInitialized is true rather than checking FPDF_GetPageCount, since we don't want this function to be called unless the *entire* initialization process was successful. So I'd suggest:

  MOZ_ASSERT(mInitiated);

but you could also use:

  MOZ_ASSERT(mInitiated && FPDF_GetPageCount);

if you prefer that.

At any rate, silently continuing and returing 0 does not seem like a good thing to do here when we really should be getting notified of the programming error.

::: widget/windows/PDFiumEngineShim.cpp:149
(Diff revision 6)
> +PDFiumEngineShim::GetPageSizeByIndex(FPDF_DOCUMENT aDocument,
> +                                     int aPageIndex,
> +                                     double* aWidth,
> +                                     double* aHeight)
> +{
> +  return FPDF_GetPageSizeByIndex ?

Similar here.

::: widget/windows/PDFiumEngineShim.cpp:156
(Diff revision 6)
> +}
> +
> +FPDF_PAGE
> +PDFiumEngineShim::LoadPage(FPDF_DOCUMENT aDocument, int aPageIndex)
> +{
> +  return FPDF_LoadPage ? FPDF_LoadPage(aDocument, aPageIndex) : nullptr;

Similar here.

::: widget/windows/PDFiumEngineShim.cpp:162
(Diff revision 6)
> +}
> +
> +void
> +PDFiumEngineShim::ClosePage(FPDF_PAGE aPage)
> +{
> +    if (FPDF_ClosePage) {

Similar here.

Also the indentation is too much in this function.

::: widget/windows/PDFiumEngineShim.cpp:173
(Diff revision 6)
> +PDFiumEngineShim::RenderPage(HDC aDC, FPDF_PAGE aPage,
> +                             int aStartX, int aStartY,
> +                             int aSizeX, int aSizeY,
> +                             int aRotate, int aFlags)
> +{
> +  if (FPDF_RenderPage) {

Similar here.

::: widget/windows/PDFiumEngineShim.cpp:182
(Diff revision 6)
> +}
> +
> +void
> +PDFiumEngineShim::RenderPage_Close(FPDF_PAGE aPage)
> +{
> +  if (FPDF_RenderPage_Close) {

And here.

Comment 72

[Jonathan Watt [:jwatt]]

Comment on attachment 8845295 [details]
Bug 1345710 - part3: implement PDFViaEMFPrintHelper to convert PDF to EMF

https://reviewboard.mozilla.org/r/118474/#review135448

::: widget/windows/PDFViaEMFPrintHelper.h:33
(Diff revision 9)
> +{
> +public:
> +  PDFViaEMFPrintHelper(PRLibrary* aPDFiumLibrary);
> +  ~PDFViaEMFPrintHelper();
> +
> +  /** Loads the specific PDF file. */

That should be "specified", not "specific".

Same in the comments for DrawPage and DrawPageToFile.

::: widget/windows/PDFViaEMFPrintHelper.h:45
(Diff revision 9)
> +
> +  /** Convert specific PDF page to EMF and draw the EMF onto the given DC. */
> +  bool DrawPage(HDC aPrinterDC, unsigned int aPageIndex,
> +                int aPageWidth, int aPageHeight);
> +
> +  /** Convert specific PDF page to EMF then save it to file. */

I think this should be "and save it to file"

::: widget/windows/PDFViaEMFPrintHelper.cpp:139
(Diff revision 9)
> +
> +  return true;
> +}
> +
> +bool
> +PDFViaEMFPrintHelper::DrawPage(HDC aPrinterDC, unsigned int aPageIndex,

Why does this function exist? It isn't clear to me why we are drawing into an EMF backed DC, then replaying the drawing into the printer DC. Why can't we simply draw direct to the printer DC in this scenario?

Comment 73

[Farmer Tseng[:fatseng](inactive)]

(In reply to C.J. Ku[:cjku](UTC+8) from comment #48)
> And, can we have gtest for this module?
Create Bug 1358076 to follow up.

Comment 74

[Farmer Tseng[:fatseng](inactive)]

(In reply to Jonathan Watt [:jwatt] from comment #71)
> Comment on attachment 8845294 [details]
> Bug 1345710 - part2: implement a PDFiumEngineShim class to link to PDFium
> symbols
> 
> https://reviewboard.mozilla.org/r/118472/#review135444
> 
> Generally looks pretty good.
> 
> ::: widget/windows/PDFiumEngineShim.h:117
> (Diff revision 6)
> > +public:
> > +
> > +  PDFiumEngineShim(PRLibrary* aLibrary);
> > +  ~PDFiumEngineShim();
> > +
> > +  void DestroyLibrary();
> 
> Given that initialization is implicit in LoadMemDocument, and that
> destruction is implicit in CloseDocument, should this method be private like
> the initialization methods?
> 
> Also I do wonder whether we should be unloading PDFium when we close a
> document. If people print one document during a browsing session there's a
> good chance they will print another, and loading and unloading the lib for
> each document seems poor. Can you at least file a follow-up bug to consider
> this and make it block the bug on shipping? We should definitely revisit
> this when we're further along with the implementation.
> 
Create Bug 1358985 to implement in the future.

Comment 75

[Farmer Tseng[:fatseng](inactive)]

(In reply to Jonathan Watt [:jwatt] from comment #72)
> Comment on attachment 8845295 [details]
> Bug 1345710 - part3: implement PDFViaEMFPrintHelper to convert PDF to EMF
> 
> https://reviewboard.mozilla.org/r/118474/#review135448
> 
> ::: widget/windows/PDFViaEMFPrintHelper.h:33
> (Diff revision 9)
> > +{
> > +public:
> > +  PDFViaEMFPrintHelper(PRLibrary* aPDFiumLibrary);
> > +  ~PDFViaEMFPrintHelper();
> > +
> > +  /** Loads the specific PDF file. */
> 
> That should be "specified", not "specific".
> 
> Same in the comments for DrawPage and DrawPageToFile.
> 
> ::: widget/windows/PDFViaEMFPrintHelper.h:45
> (Diff revision 9)
> > +
> > +  /** Convert specific PDF page to EMF and draw the EMF onto the given DC. */
> > +  bool DrawPage(HDC aPrinterDC, unsigned int aPageIndex,
> > +                int aPageWidth, int aPageHeight);
> > +
> > +  /** Convert specific PDF page to EMF then save it to file. */
> 
> I think this should be "and save it to file"
> 
> ::: widget/windows/PDFViaEMFPrintHelper.cpp:139
> (Diff revision 9)
> > +
> > +  return true;
> > +}
> > +
> > +bool
> > +PDFViaEMFPrintHelper::DrawPage(HDC aPrinterDC, unsigned int aPageIndex,
> 
> Why does this function exist? It isn't clear to me why we are drawing into
> an EMF backed DC, then replaying the drawing into the printer DC. Why can't
> we simply draw direct to the printer DC in this scenario?
I saw following message in Chromium.
(https://cs.chromium.org/chromium/src/pdf/pdfium/pdfium_engine.cc?rcl=9ad9f6860b4d6a4ec7f7f975b2c99672e02d5d49&l=4008)
"Some PDFs seems to render very slowly if FPDF_RenderPage() is directly used on a printer DC."

Comment 76

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8845294 [details]
Bug 1345710 - part2: implement a PDFiumEngineShim class to link to PDFium symbols

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118472/diff/6-7/

Comment 77

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8845295 [details]
Bug 1345710 - part3: implement PDFViaEMFPrintHelper to convert PDF to EMF

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118474/diff/9-10/

Comment 78

[Jonathan Watt [:jwatt]]

(In reply to Farmer Tseng[:fatseng] from comment #75)
> (In reply to Jonathan Watt [:jwatt] from comment #72)
> > Why does this function exist? It isn't clear to me why we are drawing into
> > an EMF backed DC, then replaying the drawing into the printer DC. Why can't
> > we simply draw direct to the printer DC in this scenario?
> I saw following message in Chromium.
> (https://cs.chromium.org/chromium/src/pdf/pdfium/pdfium_engine.
> cc?rcl=9ad9f6860b4d6a4ec7f7f975b2c99672e02d5d49&l=4008)
> "Some PDFs seems to render very slowly if FPDF_RenderPage() is directly used
> on a printer DC."

I see. This is exactly the type of thing that should be clearly commented in the code. Otherwise someone else looking at this code has no clue why it works in the way it works. Can you add a comment please?

Comment 79

[Farmer Tseng[:fatseng](inactive)]

(In reply to Jonathan Watt [:jwatt] from comment #78)
> (In reply to Farmer Tseng[:fatseng] from comment #75)
> > (In reply to Jonathan Watt [:jwatt] from comment #72)
> > > Why does this function exist? It isn't clear to me why we are drawing into
> > > an EMF backed DC, then replaying the drawing into the printer DC. Why can't
> > > we simply draw direct to the printer DC in this scenario?
> > I saw following message in Chromium.
> > (https://cs.chromium.org/chromium/src/pdf/pdfium/pdfium_engine.
> > cc?rcl=9ad9f6860b4d6a4ec7f7f975b2c99672e02d5d49&l=4008)
> > "Some PDFs seems to render very slowly if FPDF_RenderPage() is directly used
> > on a printer DC."
> 
> I see. This is exactly the type of thing that should be clearly commented in
> the code. Otherwise someone else looking at this code has no clue why it
> works in the way it works. Can you add a comment please?

Yes, I added a comment in patch.
Please help review it.

Comment 80

[Jonathan Watt [:jwatt]]

Ah, thanks.

Looking at the chromium comment you link to, it's not clear that the approach you are taking here will actually perform better than drawing directly to the printer DC. They found that sending a bitmap to the printer DC was fast. But replaying an EMF file as you're doing isn't the same as sending a bitmap, and it could be that replaying an EMF is just as slow as drawing to the printer DC directly. In fact if I were to guess I'd say it was more likely than not likely.

So do you have any idea if your approach *actually* avoids the slowness issues? If not, that's possibly fine for now, but we should file a bug to investigate, and expand the comment no note that bug number.

Comment 81

[Farmer Tseng[:fatseng](inactive)]

From google's comment, there is a problem in FPDF_RenderPage while using the print's DC. They check device-specific information of DC. If it is a print's DC, they send a bitmap to it. If not, they call FPDF_RenderPage directly. I traced the printing flow by break point. They always call FPDF_RenderPage and use the EMF DC. FPDF_RenderPage just converts PDF to EMF and they call Windows API to replay EMF. It shouldn't have performance issue by using Windows API. Therefore, I guess that using EMF could avoid the slowness issues. Maybe I am wrong.

Comment 82

[Farmer Tseng[:fatseng](inactive)]

Actually, I am not sure if my approach could avoid the slowness issues, I will file a bug to investigate.

Comment 83

[Farmer Tseng[:fatseng](inactive)]

(In reply to Jonathan Watt [:jwatt] from comment #80)
> Ah, thanks.
> 
> Looking at the chromium comment you link to, it's not clear that the
> approach you are taking here will actually perform better than drawing
> directly to the printer DC. They found that sending a bitmap to the printer
> DC was fast. But replaying an EMF file as you're doing isn't the same as
> sending a bitmap, and it could be that replaying an EMF is just as slow as
> drawing to the printer DC directly. In fact if I were to guess I'd say it
> was more likely than not likely.
> 
> So do you have any idea if your approach *actually* avoids the slowness
> issues? If not, that's possibly fine for now, but we should file a bug to
> investigate, and expand the comment no note that bug number.

Create Bug 1359298 to investigate.

Comment 84

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8845295 [details]
Bug 1345710 - part3: implement PDFViaEMFPrintHelper to convert PDF to EMF

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118474/diff/10-11/

Comment 85

[Jonathan Watt [:jwatt]]

(In reply to Farmer Tseng[:fatseng] from comment #81)
> From google's comment, there is a problem in FPDF_RenderPage while using the
> print's DC. They check device-specific information of DC. If it is a print's
> DC, they send a bitmap to it. If not, they call FPDF_RenderPage directly. I
> traced the printing flow by break point. They always call FPDF_RenderPage
> and use the EMF DC.

Sure, but it's the render-to-print-DC case that we care about being slow, and what you're describing above so far is print-to-emf-DC.

> FPDF_RenderPage just converts PDF to EMF and they call
> Windows API to replay EMF.

Are you specifically saying that the replay is (or can be) to a *print* DC? That's what matters here, and I haven't seen the code that does that.

> It shouldn't have performance issue by using Windows API.

The comment you linked to says there are issues in some circumstances.

(In reply to Farmer Tseng[:fatseng] from comment #83)
> Create Bug 1359298 to investigate.

That's great, thanks!

Comment 86

[Jonathan Watt [:jwatt]]

Comment on attachment 8845295 [details]
Bug 1345710 - part3: implement PDFViaEMFPrintHelper to convert PDF to EMF

https://reviewboard.mozilla.org/r/118474/#review135732

In addition to the comments below I still think it would be more consistant to change the name 'metafile' to 'emf' in this patch.

::: widget/windows/PDFViaEMFPrintHelper.cpp:145
(Diff revision 10)
> +                               int aPageWidth, int aPageHeight)
> +{
> +  /*
> +   * https://cs.chromium.org/chromium/src/pdf/pdfium/pdfium_engine.cc?rcl=9ad9f6860b4d6a4ec7f7f975b2c99672e02d5d49&l=4008
> +   * Some PDFs seems to render very slowly if RenderPageToDC is directly used
> +   * on a printer DC.

Ah, you added a comment, thanks.

::: widget/windows/PDFViaEMFPrintHelper.h:60
(Diff revision 11)
> +  bool RenderPageToDC(HDC aDC, unsigned int aPageIndex,
> +                      int aPageWidth, int aPageHeight);
> +
> +  UniquePtr<PDFiumEngineShim> mPDFiumEngine;
> +  FPDF_DOCUMENT               mPDFDoc;
> +  Vector<char>                mPDFData;

Can you call this mPDFFileContents to make its naming a bit more specific.

::: widget/windows/PDFViaEMFPrintHelper.cpp:78
(Diff revision 11)
> +
> +nsresult
> +PDFViaEMFPrintHelper::OpenDocument(nsIFile *aFile)
> +{
> +  MOZ_ASSERT(aFile);
> +  NS_ENSURE_FALSE(mPDFDoc, NS_ERROR_FAILURE);

It would be better to use MOZ_ASSERT in this case too:

  if (mPDFDoc) {
    MOZ_ASSERT_UNREACHABLE("We can only open one PDF at a time");
    return NS_ERROR_FAILURE;
  }

::: widget/windows/PDFViaEMFPrintHelper.cpp:80
(Diff revision 11)
> +PDFViaEMFPrintHelper::OpenDocument(nsIFile *aFile)
> +{
> +  MOZ_ASSERT(aFile);
> +  NS_ENSURE_FALSE(mPDFDoc, NS_ERROR_FAILURE);
> +
> +  Init();

I guess since this is the only caller you might as well inline the contents of Init() here.

::: widget/windows/PDFViaEMFPrintHelper.cpp:85
(Diff revision 11)
> +  Init();
> +
> +  if (!LoadPDFDataToBuffer(aFile)) {
> +    return NS_ERROR_FAILURE;
> +  }
> +  mPDFDoc = mPDFiumEngine->LoadMemDocument(mPDFData.begin(),

Can you add a comment before this line explaining why we need to open an in-memory document? Does PDFium not provide API to open a document by path? Or to we just not implement a shim to that API yet? If the latter is true, can you open a bug to do that, and add that bug number to the comment you add here?

::: widget/windows/PDFViaEMFPrintHelper.cpp:88
(Diff revision 11)
> +    return NS_ERROR_FAILURE;
> +  }
> +  mPDFDoc = mPDFiumEngine->LoadMemDocument(mPDFData.begin(),
> +                                           mPDFData.length(),
> +                                           nullptr);
> +  NS_ENSURE_TRUE(mPDFDoc, NS_ERROR_FAILURE);

I guess we should clear mPDFData if we return here.

::: widget/windows/PDFViaEMFPrintHelper.cpp:104
(Diff revision 11)
> +PDFViaEMFPrintHelper::RenderPageToDC(HDC aDC, unsigned int aPageIndex,
> +                                     int aPageWidth, int aPageHeight)
> +{
> +  MOZ_ASSERT(aDC);
> +  NS_ENSURE_TRUE(mPDFDoc, false);
> +  NS_ENSURE_TRUE(static_cast<int>(aPageIndex) <

I think both these should be MOZ_ASSERT too. We shouldn't assume that callers are going to misues the API (not check the return value of OpenDocument - unless you know that will happen for some reason?).

::: widget/windows/PDFViaEMFPrintHelper.cpp:107
(Diff revision 11)
> +  MOZ_ASSERT(aDC);
> +  NS_ENSURE_TRUE(mPDFDoc, false);
> +  NS_ENSURE_TRUE(static_cast<int>(aPageIndex) <
> +                 mPDFiumEngine->GetPageCount(mPDFDoc), false);
> +
> +  if (aPageWidth == 0 || aPageHeight == 0) {

Probably <= given that's as cheap to check.

::: widget/windows/PDFViaEMFPrintHelper.cpp:147
(Diff revision 11)
> +  /*
> +   * There is a comment in Chromium.
> +   * https://cs.chromium.org/chromium/src/pdf/pdfium/pdfium_engine.cc?rcl=9ad9f6860b4d6a4ec7f7f975b2c99672e02d5d49&l=4008
> +   * Some PDFs seems to render very slowly if RenderPageToDC is directly used
> +   * on a printer DC.
> +   * Per checking Chromium code, one of the ways they use is to render PDF to

Please replace this line and the rest of the comment with:

   * The way Chromium works around the issue at the code linked above is to
   * print to a bitmap and send that to a printer.  Instead of doing that we
   * render to an EMF file and replay that on the printer DC.  It is unclear
   * whether our approach will avoid the performance issues though.  Bug
   * 1359298 covers investigating that.

(Also it's not normal style to use /* */ comments inline in functions. The normal comment is //. Feel free to use what you want, but I just thought I'd point that out.)

::: widget/windows/PDFViaEMFPrintHelper.cpp:157
(Diff revision 11)
> +  WindowsEMF metafile;
> +  bool result = metafile.InitForDrawing();
> +  NS_ENSURE_TRUE(result, false);
> +
> +  result = RenderPageToDC(metafile.GetDC(), aPageIndex,
> +                           aPageWidth, aPageHeight);

This line is indented by one too many spaces.

::: widget/windows/PDFViaEMFPrintHelper.cpp:175
(Diff revision 11)
> +  WindowsEMF metafile;
> +  bool result = metafile.InitForDrawing(aFilePath);
> +  NS_ENSURE_TRUE(result, false);
> +
> +  result = RenderPageToDC(metafile.GetDC(), aPageIndex,
> +                           aPageWidth, aPageHeight);

This line is indented by one too many spaces.

::: widget/windows/PDFViaEMFPrintHelper.cpp:184
(Diff revision 11)
> +
> +void
> +PDFViaEMFPrintHelper::CloseDocument()
> +{
> +  if (mPDFDoc) {
> +    mPDFiumEngine->CloseDocument(mPDFDoc);

Should we null out mPDFiumEngine here?

Comment 87

[Farmer Tseng[:fatseng](inactive)]

(In reply to Jonathan Watt [:jwatt] from comment #85)
> (In reply to Farmer Tseng[:fatseng] from comment #81)
> 
> > FPDF_RenderPage just converts PDF to EMF and they call
> > Windows API to replay EMF.
> 
> Are you specifically saying that the replay is (or can be) to a *print* DC?
> That's what matters here, and I haven't seen the code that does that.
> 
Yes.

Comment 88

[Farmer Tseng[:fatseng](inactive)]

(In reply to Jonathan Watt [:jwatt] from comment #86)
> Comment on attachment 8845295 [details]
> Bug 1345710 - part3: implement PDFViaEMFPrintHelper to convert PDF to EMF
> 
> https://reviewboard.mozilla.org/r/118474/#review135732
> 
> ::: widget/windows/PDFViaEMFPrintHelper.cpp:85
> (Diff revision 11)
> > +  Init();
> > +
> > +  if (!LoadPDFDataToBuffer(aFile)) {
> > +    return NS_ERROR_FAILURE;
> > +  }
> > +  mPDFDoc = mPDFiumEngine->LoadMemDocument(mPDFData.begin(),
> 
> Can you add a comment before this line explaining why we need to open an
> in-memory document? Does PDFium not provide API to open a document by path?
> Or to we just not implement a shim to that API yet? If the latter is true,
> can you open a bug to do that, and add that bug number to the comment you
> add here?
> 
I found there is an API to open document by path in PDFium.
But google doesn't use it.
BTW, I will implement it in PDFiumEngineShim, Bug 1359713 .
> 
> ::: widget/windows/PDFViaEMFPrintHelper.cpp:184
> (Diff revision 11)
> > +
> > +void
> > +PDFViaEMFPrintHelper::CloseDocument()
> > +{
> > +  if (mPDFDoc) {
> > +    mPDFiumEngine->CloseDocument(mPDFDoc);
> 
> Should we null out mPDFiumEngine here?
We don't need to null out it here because it could be reused for next document.

Comment 89

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8845295 [details]
Bug 1345710 - part3: implement PDFViaEMFPrintHelper to convert PDF to EMF

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118474/diff/11-12/

Comment 90

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8845295 [details]
Bug 1345710 - part3: implement PDFViaEMFPrintHelper to convert PDF to EMF

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118474/diff/12-13/

Comment 91

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8845293 [details]
Bug 1345710 - part1: implement a WindowsEMF class to support Windows Enhance Metafile

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118470/diff/5-6/

Comment 92

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8845294 [details]
Bug 1345710 - part2: implement a PDFiumEngineShim class to link to PDFium symbols

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118472/diff/7-8/

Comment 93

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8845295 [details]
Bug 1345710 - part3: implement PDFViaEMFPrintHelper to convert PDF to EMF

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118474/diff/13-14/

Comment 94

[Farmer Tseng[:fatseng](inactive)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=687a7aff680b28e878799c69dd5e21aec1aff7d9

Comment 95

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/23d0ebe838a6
part1: implement a WindowsEMF class to support Windows Enhance Metafile r=cjku,jwatt
https://hg.mozilla.org/integration/autoland/rev/aff3b1f91621
part2: implement a PDFiumEngineShim class to link to PDFium symbols r=cjku,jwatt
https://hg.mozilla.org/integration/autoland/rev/bf788d19f2d3
part3: implement PDFViaEMFPrintHelper to convert PDF to EMF r=cjku,jwatt

Comment 96

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/23d0ebe838a6
https://hg.mozilla.org/mozilla-central/rev/aff3b1f91621
https://hg.mozilla.org/mozilla-central/rev/bf788d19f2d3