235188 - RFE: Mozilla should have an option to run X11 in threadsafe mode

Status: RESOLVED FIXED

(Core Graveyard :: GFX, defect)

Description

[Roland Mainz]

RFE: Mozilla should have an option to run X11 in threadsafe mode.

We need such an option for various tests, incl. the new flash plugin (and to
test whether it is possible to run plugins in seperate threads to decouple them
from the main input thread). This is also a preparation to make this option the
default in one or two years...

Comment 1

[Roland Mainz]

Attachment: Patch for 2004-02-21-trunk

Comment 2

[Roland Mainz]

Comment on attachment 141942 [details] [diff] [review]
Patch for 2004-02-21-trunk

Requesting r=(sr= and checkin...

The patch does not change any default unless either the commandline option or
the env var is set.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 141942 [details] [diff] [review]
Patch for 2004-02-21-trunk

Blizzard should look at this.

Please use #if defined(MOZ_X11) instead of that disjunction.

Comment 4

[Roland Mainz]

Attachment: New patch per roc's comments

The version also fixes the problem that the env variable check only worked when
there are any additional command line arguments (and checking the env var for
each loop cycle wasn't a good idea either).

Comment 5

[Roland Mainz]

Comment on attachment 142074 [details] [diff] [review]
New patch per roc's comments

Requestint sr= ... quick review would ne nice since we'd like to test mozilla
running in threadsafe mode against Xfree86V4.4.0RC3 and fix any outstanding
issues for that release.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  if (PL_strcmp(PR_GetEnv("MOZILLA_X11_XINITTHREADS"), "1") == 0) {

won't this crash if the variable is not defined?

Comment 7

[Roland Mainz]

Robert O'Callahan wrote:
> +  if (PL_strcmp(PR_GetEnv("MOZILLA_X11_XINITTHREADS"), "1") == 0) {
> won't this crash if the variable is not defined?

No, many of the PR_*-functions allow |nsnull|-pointers.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I *really* think Chris needs to sign off on this. Please hassle him on IRC or do
whatever you have to do to get his attention.

Comment 9

[Christopher Blizzard (:blizzard)]

Comment on attachment 142074 [details] [diff] [review]
New patch per roc's comments

>+  for( int i=1 ; i < argc ; i++ ) {

Please pick a consistent style.  This clashes with if (foo) later.  Better yet,
just use local style.

>+    if (PL_strcmp(argv[i], "-xinitthreads") == 0) {
>+      x11threadsafe = PR_TRUE;
>+      break;
>+    }
>+  }

Some vertical whitespace here, please.

>+  if (PL_strcmp(PR_GetEnv("MOZILLA_X11_XINITTHREADS"), "1") == 0) {
>+    x11threadsafe = PR_TRUE;
>+  }

This could just be shortened to:

if (PR_GetEnv("MOZILLA_X11_XINITTHREADS")) {

Comment 10

[Roland Mainz]

Attachment: Patch for 2004-03-03-trunk per blizzard's comments

Comment 11

[Roland Mainz]

Christopher Blizzard wrote:
> (From update of attachment 142074 [details] [diff] [review])
> >+  for( int i=1 ; i < argc ; i++ ) {
>
> Please pick a consistent style.  This clashes with if (foo) later.  Better 
> yet, just use local style.

Fixed. I've copied the |for()|-statement below 1:1 :)

> >+    if (PL_strcmp(argv[i], "-xinitthreads") == 0) {
> >+      x11threadsafe = PR_TRUE;
> >+      break;
> >+    }
> >+  }
> 
> Some vertical whitespace here, please.

Fixed.

> >+  if (PL_strcmp(PR_GetEnv("MOZILLA_X11_XINITTHREADS"), "1") == 0) {
>
> This could just be shortened to:
> 
> if (PR_GetEnv("MOZILLA_X11_XINITTHREADS")) {

Fixed.

Comment 12

[Roland Mainz]

Neil checked the patch "in" a while ago
(http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/xpfe/bootstrap&command=DIFF_FRAMESET&file=nsAppRunner.cpp&rev1=1.408&rev2=1.409&root=/cvsroot),
marking bug as FIXED.