When we detect a PDF with an unsupported feature, ask the user if they want t...

When we detect a PDF with an unsupported feature, ask the user if they want to view it with Adobe Reader if it's installed.  If it's not, ask them if they want to launch the url to install it.  If it's installed and out of date, show an interstitial.

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

[brettw, 2011-01-21 21:42:29]

LGTM

http://codereview.chromium.org/6259008/diff/24001/chrome/browser/plugin_servi...
File chrome/browser/plugin_service.h (right):

http://codereview.chromium.org/6259008/diff/24001/chrome/browser/plugin_servi...
chrome/browser/plugin_service.h:17: #include "base/lock.h"
The new name is base/synchronization/lock.h

http://codereview.chromium.org/6259008/diff/62001/chrome/browser/pdf_unsuppor...
File chrome/browser/pdf_unsupported_feature.cc (right):

http://codereview.chromium.org/6259008/diff/62001/chrome/browser/pdf_unsuppor...
chrome/browser/pdf_unsupported_feature.cc:30: #define kMinReaderVersionToUse 10
Can this be a static const int?

http://codereview.chromium.org/6259008/diff/62001/chrome/browser/pdf_unsuppor...
chrome/browser/pdf_unsupported_feature.cc:57: TabContents* tab,
Style: indent these 4 lines 2 more spaces.

http://codereview.chromium.org/6259008/diff/62001/chrome/browser/pdf_unsuppor...
chrome/browser/pdf_unsupported_feature.cc:120:
PDFUnsupportedFeatureConfirmInfoBarDelegate(
Can you add a comment here that reader_group may be NULL if there's no reader
installed?

http://codereview.chromium.org/6259008/diff/62001/chrome/browser/pdf_unsuppor...
chrome/browser/pdf_unsupported_feature.cc:207: #if !defined(OS_WIN)
I would have expected this entire function contents to be in an
  #if defined(OS_WIN)
block rather than have an early return on other platforms with dead code below
it.

http://codereview.chromium.org/6259008/diff/62001/chrome/browser/pdf_unsuppor...
chrome/browser/pdf_unsupported_feature.cc:209: // externall since Adobe Reader
doesn't work inside Chrome.
spelling: externally

http://codereview.chromium.org/6259008/diff/62001/chrome/browser/pdf_unsuppor...
chrome/browser/pdf_unsupported_feature.cc:230: tab, reader_group));
Indent 2 more spaces.

http://codereview.chromium.org/6259008/diff/62001/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/6259008/diff/62001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:253: // PDF.  If we're in an
iframe, then don't do anything.
Can you remove the reference to PDF in this comment?

[jam, 2011-01-21 22:32:37]

Thanks, all done except ifdefing out the code except for Windows.  This will
be used on Mac in the future, so I want to avoid bitrot on that platform in
the meantime.

On Fri, Jan 21, 2011 at 1:42 PM, <brettw@chromium.org> wrote:

> LGTM
>
>
>
>
http://codereview.chromium.org/6259008/diff/24001/chrome/browser/plugin_servi...
> File chrome/browser/plugin_service.h (right):
>
>
>
http://codereview.chromium.org/6259008/diff/24001/chrome/browser/plugin_servi...
> chrome/browser/plugin_service.h:17: #include "base/lock.h"
> The new name is base/synchronization/lock.h
>
>
>
http://codereview.chromium.org/6259008/diff/62001/chrome/browser/pdf_unsuppor...
> File chrome/browser/pdf_unsupported_feature.cc (right):
>
>
>
http://codereview.chromium.org/6259008/diff/62001/chrome/browser/pdf_unsuppor...
> chrome/browser/pdf_unsupported_feature.cc:30: #define
> kMinReaderVersionToUse 10
> Can this be a static const int?
>
>
>
http://codereview.chromium.org/6259008/diff/62001/chrome/browser/pdf_unsuppor...
> chrome/browser/pdf_unsupported_feature.cc:57: TabContents* tab,
> Style: indent these 4 lines 2 more spaces.
>
>
>
http://codereview.chromium.org/6259008/diff/62001/chrome/browser/pdf_unsuppor...
> chrome/browser/pdf_unsupported_feature.cc:120:
> PDFUnsupportedFeatureConfirmInfoBarDelegate(
> Can you add a comment here that reader_group may be NULL if there's no
> reader installed?
>
>
>
http://codereview.chromium.org/6259008/diff/62001/chrome/browser/pdf_unsuppor...
> chrome/browser/pdf_unsupported_feature.cc:207: #if !defined(OS_WIN)
> I would have expected this entire function contents to be in an
>  #if defined(OS_WIN)
> block rather than have an early return on other platforms with dead code
> below it.
>
>
>
http://codereview.chromium.org/6259008/diff/62001/chrome/browser/pdf_unsuppor...
> chrome/browser/pdf_unsupported_feature.cc:209: // externall since Adobe
> Reader doesn't work inside Chrome.
> spelling: externally
>
>
>
http://codereview.chromium.org/6259008/diff/62001/chrome/browser/pdf_unsuppor...
> chrome/browser/pdf_unsupported_feature.cc:230: tab, reader_group));
> Indent 2 more spaces.
>
>
>
http://codereview.chromium.org/6259008/diff/62001/webkit/plugins/ppapi/ppapi_...
> File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):
>
>
>
http://codereview.chromium.org/6259008/diff/62001/webkit/plugins/ppapi/ppapi_...
> webkit/plugins/ppapi/ppapi_plugin_instance.cc:253: // PDF.  If we're in
> an iframe, then don't do anything.
> Can you remove the reference to PDF in this comment?
>
>
> http://codereview.chromium.org/6259008/
>

[brettw, 2011-01-21 22:35:34]

On Fri, Jan 21, 2011 at 2:32 PM, John Abd-El-Malek <jam@chromium.org> wrote:
> Thanks, all done except ifdefing out the code except for Windows.  This will
> be used on Mac in the future, so I want to avoid bitrot on that platform in
> the meantime.

Okay.

Brett