If a user chooses to open a PDF with Reader, ask them if they want to do so a...

If a user chooses to open a PDF with Reader, ask them if they want to do so always.

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

[Chris Evans, 2011-01-26 23:31:30]

Ugh -- in terms of the "always do this" applying globally and not just for the
PDF in question. Need to think about that some more.

In the meantime, here are some review comments.

http://codereview.chromium.org/6278017/diff/4001/chrome/browser/pdf_unsupport...
File chrome/browser/pdf_unsupported_feature.cc (right):

http://codereview.chromium.org/6278017/diff/4001/chrome/browser/pdf_unsupport...
chrome/browser/pdf_unsupported_feature.cc:45: "PDF_EnableReaderInfoBarShown"));
Will this style of linebreak match the regex in extract_actions.py ? (I just
learned about this from Bernhard). Perhaps
RecordAction\nUserMetricsAction("blah")) is preferred?

http://codereview.chromium.org/6278017/diff/4001/chrome/browser/pdf_unsupport...
chrome/browser/pdf_unsupported_feature.cc:49: virtual void InfoBarClosed() {
Do you want to UMA the act of closing this bar?

http://codereview.chromium.org/6278017/diff/4001/chrome/browser/pdf_unsupport...
chrome/browser/pdf_unsupported_feature.cc:51: }
Style: newline here?

http://codereview.chromium.org/6278017/diff/4001/chrome/browser/pdf_unsupport...
chrome/browser/pdf_unsupported_feature.cc:52: virtual Type GetInfoBarType() {
I think you're missing a const -- wrong overload?

http://codereview.chromium.org/6278017/diff/4001/chrome/browser/pdf_unsupport...
chrome/browser/pdf_unsupported_feature.cc:88:
IDS_PDF_INFOBAR_QUESTION_ALWAYS_USE_READER);
This does not seem be to be defined in this CL?
We should also check the message; I don't want it to be too easy for a user
click to semi-permanently disable our faster and more secure built-in reader.

http://codereview.chromium.org/6278017/diff/4001/chrome/browser/pdf_unsupport...
chrome/browser/pdf_unsupported_feature.cc:122: old_delegate, new
PDFEnableAdobeReaderConfirmInfoBarDelegate(tab));
Do the common construction of that object once, outside the if?

http://codereview.chromium.org/6278017/diff/4001/chrome/browser/pdf_unsupport...
chrome/browser/pdf_unsupported_feature.cc:314: return;
Is there a command line flag for disabling this? I expect there will be some
security-conscious enterprises who wish to avoid offering to install (or bounce
out to) Reader all the time.

[jam, 2011-01-26 23:45:57]

http://codereview.chromium.org/6278017/diff/4001/chrome/browser/pdf_unsupport...
File chrome/browser/pdf_unsupported_feature.cc (right):

http://codereview.chromium.org/6278017/diff/4001/chrome/browser/pdf_unsupport...
chrome/browser/pdf_unsupported_feature.cc:45: "PDF_EnableReaderInfoBarShown"));
On 2011/01/26 23:31:30, Chris Evans wrote:
> Will this style of linebreak match the regex in extract_actions.py ? (I just
> learned about this from Bernhard). Perhaps
> RecordAction\nUserMetricsAction("blah")) is preferred?

ah, good point. i  forgot to run the script to verify this. fixed.

http://codereview.chromium.org/6278017/diff/4001/chrome/browser/pdf_unsupport...
chrome/browser/pdf_unsupported_feature.cc:49: virtual void InfoBarClosed() {
On 2011/01/26 23:31:30, Chris Evans wrote:
> Do you want to UMA the act of closing this bar?

that's done in virtual bool Cancel() {

http://codereview.chromium.org/6278017/diff/4001/chrome/browser/pdf_unsupport...
chrome/browser/pdf_unsupported_feature.cc:51: }
On 2011/01/26 23:31:30, Chris Evans wrote:
> Style: newline here?

Done.

