Closed Bug 187125 Opened 22 years ago Closed 17 years ago

GTK+ and Xlib gfx should support multiple device instances

Categories

(Core Graveyard :: GFX, enhancement)

All
Linux
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX
mozilla1.3beta

People

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

References

Details

Attachments

(11 obsolete files)

RFE: GTK+ and Xlib gfx should support multiple independant device instances.
Status: NEW → ASSIGNED
QA Contact: ian → smontagu
Target Milestone: --- → mozilla1.3beta
Simple patch - all globals which are read/write are moved in a per-device
context structure (|nsFontMetricsXlibContext|).

Beyond that no functionality of the code is changed (e.g. it should behave 100%
the same as the original version - except that we can now handle multiple
physical devices).
Demonstrator patch to enable PP's toolbar based on the printer name (still
incomplete - it should use PrinterFeatures instead to query if the matching
device supports multiple device instances).
Attachment #110512 - Flags: review-
Attachment #110502 - Flags: review?(smontagu)
Comment on attachment 110502 [details] [diff] [review]
Xlib gfx patch for 2002-12-28-08-trunk

Basically this looks good, but some things are unclear to me, and I have a few
nits:

Firstly, do *all* the globals need to become part of the per-device structure?
Some of them seem to be functioning more like constants, e.g.  gUnicode,
gUserDefined, gWesternLocale, and maybe also gUsersLocale. Are there technical
reasons why these can't remain globals?

