To add a new compile switch to distinguish Windows and ATK code

CLOSED FIXED

Status

SeaMonkey
Build Config
CLOSED FIXED
16 years ago
13 years ago

People

(Reporter: Kyle Yuan, Assigned: John Sun)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

V3
547 bytes, patch
hacker formerly known as seawood@netscape.com
: review+
Details | Diff | Splinter Review
(Reporter)

Description

16 years ago
So that we can use 

#ifdef ACCESSIBILITY_ATK
//ATK code here
#else
//Windows code here
#endif

to keep the binaries smaller.
(Reporter)

Updated

16 years ago
Blocks: 136315

Comment 1

16 years ago
yuck. no no no.

what is wrong with #ifdef XP_WIN ??
(Reporter)

Comment 2

16 years ago
timeless, we need this switch not only in accessibility code, but also in other 
modules, see:
http://lxr.mozilla.org/seamonkey/source/layout/html/forms/src/nsListControlFrame
.cpp#1478

However, #ifdef XP_WIN is enough for our current developing. We can decide 
whether need that switch in the future.
(Assignee)

Comment 3

16 years ago
Pls refer to http://bugzilla.mozilla.org/show_bug.cgi?id=153816
I'll add a macro MOZ_ACCESSIBILITY_ATK in mozilla/configure.
Its preconditions are XP_UNIX and MOZ_ENABLE_GTK2 defined. It make sense?
( XP_UNIX don't include BeOS and OS2 )
Status: NEW → ASSIGNED
(Reporter)

Comment 4

16 years ago
IMO, MOZ_ACCESSIBILITY_ATK = XP_UNIX && ACCESSIBILITY && MOZ_ENABLE_GTK2
(Assignee)

Comment 5

16 years ago
execuse me, even forget ACCESSIBILITY. 
(Assignee)

Comment 6

16 years ago
Created attachment 89372 [details] [diff] [review]
patch-v1

Seek r=?

Just add a macro MOZ_ACCESSIBILITY_ATK in mozilla/configure for accessbility
work.
Don't affect anything else.
(Reporter)

Comment 7

16 years ago
Is mozilla/configure generated by mozilla/configure.in? someone told me that, 
but I forgot.
aaron, who is the right person to r= this patch?

Comment 8

16 years ago
I think Chris Seawood (IRC cls, seawood@netscape.com)
Attachment #89372 - Flags: needs-work+
Comment on attachment 89372 [details] [diff] [review]
patch-v1

The patch should be against configure.in, not configure.  It seems wrong that
there are 3 different places affected by adding a AC_DEFINE() but I 'll wait
for the patch against configure.in to be certain.
(Assignee)

Comment 10

16 years ago
Created attachment 89382 [details] [diff] [review]
V2

Thank kyle for your reminding.
autoconf make configure.in -> configure.

seek r=
Attachment #89372 - Attachment is obsolete: true
(Reporter)

Updated

16 years ago
Blocks: 153852
Attachment #89382 - Flags: needs-work+
Comment on attachment 89382 [details] [diff] [review]
V2

Why recreate those specific platform tests just for this define?  About 10
lines up, there's a if-else tree that does what you're appearing to do here,
namely isolate the "unix" case.  And you missed an OpenVMS check.  I'm not sure
if the platform tests are necessary as gtk2 is only used on unix systems (so
far).
(Assignee)

Comment 12

16 years ago
Chris,

Because you don't like add AC_DEFINE in 3 different place(see commment #9),
I merge them to one place and have to re-test the platform. 
My platform test is equal to that done for XP_UNIX. Since I don't know how to
check if a marco define in AC_DEFINE, use a long check expression.

although gtk2 is not only used in unix system, mozilla don't use gtk2 as a base of
rendering in BeOS, WINNT and OS2, so our accessiblity featurev based on ATK will
invalid. 

Component: Accessibility APIs → Build Config
When you say that "Its preconditions are XP_UNIX and MOZ_ENABLE_GTK2 defined",
are you assuming that a platform that defines XP_UNIX is building using X11? 
That is an incorrect assumption.  XP_UNIX is also set on OSX & OpenVMS which do
not use gtk2 nor any other X11 toolkit by default.  You should stick with the
feature checks and just set MOZ_ACCESSIBILITY_GTK if ACCESSIBILITY & GTK2 are
enabled.  I don't understand where the XP_UNIX dependency comes in if it is not
used to assume X11.
(Assignee)

Comment 14

16 years ago
Created attachment 89504 [details] [diff] [review]
V3

Chris,
You are right. Thanks for you clarification.

seek you review again.
Attachment #89382 - Attachment is obsolete: true
Comment on attachment 89504 [details] [diff] [review]
V3

r=cls
Attachment #89504 - Flags: review+

Comment 16

16 years ago
checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
(Assignee)

Comment 17

16 years ago
Thanks  peter's help.
close the bug.
Status: RESOLVED → CLOSED
(Reporter)

Comment 18

16 years ago
John, you should use VERIFIED instead of CLOSED.
(Assignee)

Comment 19

16 years ago
Thank Peter and Kyle's reminding, I'll do as what you said for other bugs.
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.