http://codereview.chromium.org/6278017/diff/4001/chrome/browser/pdf_unsupport...
chrome/browser/pdf_unsupported_feature.cc:52: virtual Type GetInfoBarType() {
On 2011/01/26 23:31:30, Chris Evans wrote:
> I think you're missing a const -- wrong overload?

good catch, done

http://codereview.chromium.org/6278017/diff/4001/chrome/browser/pdf_unsupport...
chrome/browser/pdf_unsupported_feature.cc:88:
IDS_PDF_INFOBAR_QUESTION_ALWAYS_USE_READER);
On 2011/01/26 23:31:30, Chris Evans wrote:
> This does not seem be to be defined in this CL?

it was checked in earlier.

> We should also check the message; I don't want it to be too easy for a user
> click to semi-permanently disable our faster and more secure built-in reader.

we normally don't check this, because if the strings are out of sync with the
code, chrome is pretty hosed at this point.

http://codereview.chromium.org/6278017/diff/4001/chrome/browser/pdf_unsupport...
chrome/browser/pdf_unsupported_feature.cc:122: old_delegate, new
PDFEnableAdobeReaderConfirmInfoBarDelegate(tab));
On 2011/01/26 23:31:30, Chris Evans wrote:
> Do the common construction of that object once, outside the if?

Done.

http://codereview.chromium.org/6278017/diff/4001/chrome/browser/pdf_unsupport...
chrome/browser/pdf_unsupported_feature.cc:314: return;
On 2011/01/26 23:31:30, Chris Evans wrote:
> Is there a command line flag for disabling this? I expect there will be some
> security-conscious enterprises who wish to avoid offering to install (or
bounce
> out to) Reader all the time.

this code is doing exactly what you're asking for :)  if group policy disables
reader, we don't offer this.

[Chris Evans, 2011-01-27 00:03:27]

Round two.. almost there

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

http://codereview.chromium.org/6278017/diff/11001/chrome/browser/pdf_unsuppor...
chrome/browser/pdf_unsupported_feature.cc:49: virtual void InfoBarClosed() {
I still think this is a different user action (user presses X on right) than
Cancel() (user presses one of the buttons) -- so maybe record a UMA here? It's
how Bernhard does it in the out-of-date-blocked-plugin infobar...

http://codereview.chromium.org/6278017/diff/11001/chrome/browser/pdf_unsuppor...
chrome/browser/pdf_unsupported_feature.cc:59:
UserMetricsAction("PDF_EnableReaderInfoBarOK"));
I'm not sure which way around the buttons are. Can you either make a change, or
confirm that the leftmost button is the one that leaves the current built-in PDF
preference intact?

http://codereview.chromium.org/6278017/diff/11001/chrome/browser/pdf_unsuppor...
chrome/browser/pdf_unsupported_feature.cc:90: }
Newline

http://codereview.chromium.org/6278017/diff/11001/chrome/browser/pdf_unsuppor...
chrome/browser/pdf_unsupported_feature.cc:93:
IDS_PDF_INFOBAR_QUESTION_ALWAYS_USE_READER);
"Always use Adobe Reader to open PDF files?" -- as per above, the default must
be no, please. Also, can we at least think about indicating:
1) How the user would undo this (about:plugins)
2) Gently recommend against it on account of either the speed or security angle?

http://codereview.chromium.org/6278017/diff/11001/chrome/browser/pdf_unsuppor...
chrome/browser/pdf_unsupported_feature.cc:120: plugin.plugin.version =
ASCIIToUTF16("11.0.0.0");
One day, that will be a vulnerable version in the 11 series. Should we use
50.0.0.0 or similar to avoid random breakage 2 years down the line?

http://codereview.chromium.org/6278017/diff/11001/chrome/browser/pdf_unsuppor...
chrome/browser/pdf_unsupported_feature.cc:245: virtual bool Accept() {
As per the other infobar, please can we make sure that the leftmost button is
effectively "no"? For the majority of PDFs where this fires, the user is likely
seeing a well-rendered PDF that gives them what they want already, with no
particular need to bounce to Reader.

http://codereview.chromium.org/6278017/diff/11001/chrome/browser/pdf_unsuppor...
chrome/browser/pdf_unsupported_feature.cc:325: return;
I guess what I was hoping for is some command line, i.e.
--no-offer-adobe-reader-for-pdf or something equivalently easy?

[jam, 2011-01-27 00:31:02]

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

http://codereview.chromium.org/6278017/diff/11001/chrome/browser/pdf_unsuppor...
chrome/browser/pdf_unsupported_feature.cc:49: virtual void InfoBarClosed() {
On 2011/01/27 00:03:27, Chris Evans wrote:
> I still think this is a different user action (user presses X on right) than
> Cancel() (user presses one of the buttons) -- so maybe record a UMA here? It's
> how Bernhard does it in the out-of-date-blocked-plugin infobar...

ah, I thought Cancel() would get closed in that case.  turns out it doesn't.

I override InfoBarDismissed() now InfoBarClosed would get called in other
scenarios, i.e. clicking on Yes.

http://codereview.chromium.org/6278017/diff/11001/chrome/browser/pdf_unsuppor...
chrome/browser/pdf_unsupported_feature.cc:59:
UserMetricsAction("PDF_EnableReaderInfoBarOK"));
On 2011/01/27 00:03:27, Chris Evans wrote:
> I'm not sure which way around the buttons are.

Yes No X

> Can you either make a change, or
> confirm that the leftmost button is the one that leaves the current built-in
PDF
> preference intact?

This is done per what was agreed on in the threads and spec describing this.  If
you want it changed, the best place is to reply to that thread.

http://codereview.chromium.org/6278017/diff/11001/chrome/browser/pdf_unsuppor...
chrome/browser/pdf_unsupported_feature.cc:90: }
On 2011/01/27 00:03:27, Chris Evans wrote:
> Newline

Done.

http://codereview.chromium.org/6278017/diff/11001/chrome/browser/pdf_unsuppor...
chrome/browser/pdf_unsupported_feature.cc:93:
IDS_PDF_INFOBAR_QUESTION_ALWAYS_USE_READER);
On 2011/01/27 00:03:27, Chris Evans wrote:
> "Always use Adobe Reader to open PDF files?" -- as per above, the default must
> be no, please. Also, can we at least think about indicating:
> 1) How the user would undo this (about:plugins)
> 2) Gently recommend against it on account of either the speed or security
angle?

Sorry I don't think this is the place to change the UI.  You should have been on
the original thread, so best to discuss changes there.

http://codereview.chromium.org/6278017/diff/11001/chrome/browser/pdf_unsuppor...
chrome/browser/pdf_unsupported_feature.cc:120: plugin.plugin.version =
ASCIIToUTF16("11.0.0.0");
On 2011/01/27 00:03:27, Chris Evans wrote:
> One day, that will be a vulnerable version in the 11 series. Should we use
> 50.0.0.0 or similar to avoid random breakage 2 years down the line?

Once that happens, we'll know pretty quickly (i.e. blocked plugin will show up
and we'll see it).

http://codereview.chromium.org/6278017/diff/11001/chrome/browser/pdf_unsuppor...
chrome/browser/pdf_unsupported_feature.cc:245: virtual bool Accept() {
On 2011/01/27 00:03:27, Chris Evans wrote:
> As per the other infobar, please can we make sure that the leftmost button is
> effectively "no"? For the majority of PDFs where this fires, the user is
likely
> seeing a well-rendered PDF that gives them what they want already, with no
> particular need to bounce to Reader.
ditto

http://codereview.chromium.org/6278017/diff/11001/chrome/browser/pdf_unsuppor...
chrome/browser/pdf_unsupported_feature.cc:325: return;
On 2011/01/27 00:03:27, Chris Evans wrote:
> I guess what I was hoping for is some command line, i.e.
> --no-offer-adobe-reader-for-pdf or something equivalently easy?

I guess I don't see the scenario where a company will want to block people from
being offered to use Reader, but will still allow them to manually set it in
about:plugins.  If a company is that paranoid that an edge case (unsupported
feature) can allow a user to enable Reader, they'll most likely have already
disabled it via group policy.

[Chris Evans, 2011-01-27 01:11:06]

On 2011/01/26 23:11:44, John Abd-El-Malek wrote:

LGTM now thanks!