Closed Bug 496225 Opened 11 years ago Closed 9 years ago

Replace MOZ_DISABLE_VISTA_SDK_REQUIREMENTS in comm-central

Categories

(MailNews Core :: Build Config, defect, trivial)

x86
Windows 2000
defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: sgautherie, Assigned: sgautherie)

References

()

Details

(Keywords: helpwanted, Whiteboard: [ToDo: /mail makefiles])

Attachments

(3 files, 3 obsolete files)

[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
Assignee: nobody → sgautherie.bz
Status: NEW → ASSIGNED
Attachment #418206 - Flags: review?(kairo)
No longer depends on: CcMcBuildIssues
Flags: in-testsuite-
Target Milestone: --- → Thunderbird 3.1a1
Attachment #418206 - Flags: review?(kairo) → review+
Comment on attachment 418206 [details] [diff] [review]
(Av1-SM) Just replace it
[Checkin: Comment 2]


http://hg.mozilla.org/comm-central/rev/d4ca11753853
Attachment #418206 - Attachment description: (Av1-SM) Just replace it → (Av1-SM) Just replace it [Checkin: Comment 2]
Attached patch (Bv1-TB) Just replace it (obsolete) — Splinter Review
Attachment #418500 - Flags: review?(bugzilla)
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.
Attachment #418500 - Flags: review?(bugzilla) → review-
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.
I don't think Makefiles can do comparisons, either, actually. A build with Serge's patch fails with make complaining about an extraneous endif.
(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 :-<
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.
Attachment #418505 - Flags: review?(sgautherie.bz)
Attachment #418505 - Flags: review?(bugzilla)
apologies for the bugspam.
Attachment #418506 - Flags: review?(sgautherie.bz)
Attachment #418506 - Flags: review?(bugzilla)
Attachment #418505 - Attachment is obsolete: true
Attachment #418505 - Flags: review?(sgautherie.bz)
Attachment #418505 - Flags: review?(bugzilla)
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.
Attachment #418506 - Flags: review?(sgautherie.bz) → review+
Attachment #418506 - Flags: review+
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?
Attachment #418506 - Flags: review?(bugzilla)
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.
Siddharth, ping.
Bv1-TB, doing only the part that works :->

After thinking about the situation, I see no simpler solution :-|
Attachment #418500 - Attachment is obsolete: true
Attachment #427738 - Flags: review?(bugzilla)
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.
Attachment #427738 - Flags: review?(bugzilla) → review-
(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...
Keywords: helpwanted
Whiteboard: [ToDo: /mail makefiles]
Target Milestone: Thunderbird 3.1a1 → ---
(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...
Depends on: 557958
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).
Attachment #418506 - Attachment is obsolete: true
Attachment #461676 - Flags: review?(bugzilla)
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
Attachment #461676 - Flags: review?(bugzilla) → review+
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.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.