Closed Bug 509179 Opened 11 years ago Closed 11 years ago

Make NS_ENABLE_TSF configurable

Categories

(Firefox Build System :: General, defect)

x86_64
Windows 7
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: emk, Assigned: emk)

References

(Depends on 1 open bug)

Details

(Keywords: inputmethod)

Attachments

(3 files, 8 obsolete files)

5.31 KB, patch
emk
: review+
Details | Diff | Splinter Review
1.01 KB, patch
ted
: review+
Details | Diff | Splinter Review
2.18 KB, patch
Callek
: review+
Details | Diff | Splinter Review
Attached patch patch (obsolete) — Splinter Review
This patch will:
1. add --disable-tsf configure option, and
2. disable TSF support when using 2003SP1 SDK.
Attachment #393338 - Flags: review?(ted.mielczarek)
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.
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
(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.
(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.
Attached patch patch v2 (obsolete) — Splinter Review
> 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.
Assignee: nobody → VYV03354
Attachment #393338 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #393363 - Flags: review?(ted.mielczarek)
Attachment #393338 - Flags: review?(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 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.
Attachment #393363 - Flags: review?(ted.mielczarek) → review-
Attached patch remove --disable-tsf (obsolete) — Splinter Review
Sorry, I overlooked comment #6.
Attachment #393363 - Attachment is obsolete: true
Attachment #394897 - Flags: review?(ted.mielczarek)
Attached patch remove --disable-tsf (obsolete) — Splinter Review
Oops, the previous patch contains unrelated fix.
Attachment #394897 - Attachment is obsolete: true
Attachment #394900 - Flags: review?(ted.mielczarek)
Attachment #394897 - Flags: review?(ted.mielczarek)
Attached patch remove --disable-tsf (obsolete) — Splinter Review
widget/tests/Makefile.in was missing from the previous patch.
Attachment #394900 - Attachment is obsolete: true
Attachment #395061 - Flags: review?(ted.mielczarek)
Attachment #394900 - Flags: review?(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.
Attachment #395061 - Flags: review?(ted.mielczarek) → review+
Attachment #395061 - Attachment is obsolete: true
Attachment #395297 - Flags: review+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/c5a789064023
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Attachment #428613 - Flags: review?(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.
Attachment #428613 - Flags: review?(ted.mielczarek) → review-
(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?
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.
Attachment #428613 - Attachment is obsolete: true
Bv1-CC, with comment 16 suggestion(s).
Attachment #428610 - Attachment is obsolete: true
Attachment #428877 - Flags: review?(bugspam.Callek)
Attachment #428610 - Flags: review?(bugspam.Callek)
Attachment #428871 - Flags: review?(ted.mielczarek) → review+
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
Attachment #428871 - Attachment description: (Cv2) /js/src/configure.in: Copy (the useful part of) it, Remove checks for oleacc.idl and atlbase.h too → (Cv2) /js/src/configure.in: Copy (the useful part of) it, Remove checks for oleacc.idl and atlbase.h too [Checkin: Comment 21]
Attachment #395297 - Attachment description: moved AC_SUBST(NS_ENABLE_TSF) down → moved AC_SUBST(NS_ENABLE_TSF) down [Checkin: Comment 13]
Bv2-CC, without the chunk now removed in bug 513709.
Attachment #428877 - Attachment is obsolete: true
Attachment #430054 - Flags: review?(bugspam.Callek)
Attachment #428877 - Flags: review?(bugspam.Callek)
Attachment #430054 - Flags: review?(bugspam.Callek) → review+
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
Attachment #430054 - Attachment description: (Bv2a-CC) Copy (the useful part of) it to comm-central, Remove checks for oleacc.idl and atlbase.h too, (1.9.2+) → (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]
Depends on: 599893
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.