1401069 - Fine tune WindowsEMF and PDFViaEMFPrintHelper

Status: RESOLVED FIXED

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

Description

[u459114]

1. Change some member functions to const
2. Make WindowsEMF reusable 
3. Fine tune implementation of some functions.

Comment 1

[u459114]

Attachment: Bug 1401069 - Part 1. Release EMF handle before function exist.

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

Comment 2

[u459114]

Attachment: Bug 1401069 - Part 2. Implement ReleaseAllResource to make WindowsEMF::InitForDrawing reusable.

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

Comment 3

[u459114]

Attachment: Bug 1401069 - Part 3. Declare WindowsEMF::GetDC as const member function.

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

Comment 4

[u459114]

Attachment: Bug 1401069 - Part 4. Change argument type from 'const RECT *' to reference type in WindowsEMF::Playback.

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

Comment 5

[u459114]

Attachment: Bug 1401069 - Part 5. Remove two local variables since we do not need them.

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

Comment 6

[u459114]

Attachment: Bug 1401069 - Part 6. Fine tune ComputeScaleFactor.

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

Comment 7

[u459114]

Attachment: Bug 1401069 - Part 7. Declare GetPageCount as a const member function.

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

Comment 8

[u459114]

Attachment: Bug 1401069 - Part 8. Give a detailed comment to explain how to react when hitting this assertion in OpenDocument().

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

Comment 9

[u459114]

Attachment: Bug 1401069 - Part 1. Check the value of mPDFDoc and early return if it's invalid in both RenderPageToDC and DrawPageToFile.

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

Comment 10

[u459114]

Comment on attachment 8909580 [details]
Bug 1401069 - Part 1. Release EMF handle before function exist.

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

Comment 11

[u459114]

Comment on attachment 8909581 [details]
Bug 1401069 - Part 2. Implement ReleaseAllResource to make WindowsEMF::InitForDrawing reusable.

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

Comment 12

[u459114]

Comment on attachment 8909582 [details]
Bug 1401069 - Part 3. Declare WindowsEMF::GetDC as const member function.

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

Comment 13

[u459114]

Comment on attachment 8909583 [details]
Bug 1401069 - Part 4. Change argument type from 'const RECT *' to reference type in WindowsEMF::Playback.

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

Comment 14

[u459114]

Comment on attachment 8909584 [details]
Bug 1401069 - Part 5. Remove two local variables since we do not need them.

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

Comment 15

[u459114]

Comment on attachment 8909585 [details]
Bug 1401069 - Part 6. Fine tune ComputeScaleFactor.

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

Comment 16

[u459114]

Comment on attachment 8909586 [details]
Bug 1401069 - Part 7. Declare GetPageCount as a const member function.

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

Comment 17

[u459114]

Comment on attachment 8909587 [details]
Bug 1401069 - Part 8. Give a detailed comment to explain how to react when hitting this assertion in OpenDocument().

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

Comment 18

[u459114]

Comment on attachment 8909588 [details]
Bug 1401069 - Part 1. Check the value of mPDFDoc and early return if it's invalid in both RenderPageToDC and DrawPageToFile.

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

Comment 19

[u459114]

Comment on attachment 8909580 [details]
Bug 1401069 - Part 1. Release EMF handle before function exist.

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

Comment 20

[u459114]

Comment on attachment 8909581 [details]
Bug 1401069 - Part 2. Implement ReleaseAllResource to make WindowsEMF::InitForDrawing reusable.

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

Comment 21

[u459114]

Comment on attachment 8909582 [details]
Bug 1401069 - Part 3. Declare WindowsEMF::GetDC as const member function.

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

Comment 22

[u459114]

Comment on attachment 8909583 [details]
Bug 1401069 - Part 4. Change argument type from 'const RECT *' to reference type in WindowsEMF::Playback.

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

Comment 23

[u459114]

Comment on attachment 8909584 [details]
Bug 1401069 - Part 5. Remove two local variables since we do not need them.

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

Comment 24

[u459114]

Comment on attachment 8909585 [details]
Bug 1401069 - Part 6. Fine tune ComputeScaleFactor.

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

Comment 25

[u459114]

Comment on attachment 8909586 [details]
Bug 1401069 - Part 7. Declare GetPageCount as a const member function.

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

Comment 26

[u459114]

Comment on attachment 8909587 [details]
Bug 1401069 - Part 8. Give a detailed comment to explain how to react when hitting this assertion in OpenDocument().

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

Comment 27

[u459114]

Comment on attachment 8909588 [details]
Bug 1401069 - Part 1. Check the value of mPDFDoc and early return if it's invalid in both RenderPageToDC and DrawPageToFile.

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

Comment 28

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8909580 [details]
Bug 1401069 - Part 1. Release EMF handle before function exist.

https://reviewboard.mozilla.org/r/181040/#review186890

::: widget/windows/WindowsEMF.cpp:73
(Diff revision 3)
>    if (!FinishDocument()) {
>      return false;
>    }
>  
> -  return ::PlayEnhMetaFile(aDeviceContext, mEmf, aRect) != 0;
> +  bool result = ::PlayEnhMetaFile(aDeviceContext, mEmf, aRect) != 0;
> +  ReleaseEMFHandle();

Remind you, if calling ReleaseEMFHandle() here, you just could playback one time.

Comment 29

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8909580 [details]
Bug 1401069 - Part 1. Release EMF handle before function exist.

https://reviewboard.mozilla.org/r/181040/#review186892

Comment 30

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8909581 [details]
Bug 1401069 - Part 2. Implement ReleaseAllResource to make WindowsEMF::InitForDrawing reusable.

https://reviewboard.mozilla.org/r/181042/#review186894

Comment 31

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8909582 [details]
Bug 1401069 - Part 3. Declare WindowsEMF::GetDC as const member function.

https://reviewboard.mozilla.org/r/181044/#review186896

Comment 32

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8909583 [details]
Bug 1401069 - Part 4. Change argument type from 'const RECT *' to reference type in WindowsEMF::Playback.

https://reviewboard.mozilla.org/r/181046/#review186898

Comment 33

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8909584 [details]
Bug 1401069 - Part 5. Remove two local variables since we do not need them.

https://reviewboard.mozilla.org/r/181048/#review186900

Comment 34

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8909585 [details]
Bug 1401069 - Part 6. Fine tune ComputeScaleFactor.

https://reviewboard.mozilla.org/r/181050/#review186904

Comment 35

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8909586 [details]
Bug 1401069 - Part 7. Declare GetPageCount as a const member function.

https://reviewboard.mozilla.org/r/181052/#review186906

Comment 36

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8909587 [details]
Bug 1401069 - Part 8. Give a detailed comment to explain how to react when hitting this assertion in OpenDocument().

https://reviewboard.mozilla.org/r/181054/#review186908

Comment 37

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8909588 [details]
Bug 1401069 - Part 1. Check the value of mPDFDoc and early return if it's invalid in both RenderPageToDC and DrawPageToFile.

https://reviewboard.mozilla.org/r/181056/#review186910

Comment 38

[u459114]

Comment on attachment 8909588 [details]
Bug 1401069 - Part 1. Check the value of mPDFDoc and early return if it's invalid in both RenderPageToDC and DrawPageToFile.

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

Comment 39

[u459114]

Comment on attachment 8909581 [details]
Bug 1401069 - Part 2. Implement ReleaseAllResource to make WindowsEMF::InitForDrawing reusable.

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

Comment 40

[u459114]

Comment on attachment 8909582 [details]
Bug 1401069 - Part 3. Declare WindowsEMF::GetDC as const member function.

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

Comment 41

[u459114]

Comment on attachment 8909583 [details]
Bug 1401069 - Part 4. Change argument type from 'const RECT *' to reference type in WindowsEMF::Playback.

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

Comment 42

[u459114]

Comment on attachment 8909584 [details]
Bug 1401069 - Part 5. Remove two local variables since we do not need them.

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

Comment 43

[u459114]

Comment on attachment 8909585 [details]
Bug 1401069 - Part 6. Fine tune ComputeScaleFactor.

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

Comment 44

[u459114]

Comment on attachment 8909586 [details]
Bug 1401069 - Part 7. Declare GetPageCount as a const member function.

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

