1358076 - [PDF gtest] Implement unit test for PDFViaEMFPrintHelper

Status: RESOLVED FIXED

(Core :: Printing: Output, enhancement, P1)

Comment 1

[Farmer Tseng[:fatseng](inactive)]

Attachment: WIP gtest

Comment 2

[Farmer Tseng[:fatseng](inactive)]

(In reply to Farmer Tseng[:fatseng] from comment #1)
> Created attachment 8882661 [details] [diff] [review]
> WIP gtest

Upload WIP patch.

Comment 3

[Farmer Tseng[:fatseng](inactive)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d51d85f5849645f5dadf88aefbc89b46a1b5e36

Comment 4

[Farmer Tseng[:fatseng](inactive)]

Attachment: Bug 1358076 - Part1. Add a gtest to verify PDFViaEMFPrintHelper

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

Comment 5

[Farmer Tseng[:fatseng](inactive)]

I wrote following gtest to verify operating PDFium.
1. WindowsEMF Init test
2. Convert PDFiumTest.pdf to an EMF file and check the file size
3. Input a PDF file which does not exist
4. Test unreasonable width or height

Comment 6

[Farmer Tseng[:fatseng](inactive)]

Attachment: Bug 1358076 - Part1. Create an API to load a PDF file by |const char*|

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

Comment 7

[Farmer Tseng[:fatseng](inactive)]

Attachment: bug 1358076 - Part2. Enable gtest for PDFViaEMFPrintHelper

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

Comment 8

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8884224 [details]
Bug 1358076 - Part1. Add a gtest to verify PDFViaEMFPrintHelper

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

Comment 9

[Bruce Sun [:brsun]]

Comment on attachment 8885147 [details]
Bug 1358076 - Part1. Create an API to load a PDF file by |const char*|

https://reviewboard.mozilla.org/r/156012/#review161178

::: widget/windows/PDFViaEMFPrintHelper.h:36
(Diff revision 1)
>    PDFViaEMFPrintHelper();
>    ~PDFViaEMFPrintHelper();
>  
>    /** Loads the specified PDF file. */
>    NS_IMETHOD OpenDocument(nsIFile *aFile);
> +  NS_IMETHOD OpenDocument(const char* aFile);

This function might not be a mendatory function for using or testing PDFViaEMFPrintHelper.

Comment 10

[Bruce Sun [:brsun]]

Comment on attachment 8884224 [details]
Bug 1358076 - Part1. Add a gtest to verify PDFViaEMFPrintHelper

https://reviewboard.mozilla.org/r/155170/#review161164

I found lots of usage of ASSERT_* in other gtest files but very few usage of GTEST_ASSERT_*. I am not sure what's the difference between GTEST_ASSERT_* and ASSERT_*, but probably we should use ASSERT_* ones.

::: widget/windows/tests/gtest/TestPDFtoEMF.cpp:24
(Diff revision 2)
> +
> +UniquePtr<PDFViaEMFPrintHelper>
> +CreatePrintHelper(const char* aFile)
> +{
> +  UniquePtr<PDFViaEMFPrintHelper> PDFPrintHelper = MakeUnique<PDFViaEMFPrintHelper>();
> +  nsresult rv = PDFPrintHelper->OpenDocument(aFile);

Suggest to use nsIFile interface if possible. So that we can save one unused public function of PDFViaEMFPrintHelper.

::: widget/windows/tests/gtest/TestPDFtoEMF.cpp:39
(Diff revision 2)
> +}
> +
> +NS_IMETHODIMP
> +GetEMFFilePath(nsAutoCString& aPath)
> +{
> +  WCHAR tempPathBuffer[MAX_PATH+1];

nit: [MAX_PATH + 1] (with space before and after the + operator)

::: widget/windows/tests/gtest/TestPDFtoEMF.cpp:51
(Diff revision 2)
> +  tempLocalFile->InitWithPath(tempPath);
> +  tempLocalFile->AppendNative(NS_LITERAL_CSTRING("gtest.emf"));
> +  return tempLocalFile->GetNativePath(aPath);
> +}
> +
> +// WindowsEMF Init test

Suggest separate this test into another test file.

::: widget/windows/tests/gtest/TestPDFtoEMF.cpp:59
(Diff revision 2)
> +  WindowsEMF emf;
> +  ASSERT_TRUE(emf.InitForDrawing());
> +}
> +
> +// Convert PDFiumTest.pdf to a EMF file and check the file size
> +TEST(PDFtoEMF, SaveEMFtoFile)

Rename the test with something like TestEMFConversion.

::: widget/windows/tests/gtest/TestPDFtoEMF.cpp:62
(Diff revision 2)
> +
> +// Convert PDFiumTest.pdf to a EMF file and check the file size
> +TEST(PDFtoEMF, SaveEMFtoFile)
> +{
> +  UniquePtr<PDFViaEMFPrintHelper> PDFHelper =
> +    CreatePrintHelper("PDFiumTest.pdf");

Suppose we will have not only one PDF file for testing, so I would suggest to use a more informative name for this PDF file (ex. PDFiumTest_4961_7016.pdf or something more meaningful to the purpose).

::: widget/windows/tests/gtest/TestPDFtoEMF.cpp:63
(Diff revision 2)
> +// Convert PDFiumTest.pdf to a EMF file and check the file size
> +TEST(PDFtoEMF, SaveEMFtoFile)
> +{
> +  UniquePtr<PDFViaEMFPrintHelper> PDFHelper =
> +    CreatePrintHelper("PDFiumTest.pdf");
> +  GTEST_ASSERT_NE(PDFHelper, nullptr);

ASSERT_NE

::: widget/windows/tests/gtest/TestPDFtoEMF.cpp:66
(Diff revision 2)
> +  UniquePtr<PDFViaEMFPrintHelper> PDFHelper =
> +    CreatePrintHelper("PDFiumTest.pdf");
> +  GTEST_ASSERT_NE(PDFHelper, nullptr);
> +
> +  nsAutoCString emfPath;
> +  nsresult rv = GetEMFFilePath(emfPath);

Suggest directly get nsAutoString instead of converting from UTF8 to UTF16 each time afterwards.

::: widget/windows/tests/gtest/TestPDFtoEMF.cpp:83
(Diff revision 2)
> +  PDFHelper->CloseDocument();
> +
> +  RefPtr<nsLocalFile> savedFile = new nsLocalFile();
> +  savedFile->InitWithPath(NS_ConvertUTF8toUTF16(emfPath));
> +  RefPtr<nsFileInputStream> inputStream = new nsFileInputStream();
> +  rv = inputStream->Init(savedFile, -1, -1, 0);

nit: add more comments (i.e. /* ioFlags */, /* perm */, /* behaviorFlags */) for those hardcoded default values.

::: widget/windows/tests/gtest/TestPDFtoEMF.cpp:91
(Diff revision 2)
> +    return;
> +  }
> +
> +  int64_t size = 0;
> +  inputStream->GetSize(&size);
> +  GTEST_ASSERT_EQ(size, 19175024);

ASSERT_EQ.

By the way, would it be doable to add the corresponding EMF file into m-c for this gtest? So that we can directly compare the equality of their content.

::: widget/windows/tests/gtest/TestPDFtoEMF.cpp:97
(Diff revision 2)
> +  inputStream = nullptr;
> +  savedFile->Remove(/* aRecursive */ false);
> +}
> +
> +// Input a PDF file which does not exist
> +TEST(PDFtoEMF, InputNullPDF)

Rename the test with something like TestInputNonExistingPDF.

::: widget/windows/tests/gtest/TestPDFtoEMF.cpp:104
(Diff revision 2)
> +  UniquePtr<PDFViaEMFPrintHelper> PDFHelper = CreatePrintHelper("null.pdf");
> +  GTEST_ASSERT_EQ(PDFHelper, nullptr);
> +}
> +
> +// Test unreasonable width or height
> +TEST(PDFtoEMF, TestWidthAndHeight)

