509179 - Make NS_ENABLE_TSF configurable

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Masatoshi Kimura [:emk]]

Attachment: patch

This patch will:
1. add --disable-tsf configure option, and
2. disable TSF support when using 2003SP1 SDK.

Comment 1

[Rain [:rain1] (inactive)]

Comment on attachment 393338 [details] [diff] [review]
patch

>+    NS_ENABLE_TSF=1
>+    if test "$ac_cv_header_msctf_idl" = "no"; then
>+        AC_MSG_WARN([TSF header msctf.idl not found, disabling TSF support.])
>+        NS_ENABLE_TSF=
>+    fi
>+    ;;
>+*)
>+    NS_ENABLE_TSF=
>+    ;;

Pretty sure disabling things only based on what's available is taboo.

Comment 2

[Masatoshi Kimura [:emk]]

Why? luser suggested it in bug 88831 comment #151.
> Can we skip compiling this code if using an older SDK? We can add a header
> check to configure, or just use the windows target version. You can see an
> example of code that's conditionally compiled when targeting the Vista SDK or
Do I misread the comment?
Moreover, parental control will be disabled automatically if the SDK is older.
> if test "$MOZ_WINSDK_TARGETVER" -lt "06000000"; then
>     MOZ_DISABLE_VISTA_SDK_REQUIREMENTS=1
>     AC_DEFINE(MOZ_DISABLE_VISTA_SDK_REQUIREMENTS)
>     # We can't build parental controls either
>     MOZ_DISABLE_PARENTAL_CONTROLS=1
> fi
Furthermore, WinCE team also breaks a "taboo".
>         if test -z "$HAS_DDRAW"; then
>             AC_MSG_WARN([DirectDraw ddraw.h header not found or it's missing DDLOCK_WAITNOTBUSY, disabling DirectDraw surface.  If you have an older SDK (such as the CE5 SDK), try copying in ddraw.lib and ddraw.h from the WM6 SDK.])
>             DDRAW_SURFACE_FEATURE=
>         else
>             DDRAW_SURFACE_FEATURE="#define CAIRO_HAS_DDRAW_SURFACE 1"
>         fi
>             if test -z "$HAS_OGLES"; then
>               AC_MSG_WARN([OpenGL ES2 headers not found, disabling OpenGL acceleration surfaces.])
>               OGLES_SURFACE_FEATURE=
>             else
>               OGLES_SURFACE_FEATURE="#define CAIRO_DDRAW_USE_GL 1"
>             fi

Comment 3

[Rain [:rain1] (inactive)]

(In reply to comment #2)
> Moreover, parental control will be disabled automatically if the SDK is older.
> > if test "$MOZ_WINSDK_TARGETVER" -lt "06000000"; then
> >     MOZ_DISABLE_VISTA_SDK_REQUIREMENTS=1
> >     AC_DEFINE(MOZ_DISABLE_VISTA_SDK_REQUIREMENTS)
> >     # We can't build parental controls either
> >     MOZ_DISABLE_PARENTAL_CONTROLS=1
> > fi

Yeah, that's an explicit configuration, given by ac_add_options --with-windows-version=502.

Comment 4

[Rain [:rain1] (inactive)]

(In reply to comment #2)
> Furthermore, WinCE team also breaks a "taboo".
> >         if test -z "$HAS_DDRAW"; then
> >             AC_MSG_WARN([DirectDraw ddraw.h header not found or it's missing DDLOCK_WAITNOTBUSY, disabling DirectDraw surface.  If you have an older SDK (such as the CE5 SDK), try copying in ddraw.lib and ddraw.h from the WM6 SDK.])
> >             DDRAW_SURFACE_FEATURE=
> >         else
> >             DDRAW_SURFACE_FEATURE="#define CAIRO_HAS_DDRAW_SURFACE 1"
> >         fi
> >             if test -z "$HAS_OGLES"; then
> >               AC_MSG_WARN([OpenGL ES2 headers not found, disabling OpenGL acceleration surfaces.])
> >               OGLES_SURFACE_FEATURE=
> >             else
> >               OGLES_SURFACE_FEATURE="#define CAIRO_DDRAW_USE_GL 1"
> >             fi

Dunno about this, but bsmedberg mentioned this on IRC, saying that this looks wrong.

Comment 5

[Masatoshi Kimura [:emk]]

Attachment: patch v2

> Yeah, that's an explicit configuration, given by ac_add_options
> --with-windows-version=502.
Oh, I'd forgotten about that. Thanks for pointing out.
Now --with-windows-version=502 or --disable-tsf will be required to disable TSF support.
I've also removed wpcapi.h check because the result is not used anywhere.

Comment 6

[(not currently active) Ted Mielczarek]

Do you think this needs a separate configure option? We should be able to just make it conditional on the target SDK version, right? Is there any reason anyone would want to disable this code otherwise? We have a lot of old configure options, I'd rather not add new ones that aren't going to be widely used.

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 393363 [details] [diff] [review]
patch v2

Let's just condition this on the SDK version in use, instead of adding another configure flag.

Comment 8

[Masatoshi Kimura [:emk]]

Attachment: remove --disable-tsf

Sorry, I overlooked comment #6.

Comment 9

[Masatoshi Kimura [:emk]]

Attachment: remove --disable-tsf

Oops, the previous patch contains unrelated fix.

Comment 10

[Masatoshi Kimura [:emk]]

Attachment: remove --disable-tsf

widget/tests/Makefile.in was missing from the previous patch.

Comment 11

[(not currently active) Ted Mielczarek]

Comment on attachment 395061 [details] [diff] [review]
remove --disable-tsf

+        AC_SUBST(NS_ENABLE_TSF)

Just stick the AC_SUBST here down with the rest of the AC_SUBST calls in configure.in. Otherwise this looks fine.

Comment 12

[Masatoshi Kimura [:emk]]

Attachment: moved AC_SUBST(NS_ENABLE_TSF) down [Checkin: Comment 13]

Comment 13

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

http://hg.mozilla.org/mozilla-central/rev/c5a789064023

Comment 14

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1-CC) Port (the useful part of) it to comm-central (1.9.2+)

Comment 15

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1) Copy (the useful part of) it to /js/src

Comment 16

[(not currently active) Ted Mielczarek]

Comment on attachment 428613 [details] [diff] [review]
(Cv1) Copy (the useful part of) it to /js/src

js doesn't actually need any of those headers. If you want to change this, just rip that whole block out.

Comment 17

[Serge Gautherie (:sgautherie)]

(In reply to comment #16)
> (From update of attachment 428613 [details] [diff] [review])
> js doesn't actually need any of those headers. If you want to change this, just
> rip that whole block out.

Well, I didn't mean to do more clean-up than needed to sync'...

Yet, what about these:
http://mxr.mozilla.org/mozilla-central/search?string=atlbase&find=%2Fjs%2Fsrc%2F
?
atlbase.h at least seems to be used (just like in Core), thus a check could be worthwhile?
I mean: is this check(s) useless in Core too then?

Comment 18

[(not currently active) Ted Mielczarek]

xpconnect, while living under js/src, isn't built as part of js/src, it's built from the top-level:
http://mxr.mozilla.org/mozilla-central/source/toolkit/toolkit-tiers.mk#88

The system-headers bit is ignorable, it's just from keeping the js/src copy in sync with the top-level one.

Comment 19

[Serge Gautherie (:sgautherie)]

Attachment: (Cv2) /js/src/configure.in: Copy (the useful part of) it, Remove checks for oleacc.idl and atlbase.h too [Checkin: Comment 21]

Thanks for your explanation.

Comment 20

[Serge Gautherie (:sgautherie)]

Attachment: (Bv2-CC) Copy (the useful part of) it to comm-central, Remove checks for oleacc.idl and atlbase.h too, (1.9.2+)

Bv1-CC, with comment 16 suggestion(s).

Comment 21

[Serge Gautherie (:sgautherie)]

Comment on attachment 428871 [details] [diff] [review]
(Cv2) /js/src/configure.in: Copy (the useful part of) it, Remove checks for oleacc.idl and atlbase.h too
[Checkin: Comment 21]


http://hg.mozilla.org/mozilla-central/rev/7b5ae43691b2

Comment 22

[Serge Gautherie (:sgautherie)]

Attachment: (Bv2a-CC) Copy (the useful part of) it to comm-central, Remove checks for oleacc.idl and atlbase.h too, (1.9.2+) [Checkin: Comment 23]

Bv2-CC, without the chunk now removed in bug 513709.

Comment 23

[Serge Gautherie (:sgautherie)]

Comment on attachment 430054 [details] [diff] [review]
(Bv2a-CC) Copy (the useful part of) it to comm-central, Remove checks for oleacc.idl and atlbase.h too, (1.9.2+)
[Checkin: Comment 23]


http://hg.mozilla.org/comm-central/rev/2bfabf4b8711