Create a define for the gyp variable internal_pdf.

Create a define for the gyp variable internal_pdf.
This allows #ifdef-ing out PDF code that should not be built in Chromium.

BUG=51435
TEST=In Chromium, try to open a PDF. It should download, not show "Missing Plug-In".

[Mark Mentovai - out til August, 2010-08-06 22:28:13]

http://codereview.chromium.org/3071027/diff/1/2
File build/common.gypi (right):

http://codereview.chromium.org/3071027/diff/1/2#newcode450
build/common.gypi:450: 'defines': ['INTERNAL_PDF=1'],
Blech. We don’t really need to leak this to the entire world, do we? Can’t we
just scope this to chrome/ or even to a target where we need it?

(I know, the things before you set a pretty terrible example.)

[gene, 2010-08-06 22:39:12]

I think this issue become quite visible after I removed check for existence for
PDF plugin, since it was not working in the Linux sandbox. See
http://codereview.chromium.org/3081013.

I can disable PDF plugin completely if library is missing on the upper level.
See Patch Set 1 from http://codereview.chromium.org/3081013
It might be a better fix in this situation. No library - no plugin visible.
However, it is breaking quite a few unittests right now, and I am working on
fixing this.

On 2010/08/06 22:28:13, Mark Mentovai wrote:
> http://codereview.chromium.org/3071027/diff/1/2
> File build/common.gypi (right):
> 
> http://codereview.chromium.org/3071027/diff/1/2#newcode450
> build/common.gypi:450: 'defines': ['INTERNAL_PDF=1'],
> Blech. We don’t really need to leak this to the entire world, do we? Can’t we
> just scope this to chrome/ or even to a target where we need it?
> 
> (I know, the things before you set a pretty terrible example.)

[Robert Sesek, 2010-08-06 22:49:00]

On 2010/08/06 22:39:12, gene wrote:
> I think this issue become quite visible after I removed check for existence
for
> PDF plugin, since it was not working in the Linux sandbox. See
> http://codereview.chromium.org/3081013.
> 
> I can disable PDF plugin completely if library is missing on the upper level.
> See Patch Set 1 from http://codereview.chromium.org/3081013
> It might be a better fix in this situation. No library - no plugin visible.
> However, it is breaking quite a few unittests right now, and I am working on
> fixing this.

Yes, that CL did cause a regression; I filed
http://code.google.com/p/chromium/issues/detail?id=51435 to track it.  I don't
think we should be building anything related to the PDF plugin unless we are in
a build configuration (official) that necessitates it.  Many of our other
official-only features are ifdef-ed out this way.

[gene, 2010-08-06 22:53:10]

On 2010/08/06 22:49:00, rsesek wrote:
> On 2010/08/06 22:39:12, gene wrote:
> > I think this issue become quite visible after I removed check for existence
> for
> > PDF plugin, since it was not working in the Linux sandbox. See
> > http://codereview.chromium.org/3081013.
> > 
> > I can disable PDF plugin completely if library is missing on the upper
level.
> > See Patch Set 1 from http://codereview.chromium.org/3081013
> > It might be a better fix in this situation. No library - no plugin visible.
> > However, it is breaking quite a few unittests right now, and I am working on
> > fixing this.
> 
> Yes, that CL did cause a regression; I filed
> http://code.google.com/p/chromium/issues/detail?id=51435 to track it.  I don't
> think we should be building anything related to the PDF plugin unless we are
in
> a build configuration (official) that necessitates it.  Many of our other
> official-only features are ifdef-ed out this way.

Agree. As for PDF, I am not sure what configuration of the Chrome should not
build pdf? It should be a part of Chrome on Win, Mac, Linux and ChromeOS. And in
Chromium it is not building anyways, since it is in the internal repository and
no sources are available to Chromium users.

[gene, 2010-08-06 22:58:23]

