Print preview: Display a message when the PDF viewer is missing....

Print preview: Display a message when the PDF viewer is missing.

BUG=none
TEST=none

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

[Lei Zhang, 2010-12-06 08:31:55]

http://codereview.chromium.org/5574003/diff/1/chrome/browser/resources/print_...
File chrome/browser/resources/print_preview.js (right):

http://codereview.chromium.org/5574003/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview.js:52: $('pdf-viewer').reload();
Typo in previous CL.

http://codereview.chromium.org/5574003/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview.js:60: var pdfPlugin =
document.createElement('object');
Typo in previous CL.

http://codereview.chromium.org/5574003/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview.js:65: if (typeof pdfPlugin.onload !=
'function') {
This check does not work until we add the plugin to the DOM.

[James Hawkins, 2010-12-06 18:23:36]

http://codereview.chromium.org/5574003/diff/1/chrome/app/generated_resources.grd
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/5574003/diff/1/chrome/app/generated_resources....
chrome/app/generated_resources.grd:5496: PDF viewer not available.
Is this string from the mocks?

http://codereview.chromium.org/5574003/diff/1/chrome/browser/resources/print_...
File chrome/browser/resources/print_preview.js (right):

http://codereview.chromium.org/5574003/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview.js:65: if (typeof pdfPlugin.onload !=
'function') {
On 2010/12/06 08:31:55, Lei Zhang wrote:
> This check does not work until we add the plugin to the DOM.

You should just be able to check if (pdfPlugin.onload), assuming the problem is
the onload method doesn't exist yet.

http://codereview.chromium.org/5574003/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview.js:68:
$('no-plugin').classList.remove('hidden');
You don't ever set 'hidden' for 'no-plugin', so there's nothing to remove.

[Lei Zhang, 2010-12-06 20:47:14]

http://codereview.chromium.org/5574003/diff/1/chrome/app/generated_resources.grd
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/5574003/diff/1/chrome/app/generated_resources....
chrome/app/generated_resources.grd:5496: PDF viewer not available.
On 2010/12/06 18:23:36, James Hawkins wrote:
> Is this string from the mocks?

No, but we should deal with the case for users without the plugin.

http://codereview.chromium.org/5574003/diff/1/chrome/browser/resources/print_...
File chrome/browser/resources/print_preview.js (right):

http://codereview.chromium.org/5574003/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview.js:65: if (typeof pdfPlugin.onload !=
'function') {
On 2010/12/06 18:23:36, James Hawkins wrote:
> On 2010/12/06 08:31:55, Lei Zhang wrote:
> > This check does not work until we add the plugin to the DOM.
> 
> You should just be able to check if (pdfPlugin.onload), assuming the problem
is
> the onload method doesn't exist yet.

Done, but this still needs to happen after the plugin has been added to the DOM.

http://codereview.chromium.org/5574003/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview.js:68:
$('no-plugin').classList.remove('hidden');
On 2010/12/06 18:23:36, James Hawkins wrote:
> You don't ever set 'hidden' for 'no-plugin', so there's nothing to remove.

Whoops, added class="hidden" in the html file.

[James Hawkins, 2010-12-06 20:50:53]

LGTM

http://codereview.chromium.org/5574003/diff/1/chrome/app/generated_resources.grd
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/5574003/diff/1/chrome/app/generated_resources....
chrome/app/generated_resources.grd:5496: PDF viewer not available.
On 2010/12/06 20:47:14, Lei Zhang wrote:
> On 2010/12/06 18:23:36, James Hawkins wrote:
> > Is this string from the mocks?
> 
> No, but we should deal with the case for users without the plugin.

I'm fine with it for now, but I think we should ask UX for a better string (or
add an 'is' in there).

http://codereview.chromium.org/5574003/diff/6001/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview.html (right):

http://codereview.chromium.org/5574003/diff/6001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview.html:79: <p class="hidden" id="no-plugin"
i18n-content="noPlugin"></p>
id before class.