Rename the test with something like TestInsufficientWithAndHeight.

::: widget/windows/tests/gtest/TestPDFtoEMF.cpp:108
(Diff revision 2)
> +// Test unreasonable width or height
> +TEST(PDFtoEMF, TestWidthAndHeight)
> +{
> +  UniquePtr<PDFViaEMFPrintHelper> PDFHelper =
> +    CreatePrintHelper("PDFiumTest.pdf");
> +  GTEST_ASSERT_NE(PDFHelper, nullptr);

ASSERT_NE

::: widget/windows/tests/gtest/moz.build:7
(Diff revision 2)
> +# vim: set filetype=python:
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, you can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +Library('gfxtest')

Any reason this line is needed?

Comment 11

[Bruce Sun [:brsun]]

Comment on attachment 8885148 [details]
bug 1358076 - Part2. Enable gtest for PDFViaEMFPrintHelper

https://reviewboard.mozilla.org/r/156014/#review161186

::: widget/windows/moz.build:124
(Diff revision 1)
>    LOCAL_INCLUDES += [
>      '/gfx/skia/skia/include/config',
>      '/gfx/skia/skia/include/core',
>      '/modules/pdfium/pdfium/public',
>    ]
> +  TEST_DIRS += ['tests/gtest']

|gtest| probably deserves its own directory instead of living inside |tests| folder (by glancing other gtest folder structure of other modules).

Comment 12

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8884224 [details]
Bug 1358076 - Part1. Add a gtest to verify PDFViaEMFPrintHelper

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

Comment 13

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8885148 [details]
bug 1358076 - Part2. Enable gtest for PDFViaEMFPrintHelper

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

Comment 14

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8884224 [details]
Bug 1358076 - Part1. Add a gtest to verify PDFViaEMFPrintHelper

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

Comment 15

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8885148 [details]
bug 1358076 - Part2. Enable gtest for PDFViaEMFPrintHelper

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

Comment 16

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8884224 [details]
Bug 1358076 - Part1. Add a gtest to verify PDFViaEMFPrintHelper

https://reviewboard.mozilla.org/r/155170/#review161164

> ASSERT_EQ.
> 
> By the way, would it be doable to add the corresponding EMF file into m-c for this gtest? So that we can directly compare the equality of their content.

I will create another bug to follow up.

Comment 17

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8884224 [details]
Bug 1358076 - Part1. Add a gtest to verify PDFViaEMFPrintHelper

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

Comment 18

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8885148 [details]
bug 1358076 - Part2. Enable gtest for PDFViaEMFPrintHelper

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

Comment 19

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8884224 [details]
Bug 1358076 - Part1. Add a gtest to verify PDFViaEMFPrintHelper

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

Comment 20

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8885148 [details]
bug 1358076 - Part2. Enable gtest for PDFViaEMFPrintHelper

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

Comment 21

[Farmer Tseng[:fatseng](inactive)]

I would like dump GDI commands from EMF file and compare with reference one in gtest.

Comment 22

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8884224 [details]
Bug 1358076 - Part1. Add a gtest to verify PDFViaEMFPrintHelper

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

Comment 23

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8885148 [details]
bug 1358076 - Part2. Enable gtest for PDFViaEMFPrintHelper

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

Comment 24

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8884224 [details]
Bug 1358076 - Part1. Add a gtest to verify PDFViaEMFPrintHelper

I dump GDI commands from EMF file and compare with the corresponding EMF file.

Comment 25

[Farmer Tseng[:fatseng](inactive)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=28cad7d26e808ca90aa49962db11c55f4519e96b

Comment 26

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8884224 [details]
Bug 1358076 - Part1. Add a gtest to verify PDFViaEMFPrintHelper

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

Comment 27

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8885148 [details]
bug 1358076 - Part2. Enable gtest for PDFViaEMFPrintHelper

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

Comment 28

[Bruce Sun [:brsun]]

Comment on attachment 8884224 [details]
Bug 1358076 - Part1. Add a gtest to verify PDFViaEMFPrintHelper

https://reviewboard.mozilla.org/r/155170/#review164012

::: widget/windows/gtest/TestEMFConversion.cpp:48
(Diff revision 8)
> +
> +  return PDFPrintHelper;
> +}
> +
> +NS_IMETHODIMP
> +GetSpecifiedFilePathfromTempFolder(nsAutoString& aPath, const char* aFile)

Suggest to use a more precise name for this function. Do you mean an absolute path or a canonical path by "SpecifiedFilePath"?

::: widget/windows/gtest/TestEMFConversion.cpp:68
(Diff revision 8)
> +  }
> +  return rv;
> +}
> +
> +int CALLBACK
> +EnhMetaFileProc(HDC ahdc,

s/ahdc/aHDC

::: widget/windows/gtest/TestEMFConversion.cpp:69
(Diff revision 8)
> +  return rv;
> +}
> +
> +int CALLBACK
> +EnhMetaFileProc(HDC ahdc,
> +                HANDLETABLE* ahandleTable,

s/ahandleTable/aHandleTable

::: widget/windows/gtest/TestEMFConversion.cpp:75
(Diff revision 8)
> +                const ENHMETARECORD* aRecord,
> +                int aObjectsCount,
> +                LPARAM aParam)
> +{
> +  // Save a GDI command to a Vector
> +  Vector<DWORD>* contents = (Vector<DWORD>*) (aParam);

Would it be possible to store and to compare all parameters within ENHMETARECORD structure?

::: widget/windows/gtest/TestEMFConversion.cpp:95
(Diff revision 8)
> +  HENHMETAFILE hemf = ::GetEnhMetaFileW(aEMFPath.get());
> +  if (hemf == NULL) {
> +    return false;
> +  }
> +
> +  HDC hdc = ::GetDC(NULL);

s/hdc/hScreenDC/ if you are meant to get the DC of the entire screen.

::: widget/windows/gtest/TestEMFConversion.cpp:101
(Diff revision 8)
> +  if (hdc == NULL) {
> +    return false;
> +  }
> +
> +  RECT rect = {0, 0, aPageWidth, aPageHeight};
> +  bool result = ::EnumEnhMetaFile(hdc, hemf, EnhMetaFileProc,

Instead of using the DC of the entire screen for testing, I would suggest to use CreateCompatibleDC and CreateCompatibleBitmap to create a memory DC for testing purpose.

::: widget/windows/gtest/TestEMFConversion.cpp:145
(Diff revision 8)
> +
> +  int64_t size = 0;
> +  inputStream->GetSize(&size);
> +  ASSERT_GT(size, 0);
> +
> +  // Enumerate GDI commands from gtest.emf and save to a buffer

Suggest to split codes with a new function for comparing two existing EMF files.

::: widget/windows/gtest/TestEMFConversion.cpp:153
(Diff revision 8)
> +                           pageWidth, pageHeight);
> +  ASSERT_TRUE(result);
> +
> +  savedFile = nullptr;
> +
> +  // Get the reference file, PrinterTestPage_ref.emf.

Suggest to use a function for getting files under the current directory to reduce the duplication of codes. Probably it's also worth to further reduce the duplication with some flexibility to get the temporary path with this new function.

::: widget/windows/gtest/TestEMFConversion.cpp:177
(Diff revision 8)
> +  Vector<DWORD> emfFileContentsRef;
> +  result = SaveGDICommands(emfPathRef, &emfFileContentsRef,
> +                           pageWidth, pageHeight);
> +  ASSERT_TRUE(result);
> +
> +  // Compare GDI commands

Add an assertion for the equality of the number of records in both vectors.

::: widget/windows/gtest/moz.build:18
(Diff revision 8)
> +    '/widget/windows',
> +]
> +
> +TEST_HARNESS_FILES.gtest += [
> +    'PrinterTestPage.pdf',
> +    'PrinterTestPage_ref.emf',

Could you help share how this EMF file is generated?

Comment 29

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8884224 [details]
Bug 1358076 - Part1. Add a gtest to verify PDFViaEMFPrintHelper

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

Comment 30

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8885148 [details]
bug 1358076 - Part2. Enable gtest for PDFViaEMFPrintHelper

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

Comment 31

[Bruce Sun [:brsun]]

Comment on attachment 8884224 [details]
Bug 1358076 - Part1. Add a gtest to verify PDFViaEMFPrintHelper

https://reviewboard.mozilla.org/r/155170/#review165602

::: widget/windows/gtest/TestEMFConversion.cpp:22
(Diff revision 9)
> +using namespace mozilla;
> +namespace mozilla {
> +namespace widget {
> +
> +NS_IMETHODIMP
> +GetRealPath(const char* aSpecialDirName,

|GetRealPath| doesn't reflect the real functionality of this function.

::: widget/windows/gtest/TestEMFConversion.cpp:24
(Diff revision 9)
> +namespace widget {
> +
> +NS_IMETHODIMP
> +GetRealPath(const char* aSpecialDirName,
> +            const char* aFileName,
> +            nsIFile** aResult)

nit: Suggest to use smart pointers instead of raw pointers if it is possible. How about returning nsIFile instead of passing it as an out-param?

::: widget/windows/gtest/TestEMFConversion.cpp:42
(Diff revision 9)
> +NS_IMETHODIMP
> +GetRealPath(const char* aSpecialDirName,
> +            const char* aFileName,
> +            nsAutoString& aPath)
> +{
> +  RefPtr<nsIFile> tempLocalFile;

nsCOMPtr<nsIFile> file;

ref: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/nsCOMPtr_versus_RefPtr#General_Rule_of_Thumb_for_nsCOMPtr_versus_RefPtr

::: widget/windows/gtest/TestEMFConversion.cpp:47
(Diff revision 9)
> +  RefPtr<nsIFile> tempLocalFile;
> +  nsresult rv = GetRealPath(aSpecialDirName, aFileName,
> +                            getter_AddRefs(tempLocalFile));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsAutoCString localPath;

nit: the prefix 'local' doesn't give this variable more meaning from others. Probably |path| would suffice already. The same comment applys to other lines below as well.

::: widget/windows/gtest/TestEMFConversion.cpp:81
(Diff revision 9)
> +                const ENHMETARECORD* aRecord,
> +                int aObjectsCount,
> +                LPARAM aParam)
> +{
> +  // Save a GDI command to a Vector
> +  Vector<ENHMETARECORD>* contents = (Vector<ENHMETARECORD>*) (aParam);

I guess ENHMETARECORD structure itself doesn't contain the whole records. So if you want to compare all parameters, you probably need to go beyound |dParm|.

::: widget/windows/gtest/TestEMFConversion.cpp:170
(Diff revision 9)
> +                                          pageWidth, pageHeight);
> +  ASSERT_TRUE(result);
> +
> +  PDFHelper->CloseDocument();
> +
> +  // Check the EMF file size if it is great than zero

// Check the EMF file size and make sure it is greater than zero

::: widget/windows/gtest/TestEMFConversion.cpp:184
(Diff revision 9)
> +
> +  int64_t size = 0;
> +  inputStream->GetSize(&size);
> +  ASSERT_GT(size, 0);
> +
> +  nsCOMPtr<nsIFile> emfRef;

I guess you can get a real path directly from your utility function instead of retriving from nsIFile manually here, right?

Comment 32

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8884224 [details]
Bug 1358076 - Part1. Add a gtest to verify PDFViaEMFPrintHelper

https://reviewboard.mozilla.org/r/155170/#review168242

::: widget/windows/gtest/TestEMFConversion.cpp:81
(Diff revision 9)
> +                const ENHMETARECORD* aRecord,
> +                int aObjectsCount,
> +                LPARAM aParam)
> +{
> +  // Save a GDI command to a Vector
> +  Vector<ENHMETARECORD>* contents = (Vector<ENHMETARECORD>*) (aParam);

Every Windows platform might generate a little bit different content of EMF. Comparing all parameters would be failed. I compared the GDI commands in this test to make sure the EMF is correct and created a followed up bug to compare the bitmap (Bug 1382512).

Comment 33

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8884224 [details]
Bug 1358076 - Part1. Add a gtest to verify PDFViaEMFPrintHelper

https://reviewboard.mozilla.org/r/155170/#review165602

> I guess ENHMETARECORD structure itself doesn't contain the whole records. So if you want to compare all parameters, you probably need to go beyound |dParm|.

Every Windows platform might generate a little bit different content of EMF. Comparing all parameters would be failed. I compared the GDI commands in this test to make sure the EMF is correct and created a followed up bug to compare the bitmap (Bug 1382512).

Comment 34

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8884224 [details]
Bug 1358076 - Part1. Add a gtest to verify PDFViaEMFPrintHelper

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

Comment 35

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8885148 [details]
bug 1358076 - Part2. Enable gtest for PDFViaEMFPrintHelper

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

Comment 36

[Bruce Sun [:brsun]]

> Every Windows platform might generate a little bit different content of EMF.

So...if this is the case, I am curious about what can be guaranteed by comparing a run-time generated EMF with a pre-generated EMF file. Can we foresee any false-positive and false-negative cases by simply comparing GDI commands?

Comment 37

[Farmer Tseng[:fatseng](inactive)]

(In reply to Bruce Sun [:brsun] from comment #36)
> > Every Windows platform might generate a little bit different content of EMF.
> 
> So...if this is the case, I am curious about what can be guaranteed by
> comparing a run-time generated EMF with a pre-generated EMF file. Can we
> foresee any false-positive and false-negative cases by simply comparing GDI
> commands?

I can't guarantee it. I will remove GDI commands comparison and implement bitmap comparison(Bug 1382512).

Comment 38

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8884224 [details]
Bug 1358076 - Part1. Add a gtest to verify PDFViaEMFPrintHelper

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

Comment 39

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8885148 [details]
bug 1358076 - Part2. Enable gtest for PDFViaEMFPrintHelper

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

Comment 40

[Bruce Sun [:brsun]]

Comment on attachment 8884224 [details]
Bug 1358076 - Part1. Add a gtest to verify PDFViaEMFPrintHelper

https://reviewboard.mozilla.org/r/155170/#review169654

::: widget/windows/gtest/TestEMFConversion.cpp:70
(Diff revision 11)
> +
> +  return PDFPrintHelper;
> +}
> +
> +// Convert PrinterTestPage.pdf to a EMF file and compare GDI commands with
> +// reference one

Please change comments accordingly.

Comment 41

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8884224 [details]
Bug 1358076 - Part1. Add a gtest to verify PDFViaEMFPrintHelper

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

Comment 42

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8885148 [details]
bug 1358076 - Part2. Enable gtest for PDFViaEMFPrintHelper

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

Comment 43

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8884224 [details]
Bug 1358076 - Part1. Add a gtest to verify PDFViaEMFPrintHelper

I implemented following test cases to verify PDFViaEMFPrintHelper.
1. Test PDF to EMF conversion.
2. Test a PDF file which does not exist.
3. Test insufficient width and height.

Comment 44

[Jonathan Watt [:jwatt]]

Comment on attachment 8884224 [details]
Bug 1358076 - Part1. Add a gtest to verify PDFViaEMFPrintHelper

https://reviewboard.mozilla.org/r/155170/#review170936

::: commit-message-7b5e7:1
(Diff revision 12)
> +Bug 1358076 - Part1. Implement gtest to verify PDFViaEMFPrintHelper

Make this "Add a gtest..."

::: commit-message-7b5e7:4
(Diff revision 12)
> +Bug 1358076 - Part1. Implement gtest to verify PDFViaEMFPrintHelper
> +
> +PrinterTestPage.pdf is from
> +http://zsory-furdo.hu/evcms_medias/upload/files/testfile.pdf

I believe this is originally from:

https://opensource.apple.com/source/cups/cups-136.9/cups/test/testfile.pdf

Please change the link.

Comment 45

[Jonathan Watt [:jwatt]]

Comment on attachment 8885148 [details]
bug 1358076 - Part2. Enable gtest for PDFViaEMFPrintHelper

https://reviewboard.mozilla.org/r/156014/#review170940

Comment 46

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8884224 [details]
Bug 1358076 - Part1. Add a gtest to verify PDFViaEMFPrintHelper

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

Comment 47

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8885148 [details]
bug 1358076 - Part2. Enable gtest for PDFViaEMFPrintHelper

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

Comment 48

[Farmer Tseng[:fatseng](inactive)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=643bb0a6ef6ca85c674c47b49e76c76c59c17add

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0e1dfba3554e
Part1. Add a gtest to verify PDFViaEMFPrintHelper r=jwatt
https://hg.mozilla.org/integration/autoland/rev/76650f11d581
Part2. Enable gtest for PDFViaEMFPrintHelper r=jwatt

Comment 50

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0e1dfba3554e
https://hg.mozilla.org/mozilla-central/rev/76650f11d581