Closed Bug 1368948 Opened 7 years ago Closed 7 years ago

Integrate PDFium for converting EMF on Windows

Categories

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

Product:
Core
Component:
Printing: Output
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: brsun, Assigned: brsun)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(8 files, 30 obsolete files)

Log of build errors in GDI header files when building with mozilla-config.h which defines WIN32_LEAN_AND_MEAN.
22.32 KB, text/plain
Details
Bug 1368948: [1/7] Add a script to update PDFium from upstream; r=glandium, r=jwatt.
4.89 KB, patch
brsun
: review+
Details | Diff | Splinter Review
Bug 1368948: [2/7] Add patch files for PDFium; f=glandium, r=jwatt.
13.73 KB, patch
brsun
: review+
Details | Diff | Splinter Review
Bug 1368948: [3/7] Add PDfium files (chromium/3029) with patches; r=jwatt.
2.47 MB, application/x-zip-compressed
brsun
: review+
Details
Bug 1368948: [4/7] Add license notices for PDFium and its dependent libraries; r=gerv.
10.86 KB, patch
brsun
: review+
Details | Diff | Splinter Review
Bug 1368948: [5/7] Suppress clang-plugin errors; r=Ehsan.
3.75 KB, patch
brsun
: review+
Details | Diff | Splinter Review
Bug 1368948: [6/7] Build freetype when enabling SkiaPDF on Windows; r=glandium.
4.87 KB, patch
brsun
: review+
Details | Diff | Splinter Review
Bug 1368948: [7/7] Add moz.build for PDFium; r=glandium.
22.98 KB, patch
brsun
: review+
Details | Diff | Splinter Review 
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.
See Also: → 1345330
Blocks: pdf-printing
Depends on: 1358023
Assignee: nobody → brsun
Status: NEW → ASSIGNED
Priority: -- → P1
WIP: add PDFium files
WIP: moz.build for PDFium
Hi Jonathan,

