Closed Bug 119491 Opened 22 years ago Closed 16 years ago

[ps] Cleanup global vars in PostScript module (gfx/src/ps/)


(Core :: Printing: Output, defect, P2)






(Reporter: rods, Assigned: kherron+mozilla)




(2 files, 2 obsolete files)

OS: Windows 2000 → Linux
QA Contact: Roland.Mainz → katakai
Summary: Cleanup global vars in XPrint module → Cleanup global vars in Xprint module
Requires patch in bug 112635 to checked-in first...
Depends on: 112635
*** Bug 119492 has been marked as a duplicate of this bug. ***
I'll file a combined patch for both Xprint and PostScript modules, fixing
Summary: Cleanup global vars in Xprint module → Cleanup global vars in PostScript and Xprint modules
Target Milestone: --- → mozilla0.9.9
Sorry, I did not had enougth time for this, retargeting to Mozilla 1.1 for now.

Is that OK for you ? If not please adjust the milestone setting... :)
Target Milestone: mozilla0.9.9 → mozilla1.1
Blocks: 126802
Blocks: 127810
Nominating for mozilla1.0 (and nsbeta1) since this means printing from print
preview will be disabled on unices for mozilla1.0 if we don't fix this bug.  
Keywords: mozilla1.0, nsbeta1
Blocks: 129002
Fixing some fields (milestone, component, QA, etc.) ...
Severity: normal → major
Component: Printing: Xprint → Printing
Depends on: 128748
Priority: -- → P2
QA Contact: katakai → sujay
Target Milestone: mozilla1.1 → mozilla1.0
Blocks: 125145
nsbeta1- per ADT triage. 
Keywords: nsbeta1nsbeta1-
It seems to me that the comments in this bug were not clear as to the impact...
Until this bug is fixed, there is no way to

1)  Print two documents in parallel
2)  Print from print preview
3)  Zoom in print preview
4)  Change postscript/landscape mode in print preview

This bug effectively makes print preview useless....
putterman wrote:
> nsbeta1- per ADT triage.

... ... what does that mean ?!
That means "Netscape does not care whether print preview works on Unix".
Boris: 5) plus some crash problem in print preview :) (Unix only ?)
Blocks: 129655
Rather than make assertions about what Netscape cares about (after all, we did
provide engineering time to get this working on Linux), it is better to provide
clear information in the bug as to the actual impact, as you have done.

The information in 1-5 may not have been clear to ADT at triage time.,
re-nominating to get ADT to consider new data.
Keywords: nsbeta1-nsbeta1
Short: I am currently working on a patch for this...
No longer blocks: 125145
Depends on: 126799
No longer blocks: 129655
No longer depends on: 126799
Depends on: 126799
Blocks: 129507
Which milestone do you plan on fixing this for?  Thanks.
Depends on: 157675
Blocks: 157675
No longer depends on: 157675
Target Milestone: mozilla1.0 → Future
Samir Gehani wrote:
> Which milestone do you plan on fixing this for?

Unfortunatly - for one half of this bug the answer is:
"Never. It's not possible without rewriting the PostScript module from
scratch..." ;-(

The problem that we cannot safely use more than one |nsDeviceContextPS| is
deeply rooted in the fact that the PostScript module code makes use of global
vars - lots of global vars - which are spread wildly all over the whole source
(~48000lines of code).
My first idea of making the code reentrant was simply to create a handle-API for
these globals, however this strategy fails at the point that the global vars are
in nearly every piece of code (sure, we could just add lots of "handle"-classes
- but that will end in the issue that the code is getting more and more complex
then, making it a pain for those who will have to handle the source in the years
to come).
It is still possible to fix the issue, however we would have to dismantle,
cleanup and redesign at least 3/4 of the code - a single engineer will need _at_
_least_ a month (assuming he/she is working 24h/day, e.g. a single person would
take 2-3months in real life) for this work (incl. testing if the patch really
works - which needs someone with Rational Purify and a new breed of test
application in gfx/tests/ which runs multiple device instances in parallel with
input and then compares the output, too).

After playing around with the code and seeking for possible solutions for the
last couple of months the only usefull solution is (unless someone steps up and
wants to spend the next three months with trying the hacked solution described
above) IMHO to abadon the current code in gfx/src/ps/ and rebuild a new version
in gfx/src/ps2/ (or gfx/src/postscript/) while having the terms "global r/w-vars
are forbidden", "code must be reentrant" and "threadsafety" in mind (during the
design phase, please :)
Keywords: mozilla1.0
I've fixed the Xprint module code to support multiple device instances as part
of bug 187125 so doing multiple print jobs concurrently to different printers or
doing printing and print preview at the same time works with that module.

I'll leave the bug open for the code in gfx/src/ps/, however I do not have the
time to rewrite that code from scratch to support that feature.
Assignee: Roland.Mainz → rods
Summary: Cleanup global vars in PostScript and Xprint modules → [ps] Cleanup global vars in PostScript module (gfx/src/ps/)
Louie/Pete: any interest?
nsbeta1-. Not enough resources to go after this for next release.

-> kin, since Rod is no longer working on printing issues.
Assignee: rods → kin
Keywords: nsbeta1nsbeta1-
>Louie/Pete: any interest?
We will find some time to do this. As comment 15, there are two ways. Rewrite or
reentrant the code. Both way needs lots of time. I'm not sure which one is better. 
What about using a external process like Ghostview?

Ghostview gives an accurate view of what is going to be printed.
That does not allow you to dynamically see the effects of changing
between landscape and postscript does it?  Since the PS would have
to just be generated then passed off to the helper....

It also does not resolve the problems we currently have if you try to
print while another (long) document is printing.
> That does not allow you to dynamically see the effects of changing
> between landscape and postscript does it?  Since the PS would have
> to just be generated then passed off to the helper....

This is not clear. It looks like Ghostview uses (wraps) ghostscript. I
know for certian that when Ghostview is displaying from a file it is
definitely able to update the image when the file changes. Gecko would, of 
course, still be responsible for producing the postscript but the helper 
process could be automatically informed of the new output and re-render it.

> It also does not resolve the problems we currently have if you try to
> print while another (long) document is printing.

Yes, of course, the globals will need to be cleaned up to allow multiple 
prints to occur simultaneously.

However, using Ghostview (or some part of it) to wrap ghostview could give 
us much simplier code along with an accurate rendering of the output.
Assignee: kinmoz → kjh-5727
This replaces the global nsIPref object in nsPostScriptObj.cpp with
nsIPrefService & nsIPrefBranch objects created as needed.

Could someone volunteer to review this? No postscript knowledge is required. I
hate to keep leaning on the same two reviewers all the time.
Comment on attachment 136963 [details] [diff] [review]
Replace global nsIPref object

I know caillon wants to review this.. ;)
Attachment #136963 - Flags: review?(caillon)
This removes a font debugging flag from nsDeviceContextPS.cpp and
nsFontMetricsPS.cpp. The debugging functionality is performed through nspr's
logging subsystem instead.

Once this is checked in, the file gfx/src/ps/nsFontPSDebug.h will no longer be
needed, and should be removed from the source tree.
Attachment #137024 - Flags: review?(cbiesinger)
Comment on attachment 137024 [details] [diff] [review]
Replace global font debugging flag

nsFontPSDebug.h can be removed now, right?

+static PRLogModuleInfo *FontMetricsPSM = PR_NewLogModule("FontMetricsPS");
please name the variable gFontMetricsPSM
Attachment #137024 - Flags: review?(cbiesinger) → review+
Attached patch Font debugging flag, take two (obsolete) — Splinter Review
Renamed FontMetricsPSM -> gFontMetricsPSM as requested.

Yes, as noted gfx/src/ps/nsFontPSDebug.h should be removed when this patch is
checked in.
Attachment #137024 - Attachment is obsolete: true
Attachment #137766 - Flags: superreview?(bz-vacation)
Comment on attachment 136963 [details] [diff] [review]
Replace global nsIPref object

>+static void
>+EnumerateLangPrefs(char **plist, PRUint32 pcount, unsigned int prefixlen,
>+  nsIPrefBranch *prefbranch, nsPostScriptObj *psobj)
>+  NS_PRECONDITION(plist != nsnull, "pref list is null");
>+  for (int i = pcount; i--; ) {
>+    NS_ASSERTION(plist[i] != nsnull, "pref list contains null");
>+    PrefEnumCallback(plist[i] + prefixlen, prefbranch, psobj);
>+    nsMemory::Free(plist[i]);
>+  }
>+  nsMemory::Free(plist);

While this "works", I think it is pretty shady for this method to free memory
which it doesn't control, especially without advertising it.  I'd rather you
just call NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY from initlanggroup() which is
where this memory is being managed.  Actually, I think that this method can go
altogether.  It isn't called enough IMO to warrant its existence.  Just move
the functionality here into its caller.
Attachment #136963 - Flags: review?(caillon) → review-
I'm not likely to get to this before Jan 15...  Sorry about that.
I started to make the changes described in comment 29, but I'm afraid I just
don't agree with them.

|initlanggroup()| has to iterate over two separate arrays of preference strings,
performing identical operations on each. Factoring the iteration code into a
separate function is a good development practice. It reduces LOC, it reduces
compiled object size, and it makes |initlanggroup()| more comprehensible by
separating the high-level process of initializing lang groups from the low-level
process of iterating through an array. I don't see a benefit to rolling the code
from |EnumerateLangPrefs()| directly into |initlanggroup()|.

Regarding use of |NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY()|, normally I'm in
favor of using high-level functions as much as possible, but I don't see the
point here. NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY() requires the caller to
specify the array size and (perhaps implicitly) the function for freeing array
objects, so it doesn't provide information hiding. It's a macro, so it doesn't
reduce code size. It adds a loop with an iteration variable everywhere it's
called. And for some reason it takes a signed int for the array size, which
means converting the unsigned int returned from the code that allocated the
array in the first place. The existing code already iterates over the array
being freed, and contains places where it's natural to free the allocated items.
 I don't see any value to changing the code as written.

Regarding the fact that memory allocated in |initlanggroup()| is freed in
|EnumLangPrefs()|, this is of course because |EnumLangPrefs()| contains the code
which iterates over the array. |EnumLangPrefs()| is documented as being a helper
function for initlanggroup(), it's not called by anywhere else, and the
operation that it performs is very specific to |initlanggroup()|'s needs. In
this case, I don't see a problem with the distribution of work. I can see
amending the description for |EnumerateLangPrefs()| to to mention that the pref
string array is freed.
Comment on attachment 137766 [details] [diff] [review]
Font debugging flag, take two

>Index: gfx/src/ps/nsFontMetricsPS.cpp
>-          if (gFontPSDebug & NS_FONTPS_DEBUG_FIND_FONT) {
>+          if (PR_LOG_TEST(gFontMetricsPSM, PR_LOG_DEBUG)) {

This whole block should probably go inside #ifdef PR_LOGGING

>+    if (PR_LOG_TEST(gFontMetricsPSM, PR_LOG_DEBUG)) {
>+      nsCAutoString fontname, stylename;
>       entry->GetFamilyName(fontname);
>       entry->GetStyleName(stylename);
>+      PR_LogPrint("    -- %s '%s/%s'\n", (loaded ? "already loaded" : "load"),
>+          fontname.get(), stylename.get());
>     }


sr=bzbarsky with those two changes.  Sorry this took so long....
Attachment #137766 - Flags: superreview?(bz-vacation) → superreview+
The indicated sections have been wrapped by #ifdef PR_LOGGING. A couple of
variables were only used inside the ifdef'ed code so I also moved their

Again, gfx/src/ps/nsFontPSDebug.h should be removed when this is checked in.
Attachment #137766 - Attachment is obsolete: true
patch checked in, including removal of nsFontPSDebug.h. leaving open per
kherron's request.
Resolving fixed. This was kept open for additional work work on the remaining global variables in the module. The code at issue is obsolete on the trunk, and only major bug fixes are being taken on the branches.
Closed: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.