1368948 - Integrate PDFium for converting EMF on Windows

Status: RESOLVED FIXED

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

Description

[Bruce Sun [:brsun]]

PDFium can convert PDF into EMF on Windows. This feature is required if we are going to use PDF as the intermediate format for printing.

Comment 1

[Bruce Sun [:brsun]]

Attachment: bug1368948_pdfium_files_with_patches.wip.patch.zip

WIP: add PDFium files

Comment 2

[Bruce Sun [:brsun]]

Attachment: bug1368948_mozbulid_for_pdfium.wip.patch

WIP: moz.build for PDFium

Comment 3

[Bruce Sun [:brsun]]

Hi Jonathan,

Currently I put PDFium under /layout/pdfium in my patches. Could you suggest a better place for it?

Comment 4

[Peter Chang[:pchang]]

(In reply to Bruce Sun [:brsun] from comment #1)
> Created attachment 8875201 [details]
> bug1368948_pdfium_files_with_patches.wip.patch.zip
> 
> WIP: add PDFium files

Bruce, please submit this patch by using mozreview.

Comment 5

[Bruce Sun [:brsun]]

Thanks Peter, this is just a WIP. I will submit patches with mozreview when requesting for reviewing.

Comment 6

[Peter Chang[:pchang]]

(In reply to Bruce Sun [:brsun] from comment #3)
> Hi Jonathan,
> 
> Currently I put PDFium under /layout/pdfium in my patches. Could you suggest
> a better place for it?

As I discussed with Bruce, widget/windows might be one option to store PDFium because right now we only use it on windows.

Comment 7

[Bruce Sun [:brsun]]

> As I discussed with Bruce, widget/windows might be one option to store
> PDFium because right now we only use it on windows.

Thanks Peter. As we discussed, widget/windows/moz.build might be a good trigger point to introduce DIR of pdfium. Just want to clarify, the original question on comment 3 is for pdfium library itself.

Comment 8

[Jonathan Watt [:jwatt]]

The 'widget' directory would seem to make more sense than 'layout'. Since this is a third party library it would also be good to create a 'third_party' subdirectory to store it in, so I'd suggest widget/third_party/pdfium

Regarding whether it should go under the 'windows' subdirectory, I don't think so; we may only use PDFium on Windows initially, but since it gives us advantages over PDF.js (such as forms filling) I expect we will end up using it on other platforms too.

Comment 9

[Jet Villegas (inactive)]

Can you point out where build configurations are set? i.e., where are the defines located for "Selecting build configuration" in https://pdfium.googlesource.com/pdfium/ ?

Comment 10

[Bruce Sun [:brsun]]

Upstream PDFium uses GN as its build system. After using the GN command to set build arguments (ex. "gn args out/mybuild"), "args.gn" will be created under the corresponding sub-directory to store the set arguments (ex. "out/mybuild/args.gn").

Regarding to our build system, I will add necessary configuration into moz.build instead.

Comment 11

[Bruce Sun [:brsun]]

Attachment: update.py

Hi Jet, just for your reference. This is the script to update PDFium from upstream. I will include (almost) the same script while committing PDFium into m-c.

Comment 12

[Astley Chen (inactive)]

(In reply to Jet Villegas (:jet) from comment #9)
> Can you point out where build configurations are set? i.e., where are the
> defines located for "Selecting build configuration" in
> https://pdfium.googlesource.com/pdfium/ ?

We have 2 patches here.
1. PDFium code(including 3rd parties code it depends on).
We want to make sure the code we pull is from https://pdfium.googlesource.com/pdfium/ and also contain needed libraries that are not existing in m-c now. Given that we only need EMF conversion from PDFium, we suppose no need to have xfa, v8 support and code in the pull.
2. moz.build (https://bugzilla.mozilla.org/attachment.cgi?id=8875202)
As comment 10 said, PDFium uses GN and Ninja as build system so that we create a moz.build to get it built in m-c.This is where the build configurations at. The PDFium will be built behind build flags MOZ_ENABLE_SKIA_PDF and OS_TARGET==WINNT as well as a part of libxul.

Comment 13

[Bruce Sun [:brsun]]

> we suppose no need to have xfa, v8 support and code in the pull.

Just want to clarify, XFA related codes are parts of PDFium. I don't tend to modify PDFium too much, so XFA and v8 wrapper codes will still be contained within PDFium on m-c. And for sure the v8 library will not be added through this integration.

Comment 14

[Jet Villegas (inactive)]

Thanks! I'm looking specifically for the lines in the patches where these 2 vars are set:

pdf_enable_xfa = false  # Set false to remove XFA support (implies JS support).
pdf_enable_v8 = false  # Set false to remove Javascript support.

Comment 15

[Bruce Sun [:brsun]]

The argument "pdf_enable_xfa" in GN means "PDF_ENABLE_XFA" definition[1] in source level.
The argument "pdf_enable_v8" in GN means "PDF_ENABLE_V8" definition[2] in source level.

As long as we don't explicitly define these two values, we do disable both XFA and script features already.

[1] https://cs.chromium.org/chromium/src/third_party/pdfium/BUILD.gn?l=36
[2] https://cs.chromium.org/chromium/src/third_party/pdfium/BUILD.gn?l=40

Comment 16

[Bruce Sun [:brsun]]

Attachment: Bug 1368948: Add a script to update PDFium from upstream.

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

Comment 17

[Bruce Sun [:brsun]]

Attachment: Bug 1368948: Add patch files for PDFium.

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

Comment 18

[Bruce Sun [:brsun]]

Attachment: Bug 1368948: Add PDfium files (chromium/3029) with patches.

Corresponding chromium version: 58.0.3029.110
Update command: update.py --commit chromium/3029

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

Comment 19

[Bruce Sun [:brsun]]

Attachment: Bug 1368948: Add license notices for PDFium and its dependent libraries.

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

Comment 20

[Bruce Sun [:brsun]]

Attachment: Bug 1368948: Suppress clang-plugin errors.

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

Comment 21

[Bruce Sun [:brsun]]

Attachment: Bug 1368948: Build freetype when enabling SkiaPDF on Windows.

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

Comment 22

[Bruce Sun [:brsun]]

Attachment: Bug 1368948: Add moz.build for PDFium.

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

Comment 23

[Bruce Sun [:brsun]]

Comment on attachment 8877046 [details]
Bug 1368948: Add a script to update PDFium from upstream.

This script can be used to get PDFium sources from upstream to local directory by specifying a commit hash or a branch name. This script will do the following steps to update sources:
 - use the specified commit hash or the branch name to retrieve more specific version information from upstream; and
 - remove local pdfium folder if there is one; and
 - download pdfium sources with the specific commit hash as pdfium.tar.gz; and
 - extract pdfium.tar.gz to pdfium folder; and
 - delete the downloaded pdfium.tar.gz; and
 - apply necessary patches to pdfium; and
 - update README_MOZILLA with corresponding commit hash; and
 - remove unnecessary files and folders.

Comment 24

[Bruce Sun [:brsun]]

Comment on attachment 8877047 [details]
Bug 1368948: Add patch files for PDFium.

Add patch files for update.py to automatically modify pdfium sources:
 - a patch to fix PDFium build errors due to lacking GDI+ prerequisite headers
 - a patch to let PDFium use freetype library within Gecko
 - a patch to let PDFium use libjpeg library within Gecko
 - a patch to let PDFium use zlib library within Gecko

Comment 25

[Bruce Sun [:brsun]]

Comment on attachment 8877048 [details]
Bug 1368948: Add PDfium files (chromium/3029) with patches.

Add PDFium files with the latest chromium/3029 branch, which is currently used by stable chromium (58.0.3029.110)

Comment 26

[Bruce Sun [:brsun]]

Comment on attachment 8877049 [details]
Bug 1368948: Add license notices for PDFium and its dependent libraries.

Update license.html to include PDfium related license notices:
 - "Anti-Grain Geometry Public License" for "pdfium/third_party/agg23/"
 - "Chromium License" for "pdfium/third_party/base/"
 - "lcms License" for "pdfium/third_party/lcms2-2.6/"
 - "OpenJPEG License" for "pdfium/third_party/libopenjpeg20/"
 - "PDFium License" for "pdfium/"
 - acknowledgment of "C++ Big Integer Library" for "pdfium/third_party/bigint/"

Comment 27

[Bruce Sun [:brsun]]

Comment on attachment 8877050 [details]
Bug 1368948: Suppress clang-plugin errors.

Add explicit rules to suppress static analysis warnings for PDFium sources.

Comment 28

[Bruce Sun [:brsun]]

Comment on attachment 8877051 [details]
Bug 1368948: Build freetype when enabling SkiaPDF on Windows.

Add the dependency between "MOZ_ENABLE_SKIA_PDF" and "MOZ_TREE_FREETYPE" on Windows:
 - let |tree_freetype| returns true if |skia_pdf| returns true on Windows, and
 - avoid defining "MOZ_ENABLE_CAIRO_FT" on Windows ("cairo-ft-font.c" includes <dlfcn.h>, which only exists on posix platforms)

Comment 29

[Bruce Sun [:brsun]]

Comment on attachment 8877052 [details]
Bug 1368948: Add moz.build for PDFium.

Add moz.build for PDfium library.

Comment 30

[Bruce Sun [:brsun]]

Attachment: bug1368948_pdfium_files_with_patches.zip

ReviewBoard cannot display my patch (attachment 8877048 [details]) properly. This is the zip version of that patch.

Comment 31

[Bruce Sun [:brsun]]

Build result on the try server with PDFium compiled[1] or without PDFium compiled[2] are all green.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=deff4b2730861b0e5cd64145667e79b58cbf39d4
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=2eb22b9c9e3fe3c82915c481fcf11e9cb82a5b04

Comment 32

[Bruce Sun [:brsun]]

Comment on attachment 8877046 [details]
Bug 1368948: Add a script to update PDFium from upstream.

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

Comment 33

[Bruce Sun [:brsun]]

Comment on attachment 8877047 [details]
Bug 1368948: Add patch files for PDFium.

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

Comment 34

[Bruce Sun [:brsun]]

Comment on attachment 8877048 [details]
Bug 1368948: Add PDfium files (chromium/3029) with patches.

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

Comment 35

[Bruce Sun [:brsun]]

Comment on attachment 8877049 [details]
Bug 1368948: Add license notices for PDFium and its dependent libraries.

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

Comment 36

[Bruce Sun [:brsun]]

Comment on attachment 8877050 [details]
Bug 1368948: Suppress clang-plugin errors.

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

Comment 37

[Bruce Sun [:brsun]]

Comment on attachment 8877051 [details]
Bug 1368948: Build freetype when enabling SkiaPDF on Windows.

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

Comment 38

[Bruce Sun [:brsun]]

Comment on attachment 8877052 [details]
Bug 1368948: Add moz.build for PDFium.

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

Comment 39

[Peter Chang[:pchang]]

Attachment: bug 1368948- test pdfium

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

Comment 40

[Gervase Markham [:gerv]]

Comment on attachment 8877049 [details]
Bug 1368948: Add license notices for PDFium and its dependent libraries.

https://reviewboard.mozilla.org/r/148394/#review153372

r=gerv. Are we shipping two JPEG libraries now? <sigh>

Gerv

Comment 41

[Bruce Sun [:brsun]]

> r=gerv. Are we shipping two JPEG libraries now? <sigh>

Since JPEG and JPEG 2000 are two different standards, and PDFium uses both of them, probably this is not negligible.

Comment 42

[Astley Chen (inactive)]

Mike, would you be able to review the patches or suggest other reviewers if you are busy? Thanks.

Comment 43

[Bruce Sun [:brsun]]

Hi Astley,

PDFium has many third-party libraries on its repo[1], some of them will be included into m-c during this integration:
 - agg23
 - base
 - bigint
 - lcms2-2.6
 - libopenjpeg20

We remove rest libraries of the third-party folder intentionally:
 - freetype: reuse the existing one in Gecko
 - libjpeg: reuse the existing one in Gecko
 - libpng16: remove due to XFA feature disabled
 - libtiff: remove due to XFA feature disabled
 - pymock: remove due to no pymock tests currently
 - zlib_v128: reuse the existing one in Gecko

On the other hand, by further configuring GN arguments, some additional libraries could be downloaded from other repos[2] correspondingly. By contrast, we don't include any one of them currently.

[1] https://pdfium.googlesource.com/pdfium/+/chromium/3029/third_party/
[2] https://pdfium.googlesource.com/pdfium/+/chromium/3029/DEPS

Comment 44

[Mike Hommey [:glandium]]

Comment on attachment 8877046 [details]
Bug 1368948: Add a script to update PDFium from upstream.

https://reviewboard.mozilla.org/r/148388/#review155470

::: widget/third_party/pdfium/update.py:18
(Diff revision 2)
> +def debug_print(message):
> +    if DEBUG:
> +        print(message)
> +
> +def parse_upstream(commit_selector):
> +    upstream_url = 'https://pdfium.googlesource.com/pdfium/+/' + commit_selector

Upstream is a git repository, you should pull through git instead of parsing a web representation of it that might change in the future.

That's what we do for many other third party sources.

Comment 45

[Mike Hommey [:glandium]]

Comment on attachment 8877046 [details]
Bug 1368948: Add a script to update PDFium from upstream.

https://reviewboard.mozilla.org/r/148388/#review155474

::: widget/third_party/pdfium/README_MOZILLA:1
(Diff revision 2)
> +PDFium is a PDF library to view, search, print, and form fill PDF files.

Also, some directory under widget doesn't seem appropriate. Widget/ code is mostly about interfacing with the native OS widget toolkits (Gtk, Cocoa, etc.). PDFium is not that.

Comment 46

[Mike Hommey [:glandium]]

Comment on attachment 8877047 [details]
Bug 1368948: Add patch files for PDFium.

https://reviewboard.mozilla.org/r/148390/#review155482

::: widget/third_party/pdfium/patches/bug1368948_use_gecko_libjpeg.patch:22
(Diff revision 2)
> + #include <jpeglib.h>
> + #elif defined(USE_LIBJPEG_TURBO)
> + #include "third_party/libjpeg_turbo/jpeglib.h"
> + #else
> +-#include "third_party/libjpeg/jpeglib.h"
> ++#include "jpeglib.h"

Technically, the in-tree copy of libjpeg is libjpeg-turbo, you probably want to use the libjpeg-turbo code paths if there are other parts varying with LIBJPEG_TURBO.

::: widget/third_party/pdfium/update.py:92
(Diff revision 2)
> +    os.system("patch -p4 < patches/bug1368948_gdiplus_prerequisite.patch")
> +    # Patch to use freetype library within Gecko
> +    os.system("patch -p4 < patches/bug1368948_use_gecko_freetype.patch")
> +    # Patch to use libjpeg library within Gecko
> +    os.system("patch -p4 < patches/bug1368948_use_gecko_libjpeg.patch")
> +    # Patch to use zlib library within Gecko
> +    os.system("patch -p4 < patches/bug1368948_use_gecko_zlib.patch")

os.system kind of makes me cringe, but meh.

Comment 47

[Mike Hommey [:glandium]]

Comment on attachment 8877048 [details]
Bug 1368948: Add PDfium files (chromium/3029) with patches.

https://reviewboard.mozilla.org/r/148392/#review155484

Someone else than me (someone that will be responsible for this code, I guess) will need to rubberstamp this. My only comment here, is there are two levels of pdfium directories; that seems redundant.

Comment 48

[Mike Hommey [:glandium]]

Comment on attachment 8877051 [details]
Bug 1368948: Build freetype when enabling SkiaPDF on Windows.

https://reviewboard.mozilla.org/r/148398/#review155486

::: old-configure.in:3507
(Diff revision 2)
>  
>  if test -n "$MOZ_USE_NATIVE_POPUP_WINDOWS"; then
>    AC_DEFINE(MOZ_USE_NATIVE_POPUP_WINDOWS)
>  fi
>  
> -if test -n "$MOZ_TREE_FREETYPE"; then
> +if test -n "$MOZ_TREE_FREETYPE" -a "$OS_TARGET" != WINNT; then

why do you exclude this?

::: toolkit/moz.configure:1028
(Diff revision 2)
>  
>  set_config('SKIA_INCLUDES', skia_includes)
>  
> +# Build Freetype in the tree
> +# ==============================================================
> +@depends(target, skia_pdf)

why is this tied to _skia_ pdf? And why windows only?

Comment 49

[Mike Hommey [:glandium]]

Comment on attachment 8877052 [details]
Bug 1368948: Add moz.build for PDFium.

https://reviewboard.mozilla.org/r/148400/#review155490

::: widget/third_party/pdfium/moz.build:8
(Diff revision 2)
> +# 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/.
> +
> +with Files('**'):
> +    BUG_COMPONENT = ('Core', 'Printing: Output')

If this is for printing... why is jpeg2000 needed?

::: widget/third_party/pdfium/moz.build:537
(Diff revision 2)
> +# third_party:jpeg
> +LOCAL_INCLUDES += [
> +    '/media/libjpeg',
> +]
> +USE_LIBS += [
> +    'media_libjpeg',

You're introducing the same problem as bug 1373988.

::: widget/third_party/pdfium/moz.build:543
(Diff revision 2)
> +]
> +
> +# We allow warnings for third-party code that can be updated from upstream.
> +ALLOW_COMPILER_WARNINGS = True
> +
> +DISABLE_STL_WRAPPING = True

huh, this is rather not desirable. What made you do this?

Comment 50

[Bruce Sun [:brsun]]

Comment on attachment 8877051 [details]
Bug 1368948: Build freetype when enabling SkiaPDF on Windows.

https://reviewboard.mozilla.org/r/148398/#review155486

> why do you exclude this?

The cairo library cannot be built with "MOZ_ENABLE_CAIRO_FT" defined on Windows ("cairo-ft-font.c" includes <dlfcn.h>, which only exists on posix platforms). I guess we can safely exclude this on Windows because this definition (probably) has never been defined on Windows before.

> why is this tied to _skia_ pdf? And why windows only?

"--enable-skia-pdf" is used for printing feature, which means we use PDF as the intermediate format for the printer. On Linux and Mac, the printer interface can accept PDF as the input format directly. However, the printer interface on Windows doesn't recognize PDF format. So we have to explicitly convert PDF into EMF format for the printer on Windows.

In short, currently PDFium will be used for PDF to EMF conversion on Windows only.

Comment 51

[Bruce Sun [:brsun]]

Comment on attachment 8877046 [details]
Bug 1368948: Add a script to update PDFium from upstream.

https://reviewboard.mozilla.org/r/148388/#review155470

> Upstream is a git repository, you should pull through git instead of parsing a web representation of it that might change in the future.
> 
> That's what we do for many other third party sources.

The update.py for PDFium follows the structure of other third party libraries[1][2].

[1] http://searchfox.org/mozilla-central/source/media/libvpx/update.py
[2] http://searchfox.org/mozilla-central/source/media/libyuv/update.py

Comment 52

[Bruce Sun [:brsun]]

> Also, some directory under widget doesn't seem appropriate. Widget/ code is
> mostly about interfacing with the native OS widget toolkits (Gtk, Cocoa,
> etc.). PDFium is not that.

Hi Jonathan,

Do you have any suggestion other than /widget folder for pdfium?

Comment 53

[Mike Hommey [:glandium]]

(In reply to Bruce Sun [:brsun] from comment #50)
> Comment on attachment 8877051 [details]
> Bug 1368948: Build freetype when enabling SkiaPDF on Windows.
> 
> https://reviewboard.mozilla.org/r/148398/#review155486
> 
> > why do you exclude this?
> 
> The cairo library cannot be built with "MOZ_ENABLE_CAIRO_FT" defined on
> Windows ("cairo-ft-font.c" includes <dlfcn.h>, which only exists on posix
> platforms). I guess we can safely exclude this on Windows because this
> definition (probably) has never been defined on Windows before.

Please add a comment.

(In reply to Bruce Sun [:brsun] from comment #51)
> Comment on attachment 8877046 [details]
> Bug 1368948: Add a script to update PDFium from upstream.
> 
> https://reviewboard.mozilla.org/r/148388/#review155470
> 
> > Upstream is a git repository, you should pull through git instead of parsing a web representation of it that might change in the future.
> > 
> > That's what we do for many other third party sources.
> 
> The update.py for PDFium follows the structure of other third party
> libraries[1][2].
> 
> [1] http://searchfox.org/mozilla-central/source/media/libvpx/update.py
> [2] http://searchfox.org/mozilla-central/source/media/libyuv/update.py

Oh my... these scripts should be changed too. Or we should finally use a single script for all these things. I wonder what the status of the service to do that is. If only I could remember who was talking about it...

(All in all, they could also just be (simpler) shell scripts... see e.g modules/brotli/update.sh)

Comment 54

[Bruce Sun [:brsun]]

Comment on attachment 8877052 [details]
Bug 1368948: Add moz.build for PDFium.

https://reviewboard.mozilla.org/r/148400/#review155490

> If this is for printing... why is jpeg2000 needed?

PDFium is a library for parsing PDF files. Parsing JPEG 2000 image within PDF is just a feature of PDFium. Since OpenJPEG is tightly integrated with PDFium, it might not be an easy task to decouple them without breaking upgradability.

> You're introducing the same problem as bug 1373988.

The solution of that bug might affect our moz.build, but our integration doesn't incur that problem because PDFium is built on Windows only.

> huh, this is rather not desirable. What made you do this?

Because including <utility> without <xutility> causes build errors on Windows (ref. bug 1350261).

Comment 55

[Mike Hommey [:glandium]]

(In reply to Bruce Sun [:brsun] from comment #54)
> Comment on attachment 8877052 [details]
> Bug 1368948: Add moz.build for PDFium.
> 
> https://reviewboard.mozilla.org/r/148400/#review155490
> 
> > If this is for printing... why is jpeg2000 needed?
> 
> PDFium is a library for parsing PDF files. Parsing JPEG 2000 image within
> PDF is just a feature of PDFium. Since OpenJPEG is tightly integrated with
> PDFium, it might not be an easy task to decouple them without breaking
> upgradability.

But you're only parsing pdfs produced by skia, and they won't include jpeg2000 images.

> > huh, this is rather not desirable. What made you do this?
> 
> Because including <utility> without <xutility> causes build errors on
> Windows (ref. bug 1350261).

That bug says the errors are gone, and disabling STL wrapping is not something we want to do anyways.

Comment 56

[Bruce Sun [:brsun]]

Comment on attachment 8877052 [details]
Bug 1368948: Add moz.build for PDFium.

https://reviewboard.mozilla.org/r/148400/#review155490

> Because including <utility> without <xutility> causes build errors on Windows (ref. bug 1350261).

DISABLE_STL_WRAPPING can be avoided after applying the patch on bug 1349064[1].

[1] https://hg.mozilla.org/integration/mozilla-inbound/rev/8c35a43033bc

Comment 57

[(no longer active)]

Comment on attachment 8877050 [details]
Bug 1368948: Suppress clang-plugin errors.

https://reviewboard.mozilla.org/r/148396/#review155936

Comment 58

[Astley Chen (inactive)]

Comment on attachment 8877046 [details]
Bug 1368948: Add a script to update PDFium from upstream.

https://reviewboard.mozilla.org/r/148388/#review155998

::: widget/third_party/pdfium/README_MOZILLA:1
(Diff revision 2)
> +PDFium is a PDF library to view, search, print, and form fill PDF files.

The thought here is basically about the PDF-to-EMF conversion function provided by PDFium is *currenly* only used by Windows widget code. That's also why the third_party folder was implemented for(as a dependency for use of widget code).
Ideally, we should have a centralized placeholder to store all 3rd party libraries in m-c, however, it's not a case now...

Comment 59

[Astley Chen (inactive)]

Comment on attachment 8877048 [details]
Bug 1368948: Add PDfium files (chromium/3029) with patches.

https://reviewboard.mozilla.org/r/148392/#review155484

As the integration of PDFium library is part of layout printing work, it would be sustained by layout peers.
It would be still great to have your feedback. For review request, it's fine to redirect to layout peers.
I also agree the comment from Mike about the redundant levels of dirs.

Comment 60

[Bruce Sun [:brsun]]

> I also agree the comment from Mike about the redundant levels of dirs.

The redundant folder structure might be weird at the first glance, but it makes sense when you think about the upgrade process. By adding one additional folder to contain upstream codes, we can easily remove the old codes when upgrading to a new version.

If we don't use this kind of redundant level, we have to use a file list to differentiate files/folders between third-party ones (i.e. all folders of the library itself, readme, license notices, etc.) and non-third-party ones (i.e. update scripts, mozilla patches, readme, moz.build, etc.). Otherwise, there might be some old but unused files after executing the upgrade process.

I would suggest we keep the redundant level for simplicity of the upgrade process if there are no strong opinions to disagree this approach.

p.s. I roughly take a glance on several third-party folders. For those libraries which don't have extra level of redundant folders, update scripts don't seem to remove old files/folders properly. They just can add files/folders during the upgrade process. Some libraries even don't have the capability to upgrade automatically.


Hi Mike, Astley,

Just want to double check with you. Do you really dislike this redundant level of folders?

Comment 61

[Astley Chen (inactive)]

(In reply to Bruce Sun [:brsun] from comment #60)
> Hi Mike, Astley,
> 
> Just want to double check with you. Do you really dislike this redundant
> level of folders?

I understood the advantage and necessary to put the upstream code into a separate folder to facilitate the code maintenance. When looking into gfx/skia, it seems taking a similar way to have our own files and upstream code been separated. That's fine to me now.

Comment 62

[Astley Chen (inactive)]

By the way, as you are using reviewboard for review, it would be good to have all review comments for a particular open issue in a single thread, otehrwise, it's gonna be scattered on Bugzilla.

Comment 63

[Jonathan Watt [:jwatt]]

(In reply to Bruce Sun [:brsun] from comment #52)
> Do you have any suggestion other than /widget folder for pdfium?

Well if we're eliminating layout/ and widget/ as suitable locations, I guess we have modules/ and third_party/ (or at a push  gfx/ or image/, but they don't seem appropriate). I'd say put it in modules/. Two of the existing libraries that live there (libjar and libpref) are also built into libxul.

Comment 64

[Astley Chen (inactive)]

(In reply to Jonathan Watt [:jwatt] from comment #63)
> Well if we're eliminating layout/ and widget/ as suitable locations, I guess
> we have modules/ and third_party/ (or at a push  gfx/ or image/, but they
> don't seem appropriate). I'd say put it in modules/. Two of the existing
> libraries that live there (libjar and libpref) are also built into libxul.

I Agree to put PDFium library into modules/ folder. Thanks for your suggestion.

Comment 65

[Bruce Sun [:brsun]]

Attachment: update.sh

Hi Mike,

Would this update script make you cringe less?

Comment 66

[Bruce Sun [:brsun]]

I've tried to remove OpenJPEG library from PDFium, the build results seem good.


Hi Farmer,

Would your help to give it a try to see if everything still works well on SkiaPDF?

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=7053139ba7a0d38108d6302652b16c2882ee04b8

Comment 67

[Farmer Tseng[:fatseng](inactive)]

(In reply to Bruce Sun [:brsun] from comment #66)
> I've tried to remove OpenJPEG library from PDFium, the build results seem
> good.
> 
> 
> Hi Farmer,
> 
> Would your help to give it a try to see if everything still works well on
> SkiaPDF?
> 
> [1]
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=7053139ba7a0d38108d6302652b16c2882ee04b8

I tested Yahoo and CNN. The printing via SkiaPDF works fine.

Comment 68

[Bruce Sun [:brsun]]

Since we encounter some problems on mozreview (ref. bug 1372842), I will using the old-fashion way to submit patches for this bug.

Comment 69

[Bruce Sun [:brsun]]

Attachment: Bug 1368948: [1/7] Add a script to update PDFium from upstream.

I add update.sh in this patch to avoid using os.system(...) in the python script. Both update.py and update.sh can retrieve upstream sources automatically. I will remove one of them if the other one is more preferred from reviewer's point of view.

The idea to use two update scripts here is just for aiding the back-and-forth latency of the review process. Suppose there would be only one script on m-c.

Comment 70

[Bruce Sun [:brsun]]

Attachment: Bug 1368948: [2/7] Add patch files for PDFium.

Add patch files for update.sh to automatically modify pdfium sources:
 - a patch to fix PDFium build errors due to lacking GDI+ prerequisite headers
 - a patch to let PDFium use freetype library within Gecko
 - a patch to let PDFium use libjpeg library within Gecko
 - a patch to let PDFium use zlib library within Gecko
 - a patch to remove JPEG 2000 support from PDFium

Comment 71

[Bruce Sun [:brsun]]

Attachment: Bug 1368948: [3/7] Add PDfium files (chromium/3029) with patches.

Corresponding chromium version: 58.0.3029.110
Update command: ./update.sh --commit chromium/3029

Note: Some libraries under third_party folder are removed intentionally:
 - freetype: reuse the existing one in Gecko
 - libjpeg: reuse the existing one in Gecko
 - libopenjpeg20: remove because SkiaPDF doesn't use OpenJPEG
 - libpng16: remove due to XFA feature disabled
 - libtiff: remove due to XFA feature disabled
 - pymock: remove due to no pymock tests currently
 - zlib_v128: reuse the existing one in Gecko

Comment 72

[Bruce Sun [:brsun]]

Attachment: Bug 1368948: [4/7] Add license notices for PDFium and its dependent libraries.

Update license.html to include PDfium related license notices:
 - "Anti-Grain Geometry Public License" for "pdfium/third_party/agg23/"
 - "Chromium License" for "pdfium/third_party/base/"
 - "lcms License" for "pdfium/third_party/lcms2-2.6/"
 - "PDFium License" for "pdfium/"
 - acknowledgment of "C++ Big Integer Library" for "pdfium/third_party/bigint/"

Hi Gervase,

OpenJPEG has been removed, and the folder for PDFium has been changed to /modules/pdfium. Would you like to check license.html again for me?

Comment 73

[Bruce Sun [:brsun]]

Attachment: Bug 1368948: [5/7] Suppress clang-plugin errors.

Carry over r+ flag from comment 57.

Comment 74

[Bruce Sun [:brsun]]

Attachment: Bug 1368948: [6/7] Build freetype when enabling SkiaPDF on Windows.

Add the dependency between "MOZ_ENABLE_SKIA_PDF" and "MOZ_TREE_FREETYPE" on Windows:
 - let |tree_freetype| returns true if |skia_pdf| returns true on Windows, and
 - avoid defining "MOZ_ENABLE_CAIRO_FT" on Windows ("cairo-ft-font.c" includes <dlfcn.h>, which only exists on posix platforms)
 - add more comments in old-configure.in

Comment 75

[Bruce Sun [:brsun]]

Attachment: Bug 1368948: [7/7] Add moz.build for PDFium.

moz.build for /modules/pdfium:
 - reuse freetype for third_party:fx_freetype
 - reuse zlib for third_party:fx_zlib
 - reuse media_libjpeg for third_party:jpeg (with USE_LIBJPEG_TURBO defined)
 - avoid compiling fx_codec_jpx_opj.cpp to disable OpenJPEG support

Comment 76

[Mike Hommey [:glandium]]

Comment on attachment 8881345 [details] [diff] [review]
Bug 1368948: [1/7] Add a script to update PDFium from upstream.

Review of attachment 8881345 [details] [diff] [review]:
-----------------------------------------------------------------

Just keep the shell version.

::: modules/pdfium/update.sh
@@ +10,5 @@
> +COMMIT_SELECTOR=""
> +COMMIT=""
> +COMMIT_DATE=""
> +
> +function debug_print() {

IIRC `function` is a bashism. In POSIX shell, you can just do `name() {` (without `function` at all)

@@ +114,5 @@
> +    print_help
> +    exit 0
> +fi
> +
> +checkout_commit "$COMMIT_SELECTOR"

You should change the current directory to the directory containing this script before doing anything, so that it can be run from anywhere without surprises. something like `cd $(dirname $0)`

Comment 77

[Mike Hommey [:glandium]]

Comment on attachment 8881345 [details] [diff] [review]
Bug 1368948: [1/7] Add a script to update PDFium from upstream.

Review of attachment 8881345 [details] [diff] [review]:
-----------------------------------------------------------------

All things considered, I think you shouldn't hide the "debug" messages by default. Just replace debug_print with echo.

Comment 78

[Mike Hommey [:glandium]]

Comment on attachment 8881346 [details] [diff] [review]
Bug 1368948: [2/7] Add patch files for PDFium.

Review of attachment 8881346 [details] [diff] [review]:
-----------------------------------------------------------------

The patches seem reasonable, but you may want a review from someone that actually looked at the pdfium code, whoever that is :)

Comment 79

[Mike Hommey [:glandium]]

Comment on attachment 8881351 [details] [diff] [review]
Bug 1368948: [7/7] Add moz.build for PDFium.

Review of attachment 8881351 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/pdfium/moz.build
@@ +517,5 @@
> +
> +# We allow warnings for third-party code that can be updated from upstream.
> +ALLOW_COMPILER_WARNINGS = True
> +
> +Library('pdfium')

Technically, you don't need this line.

Comment 80

[Bruce Sun [:brsun]]

Attachment: Bug 1368948: [1/7] Add a script to update PDFium from upstream. (v2)

address comment 76 and comment 77

Comment 81

[Bruce Sun [:brsun]]

Attachment: Bug 1368948: [2/7] Add patch files for PDFium. (v2)

address comment 77

Hi Jonathan,

Would you like to share your comments as well?

Comment 82

[Bruce Sun [:brsun]]

Attachment: Bug 1368948: [7/7] Add moz.build for PDFium. (v2)

address comment 79

Comment 83

[Jonathan Watt [:jwatt]]

Comment on attachment 8881840 [details] [diff] [review]
Bug 1368948: [1/7] Add a script to update PDFium from upstream. (v2)

Review of attachment 8881840 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/pdfium/README_MOZILLA
@@ +2,5 @@
> +
> +The source in this directory was copied from upstream by running the
> +update.py script from layout/pdfium. Any changes made relative to upstream
> +should be reflected in that script, e.g. by applying patch files after the
> +copy step.

I think this last sentence would be easier to read as: "Any changes that we make to our copy of the upstream code should be made by that script (e.g. by applying patch files after the copy step)."

::: modules/pdfium/update.sh
@@ +3,5 @@
> +# 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/.
> +
> +# Script to update the mozilla in-tree copy of the PDFium library.
> +# Run this within the /modules/pdfium directory of the source tree.

It seems like this script should use:

  set -e

@@ +10,5 @@
> +COMMIT=""
> +COMMIT_DATE=""
> +
> +print_help() {
> +    echo "usage: ./update.sh [--commit COMMIT]"

The square brackets here imply that the --commit argument is optional, but it seems that it is not.

@@ +39,5 @@
> +    git -C pdfium checkout "$COMMIT"
> +}
> +
> +cleanup_files() {
> +    # Remove irrelevant source control history

Please update the comments as we discussed in person.

@@ +88,5 @@
> +    PREFIX="The git commit ID last used to import was"
> +    perl -p -i -e "s/${PREFIX} [0-9A-Fa-f]+ \(.+\)/${PREFIX} ${COMMIT} (${COMMIT_DATE})/" README_MOZILLA
> +}
> +
> +while [ "$#" -gt 0 ]

The way you handle this loop would allow things like:

  update.sh --verbose --commit 123

without notifying the user that --verbose is not recognized, which I guess is unlikely to be an issue in practice. But I don't think you need a loop here. You could just check "if no args passed, use latest trunk, else if two args arp passed and the first is '--commit' use the second as the commit ID, else error with help message".

Comment 84

[Bruce Sun [:brsun]]

Attachment: Bug 1368948: [1/7] Add a script to update PDFium from upstream. (v3)

address comment 83.

Hi Jonathan, I've updated the script accordingly. Would you like to have a look at it again?

Comment 85

[Jonathan Watt [:jwatt]]

Comment on attachment 8881841 [details] [diff] [review]
Bug 1368948: [2/7] Add patch files for PDFium. (v2)

Review of attachment 8881841 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/pdfium/patches/bug1368948_gdiplus_prerequisite.patch
@@ +3,5 @@
> +# Date 1498202642 -28800
> +#      Fri Jun 23 15:24:02 2017 +0800
> +# Node ID 7a2bc86bcc197404cd30bd275a3b41162f838a00
> +# Parent  7fdd58dbbf8e64a4bd072f2d7295723816890f4a
> +Bug 1368948: Include objidl.h between windows.h and gdiplus.h.

It would be nice to have an explanation of *why* we do this. Not just "because this causes a build error", but an explanation of why it causes a build error for us but not for normal PDFium builds.

::: modules/pdfium/patches/bug1368948_remove_openjpeg.patch
@@ +3,5 @@
> +# Date 1498202684 -28800
> +#      Fri Jun 23 15:24:44 2017 +0800
> +# Node ID f18f9827733dde6ab70ce9734633a5ba3567bc6b
> +# Parent  666c13fbd6937fa69af7eb223120da1812f45d15
> +Bug 1368948: Remove OpenJPEG due to SkiaPDF doesn't use JPEG 2000.

This patch isn't removing the OpenJPEG code (we do that in update.sh), it's rather preventing PDFium from using that removed code. I'd suggest making this message:

  Prevent PDFium from trying to use the removed OpenJPEG code.

  Our PDFium update script at modules/pdfium/update.sh removes the OpenJPEG code
  from our copy of PDFium.  This patch makes sure that the remaining PDFium code
  does not try to use OpenJPEG.

::: modules/pdfium/patches/bug1368948_use_gecko_freetype.patch
@@ +3,5 @@
> +# Date 1498202651 -28800
> +#      Fri Jun 23 15:24:11 2017 +0800
> +# Node ID f61f35d07d1483866485f8126677ace6da0dc0d3
> +# Parent  7a2bc86bcc197404cd30bd275a3b41162f838a00
> +Bug 1368948: Use existing freetype library within Gecko.

Make this:

  Make PDFium use Mozilla's in-tree copy of FreeType instead of its own copy.

::: modules/pdfium/patches/bug1368948_use_gecko_libjpeg.patch
@@ +3,5 @@
> +# Date 1498202662 -28800
> +#      Fri Jun 23 15:24:22 2017 +0800
> +# Node ID 38eedf401444bba32a749462ded4f9e93df2bf4c
> +# Parent  f61f35d07d1483866485f8126677ace6da0dc0d3
> +Bug 1368948: Use existing libjpeg library within Gecko.

Make this:
 
  Make PDFium use Mozilla's in-tree copy of libjpeg instead of its own copy.

::: modules/pdfium/patches/bug1368948_use_gecko_zlib.patch
@@ +3,5 @@
> +# Date 1498202674 -28800
> +#      Fri Jun 23 15:24:34 2017 +0800
> +# Node ID 666c13fbd6937fa69af7eb223120da1812f45d15
> +# Parent  38eedf401444bba32a749462ded4f9e93df2bf4c
> +Bug 1368948: Use existing zlib library within Gecko.

Make this:
 
  Make PDFium use Mozilla's in-tree copy of zlip instead of its own copy.

Comment 86

[Jonathan Watt [:jwatt]]

Comment on attachment 8881989 [details] [diff] [review]
Bug 1368948: [1/7] Add a script to update PDFium from upstream. (v3)

Review of attachment 8881989 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Thank you.

Comment 87

[Bruce Sun [:brsun]]

Attachment: Bug 1368948: [2/7] Add patch files for PDFium. (v3)

address comment 85 except for GDI+ one.

Comment 88

[Jonathan Watt [:jwatt]]

Comment on attachment 8882057 [details] [diff] [review]
Bug 1368948: [2/7] Add patch files for PDFium. (v3)

Thanks.

Comment 89

[Jonathan Watt [:jwatt]]

Comment on attachment 8881347 [details]
Bug 1368948: [3/7] Add PDfium files (chromium/3029) with patches.

As requested in person, please investigate removing the xfa, test and testing directories. This is quite a large amound of code and once it's in the mercurial history we have to pay the cost of having that forever. If we can avoid commiting those three directories to the repository that would be a good win.

Comment 90

[Bruce Sun [:brsun]]

By simply removing test, testing, and XFA folders locally, I still can build pdfium with the same moz.build. I will modify the script to remove these three folders programmatically.

Comment 91

[Bruce Sun [:brsun]]

Attachment: Log of build errors in GDI header files when building with mozilla-config.h which defines WIN32_LEAN_AND_MEAN.

> > +Bug 1368948: Include objidl.h between windows.h and gdiplus.h.
> 
> It would be nice to have an explanation of *why* we do this. Not just
> "because this causes a build error", but an explanation of why it causes a
> build error for us but not for normal PDFium builds.
> 

Even if <objidl.h> is not included explicitly, I can compile fx_win32_gdipext.cpp by using the following simple compiler command:
$ cl.exe -Id:/Source/gecko/mozilla-central-working/modules/pdfium/pdfium -Id:/Source/gecko/mozilla-central-working/modules/freetype2/include -DNOMINMAX -EHsc -c d:/Source/gecko/mozilla-central-working/modules/pdfium/pdfium/core/fxge/win32/fx_win32_gdipext.cpp

However, the command from our mach fails to compile the same file. Commands and errors are stored in this attached file.

It seems that this compile error is caused by our own build system. The most suspicious parts might be stl_wrappers (or system_wrappers?) and mozilla-config.h.

Comment 92

[Bruce Sun [:brsun]]

Hi Mike,

Do you have any quick idea about what might be wrong between <gdiplus.h> and <objidl.h> in our build system?

Comment 93

[Mike Hommey [:glandium]]

I don't that exact version of the SDK at hand, so I can't tell what those errors are about. Did you try to start from the full command line and remove arguments until it compiles?

Comment 94

[Bruce Sun [:brsun]]

I can get the same error messages by using the following commands:
$ cl.exe -c -Id:/Source/gecko/mozilla-central-working/obj-i686-pc-mingw32/modules/pdfium -Id:/Source/gecko/mozilla-central-working/modules/pdfium/pdfium -Id:/Source/gecko/mozilla-central-working/modules/freetype2/include -Id:/Source/gecko/mozilla-central-working/obj-i686-pc-mingw32/dist/include -FI d:/Source/gecko/mozilla-central-working/obj-i686-pc-mingw32/mozilla-config.h d:/Source/gecko/mozilla-central-working/modules/pdfium/pdfium/core/fxge/win32/fx_win32_gdipext.cpp

Not sure about why, but it is related to mozilla-config.h.

Comment 95

[Mike Hommey [:glandium]]

Can you pinpoint what part of mozilla-config.h causes that?

Comment 96

[Bruce Sun [:brsun]]

I think I found the point...

#define WIN32_LEAN_AND_MEAN 1

Comment 97

[Mike Hommey [:glandium]]

Makes sense. I don't think there's a better workaround than your patch, but at least, you can add a comment that it's because of WIN32_LEAN_AND_MEAN. The alternative would be to patch to #undef it, which I guess would be more self-explanatory.

Comment 98

[Bruce Sun [:brsun]]

I'll use #undef approach.

Comment 99

[Bruce Sun [:brsun]]

Attachment: Bug 1368948: [2/7] Add patch files for PDFium. (v4)

Hi Mike and Jonathan,

I've update GDI+ part with #undef approach. Would you like to have a look on it?

Comment 100

[Bruce Sun [:brsun]]

Attachment: Bug 1368948: [3/7] Add PDfium files (chromium/3029) with patches. (v2)

address comment 89 to remove three folders (i.e. test, testing, xfa) from pdfium directory.

Comment 101

[Bruce Sun [:brsun]]

Attachment: Bug 1368948: [1/7] Add a script to update PDFium from upstream. (v4)

let the update script to remove more folders from pdfium (i.e. test, testing, xfa)

Comment 102

[Jonathan Watt [:jwatt]]

Comment on attachment 8882385 [details] [diff] [review]
Bug 1368948: [1/7] Add a script to update PDFium from upstream. (v4)

Review of attachment 8882385 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 103

[Jonathan Watt [:jwatt]]

Comment on attachment 8882133 [details]
Log of build errors in GDI header files when building with mozilla-config.h which defines WIN32_LEAN_AND_MEAN.

No need to obsolete this file. It's useful to have this log visible in the attachments list.

Comment 104

[Bruce Sun [:brsun]]

Attachment: Bug 1368948: [2/7] Add patch files for PDFium. (v5)

Hi Jonathan,

Comments for the patch of GDI+ build errors are added within this patch.

Comment 105

[Bruce Sun [:brsun]]

Attachment: Bug 1368948: [2/7] Add patch files for PDFium. (v5.1)

The real v5 version

Comment 106

[Jonathan Watt [:jwatt]]

Comment on attachment 8882433 [details] [diff] [review]
Bug 1368948: [2/7] Add patch files for PDFium. (v5.1)

Review of attachment 8882433 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Land it! :)

::: modules/pdfium/patches/bug1368948_gdiplus_prerequisite.patch
@@ +3,5 @@
> +# Date 1498779115 25200
> +#      Thu Jun 29 16:31:55 2017 -0700
> +# Node ID 3c3a40bcfaaaefe68be48d06782034f99cc99f33
> +# Parent  003a1581ff72b5879af971aab6de87f69adb9145
> +Bug 1368948: Undefining WIN32_LEAN_AND_MEAN when building PDFium to avoid build errors in GDI+ headers.

Make that "Undefine" instead of "Undefining".

Comment 107

[Bruce Sun [:brsun]]

Attachment: Bug 1368948: [1/7] Add a script to update PDFium from upstream; r=glandium, r=jwatt.

Comment 108

[Bruce Sun [:brsun]]

Attachment: Bug 1368948: [2/7] Add patch files for PDFium; f=glandium, r=jwatt.

Comment 109

[Bruce Sun [:brsun]]

Attachment: Bug 1368948: [3/7] Add PDfium files (chromium/3029) with patches; r=jwatt.

Comment 110

[Bruce Sun [:brsun]]

Attachment: Bug 1368948: [4/7] Add license notices for PDFium and its dependent libraries; r=gerv.

Comment 111

[Bruce Sun [:brsun]]

Attachment: Bug 1368948: [5/7] Suppress clang-plugin errors; r=Ehsan.

Comment 112

[Bruce Sun [:brsun]]

Attachment: Bug 1368948: [6/7] Build freetype when enabling SkiaPDF on Windows; r=glandium.

Comment 113

[Bruce Sun [:brsun]]

Attachment: Bug 1368948: [7/7] Add moz.build for PDFium; r=glandium.

Comment 114

[Bruce Sun [:brsun]]

Build results on the try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b5f09dab20eb4fe53ec25eed051374aa8cca070

Comment 115

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ddf08e4cb5b
[1/7] Add a script to update PDFium from upstream. r=glandium, r=jwatt
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4e545936d90
[2/7] Add patch files for PDFium. f=glandium, r=jwatt
https://hg.mozilla.org/integration/mozilla-inbound/rev/aed6e75d9b0d
[3/7] Add PDfium files (chromium/3029) with patches. r=jwatt
https://hg.mozilla.org/integration/mozilla-inbound/rev/9adc0e3d0d91
[4/7] Add license notices for PDFium and its dependent libraries. r=gerv
https://hg.mozilla.org/integration/mozilla-inbound/rev/95400bc93b90
[5/7] Suppress clang-plugin errors. r=Ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/31e471d5b289
[6/7] Build freetype when enabling SkiaPDF on Windows. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd4549c33575
[7/7] Add moz.build for PDFium. r=glandium

Comment 116

[Bruce Sun [:brsun]]

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=cd4549c335752aca80ac293015d0b85ada94c332

Comment 117

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4ddf08e4cb5b
https://hg.mozilla.org/mozilla-central/rev/f4e545936d90
https://hg.mozilla.org/mozilla-central/rev/aed6e75d9b0d
https://hg.mozilla.org/mozilla-central/rev/9adc0e3d0d91
https://hg.mozilla.org/mozilla-central/rev/95400bc93b90
https://hg.mozilla.org/mozilla-central/rev/31e471d5b289
https://hg.mozilla.org/mozilla-central/rev/cd4549c33575

Comment 118

[Xidorn Quan [:xidorn] UTC+10]

Could we make them use UNIFIED_SOURCES rather than SOURCES? Would that slightly mitigate the build time hit?

Comment 119

[Bruce Sun [:brsun]]

I will try it to see how much we can earn by using UNIFIED_SOURCES.