Enable XFA-Forms for the PDFium plugin

Enable XFA-Forms for the PDFium plugin.

This is a change to gather stability, performance, and size
data and will be rolled back by 2016-03-15.

This is the gyp portion of the change; the GN variables
controlling the same things currently reside in the pdfium
product itsef.

Committed: https://crrev.com/afa43e7fa9754cff9ceb6969c665830a79a84c5b
Cr-Commit-Position: refs/heads/master@{#379622}

[Tom Sepez, 2016-03-02 23:54:46]

Description was changed from

==========
Enable XFA-Forms for the PDFium plugin
==========

to

==========
Enable XFA-Forms for the PDFium plugin.

This is the gyp portion of the change; the GN variables
controlling the same things currently reside in the pdfium
product itsef.
==========

[Tom Sepez, 2016-03-02 23:54:46]

tsepez@chromium.org changed reviewers:
+ rpop@chromium.org, thakis@chromium.org

[Tom Sepez, 2016-03-02 23:56:48]

Requires pdfium to be rolled past https://codereview.chromium.org/1763493002/
before landing this change.

[Nico, 2016-03-03 00:11:05]

lgtm

Does this change binary size much? Binary size monitoring is currently broken,
so please watch that manually.

[Tom Sepez, 2016-03-03 00:25:37]

On 2016/03/03 00:11:05, Nico wrote:
> lgtm
> 
> Does this change binary size much? Binary size monitoring is currently broken,
> so please watch that manually.

The difference in the pdfium_test binary (which links the same library) is about
3MB with the feature enabled.

[Nico, 2016-03-03 00:38:48]

…that sounds kind of prohibitively large. How on earth can this be 3MB?

On Wed, Mar 2, 2016 at 4:25 PM, <tsepez@chromium.org> wrote:

> On 2016/03/03 00:11:05, Nico wrote:
> > lgtm
> >
> > Does this change binary size much? Binary size monitoring is currently
> broken,
> > so please watch that manually.
>
> The difference in the pdfium_test binary (which links the same library) is
> about
> 3MB with the feature enabled.
>
> https://codereview.chromium.org/1761673002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Tom Sepez, 2016-03-03 00:50:58]

It adds about 160K LOC.

[Nico, 2016-03-03 00:56:09]

That too sounds unreasonably large. This is a very nice feature, but I'm not
sure it's +3MB nice. Is there any way to get rid of large chunks of the binary
size / that code? What does all that code do?

[chromium-reviews, 2016-03-03 00:57:25]

It implements the 1500 page spec at
http://partners.adobe.com/public/developer/en/xml/xfa_spec_3_3.pdf

On Wed, Mar 2, 2016 at 4:56 PM, <thakis@chromium.org> wrote:

> That too sounds unreasonably large. This is a very nice feature, but I'm
> not
> sure it's +3MB nice. Is there any way to get rid of large chunks of the
> binary
> size / that code? What does all that code do?
>
> https://codereview.chromium.org/1761673002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Tom Sepez, 2016-03-03 00:59:02]

We've asked about stripping it down before, and its authors have always said
that would be difficult.

[Nico, 2016-03-03 01:05:01]

Is all of that spec used in practice?

Is it possible to download this on demand or something?

Adding 3MB of binary size for a feature that is super useful when needed but
realistically only used very rarely does not lgtm :-/

[Tom Sepez, 2016-03-03 02:19:09]

Just to be sure my baseline sizes are reasonable, for a release build on linux I
get the following, depending upon whether XFA is present:

-rwxr-x--- 1 tsepez eng 161986440 Mar  2 17:51 out/Release/chrome
-rwxr-x--- 1 tsepez eng 158920400 Mar  2 18:17 out/Release/chrome

I haven't looked in a while, does this make sense?

[jschuh, 2016-03-03 14:08:47]

The product decision to support the full XFA spec was made quite a while ago,
and was a central reason for the purchase and open sourcing this code. So, this
review is probably not the right place to make product arguments against it.
Rather, this feature is headed to launch review, which is unquestionably the
right place to weigh the binary size increase and any other concerns that could
block shipping. However, we need to have the feature enabled on trunk ASAP, so
we can get the data we need to bring it to launch review and have that
discussion.

Tom, prior to launch review, we definitely need better data on the binary size
impact. The concerns I see are:
 * Verify that size is for a stripped Linux executable
 * Verify the release size increase on Mac
 * Verify the binary size increase on official Windows builds
 * Verify the installer size increase on official Windows builds
 * Verify pdfium/XFA are not compiled in on Android

That stated, I think we're most concerned about not increasing the binary size
on Android, and not increasing the download size on Windows -- because those are
increases that would cause genuine pain. (Anthony, please let me know if there's
anything else on that front.)

Nico, are you comfortable with enabling given the circumstances, or would you
rather someone else signed off on this CL? Also, if you're concerned about
binary size being sufficiently discussed at launch review, would you like to be
added to the calendar once it's scheduled?

[Nico, 2016-03-03 19:06:36]

On 2016/03/03 14:08:47, jschuh (very slow) wrote:
> The product decision to support the full XFA spec was made quite a while ago,
> and was a central reason for the purchase and open sourcing this code. So,
this
> review is probably not the right place to make product arguments against it.
> Rather, this feature is headed to launch review, which is unquestionably the
> right place to weigh the binary size increase and any other concerns that
could
> block shipping. However, we need to have the feature enabled on trunk ASAP, so
> we can get the data we need to bring it to launch review and have that
> discussion.
> 
> Tom, prior to launch review, we definitely need better data on the binary size
> impact. The concerns I see are:
>  * Verify that size is for a stripped Linux executable
>  * Verify the release size increase on Mac
>  * Verify the binary size increase on official Windows builds
>  * Verify the installer size increase on official Windows builds
>  * Verify pdfium/XFA are not compiled in on Android
> 
> That stated, I think we're most concerned about not increasing the binary size
> on Android, and not increasing the download size on Windows -- because those
are
> increases that would cause genuine pain. (Anthony, please let me know if
there's
> anything else on that front.)
> 
> Nico, are you comfortable with enabling given the circumstances, or would you
> rather someone else signed off on this CL? Also, if you're concerned about
> binary size being sufficiently discussed at launch review, would you like to
be
> added to the calendar once it's scheduled?

So, if the plan here is to collect data and to not ship to beta until size is
figured out, then landing this temporarily lgtm. The CL description should be
updated to say that then (ideally with a "will be reverted by $date, this is to
collect stability data and whatnot")

I don't disagree with the product decision to have XFA support at all, I think
it's a great feature. I think an implementation approach that adds 3 MB to
chrome by default is the wrong implementation approach is all. I feel that's a
technical argument, not a product decision argument.

[Tom Sepez, 2016-03-03 19:30:46]

Description was changed from

==========
Enable XFA-Forms for the PDFium plugin.

This is the gyp portion of the change; the GN variables
controlling the same things currently reside in the pdfium
product itsef.
==========

to

==========
Enable XFA-Forms for the PDFium plugin.

This is a change to gather stability, performance, and size
data and will be rolled back by 2016-03-15.

This is the gyp portion of the change; the GN variables
controlling the same things currently reside in the pdfium
product itsef.
==========

[rpop, 2016-03-03 20:49:33]

On 2016/03/03 19:06:36, Nico wrote:
> On 2016/03/03 14:08:47, jschuh (very slow) wrote:
> > The product decision to support the full XFA spec was made quite a while
ago,
> > and was a central reason for the purchase and open sourcing this code. So,
> this
> > review is probably not the right place to make product arguments against it.
> > Rather, this feature is headed to launch review, which is unquestionably the
> > right place to weigh the binary size increase and any other concerns that
> could
> > block shipping. However, we need to have the feature enabled on trunk ASAP,
so
> > we can get the data we need to bring it to launch review and have that
> > discussion.
> > 
> > Tom, prior to launch review, we definitely need better data on the binary
size
> > impact. The concerns I see are:
> >  * Verify that size is for a stripped Linux executable
> >  * Verify the release size increase on Mac
> >  * Verify the binary size increase on official Windows builds
> >  * Verify the installer size increase on official Windows builds
> >  * Verify pdfium/XFA are not compiled in on Android
> > 
> > That stated, I think we're most concerned about not increasing the binary
size
> > on Android, and not increasing the download size on Windows -- because those
> are
> > increases that would cause genuine pain. (Anthony, please let me know if
> there's
> > anything else on that front.)
> > 
> > Nico, are you comfortable with enabling given the circumstances, or would
you
> > rather someone else signed off on this CL? Also, if you're concerned about
> > binary size being sufficiently discussed at launch review, would you like to
> be
> > added to the calendar once it's scheduled?
> 
> So, if the plan here is to collect data and to not ship to beta until size is
> figured out, then landing this temporarily lgtm. The CL description should be
> updated to say that then (ideally with a "will be reverted by $date, this is
to
> collect stability data and whatnot")
> 
> I don't disagree with the product decision to have XFA support at all, I think
> it's a great feature. I think an implementation approach that adds 3 MB to
> chrome by default is the wrong implementation approach is all. I feel that's a
> technical argument, not a product decision argument.

Thanks Nico. Let's discuss in launch review. LGTM

[Tom Sepez, 2016-03-07 16:39:28]

The CQ bit was checked by tsepez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-07 16:40:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1761673002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1761673002/1

[commit-bot: I haz the power, 2016-03-07 18:03:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-07 18:03:49]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Tom Sepez, 2016-03-07 18:57:32]

The CQ bit was checked by tsepez@chromium.org

[commit-bot: I haz the power, 2016-03-07 18:57:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1761673002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1761673002/1

[commit-bot: I haz the power, 2016-03-07 20:13:25]

Description was changed from

==========
Enable XFA-Forms for the PDFium plugin.

This is a change to gather stability, performance, and size
data and will be rolled back by 2016-03-15.

This is the gyp portion of the change; the GN variables
controlling the same things currently reside in the pdfium
product itsef.
==========

to

==========
Enable XFA-Forms for the PDFium plugin.

This is a change to gather stability, performance, and size
data and will be rolled back by 2016-03-15.

This is the gyp portion of the change; the GN variables
controlling the same things currently reside in the pdfium
product itsef.
==========

[commit-bot: I haz the power, 2016-03-07 20:13:26]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-03-07 20:15:08]

Description was changed from

==========
Enable XFA-Forms for the PDFium plugin.

This is a change to gather stability, performance, and size
data and will be rolled back by 2016-03-15.

This is the gyp portion of the change; the GN variables
controlling the same things currently reside in the pdfium
product itsef.
==========

to

==========
Enable XFA-Forms for the PDFium plugin.

This is a change to gather stability, performance, and size
data and will be rolled back by 2016-03-15.

This is the gyp portion of the change; the GN variables
controlling the same things currently reside in the pdfium
product itsef.

Committed: https://crrev.com/afa43e7fa9754cff9ceb6969c665830a79a84c5b
Cr-Commit-Position: refs/heads/master@{#379622}
==========

[commit-bot: I haz the power, 2016-03-07 20:15:10]

Patchset 1 (id:??) landed as
https://crrev.com/afa43e7fa9754cff9ceb6969c665830a79a84c5b
Cr-Commit-Position: refs/heads/master@{#379622}

[Nico, 2016-03-08 02:13:34]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/1773043002/ by thakis@chromium.org.

The reason for reverting is: Fails to build with clang-cl with very very scary
warnings:

In file included from
..\..\third_party\pdfium\xfa/src/fxfa/src/app/xfa_ffapp.h:15:
..\..\third_party\pdfium\xfa/src/fgas/include/fx_fnt.h(131,23) :  error: first
operand of this 'memcmp' call is a pointer to dynamic class '_FX_FONTSIGNATURE';
vtable pointer will be compared [-Werror,-Wdynamic-class-memaccess]
         FXSYS_memcmp(&left.FontSignature, &right.FontSignature,
         ~~~~~~~~~~~~ ^
..\..\third_party\pdfium\xfa/src/fgas/include/fx_fnt.h(131,23) :  note:
explicitly cast the pointer to silence this warning
         FXSYS_memcmp(&left.FontSignature, &right.FontSignature,
                      ^
                      (void*)

Reverting for now..

[Tom Sepez, 2016-03-08 16:56:32]

On 2016/03/08 02:13:34, Nico wrote:
> A revert of this CL (patchset #1 id:1) has been created in
> https://codereview.chromium.org/1773043002/ by mailto:thakis@chromium.org.
> 
> The reason for reverting is: Fails to build with clang-cl with very very scary
> warnings:
> 
> In file included from
> ..\..\third_party\pdfium\xfa/src/fxfa/src/app/xfa_ffapp.h:15:
> ..\..\third_party\pdfium\xfa/src/fgas/include/fx_fnt.h(131,23) :  error: first
> operand of this 'memcmp' call is a pointer to dynamic class
'_FX_FONTSIGNATURE';
> vtable pointer will be compared [-Werror,-Wdynamic-class-memaccess]
>          FXSYS_memcmp(&left.FontSignature, &right.FontSignature,
>          ~~~~~~~~~~~~ ^
> ..\..\third_party\pdfium\xfa/src/fgas/include/fx_fnt.h(131,23) :  note:
> explicitly cast the pointer to silence this warning
>          FXSYS_memcmp(&left.FontSignature, &right.FontSignature,
>                       ^
>                       (void*)
> 
> Reverting for now..

I filed this as pdfium:429.