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)
Tracking
()
RESOLVED
FIXED
Future
People
(Reporter: rods, Assigned: kherron+mozilla)
References
Details
Attachments
(2 files, 2 obsolete files)
8.30 KB,
patch
|
caillon
:
review-
|
Details | Diff | Splinter Review |
15.07 KB,
patch
|
Details | Diff | Splinter Review |
Updated•23 years ago
|
OS: Windows 2000 → Linux
QA Contact: Roland.Mainz → katakai
Summary: Cleanup global vars in XPrint module → Cleanup global vars in Xprint module
Comment 2•23 years ago
|
||
*** Bug 119492 has been marked as a duplicate of this bug. ***
Comment 3•23 years ago
|
||
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
Comment 4•23 years ago
|
||
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
Comment 5•23 years ago
|
||
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
Comment 6•23 years ago
|
||
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
Comment 8•23 years ago
|
||
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....
Comment 9•23 years ago
|
||
putterman wrote:
> nsbeta1- per ADT triage.
... ... what does that mean ?!
Comment 10•23 years ago
|
||
That means "Netscape does not care whether print preview works on Unix".
Comment 11•23 years ago
|
||
Boris: 5) plus some crash problem in print preview :) (Unix only ?)
Comment 12•23 years ago
|
||
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.
Comment 13•23 years ago
|
||
Short: I am currently working on a patch for this...
Comment 14•22 years ago
|
||
Roland,
Which milestone do you plan on fixing this for? Thanks.
Updated•22 years ago
|
Updated•22 years ago
|
Target Milestone: mozilla1.0 → Future
Comment 15•22 years ago
|
||
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
Comment 16•22 years ago
|
||
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/)
Comment 17•22 years ago
|
||
Louie/Pete: any interest?
Comment 18•22 years ago
|
||
nsbeta1-. Not enough resources to go after this for next release.
-> kin, since Rod is no longer working on printing issues.
Comment 19•22 years ago
|
||
>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.
Comment 20•22 years ago
|
||
What about using a external process like Ghostview?
Ghostview gives an accurate view of what is going to be printed.
Comment 21•22 years ago
|
||
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.
Comment 22•22 years ago
|
||
> 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 | ||
Comment 24•21 years ago
|
||
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 25•21 years ago
|
||
Comment on attachment 136963 [details] [diff] [review]
Replace global nsIPref object
I know caillon wants to review this.. ;)
Attachment #136963 -
Flags: review?(caillon)
Assignee | ||
Comment 26•21 years ago
|
||
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.
Assignee | ||
Updated•21 years ago
|
Attachment #137024 -
Flags: review?(cbiesinger)
Comment 27•21 years ago
|
||
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+
Assignee | ||
Comment 28•21 years ago
|
||
Renamed FontMetricsPSM -> gFontMetricsPSM as requested.
Yes, as noted gfx/src/ps/nsFontPSDebug.h should be removed when this patch is
checked in.
Assignee | ||
Updated•21 years ago
|
Attachment #137024 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #137766 -
Flags: superreview?(bz-vacation)
Comment 29•21 years ago
|
||
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-
Comment 30•21 years ago
|
||
I'm not likely to get to this before Jan 15... Sorry about that.
Assignee | ||
Comment 31•21 years ago
|
||
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 32•21 years ago
|
||
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+
Assignee | ||
Comment 33•21 years ago
|
||
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
Comment 34•21 years ago
|
||
patch checked in, including removal of nsFontPSDebug.h. leaving open per
kherron's request.
Assignee | ||
Comment 35•18 years ago
|
||
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.
Description
•