pepper: don't warn if PDF plugin is unavailable

pepper: don't warn if PDF plugin is unavailable

We preload plugins before we sandbox, then later attempt to fully
load them once we're sandboxed.  We can't ask whether the plugin
exists at the second point because we don't have disk access at
that point.  So instead, the first time we're asked about plugins
(before we're sandboxed), record whether the plugin is actually
available and use that to skip loading it in the second pass.

BUG=51546
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=55609

[viettrungluu, 2010-08-10 18:16:04]

LGTM in principle, just some nits.

http://codereview.chromium.org/3114005/diff/1/2
File chrome/common/pepper_plugin_registry.cc (right):

http://codereview.chromium.org/3114005/diff/1/2#newcode112
chrome/common/pepper_plugin_registry.cc:112: #if !defined(GOOGLE_CHROME_BUILD)
We can probably leave this check in for Chrome too.

http://codereview.chromium.org/3114005/diff/1/3
File chrome/common/pepper_plugin_registry.h (right):

http://codereview.chromium.org/3114005/diff/1/3#newcode11
chrome/common/pepper_plugin_registry.h:11: #include <set>
Eh?

http://codereview.chromium.org/3114005/diff/1/3#newcode64
chrome/common/pepper_plugin_registry.h:64: static bool skip_pdf_plugin_;
We can probably have this as a file-level static variable, rather than putting
it in the class.

[gene1, 2010-08-10 18:32:23]

I used a different approach to fix this issue, see here:
http://codereview.chromium.org/3121002/show
Also, I don't think we should #ifdef CHROME for pdf plugin


On Tue, Aug 10, 2010 at 11:16 AM, <viettrungluu@chromium.org> wrote:

> LGTM in principle, just some nits.
>
>
> http://codereview.chromium.org/3114005/diff/1/2
> File chrome/common/pepper_plugin_registry.cc (right):
>
> http://codereview.chromium.org/3114005/diff/1/2#newcode112
> chrome/common/pepper_plugin_registry.cc:112: #if
> !defined(GOOGLE_CHROME_BUILD)
> We can probably leave this check in for Chrome too.
>
> http://codereview.chromium.org/3114005/diff/1/3
> File chrome/common/pepper_plugin_registry.h (right):
>
> http://codereview.chromium.org/3114005/diff/1/3#newcode11
> chrome/common/pepper_plugin_registry.h:11: #include <set>
> Eh?
>
> http://codereview.chromium.org/3114005/diff/1/3#newcode64
> chrome/common/pepper_plugin_registry.h:64: static bool skip_pdf_plugin_;
> We can probably have this as a file-level static variable, rather than
> putting it in the class.
>
>
> http://codereview.chromium.org/3114005/show
>

[Evan Martin, 2010-08-10 18:57:35]

Please take another look, it's much simpler now.

Gene, I'm fixing a different issue: that we now print a warning when the file
doesn't exist.  I looked at your previous patch for inspiration on how to fix
this issue.

[viettrungluu, 2010-08-10 19:29:29]

lgtm

http://codereview.chromium.org/3114005/diff/11001/12001
File chrome/common/pepper_plugin_registry.cc (right):

http://codereview.chromium.org/3114005/diff/11001/12001#newcode104
chrome/common/pepper_plugin_registry.cc:104: // Once we're sandboxed, we can't
know if the PDF plugin is
You should probably note that this applies to Linux.

[gene1, 2010-08-10 19:46:20]

Sorry, my brain is numb today :).

But I think this will fix your issue as well:
http://codereview.chromium.org/3121002
That fix will not display warning if there is no plugin.
I have not submitted it yet, since Brett asked me to remove ifdef LINUX, and
I am home sick today.

Sorry if I missed something,
Gene

On Tue, Aug 10, 2010 at 11:57 AM, <evan@chromium.org> wrote:

> Please take another look, it's much simpler now.
>
> Gene, I'm fixing a different issue: that we now print a warning when the
> file
> doesn't exist.  I looked at your previous patch for inspiration on how to
> fix
> this issue.
>
>
> http://codereview.chromium.org/3114005/show
>

[viettrungluu, 2010-08-10 19:50:53]

Gene, your change will result in the function doing the wrong thing if 
called from a Chromium (not Google Chrome) renderer on Linux.

- Trung

Gene Gutnik wrote:
> Sorry, my brain is numb today :).
> 
> But I think this will fix your issue as 
> well: http://codereview.chromium.org/3121002
> That fix will not display warning if there is no plugin.
> I have not submitted it yet, since Brett asked me to remove ifdef LINUX, 
> and I am home sick today.
> 
> Sorry if I missed something,
> Gene
> 
> On Tue, Aug 10, 2010 at 11:57 AM, <evan@chromium.org 
> <mailto:evan@chromium.org>> wrote:
> 
>     Please take another look, it's much simpler now.
> 
>     Gene, I'm fixing a different issue: that we now print a warning when
>     the file
>     doesn't exist.  I looked at your previous patch for inspiration on
>     how to fix
>     this issue.
> 
> 
>     http://codereview.chromium.org/3114005/show
> 
> 

[gene1, 2010-08-10 20:09:28]

Hi evan,
I am looking at the latest version of your code. I am not Linux expert, so
it looks quite confusing to me. I just wanted to check that everything is
working as expected.
When I was testing this issue, it looked like GetExtraPlugins was from zygot
and then from the renderer process. Will the static variable persist its
state from zygot to renderer? (is it some kind of fork trick?)
If that is the case, it is LGTM from me.
Sorry, for the previous confusions :)
Gene

On Tue, Aug 10, 2010 at 11:57 AM, <evan@chromium.org> wrote:

> Please take another look, it's much simpler now.
>
> Gene, I'm fixing a different issue: that we now print a warning when the
> file
> doesn't exist.  I looked at your previous patch for inspiration on how to
> fix
> this issue.
>
>
> http://codereview.chromium.org/3114005/show
>