On 2010/08/06 22:53:10, gene wrote:
> On 2010/08/06 22:49:00, rsesek wrote:
> > On 2010/08/06 22:39:12, gene wrote:
> > > I think this issue become quite visible after I removed check for
existence
> > for
> > > PDF plugin, since it was not working in the Linux sandbox. See
> > > http://codereview.chromium.org/3081013.
> > > 
> > > I can disable PDF plugin completely if library is missing on the upper
> level.
> > > See Patch Set 1 from http://codereview.chromium.org/3081013
> > > It might be a better fix in this situation. No library - no plugin
visible.
> > > However, it is breaking quite a few unittests right now, and I am working
on
> > > fixing this.
> > 
> > Yes, that CL did cause a regression; I filed
> > http://code.google.com/p/chromium/issues/detail?id=51435 to track it.  I
don't
> > think we should be building anything related to the PDF plugin unless we are
> in
> > a build configuration (official) that necessitates it.  Many of our other
> > official-only features are ifdef-ed out this way.
> 
> Agree. As for PDF, I am not sure what configuration of the Chrome should not
> build pdf? It should be a part of Chrome on Win, Mac, Linux and ChromeOS. And
in
> Chromium it is not building anyways, since it is in the internal repository
and
> no sources are available to Chromium users.


Oops. I think I misunderstood your comment, confused "building PDF" with
"building related to PDF code". Sorry.
This might be ok too. I am not a big fan of ifdefs. If we can have the same
logic for all plugins that disables plugin if library is missing it might be
better, IMHO.

[Robert Sesek, 2010-08-06 23:17:20]

On 2010/08/06 22:58:23, gene wrote:
> On 2010/08/06 22:53:10, gene wrote:
> > On 2010/08/06 22:49:00, rsesek wrote:
> > > On 2010/08/06 22:39:12, gene wrote:
> > > > I think this issue become quite visible after I removed check for
> existence
> > > for
> > > > PDF plugin, since it was not working in the Linux sandbox. See
> > > > http://codereview.chromium.org/3081013.
> > > > 
> > > > I can disable PDF plugin completely if library is missing on the upper
> > level.
> > > > See Patch Set 1 from http://codereview.chromium.org/3081013
> > > > It might be a better fix in this situation. No library - no plugin
> visible.
> > > > However, it is breaking quite a few unittests right now, and I am
working
> on
> > > > fixing this.
> > > 
> > > Yes, that CL did cause a regression; I filed
> > > http://code.google.com/p/chromium/issues/detail?id=51435 to track it.  I
> don't
> > > think we should be building anything related to the PDF plugin unless we
are
> > in
> > > a build configuration (official) that necessitates it.  Many of our other
> > > official-only features are ifdef-ed out this way.
> > 
> > Agree. As for PDF, I am not sure what configuration of the Chrome should not
> > build pdf? It should be a part of Chrome on Win, Mac, Linux and ChromeOS.
And
> in
> > Chromium it is not building anyways, since it is in the internal repository
> and
> > no sources are available to Chromium users.
> 
> 
> Oops. I think I misunderstood your comment, confused "building PDF" with
> "building related to PDF code". Sorry.
> This might be ok too. I am not a big fan of ifdefs. If we can have the same
> logic for all plugins that disables plugin if library is missing it might be
> better, IMHO.

I think that plugin disabling logic isn't mutually exclusive with this ifdef.  I
still don't think that we should be building this code on non-official builds (a
la the remoting block below this).  I've moved the define to only exist in the
chrome/common/ module.

[Mark Mentovai - out til August, 2010-08-06 23:23:26]

LGTM

[gene, 2010-08-06 23:36:32]