Currently I put PDFium under /layout/pdfium in my patches. Could you suggest a better place for it?
Flags: needinfo?(jwatt)
(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.
Thanks Peter, this is just a WIP. I will submit patches with mozreview when requesting for reviewing.
(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.
> 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.
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.
Flags: needinfo?(jwatt)
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/ ?
Flags: needinfo?(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.
Flags: needinfo?(brsun)
Attached file update.py (obsolete) — 
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.
(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.
> 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.
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.
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
Attachment #8875201 - Attachment is obsolete: true
Attachment #8875202 - Attachment is obsolete: true
Attachment #8876969 - Attachment is obsolete: true
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.
Attachment #8877046 - Flags: review?(mh+mozilla)
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
Attachment #8877047 - Flags: review?(mh+mozilla)
Attachment #8877048 - Flags: review?(mh+mozilla)
Attachment #8877048 - Flags: feedback?(jwatt)
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 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/"
Attachment #8877049 - Flags: review?(ellee)
Comment on attachment 8877050 [details]
Bug 1368948: Suppress clang-plugin errors.

Add explicit rules to suppress static analysis warnings for PDFium sources.
Attachment #8877050 - Flags: review?(mh+mozilla)
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)
Attachment #8877051 - Flags: review?(mh+mozilla)
Comment on attachment 8877052 [details]
Bug 1368948: Add moz.build for PDFium.

Add moz.build for PDfium library.
Attachment #8877052 - Flags: review?(mh+mozilla)
ReviewBoard cannot display my patch (attachment 8877048 [details]) properly. This is the zip version of that patch.
Attachment #8877049 - Flags: review?(ellee) → review?(gerv)
Attachment #8877046 - Flags: review?(mh+mozilla)
Attachment #8877047 - Flags: review?(mh+mozilla)
Attachment #8877048 - Flags: review?(mh+mozilla)
Attachment #8877049 - Flags: review?(gerv)
Attachment #8877050 - Flags: review?(mh+mozilla)
Attachment #8877051 - Flags: review?(mh+mozilla)
Attachment #8877052 - Flags: review?(mh+mozilla)
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
Attachment #8877049 - Flags: review?(gerv) → review+
Attachment #8877464 - Attachment is obsolete: true
Attachment #8877464 - Flags: review?(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.
See Also: → 1372842
Mike, would you be able to review the patches or suggest other reviewers if you are busy? Thanks.
Flags: needinfo?(mh+mozilla)
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 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.
Attachment #8877046 - Flags: review?(mh+mozilla)
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 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.
Attachment #8877047 - Flags: review?(mh+mozilla)
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.
Attachment #8877048 - Flags: review?(mh+mozilla)
Flags: needinfo?(mh+mozilla)
Attachment #8877050 - Flags: review?(mh+mozilla) → review?(ehsan)
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?
Attachment #8877051 - Flags: review?(mh+mozilla)
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?
Attachment #8877052 - Flags: review?(mh+mozilla)
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 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
> 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?
Flags: needinfo?(jwatt)
(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 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).
(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 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 on attachment 8877050 [details]
Bug 1368948: Suppress clang-plugin errors.

https://reviewboard.mozilla.org/r/148396/#review155936
Attachment #8877050 - Flags: review?(ehsan) → review+
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 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.
> 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?
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(aschen)
(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.
Flags: needinfo?(aschen)
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.
(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.
Flags: needinfo?(jwatt)
Depends on: 1349064
(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.
Attached file update.sh (obsolete) — 
Hi Mike,

Would this update script make you cringe less?
Attachment #8880689 - Flags: feedback?(mh+mozilla)
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
Flags: needinfo?(fatseng)
(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.
Flags: needinfo?(fatseng)
Since we encounter some problems on mozreview (ref. bug 1372842), I will using the old-fashion way to submit patches for this bug.
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.
Attachment #8877046 - Attachment is obsolete: true
Attachment #8880689 - Attachment is obsolete: true
Attachment #8880689 - Flags: feedback?(mh+mozilla)
Flags: needinfo?(mh+mozilla)
Attachment #8881345 - Flags: review?(mh+mozilla)
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
Attachment #8877047 - Attachment is obsolete: true
Attachment #8881346 - Flags: review?(mh+mozilla)
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
Attachment #8877048 - Attachment is obsolete: true
Attachment #8877089 - Attachment is obsolete: true
Attachment #8881347 - Flags: review?(jwatt)
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?
Attachment #8877049 - Attachment is obsolete: true
Attachment #8881348 - Flags: review?(gerv)
Carry over r+ flag from comment 57.
Attachment #8881349 - Flags: review+
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
Attachment #8877051 - Attachment is obsolete: true
Attachment #8881350 - Flags: review?(mh+mozilla)
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
Attachment #8877052 - Attachment is obsolete: true
Attachment #8881351 - Flags: review?(mh+mozilla)
Attachment #8877050 - Attachment is obsolete: true
Attachment #8881348 - Flags: review?(gerv) → review+
Attachment #8881347 - Attachment is patch: true
Attachment #8881347 - Attachment mime type: application/x-zip-compressed → text/plain
Attachment #8881347 - Attachment is patch: false
Attachment #8881347 - Attachment mime type: text/plain → application/x-zip-compressed
Depends on: 1376057
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)`
Attachment #8881345 - Flags: review?(mh+mozilla) → feedback+
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 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 :)
Attachment #8881346 - Flags: review?(mh+mozilla) → feedback+
Attachment #8881350 - Flags: review?(mh+mozilla) → review+
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.
Attachment #8881351 - Flags: review?(mh+mozilla) → review+
address comment 76 and comment 77
Attachment #8881345 - Attachment is obsolete: true
Attachment #8881840 - Flags: review?(mh+mozilla)
address comment 77

Hi Jonathan,

Would you like to share your comments as well?
Attachment #8881346 - Attachment is obsolete: true
Attachment #8881841 - Flags: review?(jwatt)
Attachment #8881841 - Flags: feedback+
address comment 79
Attachment #8881351 - Attachment is obsolete: true
Attachment #8881843 - Flags: review+
Attachment #8881840 - Flags: review?(mh+mozilla) → review+
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".
address comment 83.

Hi Jonathan, I've updated the script accordingly. Would you like to have a look at it again?
Attachment #8881840 - Attachment is obsolete: true
Attachment #8881989 - Flags: review?(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.
Attachment #8881841 - Flags: review?(jwatt) → review+
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.
Attachment #8881989 - Flags: review?(jwatt) → review+
address comment 85 except for GDI+ one.
Attachment #8881841 - Attachment is obsolete: true
Comment on attachment 8882057 [details] [diff] [review]
Bug 1368948: [2/7] Add patch files for PDFium. (v3)

Thanks.
Attachment #8882057 - Flags: review+
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.
Attachment #8881347 - Flags: review?(jwatt)
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.
> > +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.
Hi Mike,

Do you have any quick idea about what might be wrong between <gdiplus.h> and <objidl.h> in our build system?
Flags: needinfo?(mh+mozilla)
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?
Flags: needinfo?(mh+mozilla)
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.
Can you pinpoint what part of mozilla-config.h causes that?
I think I found the point...

#define WIN32_LEAN_AND_MEAN 1
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.
I'll use #undef approach.
Hi Mike and Jonathan,

I've update GDI+ part with #undef approach. Would you like to have a look on it?
Attachment #8882057 - Attachment is obsolete: true
Attachment #8882133 - Attachment is obsolete: true
Attachment #8882376 - Flags: review?(mh+mozilla)
Attachment #8882376 - Flags: review?(jwatt)
address comment 89 to remove three folders (i.e. test, testing, xfa) from pdfium directory.
Attachment #8881347 - Attachment is obsolete: true
Attachment #8882380 - Flags: review?(jwatt)
let the update script to remove more folders from pdfium (i.e. test, testing, xfa)
Attachment #8881989 - Attachment is obsolete: true
Attachment #8882385 - Flags: review?(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!
Attachment #8882385 - Flags: review?(jwatt) → review+
Attachment #8882380 - Flags: review?(jwatt) → review+
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.
Attachment #8882133 - Attachment description: fx_win32_gdipext_cpp_compile_errors.txt → Log of build errors in GDI header files when building with mozilla-config.h which defines WIN32_LEAN_AND_MEAN.
Attachment #8882133 - Attachment is obsolete: false
Hi Jonathan,

Comments for the patch of GDI+ build errors are added within this patch.
Attachment #8882376 - Attachment is obsolete: true
Attachment #8882376 - Flags: review?(mh+mozilla)
Attachment #8882376 - Flags: review?(jwatt)
Attachment #8882432 - Flags: review?(jwatt)
The real v5 version
Attachment #8882432 - Attachment is obsolete: true
Attachment #8882432 - Flags: review?(jwatt)
Attachment #8882433 - Flags: review?(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".
Attachment #8882433 - Flags: review?(jwatt) → review+

        Attachment #8881349 -
        Attachment is obsolete: true

        Attachment #8882454 -
        Flags: review+

    
    

    
  


  

        Attachment #8881843 -
        Attachment is obsolete: true

        Attachment #8882457 -
        Flags: review+

    
  
No longer depends on: 1349064

    
    

    
  
Pushed by brsun@mozilla.com:
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
Blocks: 1378050

    
    

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

    
    

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

    
  
Blocks: 1378608
Depends on: CVE-2017-5095
Depends on: 1416992

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: