Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6982030:
Print Preview: Detecting plugin existence before generating the preview. (Closed)
|
Created:
9 years, 7 months ago by dpapad Modified:
9 years, 7 months ago CC:
chromium-reviews, arv (Not doing code reviews) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPrint Preview: Detecting plugin existence before generating the preview.
In case there is no compatible pdf plugin the user is immediately notified
that preview is not available instead of waiting for the preview to be
generated and then being notified.
BUG=82202
TEST=Load a multi-page doc in print preview, observe the error messages.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=85215
Patch Set 1 #Patch Set 2 : Minor fix #
Total comments: 4
Patch Set 3 : Addressing comments #
Total comments: 8
Patch Set 4 : Removing unnecessary checks #Patch Set 5 : Addressing comments #
Total comments: 8
Patch Set 6 : Removing pdf string completely #
Total comments: 2
Patch Set 7 : Removing dummy plugin from the DOM #
Total comments: 2
Patch Set 8 : Fixing nit #
Total comments: 4
Patch Set 9 : Removing unnecessary else if statement #
Messages
Total messages: 22 (0 generated)
http://codereview.chromium.org/6982030/diff/1001/chrome/browser/resources/pri... File chrome/browser/resources/print_preview.html (right): http://codereview.chromium.org/6982030/diff/1001/chrome/browser/resources/pri... chrome/browser/resources/print_preview.html:126: src="chrome://print/dummy.pdf"/> We can probably do better than dummy.pdf. Maybe empty_page.pdf or blank.pdf ? http://codereview.chromium.org/6982030/diff/1001/chrome/browser/ui/webui/prin... File chrome/browser/ui/webui/print_preview_ui_html_source.cc (right): http://codereview.chromium.org/6982030/diff/1001/chrome/browser/ui/webui/prin... chrome/browser/ui/webui/print_preview_ui_html_source.cc:165: std::string dummy_string("%PDF-1.4\n4 0 obj\n<</Type /Catalog /Pages 1 0 R" I squished this down a bit more (to 316 bytes): std::string empty_pdf("%PDF-1.4\n3 0 obj<</Type/Catalog/Pages 1 0 R>>" "endobj 2 0 obj<</Type/Page/Parent 1 0 R/Resources" "<<>>/MediaBox[0 0 612 792]>>endobj 1 0 obj<</Type" "/Pages/Kids[2 0 R]/Count 1>>endobj\nxref\n0 4" "0000000000 65535 f \n0000000129 00000 n \n" "0000000052 00000 n \n0000000009 00000 n \ntrailer<<" "/Size 4/Root 3 0 R>>startxref\n178\n%%EOF\n");
http://codereview.chromium.org/6982030/diff/1001/chrome/browser/resources/pri... File chrome/browser/resources/print_preview.html (right): http://codereview.chromium.org/6982030/diff/1001/chrome/browser/resources/pri... chrome/browser/resources/print_preview.html:126: src="chrome://print/dummy.pdf"/> On 2011/05/11 18:00:05, vandebo wrote: > We can probably do better than dummy.pdf. Maybe empty_page.pdf or blank.pdf ? Done. http://codereview.chromium.org/6982030/diff/1001/chrome/browser/ui/webui/prin... File chrome/browser/ui/webui/print_preview_ui_html_source.cc (right): http://codereview.chromium.org/6982030/diff/1001/chrome/browser/ui/webui/prin... chrome/browser/ui/webui/print_preview_ui_html_source.cc:165: std::string dummy_string("%PDF-1.4\n4 0 obj\n<</Type /Catalog /Pages 1 0 R" On 2011/05/11 18:00:05, vandebo wrote: > I squished this down a bit more (to 316 bytes): > > std::string empty_pdf("%PDF-1.4\n3 0 obj<</Type/Catalog/Pages 1 0 R>>" > "endobj 2 0 obj<</Type/Page/Parent 1 0 R/Resources" > "<<>>/MediaBox[0 0 612 792]>>endobj 1 0 obj<</Type" > "/Pages/Kids[2 0 R]/Count 1>>endobj\nxref\n0 4" > "0000000000 65535 f \n0000000129 00000 n \n" > "0000000052 00000 n \n0000000009 00000 n \ntrailer<<" > "/Size 4/Root 3 0 R>>startxref\n178\n%%EOF\n"); Done.
http://codereview.chromium.org/6982030/diff/4001/chrome/browser/ui/webui/prin... File chrome/browser/ui/webui/print_preview_ui_html_source.cc (right): http://codereview.chromium.org/6982030/diff/4001/chrome/browser/ui/webui/prin... chrome/browser/ui/webui/print_preview_ui_html_source.cc:164: scoped_refptr<RefCountedBytes> dummy_bytes(new RefCountedBytes); dummy_bytes != blank_pdf
http://codereview.chromium.org/6982030/diff/4001/chrome/browser/ui/webui/prin... File chrome/browser/ui/webui/print_preview_ui_html_source.cc (right): http://codereview.chromium.org/6982030/diff/4001/chrome/browser/ui/webui/prin... chrome/browser/ui/webui/print_preview_ui_html_source.cc:164: scoped_refptr<RefCountedBytes> dummy_bytes(new RefCountedBytes); On 2011/05/11 18:37:18, vandebo wrote: > dummy_bytes != blank_pdf Nevermind, I read it wrong. http://codereview.chromium.org/6982030/diff/4001/chrome/browser/ui/webui/prin... chrome/browser/ui/webui/print_preview_ui_html_source.cc:165: std::string blank_pdf("%PDF-1.4\n3 0 obj<</Type/Catalog/Pages 1 0 R>>" Should this be a constant? const char kBlankPdf[] = Then I think you could use RefCountedStaticMemory and it's constructor.
http://codereview.chromium.org/6982030/diff/4001/chrome/browser/resources/pri... File chrome/browser/resources/print_preview.html (right): http://codereview.chromium.org/6982030/diff/4001/chrome/browser/resources/pri... chrome/browser/resources/print_preview.html:125: <object id="dummy-viewer" type="application/pdf" does this need to be hidden? http://codereview.chromium.org/6982030/diff/4001/chrome/browser/resources/pri... File chrome/browser/resources/print_preview.js (right): http://codereview.chromium.org/6982030/diff/4001/chrome/browser/resources/pri... chrome/browser/resources/print_preview.js:503: if (!pdfPlugin.onload) { you can probably remove this code here.
http://codereview.chromium.org/6982030/diff/4001/chrome/browser/resources/pri... File chrome/browser/resources/print_preview.html (right): http://codereview.chromium.org/6982030/diff/4001/chrome/browser/resources/pri... chrome/browser/resources/print_preview.html:125: <object id="dummy-viewer" type="application/pdf" On 2011/05/11 20:27:43, Lei Zhang wrote: > does this need to be hidden? I set it to hidden in the onLoad() method. Otherwise it shows up in the screen. http://codereview.chromium.org/6982030/diff/4001/chrome/browser/resources/pri... File chrome/browser/resources/print_preview.js (right): http://codereview.chromium.org/6982030/diff/4001/chrome/browser/resources/pri... chrome/browser/resources/print_preview.js:503: if (!pdfPlugin.onload) { On 2011/05/11 20:27:43, Lei Zhang wrote: > you can probably remove this code here. Done. Also removed variable hasCompatiblePDFPlugin. http://codereview.chromium.org/6982030/diff/4001/chrome/browser/ui/webui/prin... File chrome/browser/ui/webui/print_preview_ui_html_source.cc (right): http://codereview.chromium.org/6982030/diff/4001/chrome/browser/ui/webui/prin... chrome/browser/ui/webui/print_preview_ui_html_source.cc:165: std::string blank_pdf("%PDF-1.4\n3 0 obj<</Type/Catalog/Pages 1 0 R>>" On 2011/05/11 19:04:55, vandebo wrote: > Should this be a constant? const char kBlankPdf[] = > Then I think you could use RefCountedStaticMemory and it's constructor. Done.
http://codereview.chromium.org/6982030/diff/8011/chrome/browser/resources/pri... File chrome/browser/resources/print_preview.js (right): http://codereview.chromium.org/6982030/diff/8011/chrome/browser/resources/pri... chrome/browser/resources/print_preview.js:55: Should we remove the dummy-viewer is some way so that we aren't running two instances of the plugin? http://codereview.chromium.org/6982030/diff/8011/chrome/browser/ui/webui/prin... File chrome/browser/ui/webui/print_preview_ui_html_source.cc (right): http://codereview.chromium.org/6982030/diff/8011/chrome/browser/ui/webui/prin... chrome/browser/ui/webui/print_preview_ui_html_source.cc:164: const unsigned char blank_pdf[] = "%PDF-1.4\n3 0 obj<</Type/Catalog/Pages" kBlankPDF All the start quotes should line up. http://codereview.chromium.org/6982030/diff/8011/chrome/browser/ui/webui/prin... chrome/browser/ui/webui/print_preview_ui_html_source.cc:171: scoped_refptr<RefCountedStaticMemory> dummy_bytes( dummy_bytes -> blank_bytes.
Otherwise LGTM with vandebo's LGTM. http://codereview.chromium.org/6982030/diff/8011/chrome/browser/resources/pri... File chrome/browser/resources/print_preview.js (right): http://codereview.chromium.org/6982030/diff/8011/chrome/browser/resources/pri... chrome/browser/resources/print_preview.js:51: if(!checkCompatiblePluginExists()) { nit: space after if
http://codereview.chromium.org/6982030/diff/8011/chrome/browser/resources/pri... File chrome/browser/resources/print_preview.js (right): http://codereview.chromium.org/6982030/diff/8011/chrome/browser/resources/pri... chrome/browser/resources/print_preview.js:51: if(!checkCompatiblePluginExists()) { On 2011/05/11 22:19:51, Lei Zhang wrote: > nit: space after if Done. http://codereview.chromium.org/6982030/diff/8011/chrome/browser/resources/pri... chrome/browser/resources/print_preview.js:55: On 2011/05/11 22:13:54, vandebo wrote: > Should we remove the dummy-viewer is some way so that we aren't running two > instances of the plugin? The plugin is removed from the page on line 49. If there is a better way to remove it sure, but I dont have something in mind. http://codereview.chromium.org/6982030/diff/8011/chrome/browser/ui/webui/prin... File chrome/browser/ui/webui/print_preview_ui_html_source.cc (right): http://codereview.chromium.org/6982030/diff/8011/chrome/browser/ui/webui/prin... chrome/browser/ui/webui/print_preview_ui_html_source.cc:164: const unsigned char blank_pdf[] = "%PDF-1.4\n3 0 obj<</Type/Catalog/Pages" On 2011/05/11 22:13:54, vandebo wrote: > kBlankPDF > All the start quotes should line up. It seems like there is no need to pass actual pdf data. Passing no data works just as well. http://codereview.chromium.org/6982030/diff/8011/chrome/browser/ui/webui/prin... chrome/browser/ui/webui/print_preview_ui_html_source.cc:171: scoped_refptr<RefCountedStaticMemory> dummy_bytes( On 2011/05/11 22:13:54, vandebo wrote: > dummy_bytes -> blank_bytes. Done.
http://codereview.chromium.org/6982030/diff/8013/chrome/browser/resources/pri... File chrome/browser/resources/print_preview.html (right): http://codereview.chromium.org/6982030/diff/8013/chrome/browser/resources/pri... chrome/browser/resources/print_preview.html:126: src="chrome://print/blank.pdf"/> nit: this is no longer a blank pdf, but a dummy pdf as you had before.
http://codereview.chromium.org/6982030/diff/8013/chrome/browser/resources/pri... File chrome/browser/resources/print_preview.html (right): http://codereview.chromium.org/6982030/diff/8013/chrome/browser/resources/pri... chrome/browser/resources/print_preview.html:126: src="chrome://print/blank.pdf"/> On 2011/05/11 23:38:19, vandebo wrote: > nit: this is no longer a blank pdf, but a dummy pdf as you had before. Done.
LGTM http://codereview.chromium.org/6982030/diff/6005/chrome/browser/ui/webui/prin... File chrome/browser/ui/webui/print_preview_ui_html_source.cc (right): http://codereview.chromium.org/6982030/diff/6005/chrome/browser/ui/webui/prin... chrome/browser/ui/webui/print_preview_ui_html_source.cc:164: scoped_refptr<RefCountedStaticMemory> blank_bytes( nit: blank_bytes -> dummy_bytes
http://codereview.chromium.org/6982030/diff/6005/chrome/browser/ui/webui/prin... File chrome/browser/ui/webui/print_preview_ui_html_source.cc (right): http://codereview.chromium.org/6982030/diff/6005/chrome/browser/ui/webui/prin... chrome/browser/ui/webui/print_preview_ui_html_source.cc:164: scoped_refptr<RefCountedStaticMemory> blank_bytes( On 2011/05/12 00:07:14, vandebo wrote: > nit: blank_bytes -> dummy_bytes Done.
http://codereview.chromium.org/6982030/diff/8014/chrome/browser/ui/webui/prin... File chrome/browser/ui/webui/print_preview_ui_html_source.cc (right): http://codereview.chromium.org/6982030/diff/8014/chrome/browser/ui/webui/prin... chrome/browser/ui/webui/print_preview_ui_html_source.cc:163: } else if (path == "dummy.pdf") { It occurs to me that since the plugin doesn't need a valid PDF, we don't even need this code. The error page would be returned instead.
http://codereview.chromium.org/6982030/diff/8014/chrome/browser/ui/webui/prin... File chrome/browser/ui/webui/print_preview_ui_html_source.cc (right): http://codereview.chromium.org/6982030/diff/8014/chrome/browser/ui/webui/prin... chrome/browser/ui/webui/print_preview_ui_html_source.cc:163: } else if (path == "dummy.pdf") { On 2011/05/12 02:10:12, vandebo wrote: > It occurs to me that since the plugin doesn't need a valid PDF, we don't even > need this code. The error page would be returned instead. I would double check and make sure that doesn't crash the PDF viewer.
http://codereview.chromium.org/6982030/diff/8014/chrome/browser/ui/webui/prin... File chrome/browser/ui/webui/print_preview_ui_html_source.cc (right): http://codereview.chromium.org/6982030/diff/8014/chrome/browser/ui/webui/prin... chrome/browser/ui/webui/print_preview_ui_html_source.cc:163: } else if (path == "dummy.pdf") { On 2011/05/12 02:19:34, Lei Zhang wrote: > On 2011/05/12 02:10:12, vandebo wrote: > > It occurs to me that since the plugin doesn't need a valid PDF, we don't even > > need this code. The error page would be returned instead. > > I would double check and make sure that doesn't crash the PDF viewer. Removed the "else if". As far as detecting the existence of the plugin it works. Do we care if the dummy-plugin crashes? It is removed from the DOM tree right after checking if it is compatible.
http://codereview.chromium.org/6982030/diff/8014/chrome/browser/ui/webui/prin... File chrome/browser/ui/webui/print_preview_ui_html_source.cc (right): http://codereview.chromium.org/6982030/diff/8014/chrome/browser/ui/webui/prin... chrome/browser/ui/webui/print_preview_ui_html_source.cc:163: } else if (path == "dummy.pdf") { On 2011/05/12 17:26:44, dpapad wrote: > On 2011/05/12 02:19:34, Lei Zhang wrote: > > On 2011/05/12 02:10:12, vandebo wrote: > > > It occurs to me that since the plugin doesn't need a valid PDF, we don't > even > > > need this code. The error page would be returned instead. > > > > I would double check and make sure that doesn't crash the PDF viewer. > > Removed the "else if". As far as detecting the existence of the plugin it works. > Do we care if the dummy-plugin crashes? It is removed from the DOM tree right > after checking if it is compatible. Well, if it crashes, then it is taking down the renderer with it, so we do care. But it sounds like you tested it and that's not happening.
On 2011/05/12 17:50:45, Lei Zhang wrote: > http://codereview.chromium.org/6982030/diff/8014/chrome/browser/ui/webui/prin... > File chrome/browser/ui/webui/print_preview_ui_html_source.cc (right): > > http://codereview.chromium.org/6982030/diff/8014/chrome/browser/ui/webui/prin... > chrome/browser/ui/webui/print_preview_ui_html_source.cc:163: } else if (path == > "dummy.pdf") { > On 2011/05/12 17:26:44, dpapad wrote: > > On 2011/05/12 02:19:34, Lei Zhang wrote: > > > On 2011/05/12 02:10:12, vandebo wrote: > > > > It occurs to me that since the plugin doesn't need a valid PDF, we don't > > even > > > > need this code. The error page would be returned instead. > > > > > > I would double check and make sure that doesn't crash the PDF viewer. > > > > Removed the "else if". As far as detecting the existence of the plugin it > works. > > Do we care if the dummy-plugin crashes? It is removed from the DOM tree right > > after checking if it is compatible. > > Well, if it crashes, then it is taking down the renderer with it, so we do care. > But it sounds like you tested it and that's not happening. Yes the renderer did not crash. You can test it also by navigating chrome://print/foo.pdf (ToT, no need to apply my patch). A grey page is shown but no crash.
Only js left, but LGTM
LGTM 2
Change committed as 85215 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