Comment 45

[u459114]

Comment on attachment 8909587 [details]
Bug 1401069 - Part 8. Give a detailed comment to explain how to react when hitting this assertion in OpenDocument().

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

Comment 46

[u459114]

Comment on attachment 8909583 [details]
Bug 1401069 - Part 4. Change argument type from 'const RECT *' to reference type in WindowsEMF::Playback.

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

Comment 47

[u459114]

Comment on attachment 8909584 [details]
Bug 1401069 - Part 5. Remove two local variables since we do not need them.

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

Comment 48

[u459114]

Comment on attachment 8909585 [details]
Bug 1401069 - Part 6. Fine tune ComputeScaleFactor.

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

Comment 49

[u459114]

Comment on attachment 8909586 [details]
Bug 1401069 - Part 7. Declare GetPageCount as a const member function.

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

Comment 50

[u459114]

Comment on attachment 8909587 [details]
Bug 1401069 - Part 8. Give a detailed comment to explain how to react when hitting this assertion in OpenDocument().

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

Comment 51

[u459114]

Comment on attachment 8909583 [details]
Bug 1401069 - Part 4. Change argument type from 'const RECT *' to reference type in WindowsEMF::Playback.

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

Comment 52

[u459114]

Comment on attachment 8909584 [details]
Bug 1401069 - Part 5. Remove two local variables since we do not need them.

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

Comment 53

[u459114]

Comment on attachment 8909585 [details]
Bug 1401069 - Part 6. Fine tune ComputeScaleFactor.

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

Comment 54

[u459114]

Comment on attachment 8909586 [details]
Bug 1401069 - Part 7. Declare GetPageCount as a const member function.

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

Comment 55

[u459114]

Comment on attachment 8909587 [details]
Bug 1401069 - Part 8. Give a detailed comment to explain how to react when hitting this assertion in OpenDocument().

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

Comment 56

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/74f2aee990a9
Part 1. Check the value of mPDFDoc and early return if it's invalid in both RenderPageToDC and DrawPageToFile. r=fatseng
https://hg.mozilla.org/integration/autoland/rev/44ec9e64a5e3
Part 2. Implement ReleaseAllResource to make WindowsEMF::InitForDrawing reusable. r=fatseng
https://hg.mozilla.org/integration/autoland/rev/c5dfc2e657a4
Part 3. Declare WindowsEMF::GetDC as const member function. r=fatseng
https://hg.mozilla.org/integration/autoland/rev/5ae21f43fbd3
Part 4. Change argument type from 'const RECT *' to reference type in WindowsEMF::Playback. r=fatseng
https://hg.mozilla.org/integration/autoland/rev/66cbdf9b1df0
Part 5. Remove two local variables since we do not need them. r=fatseng
https://hg.mozilla.org/integration/autoland/rev/f6ec11335d52
Part 6. Fine tune ComputeScaleFactor. r=fatseng
https://hg.mozilla.org/integration/autoland/rev/5e23e6f7d6d5
Part 7. Declare GetPageCount as a const member function. r=fatseng
https://hg.mozilla.org/integration/autoland/rev/3e576101b9d3
Part 8. Give a detailed comment to explain how to react when hitting this assertion in OpenDocument(). r=fatseng

Comment 57

[Farmer Tseng[:fatseng](inactive)]

I assumed you ran the tests.

Comment 58

[u459114]

(In reply to Farmer Tseng[:fatseng] from comment #57)
> I assumed you ran the tests.

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

Comment 59

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/74f2aee990a9
https://hg.mozilla.org/mozilla-central/rev/44ec9e64a5e3
https://hg.mozilla.org/mozilla-central/rev/c5dfc2e657a4
https://hg.mozilla.org/mozilla-central/rev/5ae21f43fbd3
https://hg.mozilla.org/mozilla-central/rev/66cbdf9b1df0
https://hg.mozilla.org/mozilla-central/rev/f6ec11335d52
https://hg.mozilla.org/mozilla-central/rev/5e23e6f7d6d5
https://hg.mozilla.org/mozilla-central/rev/3e576101b9d3