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

RESOLVED FIXED in mozilla1.5beta

Status

()

RESOLVED FIXED
16 years ago
22 days ago

People

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

Tracking

Trunk
mozilla1.5beta
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 13 obsolete attachments)

44.57 KB, patch
Details | Diff | Splinter Review
292.13 KB, application/octet-stream
Details
60.04 KB, image/jpeg
Details
(Reporter)

Description

16 years ago
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
(Reporter)

Comment 1

16 years ago
Created attachment 117957 [details] [diff] [review]
a patch just for nsRenderingContextPS
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?)

Comment 3

16 years ago
>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.
(Reporter)

Updated

16 years ago
Attachment #117957 - Attachment is obsolete: true
(Reporter)

Comment 4

16 years ago
Created attachment 117968 [details] [diff] [review]
A patch just for nsRenderingContextPS

Two files changed. 
nsObjectFrame.cpp and Makefile.in
(Reporter)

Updated

16 years ago
Attachment #117968 - Attachment is obsolete: true
(Reporter)

Comment 5

16 years ago
Created attachment 119028 [details] [diff] [review]
Modified NPWindow passed to plugin

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 6

16 years ago
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-

Comment 7

16 years ago
.
Assignee: frame → leon.sha
(Reporter)

Comment 8

16 years ago
Created attachment 125070 [details] [diff] [review]
Add ifdef

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
(Reporter)

Updated

16 years ago
Attachment #125070 - Flags: review?(timeless)
(Assignee)

Comment 9

16 years ago
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...
(Assignee)

Updated

16 years ago
Attachment #125070 - Flags: review?(timeless) → review-
(Reporter)

Comment 10

16 years ago
Created attachment 125391 [details] [diff] [review]
Patch for plugin print
Attachment #125070 - Attachment is obsolete: true
(Reporter)

Updated

16 years ago
Attachment #125391 - Flags: review?(Roland.Mainz)
(Assignee)

Comment 11

16 years ago
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)
(Reporter)

Comment 12

15 years ago
Created attachment 127134 [details] [diff] [review]
Full support
(Reporter)

Updated

15 years ago
Attachment #125391 - Attachment is obsolete: true
(Assignee)

Comment 13

15 years ago
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
(Assignee)

Comment 14

15 years ago
Comment on attachment 127134 [details] [diff] [review]
Full support

I'll file a corrected patch in a few mins...
Attachment #127134 - Flags: review-
(Reporter)

Comment 15

15 years ago
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
(Assignee)

Comment 16

15 years ago
Created attachment 127336 [details] [diff] [review]
New patch for 2003-06-14-08-trunk

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
(Assignee)

Comment 17

15 years ago
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.
(Assignee)

Comment 18

15 years ago
Taking bug myself, new patch coming-up...
Assignee: leon.sha → Roland.Mainz
(Assignee)

Updated

15 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 19

15 years ago
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?
(Assignee)

Comment 21

15 years ago
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?!)) ?!
(Assignee)

Comment 22

15 years ago
Created attachment 127535 [details] [diff] [review]
New patch for 2003-07-09-08-trunk_cvs

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
(Assignee)

Comment 23

15 years ago
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 24

15 years ago
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+
(Assignee)

Comment 25

15 years ago
Created attachment 127560 [details] [diff] [review]
New patch per peterl's review comments

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
(Assignee)

Comment 26

15 years ago
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)

Updated

15 years ago
Attachment #127560 - Flags: review?(peterl) → review+
(Assignee)

Comment 27

15 years ago
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)
(Reporter)

Comment 28

15 years ago
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.
(Assignee)

Comment 29

15 years ago
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) ?
(Reporter)

Comment 30

15 years ago
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.
(Reporter)

Comment 31

15 years ago
Created attachment 127778 [details]
sample

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.
(Reporter)

Comment 32

15 years ago
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+

Comment 33

15 years ago
| 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.
(Assignee)

Comment 34

15 years ago
Bjørn Kutter:
Please do not quote huge parts of FAQ text, just post links, please.
(Assignee)

Updated

15 years ago
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).
(Assignee)

Comment 37

15 years ago
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.
(Assignee)

Comment 38

15 years ago
s/This is correct/No, this is correct/
(Assignee)

Comment 39

15 years ago
Created attachment 127915 [details] [diff] [review]
New patch per bz's and roc+moz's IRC comments

New patch after discussion on IRC per bz and roc+moz's comments...
(Assignee)

Updated

15 years ago
Attachment #127560 - Attachment is obsolete: true
(Assignee)

Comment 40

15 years ago
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)
(Assignee)

Comment 41

15 years ago
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-
(Assignee)

Comment 43

15 years ago
Created attachment 128284 [details] [diff] [review]
New patch per previous comments
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+
(Assignee)

Comment 45

15 years ago
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.
(Reporter)

Comment 47

15 years ago
>>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;
(Assignee)

Comment 48

15 years ago
Created attachment 128296 [details] [diff] [review]
New patch for 2003-07-09-08-trunk_cvs per bz's comments

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
(Assignee)

Comment 49

15 years ago
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...
(Assignee)

Updated

15 years ago
Target Milestone: --- → mozilla1.5beta
(Assignee)

Updated

15 years ago
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.
(Assignee)

Comment 52

15 years ago
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 :) ...
(Assignee)

Comment 53

15 years ago
Created attachment 128317 [details] [diff] [review]
Final patch for checkin...
Attachment #128296 - Attachment is obsolete: true
(Assignee)

Comment 54

15 years ago
Created attachment 128318 [details]
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)

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)
(Assignee)

Comment 55

15 years ago
Created attachment 128321 [details]
Animated Yeti sample PostScript job (attachment 128318 [details]) page 1 of 1 converted to JPEG
Attachment #127778 - Attachment is obsolete: true
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.

Comment 57

15 years ago
It looks like this landed on 7/23. Can we resolve this bug?
(Reporter)

Updated

15 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Updated

25 days ago
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.