Please remove (or put in #ifdef DEBUG) every printf, puts, etc.

>+/* Copy an array of nsFontCharSetMapXlib structures:
>+ * |mName| is unique, |mFontLangGroup| and |mInfo| must be copied
>+ * without creating duplicates.
>+ */
>+static
>+PRBool CopyFontCharSetMapXlib(nsFontMetricsXlibContext *fmctx)

I don't understand what this method is doing and why. "|mFontLangGroup| and
|mInfo| must be copied without creating duplicates" is not very clear. Can you
add more comments specifying the input and the output, what exactly you are
preventing by not creating duplicates (what is not allowed to be a duplicate of
what and why not), and how you are preventing it.

>-  if (mDeviceContext) {
>+  if (mDeviceContext)
>+  {

Hyper-nit: don't change the brace positioning from the prevailing style.

>@@ -1665,7 +1817,7 @@
> 
> NS_IMETHODIMP  nsFontMetricsXlib::Destroy()
> {
>-  mDeviceContext = nsnull;

Why is this line removed?

>+  mFontMetricsContext = nsnull;
>   return NS_OK;
> }


>@@ -445,11 +428,13 @@
>   PRUint8           mTriedAllGenerics;
>   PRUint8           mIsUserDefined;
> 
>+  nsFontMetricsXlibContext *mFontMetricsContext;
>+  nsIDeviceContext         *mDeviceContext;
>+
> protected:
>   void RealizeFont();
>   nsFontXlib *LocateFont(PRUint32 aChar, PRInt32 & aCount);
> 
>-  nsIDeviceContext   *mDeviceContext;
>   nsFont             *mFont;
>   nsFontXlib         *mWesternFont;
> 

Why does mDeviceContext become public?

>diff -N -r -u original/mozilla/gfx/src/xprint/nsXPrintContext.cpp mozilla/gfx/src/xprint/nsXPrintContext.cpp
>--- original/mozilla/gfx/src/xprint/nsXPrintContext.cpp	Sun Nov 17 06:16:15 2002
>+++ mozilla/gfx/src/xprint/nsXPrintContext.cpp	Mon Dec 30 11:03:38 2002
>@@ -206,7 +206,7 @@
> 
>   XlibRgbArgs xargs;
>   memset(&xargs, 0, sizeof(xargs));
>-  xargs.handle_name           = "xprint";
>+  xargs.handle_name           = nsnull;
>   xargs.disallow_image_tiling = True; /* XlibRGB's "Image tiling"-hack is 
>                                        * incompatible to Xprint API */
>   

What does this change do?
Attachment #110502 - Attachment is obsolete: true
Simon Montagu wrote:
> (From update of attachment 110502 [details] [diff] [review])
> Basically this looks good, but some things are unclear to me, and I have a few
> nits:
> 
> Firstly, do *all* the globals need to become part of the per-device structure?
> Some of them seem to be functioning more like constants, e.g.  gUnicode,
> gUserDefined, gWesternLocale, and maybe also gUsersLocale. Are there technical
> reasons why these can't remain globals?

Well, I moved all vars which start with ^g[A-Z] into the per-device context. One
advantage is that I now can use nsCOMPtr for resource tracking (which was
previously not possible since global nsCOMPtr's are forbidden per mozilla.org's
coding policy), another one is that I do not need any tracking and locking
mechanisms for sharing of these objects.
This point was the main motivation - doing a _complete_ _seperation_ between two
device instances, avoiding that there may be _any_ possible mixup/interaction
between two devices - I simply do not want to end-up like the gfx/src/windows
code which does some "smart" sharing between display and print device code -
which works in most cases - but due a small, still unknown flaw some drivers go
nuts and send the whole kernel to h*ll(=BSOD crashes due issues in our GDI API
usage). I don't want to find myself in that h*ll, too.

> Please remove (or put in #ifdef DEBUG) every printf, puts, etc.

I've changed these parts to use the PR_LOG-facility...

> >+/* Copy an array of nsFontCharSetMapXlib structures:
> >+ * |mName| is unique, |mFontLangGroup| and |mInfo| must be copied
> >+ * without creating duplicates.
> >+ */
> >+static
> >+PRBool CopyFontCharSetMapXlib(nsFontMetricsXlibContext *fmctx)
> 
> I don't understand what this method is doing and why. "|mFontLangGroup| and
> |mInfo| must be copied without creating duplicates" is not very clear. Can you
> add more comments specifying the input and the output, what exactly you are
> preventing by not creating duplicates
> (what is not allowed to be a duplicate of what and why not),
> and how you are preventing it.

-- snip --
|CopyFontCharSetMapXlib()| copies |gCharSetMap|, |gNoneCharSetMap| and
|gSpecialCharSetMap| (now named |gConst...|) and it's linked structures since
they are being used read/write by the X11 fontmetrics code. Duplicates need to
be avoided since the pointers of the structures are being used for comparisation
("equal", "not equal") - having duplicate structures with the same content but
different pointers will cause havoc (either crash or the code misbehaves).
-- snip --
Is that comment OK ?

> >-  if (mDeviceContext) {
> >+  if (mDeviceContext)
> >+  {
> 
> Hyper-nit: don't change the brace positioning from the prevailing style.

Mhhh, I only saw one occurence of that mistake... fixed... hopefully... :)

> >@@ -1665,7 +1817,7 @@
> > 
> > NS_IMETHODIMP  nsFontMetricsXlib::Destroy()
> > {
> >-  mDeviceContext = nsnull;
> 
> Why is this line removed?

Because I tried to remove |mDeviceContext| during patch development and had to
undo that change after figuring out that this was a bad idea.
Fixed.

[snip]
> >@@ -445,11 +428,13 @@
> >   PRUint8           mTriedAllGenerics;
> >   PRUint8           mIsUserDefined;
> > 
> >+  nsFontMetricsXlibContext *mFontMetricsContext;
> >+  nsIDeviceContext         *mDeviceContext;
> >+
> > protected:
> >   void RealizeFont();
> >   nsFontXlib *LocateFont(PRUint32 aChar, PRInt32 & aCount);
> > 
> >-  nsIDeviceContext   *mDeviceContext;
> >   nsFont             *mFont;
> >   nsFontXlib         *mWesternFont;
> > 
> 
> Why does mDeviceContext become public?

|mFontMetricsContext| and |mDeviceContext| both reference device information
(|mFontMetricsContext| the fontmetrics context for the physical device,
|mDeviceContext| the reference for the logical nsIDeviceContext info.
Either both are |public| or both are |protected|/|private| (to be consistent).
Since |mFontMetricsContext| must be |public| (otherwise the compillation aborts
with 'gfx/src/xlib/nsFontMetricsXlib.cpp", line 5178: Error: mFontMetricsContext
is not accessible from PrefEnumCallback(const char*, void*).' I changed
|mDeviceContext| to |public|, too.

> >diff -N -r -u original/mozilla/gfx/src/xprint/nsXPrintContext.cpp
mozilla/gfx/src/xprint/nsXPrintContext.cpp
> >--- original/mozilla/gfx/src/xprint/nsXPrintContext.cpp        Sun Nov 17
06:16:15 2002
> >+++ mozilla/gfx/src/xprint/nsXPrintContext.cpp Mon Dec 30 11:03:38 2002
> >@@ -206,7 +206,7 @@
> > 
> >   XlibRgbArgs xargs;
> >   memset(&xargs, 0, sizeof(xargs));
> >-  xargs.handle_name           = "xprint";
> >+  xargs.handle_name           = nsnull;
> >   xargs.disallow_image_tiling = True; /* XlibRGB's "Image tiling"-hack is 
> >                                        * incompatible to Xprint API */
> 
> What does this change do?

XlibRgb library has an API to register |XlibRgbHandle| handles via a "public
name" - which can later be looked-up using that name (that was required by
widget/plugin code since global vars cannot be used across module bounds,
therefore I added this "public handle name" API to pass the main display handle
around between widget, gfx and plugin module code). The API requires that unique
names must be choosen for public handle names - however the Xprint module code
never made use of that feature, so registering a name is both a NO-OP and it
would not be possible to obtain a |XlibRgbHandle| handle with the same public
name twice (or more often).
Attachment #110686 - Flags: review?(smontagu)
Attachment #110686 - Attachment is obsolete: true
Attachment #110822 - Attachment is obsolete: true
Comment on attachment 110823 [details] [diff] [review]
More adjustments per smontagu's IRC comments [CHECKED-IN]

r=smontagu
Attachment #110823 - Flags: review+
Comment on attachment 110823 [details] [diff] [review]
More adjustments per smontagu's IRC comments [CHECKED-IN]

Requesting sr= ...
Attachment #110823 - Flags: superreview?(bzbarsky)
Comment on attachment 110823 [details] [diff] [review]
More adjustments per smontagu's IRC comments [CHECKED-IN]

Over to rbs, bz is little big overburned and we have to catch 1.3b before the
tree freezes...
Attachment #110823 - Flags: superreview?(bzbarsky) → superreview?(rbs)
Comment on attachment 110823 [details] [diff] [review]
More adjustments per smontagu's IRC comments [CHECKED-IN]

looks reasonable although mPrefs / mFontSubConverter seem overkill, but keeping
such a total migration makes the code uniform/easier to read/maintain.
sr=rbs
Attachment #110823 - Flags: superreview?(rbs) → superreview+
Still incomplete:
ToDo:
- needs to be ported to gfx/src/xlib and gfx/src/qt ...
Attachment #110512 - Attachment is obsolete: true
This patch enables the print preview GUI again if the underlying device code
supports multiple devicecontext instances (and it fixes a nit from the previous
patch where a nsRenderingContextXlib global variable was not moved into a
handle, causing memory corruption) ...
Attachment #111009 - Attachment is obsolete: true
Attachment #111015 - Flags: review?(smontagu)
Comment on attachment 111015 [details] [diff] [review]
Patch for 2003-01-06-08-trunk to get the print preview GUI enabled again (incl. glue for GTK+, Qt and Xlib toolkits) [CHECKED-IN]

r=smontagu
Attachment #111015 - Flags: review?(smontagu) → review+
Comment on attachment 111015 [details] [diff] [review]
Patch for 2003-01-06-08-trunk to get the print preview GUI enabled again (incl. glue for GTK+, Qt and Xlib toolkits) [CHECKED-IN]

Requesting sr= ...
Attachment #111015 - Flags: superreview?(rbs)
Comment on attachment 111015 [details] [diff] [review]
Patch for 2003-01-06-08-trunk to get the print preview GUI enabled again (incl. glue for GTK+, Qt and Xlib toolkits) [CHECKED-IN]

sr=rbs
Attachment #111015 - Flags: superreview?(rbs) → superreview+
Comment on attachment 111015 [details] [diff] [review]
Patch for 2003-01-06-08-trunk to get the print preview GUI enabled again (incl. glue for GTK+, Qt and Xlib toolkits) [CHECKED-IN]

Patch checked-in
(http://bonsai.mozilla.org/cvsquery.cgi?module=MozillaTinderboxAll&branch=HEAD&
cvsroot=/cvsroot&date=explicit&mindate=1042159800&maxdate=1042160340&who=bzbars
ky%25mit.edu) ...
Attachment #111015 - Attachment description: Patch for 2003-01-06-08-trunk to get the print preview GUI enabled again (incl. glue for GTK+, Qt and Xlib toolkits) → Patch for 2003-01-06-08-trunk to get the print preview GUI enabled again (incl. glue for GTK+, Qt and Xlib toolkits) [CHECKED-IN]
Attachment #111015 - Attachment is obsolete: true
Comment on attachment 110823 [details] [diff] [review]
More adjustments per smontagu's IRC comments [CHECKED-IN]

Patch was checked-in yesterday
(http://bonsai.mozilla.org/cvsquery.cgi?module=MozillaTinderboxAll&branch=HEAD&
cvsroot=/cvsroot&date=explicit&mindate=1042072260&maxdate=1042073340&who=smonta
gu%25netscape.com) by smontagu...
Attachment #110823 - Attachment description: More adjustments per smontagu's IRC comments → More adjustments per smontagu's IRC comments [CHECKED-IN]
Attachment #110823 - Attachment is obsolete: true
Attachment #111082 - Attachment is obsolete: true
Patch for 2003-01-06-08-trunk to get "speedracer" tinderbox green again
(currently we crash in |CopyFontCharSetMapXlib|).
It seems that gcc on SPARC does not ensure memory alignment requirements like
Sun Workshop does (non-RISC platforms are not affected by this issue).
Previous patch's |ALIGN_PTR()| did not round-up... ;-(
Attachment #111173 - Attachment is obsolete: true
Comment on attachment 111180 [details] [diff] [review]
Better patch for 2003-01-06-08-trunk to get "speedracer" tinderbox green again [CHECKED-IN]

I've been asked to review this on IRC (actually, the previous patch, but I
found a bug) and this appears to be bug-free although I've no way to be 100%
sure.
Attachment #111180 - Flags: review?(neil)
Comment on attachment 111180 [details] [diff] [review]
Better patch for 2003-01-06-08-trunk to get "speedracer" tinderbox green again [CHECKED-IN]

for what it's worth...
Attachment #111180 - Flags: review?(neil) → review+
Attachment #111180 - Flags: superreview?(rbs)
Comment on attachment 111180 [details] [diff] [review]
Better patch for 2003-01-06-08-trunk to get "speedracer" tinderbox green again [CHECKED-IN]

sr=rbs

(I wonder if CopyFontCharSetMapXlib can't be rewritten differently. Why didn't
you code it just as a memcpy?)
Attachment #111180 - Flags: superreview?(rbs) → superreview+
rbs wrote:
> sr=rbs

Thanks! :)

> (I wonder if CopyFontCharSetMapXlib can't be rewritten differently. Why didn't
> you code it just as a memcpy?)

Take a look at the comment above the function:
-- snip --
 * When copying, we ensure that structures with identical content 
 * have identical pointers (since fontmetrics code uses such pointers
 * for |equal|, |not equal| comparisons etc.).
 */
-- snip --
A |memcpy()| would only do half the job - we have to adjust the pointers of the
linked structures and create copies of them, too.

Maybe another approach would be to do three |calloc()|s (which would return
properly aligned pointers automagically) instead of one, but I thought this
solution has less overhead and requires less compliciated error-checking (but
obviously I found a new way to shoot myself into the feet).

----

ToDo:
- I'll file a bug to get these macros integrated into the NSPR headers...
Comment on attachment 111180 [details] [diff] [review]
Better patch for 2003-01-06-08-trunk to get "speedracer" tinderbox green again [CHECKED-IN]

Patch checked-in
(http://bonsai.mozilla.org/cvsquery.cgi?module=MozillaTinderboxAll&branch=HEAD&
cvsroot=/cvsroot&date=explicit&mindate=1042275540&maxdate=1042277040&who=bryner
%25netscape.com) ...
Attachment #111180 - Attachment description: Better patch for 2003-01-06-08-trunk to get "speedracer" tinderbox green again → Better patch for 2003-01-06-08-trunk to get "speedracer" tinderbox green again [CHECKED-IN]
Attachment #111180 - Attachment is obsolete: true
Attachment #110686 - Flags: review?(smontagu)
Comment on attachment 111316 [details] [diff] [review]
Port the PrintPreview glue change from mozilla/xpfe/communicator/resources/content/printPreviewBindings.xml to Phoenix's mozilla/browser/base/content/printPreviewBindings.xml [CHECKED-IN]

Requesting r= (and checkin, too) for the Phoenix version of the patch (this is
a 1:1 port from the mozillA/xpfe changes) ...
Attachment #111316 - Flags: review?(bryner)
Comment on attachment 111316 [details] [diff] [review]
Port the PrintPreview glue change from mozilla/xpfe/communicator/resources/content/printPreviewBindings.xml to Phoenix's mozilla/browser/base/content/printPreviewBindings.xml [CHECKED-IN]

checked in
Attachment #111316 - Flags: review?(bryner) → review+
Filed bug 188849 ("Need macro to align a memory pointer to match the platform's
memory alignment policy") to get the alignment macros moved to NSPR...
Attachment #111316 - Attachment description: Port the PrintPreview glue change from mozilla/xpfe/communicator/resources/content/printPreviewBindings.xml to Phoenix's mozilla/browser/base/content/printPreviewBindings.xml → Port the PrintPreview glue change from mozilla/xpfe/communicator/resources/content/printPreviewBindings.xml to Phoenix's mozilla/browser/base/content/printPreviewBindings.xml [CHECKED-IN]
Attachment #111316 - Attachment is obsolete: true
> This point was the main motivation - doing a _complete_ _seperation_ between
> two device instances, avoiding that there may be _any_ possible
> mixup/interaction between two devices - I simply do not want to end-up like
> the gfx/src/windows code which does some "smart" sharing between display and
> print device code - which works in most cases - but due a small, still unknown
> flaw some drivers go nuts and send the whole kernel to h*ll(=BSOD crashes due
> issues in our GDI API usage). I don't want to find myself in that h*ll, too.

Got a bug number about this?
Blocks: 125482
Attachment #110502 - Flags: review?(smontagu)
Comment on attachment 110823 [details] [diff] [review]
More adjustments per smontagu's IRC comments [CHECKED-IN]

>+  nsCOMPtr<nsIAtom>                     mUsersLocale;

>-static nsIAtom* gUsersLocale = nsnull;

>@@ -1227,26 +1410,27 @@
>   nsCOMPtr<nsILanguageAtomService> langService;
>   langService = do_GetService(NS_LANGUAGEATOMSERVICE_CONTRACTID);
>   if (langService) {
>-    langService->GetLocaleLanguageGroup(&gUsersLocale);
>+    nsIAtom *atom;
>+    langService->GetLocaleLanguageGroup(&atom);
>+    mUsersLocale = atom;

leak (and you went to quite a bit of trouble to leak, rather than just using
getter_AddRefs).

>-  if (!gUsersLocale) {
>-    gUsersLocale = NS_NewAtom("x-western");
>+  if (!mUsersLocale) {
>+    mUsersLocale = NS_NewAtom("x-western");

leak.

Likewise for some of the other variables.
Target milestone should probably go to Future because mozilla1.3beta was a long time ago.
Is this still relevant with Gtk2 and Thebes? If not I'll WONTFIX this...
No reaction probably means that nobody disagrees.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → WONTFIX
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: