Closed Bug 198515 Opened 22 years ago Closed 21 years ago

When print a plugin, browser give the wrong file pointer to plugin.

Categories

(Core :: Layout: Images, Video, and HTML Frames, defect)

Product:
Core
Component:
Layout: Images, Video, and HTML Frames
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.5beta

People

(Reporter: leon.sha, Assigned: roland.mainz)

Details

Attachments

(3 files, 13 obsolete files)

Final patch for checkin...
44.57 KB, patch
Details | Diff | Splinter Review
Gzipp'ed PostScript job printing http://www.nichtlustig.de/suche.php?input=yeti&submit.x=0 using Moz 2003-07-09-08-trunk_cvs+patch and Xprint module (Xprint server was the "ancalagon" 009 prototype)
292.13 KB, application/octet-stream
Details
Animated Yeti sample PostScript job (attachment 128318) page 1 of 1 converted to JPEG
60.04 KB, image/jpeg
Details
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3b) Gecko/20030210 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3b) Gecko/20030210 In nsObjectFrame.cpp line 1729-1743, browser should give the ps file pointer to plugin. So plugin can't write the postscript data to the correct file pointer. Reproducible: Always Steps to Reproduce: 1. 2. 3. Actual Results: Plugin can't be printed in Linux/unix Expected Results: Plugin can be printed in Linux/unix
Status: UNCONFIRMED → NEW
Ever confirmed: true
So... when printing directly through lpr, will this work? (as in, do we print to a temp file then?)
>So... when printing directly through lpr, will this work? What's different between using lp and lpr? I think in both way, we should generate the ps file first. >(as in, do we print to a temp file then?) If you print to a file, the file pointer just point to that file. If you print to thr printer directly, the file pointer is just the temp file.
Attachment #117957 - Attachment is obsolete: true
Two files changed. nsObjectFrame.cpp and Makefile.in
Attachment #117968 - Attachment is obsolete: true
In Linux/unix we use postscript to print. The coords is different between postscript(start form left bottom) and npwindow(start from left top). Because plugin don't know the page size, we should adjust npwindow before passed it to plugin. Also there is another solution. The clipRect of npwindow is useless. Maybe we can use this to pass the page size.
Comment on attachment 119028 [details] [diff] [review] Modified NPWindow passed to plugin you can't just require postscript, it isn't always built. you'll either need to use ifdefs or xpcom
Attachment #119028 - Flags: review-
.
Assignee: frame → leon.sha
Attached patch Add ifdef (obsolete) — — Splinter Review
Yes, this patch is only for postscript. I don't know xprint well. Maye it can also provide a temp file pointer. But it will be a problem to plugin developers. How can he/she know what print system mozilla use? What kind of data should he/she write into such a file pointer?
Attachment #119028 - Attachment is obsolete: true
Attachment #125070 - Flags: review?(timeless)
Leon Sha wrote: > Created an attachment (id=125070) > Add ifdef This patch is slightly wrong in multiple ways: - the patch assumes that the passed renderingcontext is a |nsRenderingContextPS|. if this condition is not true mozilla will crash: -- snp -- + nsPostScriptObj* postscriptobj = ((nsRenderingContextPS&)aRenderingContext).GetPostScriptObj(); -- snip -- - Looking at this: -- snip -- NPPrintCallbackStruct npPrintInfo; npPrintInfo.type = NP_PRINT; - npPrintInfo.fp = (FILE*)printfile; + npPrintInfo.fp = postscriptobj->mPrintSetup->tmpBody; -- snip -- Are you sure that this is a good idea ? |postscriptobj->mPrintSetup->tmpBody| is AFAIK a semi-private field in |nsPostScriptObj| and really not be passed to external code which may do unknown things with the file pointer (for example a simple |seek()| in the plugin code may screw-up the whole postscript code since the PS module is not aware of the seek). Better way may be to create a temp. file handle, pass that to the plugin and then copy those data to the print job spool file using a new method in |nsDeviceContextPS|/etc. > Yes, this patch is only for postscript. I don't know xprint well. > Maye it can also provide a temp file pointer. Well, Xprint has two ways to embed PostScript data: 1. Pass a complete, "raw" PostScript job to the printer and 2. Embed a piece of PostScript code within a single page (note that there is a slight problem: Not all Xprint DDX support embedded PostScript code, the PCL3/PDF/XWD/GDI DDX can't support that and the PCL5 driver can only support it when the printer supports it. Figuring out whether embedding PostScript is supported is not an issue - Xprint provides an API for that. But the original problem remains then: What can we do if there is no way to send PostScript code down to the printer ?!) I am not familar with the NS4.x plugin API... does the plugin write a whole, complete postscript job (incl. prolog, header/footer) to the provided file handle or only a postscript fragment (e.g. a "frame") which needs to be surrounded by PostScript prolog+header+footer ? > But it will be a problem to plugin developers. > How can he/she know what print system mozilla use? From the view of the plugin it does not matter whether Mozilla's Xprint module or the PostScript module are being used - both _can_ accept PostScript code. But there is still the problem that this option may not be available - what should we do then (one option would be to send a _image_ (sort of screenshot) to the printer instead. Not a perfect, high-quality way but it would at least print something instead of leaving the plugin area blank... ;-/) ... > What kind of data should he/she write into such a file pointer? See answer above...
Attachment #125070 - Flags: review?(timeless) → review-
Attached patch Patch for plugin print (obsolete) — — Splinter Review
Attachment #125070 - Attachment is obsolete: true
Attachment #125391 - Flags: review?(Roland.Mainz)
Comment on attachment 125391 [details] [diff] [review] Patch for plugin print Removing review request, leon is working on a new patch with full Xprint support right now... :)
Attachment #125391 - Flags: review?(Roland.Mainz)
Attached patch Full support (obsolete) — — Splinter Review
Attachment #125391 - Attachment is obsolete: true
I see some issues in this patch: > + npprint.print.embedPrint.window = window; > + // send off print info to plugin > + rv = pi->Print(&npprint); > #elif defined(XP_UNIX) && !defined(XP_MACOSX) > // UNIX does things completely differently > - PRUnichar *printfile = nsnull; > - if (printSettings) { > - printSettings->GetToFileName(&printfile); > - } > - if (!printfile) > - return NS_OK; // XXX what to do on UNIX when we don't have a PS file???? > - > - NPPrintCallbackStruct npPrintInfo; > - npPrintInfo.type = NP_PRINT; > - npPrintInfo.fp = (FILE*)printfile; > - npprint.print.embedPrint.platformPrint = (void*) &npPrintInfo; > > + > + nsCOMPtr<nsIDeviceContext> deviceContext; > + aRenderingContext.GetDeviceContext(*getter_AddRefs(deviceContext)); > +#ifdef USE_POSTSCRIPT > + nsCOMPtr<nsIDeviceContextPS> deviceContextps = do_QueryInterface(deviceContext); > + if(deviceContextps){ > + nsPostScriptObj* postscriptobj = ((nsRenderingContextPS&)aRenderingContext).GetPostScriptObj(); > + NPPrintCallbackStruct npPrintInfo; > + npPrintInfo.type = NP_PRINT; > + FILE* plugintmpfile = tmpfile(); What about error checking if |tmpfile()| returns |NULL| (for example: process out of file handles etc. etc.) ? > + npPrintInfo.fp = plugintmpfile; > + npprint.print.embedPrint.platformPrint = (void*) &npPrintInfo; > +// window.y = postscriptobj->mPrintSetup->height-window.y-window.height; Uhm, what does that mean ? > + npprint.print.embedPrint.window = window; > + // send off print info to plugin > + fprintf(plugintmpfile,"1 -1 scale\n"); > + fprintf(plugintmpfile,"0 %d translate\n",-(postscriptobj->mPrintSetup->height)); Writing into |plugintmpfile| and then passing that file handle to the plugin code doesn't seem to be a good idea (remember my concern about passing the PS module file handle directly to the plugin code - this is the same problem: We don't know what the plugin does, the worst case is that these data may be overwritten... ;-( ) ... > + rv = pi->Print(&npprint); > + rewind(plugintmpfile); > + int c; > + while((c=getc(plugintmpfile))!=EOF) > + putc(c,postscriptobj->mPrintSetup->tmpBody); In theory this could be made more efficient via using block copies instead of character copies (however the amount of data transferred isn't that big...) ... > + fclose(plugintmpfile); > + } > +#endif > +#ifdef USE_XPRINT > + nsCOMPtr<nsIDeviceContextXp> deviceContextxprint = do_QueryInterface(deviceContext); > + if(deviceContextxprint){ > + nsXPrintContext* xprintcontext; > + deviceContextxprint->GetPrintContext(xprintcontext); > + NPPrintCallbackStruct npPrintInfo; > + npPrintInfo.type = NP_PRINT; > + FILE* plugintmpfile = tmpfile(); Error handling ? > + npPrintInfo.fp = plugintmpfile; > + npprint.print.embedPrint.platformPrint = (void*) &npPrintInfo; > + npprint.print.embedPrint.window = window; > + // send off print info to plugin > + rv = pi->Print(&npprint); > + unsigned char *pluginbuffer; > + int fileLength; > + fseek(plugintmpfile, 0, 2); Ewwww, please do not use direct numeric values - ANSI C defines |SEEK_END| ... > + fileLength = ftell(plugintmpfile); > + rewind(plugintmpfile); > + pluginbuffer = new unsigned char[fileLength]; Error handling (out of memory) ? ... and what will happen if |fileLength| contains silly values (like 300MB) ? > + char c; > + int n=0; > + while((c=getc(plugintmpfile))!=EOF) > + pluginbuffer[n++] = c; Slow byte-copy... > + xprintcontext->EmbedData(pluginbuffer,fileLength); > + fclose(plugintmpfile); > + delete [] pluginbuffer; > + } > +#endif
Comment on attachment 127134 [details] [diff] [review] Full support I'll file a corrected patch in a few mins...
Attachment #127134 - Flags: review-
I see some issues in this patch: > + npprint.print.embedPrint.window = window; > + // send off print info to plugin > + rv = pi->Print(&npprint); > #elif defined(XP_UNIX) && !defined(XP_MACOSX) > // UNIX does things completely differently > - PRUnichar *printfile = nsnull; > - if (printSettings) { > - printSettings->GetToFileName(&printfile); > - } > - if (!printfile) > - return NS_OK; // XXX what to do on UNIX when we don't have a PS file???? > - > - NPPrintCallbackStruct npPrintInfo; > - npPrintInfo.type = NP_PRINT; > - npPrintInfo.fp = (FILE*)printfile; > - npprint.print.embedPrint.platformPrint = (void*) &npPrintInfo; > > + > + nsCOMPtr<nsIDeviceContext> deviceContext; > + aRenderingContext.GetDeviceContext(*getter_AddRefs(deviceContext)); > +#ifdef USE_POSTSCRIPT > + nsCOMPtr<nsIDeviceContextPS> deviceContextps = do_QueryInterface(deviceContext); > + if(deviceContextps){ > + nsPostScriptObj* postscriptobj = ((nsRenderingContextPS&)aRenderingContext).GetPostScriptObj(); > + NPPrintCallbackStruct npPrintInfo; > + npPrintInfo.type = NP_PRINT; > + FILE* plugintmpfile = tmpfile(); >What about error checking if |tmpfile()| returns |NULL| (for example: process >out of file handles etc. etc.) ? OK,I will add exception handle. > + npPrintInfo.fp = plugintmpfile; > + npprint.print.embedPrint.platformPrint = (void*) &npPrintInfo; > +// window.y = postscriptobj->mPrintSetup->height-window.y-window.height; >Uhm, what does that mean ? > + npprint.print.embedPrint.window = window; > + // send off print info to plugin > + fprintf(plugintmpfile,"1 -1 scale\n"); > + fprintf(plugintmpfile,"0 %d >translate\n",-(postscriptobj->mPrintSetup->height)); >Writing into |plugintmpfile| and then passing that file handle to the plugin >code doesn't seem to be a good idea (remember my concern about passing the PS >module file handle directly to the plugin code - this is the same problem: We >don't know what the plugin does, the worst case is that these data may be >overwritten... ;-( ) ... You can take a look at commit#5 of this bug. You will know the reason. The coords is different between postscript and xprint. Plugin writer should not know which print system mozilla use. Any suggestions? > + rv = pi->Print(&npprint); > + rewind(plugintmpfile); > + int c; > + while((c=getc(plugintmpfile))!=EOF) > + putc(c,postscriptobj->mPrintSetup->tmpBody); >In theory this could be made more efficient via using block copies instead of >character copies (however the amount of data transferred isn't that big...) ... I will make some modification. > + fclose(plugintmpfile); > + } > +#endif > +#ifdef USE_XPRINT > + nsCOMPtr<nsIDeviceContextXp> deviceContextxprint = do_QueryInterface(deviceContext); > + if(deviceContextxprint){ > + nsXPrintContext* xprintcontext; > + deviceContextxprint->GetPrintContext(xprintcontext); > + NPPrintCallbackStruct npPrintInfo; > + npPrintInfo.type = NP_PRINT; > + FILE* plugintmpfile = tmpfile(); >Error handling ? The same with the formal one? > + npPrintInfo.fp = plugintmpfile; > + npprint.print.embedPrint.platformPrint = (void*) &npPrintInfo; > + npprint.print.embedPrint.window = window; > + // send off print info to plugin > + rv = pi->Print(&npprint); > + unsigned char *pluginbuffer; > + int fileLength; > + fseek(plugintmpfile, 0, 2); Ewwww, please do not use direct numeric values - ANSI C defines |SEEK_END| ... > + fileLength = ftell(plugintmpfile); > + rewind(plugintmpfile); > + pluginbuffer = new unsigned char[fileLength]; >Error handling (out of memory) ? >... and what will happen if |fileLength| contains silly values (like 300MB) ? You mean we should add a limitation? 8M?16M? > + char c; > + int n=0; > + while((c=getc(plugintmpfile))!=EOF) > + pluginbuffer[n++] = c; Slow byte-copy... > + xprintcontext->EmbedData(pluginbuffer,fileLength); > + fclose(plugintmpfile); > + delete [] pluginbuffer; > + } > +#endif
New patch for 2003-06-14-08-trunk, however I couldn't it since plugin registration is broken in my build (the issue is _not_ related to this patch, the tree is just old and obviously pulled at a time where this issue was present in the tree... ;-(( ).
Attachment #127134 - Attachment is obsolete: true
Thoughts: It may be better to add a general |EmbedPostScriptData| method to |nsIDeviceContext| instead of exposing implementation details of the various print modules in layout/. That would minimize the dependicies and would allow that layout/ only uses one API for both print modules and the print modules itself handle the implementation details internally.
Taking bug myself, new patch coming-up...
Assignee: leon.sha → Roland.Mainz
Status: NEW → ASSIGNED
More issues: + #elif defined(XP_UNIX) && !defined(XP_MACOSX) This is WRONG since we have a X11 (GTK+/Xlib) port for MacOSX...
we have an MOZ_X11 define, right?
Christian Biesinger wrote: > we have an MOZ_X11 define, right? Well, yes... but what about BeOS (to be honestly I have no clue "how" or "if" BeOS actually ever implemented the NS4.x plugin API (was NS4.x ever be available for BeOS?!)) ?!
There is one issue where I am not sure about whether it is allowed or not: -- snip -- - if (!printfile) - return NS_OK; // XXX what to do on UNIX when we don't have a PS file???? - + /* UNIX does things completely differently: + * We call the plugin and it sends generated PostScript data into a + * file handle we provide. If the plugin returns with success we embed + * this PostScript code fragment into the PostScript job we send to the + * printer. + */ + + FILE *plugintmpfile = tmpfile(); + if( !plugintmpfile ) + return NS_ERROR_FAILURE; -- snip -- Is it allowed in this piece of code to |return| to the caller or MUST the code call the cleanup below, e.g. -- snip -- // XXX Nav 4.x always sent a SetWindow call after print. Should we do the same? nsCOMPtr<nsIPresContext> screenPcx; shell->GetPresContext(getter_AddRefs(screenPcx)); nsDidReflowStatus status = NS_FRAME_REFLOW_FINISHED; // should we use a special status? frame->DidReflow(screenPcx, nsnull, status); // DidReflow will take care of it return rv; // done with printing } -- snip --
Attachment #127336 - Attachment is obsolete: true
Comment on attachment 127535 [details] [diff] [review] New patch for 2003-07-09-08-trunk_cvs Requesting r= (and an answer to the question in comment #22) from peterl...
Attachment #127535 - Flags: review?(peterl)
Comment on attachment 127535 [details] [diff] [review] New patch for 2003-07-09-08-trunk_cvs We just need the call to SetWindow in DidReflow which should probably only happen on success and be directly made. All of this printing code would be better suited in a Print() method rather than inside Paint(). You also skip the call to fclose(plugintmpfile) when you return early. Make that change and r=peterl.
Attachment #127535 - Flags: review?(peterl) → review+
Changes: - Fixed issues listed in peterl's review - Added PR_LOG() code which is enabled in non-debug builds to be able to debug the new code in the "wilderness" (e.g. without sending debug builds to customers... =:-) - Fixed leak of the string returned by |XpGetOneAttribute|
Attachment #127535 - Attachment is obsolete: true
Comment on attachment 127560 [details] [diff] [review] New patch per peterl's review comments Requesting r= for the new, improved version of the patch...
Attachment #127560 - Flags: review?(peterl)
Attachment #127560 - Flags: review?(peterl) → review+
Comment on attachment 127560 [details] [diff] [review] New patch per peterl's review comments leon sha: Can you test the patch on your side and r= it if iz OK for you, please ?
Attachment #127560 - Flags: review+ → review?(leon.sha)
I tried it. Ps printer is OK. Xprint printer didn't work. The error is "badvalue". I tried xprint008 and xprint009. I will find the reason tomorrow. Too busy today.
Leon Sha wrote: > Xprint printer didn't work. > The error is "badvalue". > I tried xprint008 and xprint009. > I will find the reason tomorrow. Too busy today. Which plugin did you use (and where can I download it from) ?
The problem is here: XpPutDocumentData(mPDisplay, mDrawable, (unsigned char *)aData, aDatalen, type, option); If I use xprint-2003-04-16-release_008-0.8 then the type should be "POSTSCRIPT 2". Otherwise the badvalue will occur. If I use xprint-2003-07-10-ancalagontest_009-0.9, the badvalue will always occur. Another problem is xprint always print the whole page in the 1/4 corner. I do set the page into iso-a4. And also the printer don't print double-side.
Attached image sample (obsolete) —
This is an example printed by xprint. I can't view ps file printed by xprint. May be my ggv or gs should be updated.
Comment on attachment 127560 [details] [diff] [review] New patch per peterl's review comments I have checked out the new patch of xprint server. It works!
Attachment #127560 - Flags: review?(leon.sha) → review+
| Another problem is xprint always print the whole page in the 1/4 corner. I do | set the page into iso-a4. http://www.mozdev.org/lxr/http/source/xprint/src/xprint_main/xc/doc/hardcopy/XPRINT/Xprint_FAQ.txt#L1175 1175 - Q: "Printing itself works but the printout covers only 1/4 of the paper 1176 - what am I doing wrong ?" 1177 A: This is usually an indicator for a wrong DPI setting. The default 1178 "PSdefault" model config uses 300 DPI but some printers only support 1179 600 DPI. 1180 - Workaround: 1181 Edit ${XPCONFIGDIR}/C/print/attributes/document and replace the line 1182 "*default-printer-resolution: 300" with 1183 "*default-printer-resolution: 600" 1184 (Note that locale-specific settings in 1185 ${XPCONFIGDIR}/${LANG}/print/attributes/document always override values 1186 set in ${XPCONFIGDIR}/C/print/attributes/document.) 1187 - Solution: 1188 Create a model-config for your printer which only contains attributes 1189 supported by your printer ("printer-resolutions-supported" is the 1190 attribute in the "model-config" which holds the space-seperated list 1191 of DPI values which are supported by the printer). | And also the printer don't print double-side. http://www.mozdev.org/lxr/http/source/xprint/src/xprint_main/xc/doc/hardcopy/XPRINT/Xprint_FAQ.txt#L577 577 - Q: How do I change the defaults for double-sided/single-sided/etc. 578 printing ? 579 A: This is controlled via the "plex" attribute in the document attribute 580 pool (${XPCONFIGDIR}/${LANG}/print/attributes/document and/or 581 ${XPCONFIGDIR}/C/print/attributes/document). 582 Examples: 583 1. Adding/modifying the following line to/in 584 ${XPCONFIGDIR}/C/print/attributes/document sets the default plex for 585 all printers to "duplex": 586 -- snip -- 587 *plex: duplex 588 -- snip -- 589 2. Adding/modifying the following two lines to/in 590 ${XPCONFIGDIR}/C/print/attributes/document sets the default plex for 591 all printers to "duplex" except for printer "ps003" which should 592 default to "simplex": 593 -- snip -- 594 *plex: duplex 595 ps003.plex: simplex 596 -- snip -- 597 Notes: 598 - Not all printers support all plex modes. The model-config may restrict 599 the available plex modes. 600 - Setting a plex mode which is not supported by either the DDX(=driver) 601 or not specified in the model-config will cause Xprt to not set a 602 default plex. 603 - The PostScript DDX supports plex modes "simplex", "duplex" and "tumble". 604 - Verification: 605 Use '% xplsprinters -l | egrep "^printer:|default_plex=|plex="' to view 606 the plex settings for all printers.
Bjørn Kutter: Please do not quote huge parts of FAQ text, just post links, please.
Attachment #127560 - Flags: superreview?(bzbarsky)
Comment on attachment 127560 [details] [diff] [review] New patch per peterl's review comments This needs review from someone who can be considered a module owner for the nsIRenderingContext interface. Leon, I'm sorry to say, is not that. In particular, I'm not happy with having the word "postscript" in nsIRenderingContext. On the code level, is there a reason not to use PL_strcasestr instead of writing your own version? The PR_LOGGING stuff needs to be wrapped in some MOZ_LOGGING ifdefs, no? Defining PR_LOGGING to 1 yourself is totally incorrect. I didn't really read the guts of the nsObjectFrame code because it makes no sense to do that until the basic architecture of the solution has been OKed.
Attachment #127560 - Flags: superreview?(bzbarsky) → superreview-
Comment on attachment 127560 [details] [diff] [review] New patch per peterl's review comments One more thing. Please use the -p switch to cvs diff, and you want a lot more than 3 lines of context (-u8 is probably a good start; then read the patch and raise the number if warranted).
Boris Zbarsky wrote: > (From update of attachment 127560 [details] [diff] [review]) > This needs review from someone who can be considered a module owner for the > nsIRenderingContext interface. Leon, I'm sorry to say, is not that. > > In particular, I'm not happy with having the word "postscript" in > nsIRenderingContext. How would you name the function ? If don't add that method we will end-up in the way how the original patch was designed - exposing the print module internals to nsObjectFrame with all the nasty problems which will result from that. > On the code level, is there a reason not to use PL_strcasestr instead of > writing your own version? Erm, because I did not know about that function... :) > The PR_LOGGING stuff needs to be wrapped in some MOZ_LOGGING ifdefs, no? > Defining PR_LOGGING to 1 yourself is totally incorrect. This is correct - at least the code in gfx/ uses this to force logging to be included even in non-debug builds (as I said, this is required to be able to get a minimum of debug data on the customers side if something goes wrong). Code size cannot be the problem here since the amount of added code is small. And if someone compiles with --disable-logging |FORCE_PR_LOG| is a NO-OP.
s/This is correct/No, this is correct/
New patch after discussion on IRC per bz and roc+moz's comments...
Attachment #127560 - Attachment is obsolete: true
Comment on attachment 127915 [details] [diff] [review] New patch per bz's and roc+moz's IRC comments Requesting sr= ...
Attachment #127915 - Flags: superreview?(roc+moz)
Comment on attachment 127915 [details] [diff] [review] New patch per bz's and roc+moz's IRC comments Moveing the sr=-request to someone with a less filled-up review queue (today's victim: bzbarsky :)
Attachment #127915 - Flags: superreview?(roc+moz) → superreview?(bzbarsky)
Comment on attachment 127915 [details] [diff] [review] New patch per bz's and roc+moz's IRC comments >Index: gfx/public/nsIRenderingContext.h First thing is that you want blank lines (or rather lines containing nothing but a single '*') between the distinct parts of this long comment. >+ /** >+ * Send PostScript data fragment which should be included into the output >+ * of a print device which supports embedding of PostScript code fragments >+ * on the drawing surface this nsIRenderingContext is currently rendering >+ * on (e.g. the embedded PostScript code "draws" on this surface as if the >+ * matching nsIRenderingContext methods would have been called). Hmm... Why not something more like: Render the provided postscript fragment to the current rendering surface. There is no reason that it will actually be included (eg the output device may not be a postscript one), but the device complex impl could well include a postscript interpreter.... >+ * The PostScript code framents MUST NOT contain any code which begins a >+ * new page. >+ * This must be called after nsIDeviceContext::BeginPage() and >+ * before nsIDeviceContext::EndPage(). There should be blank lines around this chunk >+ * There is no gurantee that the PostScript data are included into the >+ * output and it may happen that the PostScript code fragment passed via >+ * RenderPostScriptDataFragment() may be included multiple times into the >+ * output (for example if PS data come from a plugin which is split over >+ * multiple pages the data may be embedded for each page and the matching >+ * visible area is clipped). Perhaps "painted" is better than "included"? >+ * The PostScript code's current position is UNDEFINED and has to be set by >+ * the fragment code relative to the current PostScript context. I just looked again at how coordinates work in postscript, and this is impossible. In PostScript, the coordinate system is set by rotate and translate calls; both work relative to the _current_ coordinate system. Therefore, if the original coordinate system is unknown there is simply no way for the code running within it to deal. There is no concept of an "absolute" coordinate system that I am aware of (if I am wrong, please correct me). I think what we want to do is to explicitly state the the initial coordinate system is aligned with the left and bottom edges of the clipping rect or something (and then implement it that way, which I assume you did). That does not address the issue of the initial scale of the postscript coordinate system, also. That's dependent on the scaling factor in the current transform, no? >+ * Note that nsIRenderingContext and PostScript have the vertical axis >+ * flipped (e.g. "0,0" means "left, top" edge for nsIRenderingContext >+ * while "0,0" means "left, bottom" edge in PostScript). This chunk should have blank lines around it >Index: gfx/src/ps/nsRenderingContextPS.cpp >+ fprintf(postscriptobj->mPrintSetup->tmpBody, "1 -1 scale\n"); >+ fprintf(postscriptobj->mPrintSetup->tmpBody, "0 %d translate\n", -(postscriptobj->mPrintSetup->height)); Why the scale? This will make things upside-down if the caller actually passes them in with the correct orientation, no? Actually, this code seems to in fact be putting the origin at the _top_ left of the area we plan to render the postscript into. Is that what we really want to be doing? If so, adjust the interface comment accordingly. >Index: gfx/src/xprint/nsXPrintContext.cpp >+ * To avoid problems we simply use |str_case_str()| No, you use PL_strcasestr... fix the comment? > + /* Note that the emebdded PostScript code uses the same "embedded" > + * coordinate space as currently be used by the DDX (if you don not "do not", not "don not". >+ char *type = "PostScript 2"; /* Format of embedded data >+ char *option = ""; /* PostScript DDX does not Is there are reason those are not static const char type[] = "PostScript 2"; and static const char option[] = ""; ? >Index: layout/html/base/src/nsObjectFrame.cpp >+#define FORCE_PR_LOG 1 /* Allow logging in the release build */ >+#define PR_LOGGING 1 >+#include "prlog.h" Perhaps you misunderstood me.... You need to wrap this in MOZ_LOGGING ifdefs. Period. I realize that you are trying to force logging on in release builds, and defining FORCE_PR_LOG to 1 will do that. However, embedders should be able to disable all logging by undefining the MOZ_LOGGING define (--disable-logging, as you mentioned). FORCE_PR_LOG is NOT a no-op on its own with --disable-logging as far as I can see. The proper way to do this part is: #ifdef MOZ_LOGGING #define FORCE_PR_LOG /* Allow logging in the release build */ #endif /* MOZ_LOGGING */ #include "prlog.h" Now prlog.h will define PR_LOGGING if either FORCE_PR_LOG is defined or DEBUG is defined, so you will be set. And for the people who use --disable-logging (eg minimo) code size is a concern no matter how small the code is. I notice that the xprint code is making the same mistake; please file a bug on that. >+ npPrintInfo.type = NP_PRINT; Don't add spaces to the ends of lines, please. >+ npPrintInfo.fp = plugintmpfile; >+ npprint.print.embedPrint.platformPrint = (void *)&npPrintInfo; >+ npprint.print.embedPrint.window = window; Imo, this lining up of equals signs just makes the code less readable, but either way.... >+ * (make sure we clamp the size to a reasonable value (say... 128 MB)) */ Personally, I would make the clamp size a pref with a default value of 128MB... but either way. >+ pluginbuffer = new unsigned char[fileLength+1]; I almost wonder whether it's better to pass an nsIInputStream to the rendering context instead of passing a char buffer. Then you could just open a file stream and pass it in and the rendering context could read it in nice bite-size chunks and move it over to where it really wants it. This would prevent allocations of potentially large buffers if done right. Thoughts on that? If you think we should do it but want to do it in a follow-up patch, I would be fine with that, as long as it actually gets done.... >- char *mozName; >- char *javaName; >+ const char *mozName; >+ const char *javaName; This change will break some of our compilers; don't do that.
Attachment #127915 - Flags: superreview?(bzbarsky) → superreview-
Attached patch New patch per previous comments (obsolete) — — Splinter Review
Attachment #127915 - Attachment is obsolete: true
Comment on attachment 128284 [details] [diff] [review] New patch per previous comments >Index: gfx/public/nsIRenderingContext.h >+ * This is usually implemented via including a PostScript data fragment .... Is this paragraph really needed? >+ * The initial PostScript coordinate system is aligned with the left and >+ * bottom edges of the clipping rectangle. I'd put this paragraph after the one that explains that the clipping area determines the drawable area for the PostScript.... Also mention what scaling looks like (as in, if I change the scaling in my transform matrix, what will that do to how the postscript is rendered?). Does it have an effect? >+ * For further information about this API see >+ * http://bugzilla.mozilla.org/show_bug.cgi?id=198515 ("When print a >+ * plugin, browser give the wrong file pointer to plugin."). Just drop this part; the comments on the API itself need to be clear enough that this is not necessary. >Index: gfx/src/ps/nsRenderingContextPS.cpp >+ fprintf(postscriptobj->mPrintSetup->tmpBody, "1 -1 scale\n"); >+ fprintf(postscriptobj->mPrintSetup->tmpBody, "0 %d translate\n", -(postscriptobj->mPrintSetup->height)); I'm still confused by this part. Is the plugin expecting to render in a coord system that's flipped in the same direction as the rendering context's coord system? Has someone tested this codepath? >Index: gfx/src/xprint/nsXPrintContext.cpp >+ const char *type = "PostScript 2"; /* Format of embedded data >+ const char *option = ""; /* PostScript DDX does not Per the irc conversation, should these not be non-const? In any case, do make them static. >Index: layout/html/base/src/nsObjectFrame.cpp >+#define FORCE_PR_LOG 1 /* Allow logging in the release build */ >+#define PR_LOGGING 1 >+#include "prlog.h" It looks like you forgor to fix this part like the others... Fix the minor nits and resolve the nsRenderingContextPS.cpp coord issue, and sr=bzbarsky
Attachment #128284 - Flags: superreview+
Boris Zbarsky wrote: > > (From update of attachment 127915 [details] [diff] [review]) > >Index: gfx/public/nsIRenderingContext.h > > First thing is that you want blank lines (or rather lines containing nothing > but a single '*') between the distinct parts of this long comment. Fixed. > >+ /** > >+ * Send PostScript data fragment which should be included into the output > >+ * of a print device which supports embedding of PostScript code fragments > >+ * on the drawing surface this nsIRenderingContext is currently rendering > >+ * on (e.g. the embedded PostScript code "draws" on this surface as if the > >+ * matching nsIRenderingContext methods would have been called). > > Hmm... Why not something more like: > > Render the provided postscript fragment to the current rendering > surface. Fixed. [snip] > >+ * The PostScript code framents MUST NOT contain any code which begins a > >+ * new page. > >+ * This must be called after nsIDeviceContext::BeginPage() and > >+ * before nsIDeviceContext::EndPage(). > > There should be blank lines around this chunk Fixed. > >+ * There is no gurantee that the PostScript data are included into the > >+ * output and it may happen that the PostScript code fragment passed via > >+ * RenderPostScriptDataFragment() may be included multiple times into the > >+ * output (for example if PS data come from a plugin which is split over > >+ * multiple pages the data may be embedded for each page and the matching > >+ * visible area is clipped). > > Perhaps "painted" is better than "included"? No. I am talking about the issue that the passed PostScript fragment may appear multiple times in the resulting PostScript output (the text lists an example) > >+ * The PostScript code's current position is UNDEFINED and has to be set > by > >+ * the fragment code relative to the current PostScript context. > > I just looked again at how coordinates work in postscript, and this is > impossible. In PostScript, the coordinate system is set by rotate and > translate calls; both work relative to the _current_ coordinate system. > Therefore, if the original coordinate system is unknown there is simply no way > for the code running within it to deal. There is no concept of an "absolute" > coordinate system that I am aware of (if I am wrong, please correct me). > > I think what we want to do is to explicitly state the the initial coordinate > system is aligned with the left and bottom edges of the clipping rect or > something (and then implement it that way, which I assume you did). Fixed. > That does not address the issue of the initial scale of the postscript > coordinate system, also. That's dependent on the scaling factor in the > current transform, no? Well, this is tricky since the currently available two implementations (PostScript, Xprint) differ here. The current plugin implementations I know about simply set their own scale and make sure they fit exactly into the boundary (e.g. width/height) of the clip rectange. > >+ * Note that nsIRenderingContext and PostScript have the vertical axis > >+ * flipped (e.g. "0,0" means "left, top" edge for nsIRenderingContext > >+ * while "0,0" means "left, bottom" edge in PostScript). > > This chunk should have blank lines around it Fixed. > >Index: gfx/src/ps/nsRenderingContextPS.cpp > >+ fprintf(postscriptobj->mPrintSetup->tmpBody, "1 -1 scale\n"); > >+ fprintf(postscriptobj->mPrintSetup->tmpBody, "0 %d translate\n", -(postscriptobj->mPrintSetup->height)); > > Why the scale? This will make things upside-down if the caller actually passes > them in with the correct orientation, no? > > Actually, this code seems to in fact be putting the origin at the _top_ left of > the area we plan to render the postscript into. Is that what we really want to > be doing? If so, adjust the interface comment accordingly. Please ask leon.sha about that part (I didn't wrote the gfx/src/ps/-specific path of the code...) ... > >Index: gfx/src/xprint/nsXPrintContext.cpp > >+ * To avoid problems we simply use |str_case_str()| > > No, you use PL_strcasestr... fix the comment? Fixed. > > + /* Note that the emebdded PostScript code uses the same > > "embedded" Fixed. > > + * coordinate space as currently be used by the DDX (if you don not > > "do not", not "don not". > > >+ char *type = "PostScript 2"; /* Format of embedded data > >+ char *option = ""; /* PostScript DDX does not > > Is there are reason those are not > > static const char type[] = "PostScript 2"; > > and > > static const char option[] = ""; > > ? Because all the X11 includes do not use |const| for xx@@@-"historical" reasons. The all strings passed there are treated as |const| (as is it the normal behaviour for most X11 functions). I changed the string pointers to |const| and added casts in the XpPutDocumentData() call... > >Index: layout/html/base/src/nsObjectFrame.cpp > >+#define FORCE_PR_LOG 1 /* Allow logging in the release build */ > >+#define PR_LOGGING 1 > >+#include "prlog.h" > > Perhaps you misunderstood me.... You need to wrap this in MOZ_LOGGING ifdefs. > Period. I realize that you are trying to force logging on in release builds, > and defining FORCE_PR_LOG to 1 will do that. However, embedders should be able > to disable all logging by undefining the MOZ_LOGGING define (--disable-logging, > as you mentioned). FORCE_PR_LOG is NOT a no-op on its own with > --disable-logging as far as I can see. The proper way to do this part is: > > #ifdef MOZ_LOGGING > #define FORCE_PR_LOG /* Allow logging in the release build */ > #endif /* MOZ_LOGGING */ > #include "prlog.h" > > Now prlog.h will define PR_LOGGING if either FORCE_PR_LOG is defined or DEBUG > is defined, so you will be set. > > And for the people who use --disable-logging (eg minimo) code size is a concern > no matter how small the code is. Fixed. > I notice that the xprint code is making the same mistake; please file a bug on > that. I fixed that in this patch... > >+ npPrintInfo.type = NP_PRINT; > > Don't add spaces to the ends of lines, please. I wish "nedit" would simply avoid such spaces... ;-( > >+ npPrintInfo.fp = plugintmpfile; > >+ npprint.print.embedPrint.platformPrint = (void *)&npPrintInfo; > >+ npprint.print.embedPrint.window = window; > > Imo, this lining up of equals signs just makes the code less readable, but > either way.... Well, I think it makes the code more readable... oh... well... lets blame K&R for making the C syntax that flexible... =:-) > >+ * (make sure we clamp the size to a reasonable value (say... 128 MB)) */ > > Personally, I would make the clamp size a pref with a default value of 128MB... > but either way. Erm... adding more useless prefs ?! Please no... this is only thought as a crash protection against the case that the plugin goes totally mad (like... fseek() 3GB forward and write the fragment data there... =:-) > >+ pluginbuffer = new unsigned char[fileLength+1]; > > I almost wonder whether it's better to pass an nsIInputStream to the rendering > context instead of passing a char buffer. Then you could just open a file > stream and pass it in and the rendering context could read it in nice bite-size > chunks and move it over to where it really wants it. This would prevent > allocations of potentially large buffers if done right. Thoughts on that? Please no... I don't want to make the code nor the API more compliciated - and as I said on IRC using |nsIInputStream| on a |FILE *| from |tmpfile()| is a VERY VERY bad idea. Some |tmpfile()|-implementations really do nasty stuff and I have no clue whether the code in |nsIInputStream| is ready for that. Presonally I would prefer to keep the API and the code as simple as possible... it isn't called very often and using complex code may lead to complex bugs... ;-( ... and the argumentation that using |nsIInputStream| saves huge memory allocations is for /dev/null - loading a single 16bit X11 (core) font involves more memory allocations than this little method does (and it is only scratch memory, no permanent allocation). if you want an optimisation here we could talk about using |mmap()| instead of |fseek(), malloc(), fread(), free()|, but then we have to abadon the use of |tmpfile()| ... > >- char *mozName; > >- char *javaName; > >+ const char *mozName; > >+ const char *javaName; > > This change will break some of our compilers; don't do that. Fixed... ... but can you list the compiler(s) which fail here ? Sun Workshop 6U2/7, gcc 2.95.1, gcc 3.2.x and AIX xlc_r accept that code without problems...
> Please ask leon.sha about that part Leon? We need this issue resolved before the patch can go in.... > Erm... adding more useless prefs ?! I hardly consider a pref that lets someone print when some idiotic hardcoded defauly won't let them do so "useless". 128MB is pretty small, for some printing applications... > The current plugin implementations I know about simply set their own scale Whatever they set is relative to the initial coordinate scaling.... So it would be good to actually specify this explicitly. If there are differences between XPrint and PS, some scale calls in PS can be used to resolve them. Let's move the nsIInputStream discussion to a separate bug, as I suggested in comment 42. > can you list the compiler(s) which fail here Not offhand; it was either windows or mac.
>>Index: gfx/src/ps/nsRenderingContextPS.cpp >>+ fprintf(postscriptobj->mPrintSetup->tmpBody, "1 -1 scale\n"); >>+ fprintf(postscriptobj->mPrintSetup->tmpBody, "0 %d translate\n", >-(postscriptobj->mPrintSetup->height)); >I'm still confused by this part. Is the plugin expecting to render in a coord >system that's flipped in the same direction as the rendering context's coord >system? Has someone tested this codepath? The rendering context's coord is the same as xprint's, top-left. But the postscript's coord is top-bottom. We let plugin to render in a coord system which is the same as rendering context. Also we can let plugin to render in postscript coord. If so, we need the similiar code in xprint. I have test both way, they all work well. I think this way is better because we need not write code like this. http://bugzilla.mozilla.org/attachment.cgi?id=119028&action=view + window.y = postscriptobj->mPrintSetup->height-window.y; + npprint.print.embedPrint.window = window;
Boris Zbarsky wrote > (From update of attachment 128284 [details] [diff] [review]) > >Index: gfx/public/nsIRenderingContext.h > > >+ * This is usually implemented via including a PostScript data fragment > .... > > Is this paragraph really needed? Well, I just want to provide some context... some info is duplicate... but better more information than too few... :) > >+ * The initial PostScript coordinate system is aligned with the left and > >+ * bottom edges of the clipping rectangle. > > I'd put this paragraph after the one that explains that the clipping area > determines the drawable area for the PostScript.... Done. > Also mention what scaling looks like (as in, if I change the scaling in my > transform matrix, what will that do to how the postscript is rendered?). Does > it have an effect? See Leon Sha's comment... > >+ * For further information about this API see > >+ * http://bugzilla.mozilla.org/show_bug.cgi?id=198515 ("When print a > >+ * plugin, browser give the wrong file pointer to plugin."). > > Just drop this part; the comments on the API itself need to be clear enough > that this is not necessary. Done. ;-( > >Index: gfx/src/ps/nsRenderingContextPS.cpp > >+ fprintf(postscriptobj->mPrintSetup->tmpBody, "1 -1 scale\n"); > >+ fprintf(postscriptobj->mPrintSetup->tmpBody, "0 %d translate\n", -(postscriptobj->mPrintSetup->height)); > > I'm still confused by this part. Is the plugin expecting to render in a coord > system that's flipped in the same direction as the rendering context's coord > system? Has someone tested this codepath? See Leon Sha's comment... > >Index: gfx/src/xprint/nsXPrintContext.cpp > > >+ const char *type = "PostScript 2"; /* Format of embedded data > >+ const char *option = ""; /* PostScript DDX does not > > Per the irc conversation, should these not be non-const? See the new patch... I made it |const| and then I > In any case, do make them static. Huh? Why do you want them |static| ? That are no global vars... |static| doesn't make much sense here... > >Index: layout/html/base/src/nsObjectFrame.cpp > > >+#define FORCE_PR_LOG 1 /* Allow logging in the release build */ > >+#define PR_LOGGING 1 > >+#include "prlog.h" > > It looks like you forgor to fix this part like the others... Eeeeeeeks. I did not forgot that. But I forgot to save the editor window before making the patch... ;-/
Attachment #128284 - Attachment is obsolete: true
Boris Zbarsky wrote: > > Erm... adding more useless prefs ?! > > I hardly consider a pref that lets someone print when some idiotic hardcoded > defauly won't let them do so "useless". 128MB is pretty small, for some > printing applications... Erm... are you aware that we are talking about 128MB for _ONE_ page ? Having large PostScript jobs is no problem if the size is distributed over many pages. But having 128MB in one page makes it nearly impossible to print that job since the printer needs _at_least_ 128MB main memory. And the "128MB-clamp" is enougth for most purposes - even a image printed at 600DPI covering a _whole_ DIN-A4 page takes less than 60MB (assuming no compression is used - PostScript jobs can contain compressed images as well...) - and remember that most pictues have far far lower resolutions (like 90DPI), resulting in far smaller PostScript fragments. And not all plugins rasterize their screen image and dump that as Im24 image data block to the printer...
Target Milestone: --- → mozilla1.5beta
Attachment #128296 - Flags: superreview?(bzbarsky)
Comment on attachment 128296 [details] [diff] [review] New patch for 2003-07-09-08-trunk_cvs per bz's comments > Huh? Why do you want them |static| ? Because they are not going to change during the runtime of the program? Hence there is no need to initialize them every time. Per discussion on irc, it seems that if I am a consumer and I take the coord system description in the interface header at face value, I will get upside-down rendering. So perhaps we should really say that the coord system for the postscript is aligned with the top and left edges of the clip rect, with the positive X axis along the top edge and the positive Y axis along the left edge? Is that a correct summary of the situation? In any case, sr=me; figure out what that comment should really say and make it say it.
Attachment #128296 - Flags: superreview?(bzbarsky) → superreview+
Note that the top/left thing I just said can't be quite right because it would not be consistent with the PS printing code as far as I can tell.... Please fix it to accurately describe the situation.
Boris Zbarsky wrote: > (From update of attachment 128296 [details] [diff] [review]) > > Huh? Why do you want them |static| ? > > Because they are not going to change during the runtime of the program? Hence > there is no need to initialize them every time. Erm... those string literals are already |const char *| and usually (at least for gcc and Sun Workshop) put into a read-only ELF chunk. Making the string pointer |static| doesn't make sense (and doing so in a small "hello world" application causes a code-size increment - without having any gain from that...) [snip] > In any case, sr=me; figure out what that comment should really say and make it > say it. OK, new patch after lunch (MET timezone :) ...
Attachment #128296 - Attachment is obsolete: true
Gzipp'ed sample PostScript job printing http://www.nichtlustig.de/suche.php?input=yeti&submit.x=0 using Mozilla 2003-07-09-08-trunk_cvs+patch and Xprint module (Xprint server was the "ancalagon" 009 prototype). (the Yeti in the upper left corner is the flash plugin)
OK, that seems reasonable (the comment). The idea of making the strings static is to keep from having to reinit them, not to make them take less codesize.... In any case, I have no more time to spend on reviewing this patch and arguing about it... sr=me as it stands, and please file a followup bug on not allocating a huge chunk of memory -- I don't care if other code does it, but I would like to be able to print plugins on the PS2 if possible.
It looks like this landed on 7/23. Can we resolve this bug?
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
Component: Layout: HTML Frames → Layout: Images
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: