Closed Bug 119491 Opened 23 years ago Closed 18 years ago

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

Categories

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

x86
Linux
defect

Tracking

()

RESOLVED FIXED
Future

People

(Reporter: rods, Assigned: kherron+mozilla)

References

Details

Attachments

(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...
Status: NEW → ASSIGNED
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. rods: 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
Roland, 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
Status: ASSIGNED → NEW
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.
Taking.
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()); > } Same. 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 declaration. 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.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: