187125 - GTK+ and Xlib gfx should support multiple device instances

Status: RESOLVED WONTFIX

(Core Graveyard :: GFX, enhancement)

Description

[Roland Mainz]

RFE: GTK+ and Xlib gfx should support multiple independant device instances.

Comment 1

[Roland Mainz]

Attachment: Xlib gfx patch for 2002-12-28-08-trunk

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

Comment 2

[Roland Mainz]

Attachment: Demonstrator patch to enable PP's toolbar based on the printer name (still incomplete)

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

Comment 3

[Simon Montagu :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?

Comment 4

[Roland Mainz]

Attachment: New Xlib patch for 2002-12-28-08-trunk per smontagu's comments

Comment 5

[Roland Mainz]

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

Comment 6

[Roland Mainz]

Attachment: New patch for 2002-12-28-08-trunk per smontagu's IRC comments

Comment 7

[Roland Mainz]

Attachment: More adjustments per smontagu's IRC comments [CHECKED-IN]

Comment 8

[Simon Montagu :smontagu]

Comment on attachment 110823 [details] [diff] [review]
More adjustments per smontagu's IRC comments [CHECKED-IN]

r=smontagu

Comment 9

[Roland Mainz]

Comment on attachment 110823 [details] [diff] [review]
More adjustments per smontagu's IRC comments [CHECKED-IN]

Requesting sr= ...

Comment 10

[Roland Mainz]

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...

Comment 11

[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

Comment 12

[Roland Mainz]

Attachment: Patch for 2003-01-06-08-trunk to get the print preview GUI enabled again

Still incomplete:
ToDo:
- needs to be ported to gfx/src/xlib and gfx/src/qt ...

Comment 13

[Roland Mainz]

Attachment: 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]

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

Comment 14

[Roland Mainz]

Attachment: Same patch as previous one (=attachment 111015), but diffed using % gdiff -u15 # for better review

Comment 15

[Simon Montagu :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

Comment 16

[Roland Mainz]

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= ...

Comment 17

[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

Comment 18

[Roland Mainz]

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

Comment 19

[Roland Mainz]

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...

Comment 20

[Roland Mainz]

Attachment: Patch for 2003-01-06-08-trunk to get "speedracer" tinderbox green again

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

Comment 21

[Roland Mainz]

Attachment: Better patch for 2003-01-06-08-trunk to get "speedracer" tinderbox green again [CHECKED-IN]

Previous patch's |ALIGN_PTR()| did not round-up... ;-(

Comment 22

[[e-mail]]

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.

Comment 23

[[e-mail]]

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...

Comment 24

[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?)

Comment 25

[Roland Mainz]

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 26

[Roland Mainz]

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

Comment 27

[Roland Mainz]

Attachment: Port the PrintPreview glue change from mozilla/xpfe/communicator/resources/content/printPreviewBindings.xml to Phoenix's mozilla/browser/base/content/printPreviewBindings.xml [CHECKED-IN]

Comment 28

[Roland Mainz]

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

Comment 29

[Brian Ryner (not reading)]

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

Comment 30

[Roland Mainz]

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...

Comment 31

[rbs]

> 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?

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 33

[AndrewM]

Target milestone should probably go to Future because mozilla1.3beta was a long time ago.

Comment 34

[Peter Weilbacher]

Is this still relevant with Gtk2 and Thebes? If not I'll WONTFIX this...

Comment 35

[Peter Weilbacher]

No reaction probably means that nobody disagrees.