On 2010/08/06 23:17:20, rsesek wrote:
> On 2010/08/06 22:58:23, gene wrote:
> > On 2010/08/06 22:53:10, gene wrote:
> > > On 2010/08/06 22:49:00, rsesek wrote:
> > > > On 2010/08/06 22:39:12, gene wrote:
> > > > > I think this issue become quite visible after I removed check for
> > existence
> > > > for
> > > > > PDF plugin, since it was not working in the Linux sandbox. See
> > > > > http://codereview.chromium.org/3081013.
> > > > > 
> > > > > I can disable PDF plugin completely if library is missing on the upper
> > > level.
> > > > > See Patch Set 1 from http://codereview.chromium.org/3081013
> > > > > It might be a better fix in this situation. No library - no plugin
> > visible.
> > > > > However, it is breaking quite a few unittests right now, and I am
> working
> > on
> > > > > fixing this.
> > > > 
> > > > Yes, that CL did cause a regression; I filed
> > > > http://code.google.com/p/chromium/issues/detail?id=51435 to track it.  I
> > don't
> > > > think we should be building anything related to the PDF plugin unless we
> are
> > > in
> > > > a build configuration (official) that necessitates it.  Many of our
other
> > > > official-only features are ifdef-ed out this way.
> > > 
> > > Agree. As for PDF, I am not sure what configuration of the Chrome should
not
> > > build pdf? It should be a part of Chrome on Win, Mac, Linux and ChromeOS.
> And
> > in
> > > Chromium it is not building anyways, since it is in the internal
repository
> > and
> > > no sources are available to Chromium users.
> > 
> > 
> > Oops. I think I misunderstood your comment, confused "building PDF" with
> > "building related to PDF code". Sorry.
> > This might be ok too. I am not a big fan of ifdefs. If we can have the same
> > logic for all plugins that disables plugin if library is missing it might be
> > better, IMHO.
> 
> I think that plugin disabling logic isn't mutually exclusive with this ifdef. 
I
> still don't think that we should be building this code on non-official builds
(a
> la the remoting block below this).  I've moved the define to only exist in the
> chrome/common/ module.

They are surely not mutual exclusive. Just keep in mind that user should not see
"PDF plugin" on the "about:plugins". And, ifdef should be ON in all Chrome
builds. Even the comment on the remoting block suggesting to think about other
approached instead of ifdef'ing.

[Robert Sesek, 2010-08-06 23:42:07]

On 2010/08/06 23:36:32, gene wrote:
> They are surely not mutual exclusive. Just keep in mind that user should not
see
> "PDF plugin" on the "about:plugins". And, ifdef should be ON in all Chrome
> builds. Even the comment on the remoting block suggesting to think about other
> approached instead of ifdef'ing.

At the moment, the user DOES see "PDF Plugin" in Chromium; this fixes that.  The
ifdef is gated to the internal_pdf gyp variable, which is what includes the
internal PDF gyp file into the build system.

[gene, 2010-08-07 00:03:45]

On 2010/08/06 23:42:07, rsesek wrote:
> On 2010/08/06 23:36:32, gene wrote:
> > They are surely not mutual exclusive. Just keep in mind that user should not
> see
> > "PDF plugin" on the "about:plugins". And, ifdef should be ON in all Chrome
> > builds. Even the comment on the remoting block suggesting to think about
other
> > approached instead of ifdef'ing.
> 
> At the moment, the user DOES see "PDF Plugin" in Chromium; this fixes that. 
The
> ifdef is gated to the internal_pdf gyp variable, which is what includes the
> internal PDF gyp file into the build system.

Unfortunately PDF-related logic is spread in a few other places
(chrome/browser/plugin_updater.cc). This will guard just one of them. Don't make
me wrong, this CL will fix the issue. And I am OK with it.
It just seems like the PDF logic in the code was designed around the fact of
presence of the PDF plugin binary. (I think you can copy pdf binary to the
chromium folder and it will work). And this CL introduces another compile-time
switch that developers have to remember and maintain.
I can see some people reverting this CL later, once we have PDF detection logic
fixed. I that is OK with you, I can lgtm in order to fix user experience right
now.

[Robert Sesek, 2010-08-07 00:14:01]

On Fri, Aug 6, 2010 at 8:03 PM, <gene@chromium.org> wrote:

> And this CL introduces another compile-time
> switch that developers have to remember and maintain.
>

No it does not.  As stated earlier, this is tied to the internal_pdf gyp
variable.  If you're building the PDF plugin, this define will kick in
automatically.

The logic in plugin_updater.cc is probably more OK in that it has both the
path-existence check and the command line check.  I'd be okay with not
checking this in depending on the ETA of the detection logic.

http://codereview.chromium.org/3071027/show

[viettrungluu, 2010-08-08 05:20:46]

The reason the check for the PDf plugin was removed was that in the Linux chroot
sandbox, the check cannot be performed (and thus always fails).

An only slightly-hackish fix is to introduce a flag (say,
"--force-pdf-plugin-detected"), and have the zygote do the file check and add
that to each renderer's command line. Then the original check in
PepperPluginRegistry::GetExtraPlugins() can be replaced with one which says "has
this flag || file exists".