496225 - Replace MOZ_DISABLE_VISTA_SDK_REQUIREMENTS in comm-central

Status: RESOLVED FIXED

(MailNews Core :: Build Config, defect)

Description

[Serge Gautherie (:sgautherie)]

[To be done after branching to m-c from m-1.9.1.]

Bug 493008 comment 12
{
From  Serge Gautherie (:sgautherie)   2009-05-17 18:40:38 PDT

Created an attachment (id=377986) [details]
(Cv1) Part 3, add MOZ_NTDDI_* defines

The 'switch code over to use these defines' will be a follow-up bug, to do
after c-c has branched.
}

See
https://bugzilla.mozilla.org/attachment.cgi?id=374083&action=diff

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Av1-SM) Just replace it [Checkin: Comment 2]

Comment 2

[Serge Gautherie (:sgautherie)]

Comment on attachment 418206 [details] [diff] [review]
(Av1-SM) Just replace it
[Checkin: Comment 2]


http://hg.mozilla.org/comm-central/rev/d4ca11753853

Comment 3

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1-TB) Just replace it

Comment 4

[Rain [:rain1] (inactive)]

Comment on attachment 418500 [details] [diff] [review]
(Bv1-TB) Just replace it

Preprocessor comparisons don't work with the JavaScript Preprocessor.py -- we need to come up with a different solution there. I have an idea.

Comment 5

[Robert Kaiser]

We have that block in mail/components/search/Makefile.in anyhow, should be able to put some defined in there that gets used in the .js then.

Comment 6

[Rain [:rain1] (inactive)]

I don't think Makefiles can do comparisons, either, actually. A build with Serge's patch fails with make complaining about an extraneous endif.

Comment 7

[Serge Gautherie (:sgautherie)]

(In reply to comment #4)
> (From update of attachment 418500 [details] [diff] [review])
> Preprocessor comparisons don't work with the JavaScript Preprocessor.py -- we
> need to come up with a different solution there. I have an idea.

Oh: I had found
http://mxr.mozilla.org/mozilla-central/search?string=%5C%23if.*%3D&regexp=on&case=on&find=%2Fall.js
and assumed it would work somehow :-(

(In reply to comment #6)
> I don't think Makefiles can do comparisons, either, actually. A build with
> Serge's patch fails with make complaining about an extraneous endif.

Hum ... I probably need to do something like
https://bugzilla.mozilla.org/attachment.cgi?id=374083&action=diff
did for downloads/*.!.

Sorry for being over-optimistic on this one :-<

Comment 8

[Rain [:rain1] (inactive)]

Attachment: required changes for search integration

Serge, could you please check whether this works as expected (i.e. nothing blows up)?

This can be checked in independently of the rest, so I'm going to ask Standard8 for review too. This idiom has been used for taskbar integration by Firefox in <http://mxr.mozilla.org/mozilla-central/source/browser/components/wintaskbar/WindowsPreviewPerTab.jsm#561>, so it seems sound.

Comment 9

[Rain [:rain1] (inactive)]

Attachment: contract id style changed to be consistent with what we use for C++

apologies for the bugspam.

Comment 10

[Serge Gautherie (:sgautherie)]

Comment on attachment 418506 [details] [diff] [review]
contract id style changed to be consistent with what we use for C++

>diff --git a/mail/components/search/WinSearchIntegration.js b/mail/components/search/WinSearchIntegration.js

I tested this successfully on
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.3a1pre) Gecko/20091219 Shredder/3.1a1pre] (nightly)

>@@ -164,16 +169,24 @@ let SearchIntegration =

Nits:

>   _init: function winsearch_init()
>   {
>     this._initLogging();
>+    // If the helper service isn't present, we weren't compiled with the needed

I would suggest to remind explicitly what the needed support condition is,
so we find this code whenever m-c stops supporting the older Windows versions.

>+    // support. Mark ourselves null and return
>+    if (!(WINSEARCHHELPER_CONTRACTID in Cc))
>+    {

If you do want to enable this._initLogging(), then I would suggest to add a this._log.fatal(...) here too.

>+      SearchIntegration = null;
>+      return;
>+    }
>+
>     // We're currently only enabled on Vista and above
>     let sysInfo = Cc["@mozilla.org/system-info;1"]
>                     .getService(Ci.nsIPropertyBag2);
>     let windowsVersion = sysInfo.getProperty("version");
>     if (parseFloat(windowsVersion) < 6)
>     {
>       this._log.fatal("Windows version " + windowsVersion + " < 6.0");
>       this.osVersionTooLow = true;

Or, to be more like current code, you could simply do
{
if (WINSEARCHHELPER_CONTRACTID in Cc)
  SearchIntegration._init();
else
  SearchIntegration = null;
}
at the end of the file.

Comment 11

[Mark Banner (:standard8)]

Comment on attachment 418506 [details] [diff] [review]
contract id style changed to be consistent with what we use for C++

>   _init: function winsearch_init()
>   {
>     this._initLogging();
>+    // If the helper service isn't present, we weren't compiled with the needed
>+    // support. Mark ourselves null and return
>+    if (!(WINSEARCHHELPER_CONTRACTID in Cc))
>+    {
>+      SearchIntegration = null;
>+      return;
>+    }
>+

This patch actually doesn't seem quite right. If nsMailWinSearchHelper isn't built if WinSearchIntegration.js isn't built. If nsMailWinSearchHelper is built then WinSearchIntegration.js is built.

Therefore there's no point in this check afaict. Did you miss including a makefile change?

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 418506 [details] [diff] [review]
contract id style changed to be consistent with what we use for C++

Cancelling review until comment 11 is answered.

Comment 13

[Serge Gautherie (:sgautherie)]

Siddharth, ping.

Comment 14

[Serge Gautherie (:sgautherie)]

Attachment: (Bv2) Remove the option and its variable, Update TB *.cpp files

Bv1-TB, doing only the part that works :->

After thinking about the situation, I see no simpler solution :-|

Comment 15

[Mark Banner (:standard8)]

Comment on attachment 427738 [details] [diff] [review]
(Bv2) Remove the option and its variable, Update TB *.cpp files

From what I can tell this might work, but I'd prefer just have one patch that does all the required work here rather than trying to do it piecemeal.

Comment 16

[Serge Gautherie (:sgautherie)]

(In reply to comment #15)
> From what I can tell this might work, but I'd prefer just have one patch that
> does all the required work here rather than trying to do it piecemeal.

I had a look but I'm not familiar with "WinSearchIntegration", so helpwanted on that part...

Comment 17

[Serge Gautherie (:sgautherie)]

(In reply to comment #15)
> From what I can tell this might work, but I'd prefer just have one patch that
> does all the required work here rather than trying to do it piecemeal.

In addition, unless we can actually move the comparison into the related .cpp files,
we have to keep a configure var to use in the Makefiles,
and that var can just be the existing one:
so the current patch would be all we need to do actually...

Comment 18

[Rain [:rain1] (inactive)]

Attachment: Update patch + add makefile changes

Again, Serge, I'm going to have to ask you to try building with this.

I did indeed miss a few makefile changes (and I apologize for not updating this bug for all these months).

Comment 19

[Mark Banner (:standard8)]

Comment on attachment 461676 [details] [diff] [review]
Update patch + add makefile changes

r=Standard8 if you fix the ifdef around NS_DEFINE_NAMED_CID(NS_MAILWINSEARCHHELPER_CID);
as well

Comment 20

[Rain [:rain1] (inactive)]

I caught one more ifndef nearby.

http://hg.mozilla.org/comm-central/rev/9359a1376625

I'm going to mark this fixed, since I don't know of any way to fix the one remaining use.