523820 - Remove old MOZILLA_1_9_1_BRANCH ifdefs from comm-central code

Status: RESOLVED FIXED

(MailNews Core :: Build Config, defect)

Description

[Mark Banner (:standard8)]

Now that we have branched, we should remove the MOZILLA_1_9_1_BRANCH ifdefs from comm-central.

However in the interests of code stability for developers, I'd like to limit what we do until after Thunderbird has gone into its code freeze for rc1. Hence, mailnews/ and mail/ should be largely left alone until after code freeze unless there's specific tidy up we want to do for other reasons.

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Av1-SM) Remove obsolete cases

Comment 2

[[e-mail]]

Comment on attachment 408165 [details] [diff] [review]
(Av1-SM) Remove obsolete cases

>-# We need to do this before we include config.mk and thus app-config.mk
>+# before we include config.mk and thus app-config.mk.
This comment change is wrong. DEFINES needs to be set before config.mk is included, because app-config.mk is loaded after config.mk reads DEFINES.

At least, I think that's why I had to do it this way to get it to compile.

Comment 3

[Serge Gautherie (:sgautherie)]

Attachment: (Av1a-SM) Remove obsolete cases [Checkin: Comment 5]

Av1-SM, with comment 2 suggestion(s).

Comment 4

[Robert Kaiser]

Comment on attachment 408401 [details] [diff] [review]
(Av1a-SM) Remove obsolete cases
[Checkin: Comment 5]

>-#ifdef MOZILLA_1_9_1_BRANCH
>-bin/components/liboji.so
>-#endif

Could you check if this really needs to be removed or changed into a 1_9_2 ifdef? I heard that possibly they have re-enabled OJI for 1.9.2 only.
Of course, the same applies for any other places in the bug that mention "oji" somehow.

r=me with that taken in mind.

Comment 5

[Serge Gautherie (:sgautherie)]

Comment on attachment 408401 [details] [diff] [review]
(Av1a-SM) Remove obsolete cases
[Checkin: Comment 5]


http://hg.mozilla.org/comm-central/rev/955b4e732760


(In reply to comment #4)
> they have re-enabled OJI for 1.9.2 only.

Yes, but on MacOSX only: see bug 521624.

Comment 6

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1-TB) Clean /mail/installer/* up, Improve sort order, Remove obsolete jsctypes.xpt.

Comment 7

[Serge Gautherie (:sgautherie)]

Comment on attachment 412156 [details] [diff] [review]
(Bv1-TB) Clean /mail/installer/* up, Improve sort order, Remove obsolete jsctypes.xpt.

Bug 521293 will add some m-1.9.2 support (back)...

Comment 8

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1) Clean /mailnews/* up [Checkin: See comment 11+18]

Comment 9

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1) Clean "configure.in" up, the obvious part [Checkin: Comment 12]

Comment 10

[Serge Gautherie (:sgautherie)]

Attachment: (Ev1-SM-191) Clean /suite/installer/* up [Checkin: Comment 22]

We still have packaging issues on SM 2.0.x: this should make life easier in future.

Comment 11

[Serge Gautherie (:sgautherie)]

Comment on attachment 412162 [details] [diff] [review]
(Cv1) Clean /mailnews/* up
[Checkin: See comment 11+18]


http://hg.mozilla.org/comm-central/rev/776a79a690ea

Comment 12

[Serge Gautherie (:sgautherie)]

Comment on attachment 412164 [details] [diff] [review]
(Dv1) Clean "configure.in" up, the obvious part
[Checkin: Comment 12]


http://hg.mozilla.org/comm-central/rev/6c647a81c780

Comment 13

[Ben Bucksch (:BenB)]

mailnews/imap/src/nsImapMailFolder.cpp
-  // Note: make cast in fillArrayWithKeys() match

Please leave this in, thanks.

Comment 14

[Serge Gautherie (:sgautherie)]

(In reply to comment #13)
> mailnews/imap/src/nsImapMailFolder.cpp
> -  // Note: make cast in fillArrayWithKeys() match
> 
> Please leave this in, thanks.

You added this there in bug 522342.
Why should I/we keep a m-1.9.1 only comment ?

Comment 15

[Ben Bucksch (:BenB)]

> Why should I/we keep a m-1.9.1 only comment

It wasn't a m-1.9.1 only comment, it applied to both and still applies, that's fairly obvious, I just accidentally put it inside the ifdef. The comment is important because I indeed forgot that when adapting to trunk and it caused a crash.

Comment 16

[Serge Gautherie (:sgautherie)]

(In reply to comment #15)

Then feel free to just re-add the right comment in the right place. (You know better than I do.)

Comment 17

[Ben Bucksch (:BenB)]

I don't want to go through all the reviews. You removed it, you re-add it, please.
Add it exactly where it was, above
  nsTArray<nsCString>* resultArray = new nsTArray<nsCString>;

Comment 18

[Ben Bucksch (:BenB)]

Checked in, without reviews, because it's just restoring old code.
<http://hg.mozilla.org/comm-central/rev/d6dab51ecb69>

Comment 19

[Serge Gautherie (:sgautherie)]

Attachment: (Fv1-SM) Remove/Update missed cases

Comment 20

[Mark Banner (:standard8)]

Comment on attachment 412156 [details] [diff] [review]
(Bv1-TB) Clean /mail/installer/* up, Improve sort order, Remove obsolete jsctypes.xpt.

Sorry, but bug 530010 does this in a cleaner way and makes it work for 1.9.2 as well (unfortunately we were missing some 1.9.2 ifdefs).

Comment 21

[Robert Kaiser]

Comment on attachment 414749 [details] [diff] [review]
(Fv1-SM) Remove/Update missed cases

>diff --git a/suite/config/version-191.txt b/suite/config/version-192.txt
>rename from suite/config/version-191.txt
>rename to suite/config/version-192.txt
>--- a/suite/config/version-191.txt
>+++ b/suite/config/version-192.txt
>@@ -1,1 +1,1 @@
>-2.0.1pre
>\ No newline at end of file
>+2.1a1pre
>\ No newline at end of file

I don't really agree with that, as we have no decision if we even target 1.9.2 with anything, and if we set version.txt and the branch one to the same value, we can do away with the distinction altogether, doesn't make any difference.

Comment 22

[Serge Gautherie (:sgautherie)]

Comment on attachment 412218 [details] [diff] [review]
(Ev1-SM-191) Clean /suite/installer/* up
[Checkin: Comment 22]


http://hg.mozilla.org/releases/comm-1.9.1/rev/7698a02571a5

Comment 23

[Serge Gautherie (:sgautherie)]

(In reply to comment #21)
> (From update of attachment 414749 [details] [diff] [review])
> >diff --git a/suite/config/version-191.txt b/suite/config/version-192.txt
> 
> I don't really agree with that, as we have no decision if we even target 1.9.2
> with anything, and if we set version.txt and the branch one to the same value,

Well, "2.1a1pre" is what one currently gets when building with m-1.9.2: I'm not changing the value, just updating the code.

> we can do away with the distinction altogether, doesn't make any difference.

I thought the "rv:1.9.2xxx" part would be enough ftb,
yet I fully agree that we could use a different value: "2.1a0pre"?!

The point is: unless we would error out when building on m-1.9.2, we have to use some placeholder value :-|
(I know what the situation is wrt SM2.x/m-1.9.2: I just try to keep things open until a decision is made...)

Comment 24

[Serge Gautherie (:sgautherie)]

Bug 530010 is doing TB part...
I filed bug 532573 about "Calendar".

Comment 25

[Serge Gautherie (:sgautherie)]

Attachment: (Fv2-SM) Remove/Update missed cases

Fv1-SM, with comment 21 suggestion(s).

Per your irc reply to my comment 23 question, this reverts confvars.sh to how it was before bug 502033: no more 1.9.1 support and no 1.9.2 support yet :-|

Comment 26

[Serge Gautherie (:sgautherie)]

Attachment: (Gv1) Clean 'configure.in' up, the remaining part [Checkin: Comment 35]

Comment 27

[Serge Gautherie (:sgautherie)]

Attachment: (Hv1-SM) Remove/Update (2 of 3) missed cases, Use waitForFocus() in 2 tests.

waitForFocus(): I'm not fixing failures, I'm just being more safe/explicit.

Comment 28

[[e-mail]]

Comment on attachment 418554 [details] [diff] [review]
(Hv1-SM) Remove/Update (2 of 3) missed cases, Use waitForFocus() in 2 tests.

The second use of waitForFocus looks entirely reasonable to me but I don't understand the first one; is there a similar patch for Firefox?

Comment 29

[Serge Gautherie (:sgautherie)]

Attachment: (Hv1a-SM) Remove/Update (2 of 3) missed cases, Use waitForFocus() [Checkin: Comment 30]

Hv1-SM, with comment 28 suggestion(s).

(In reply to comment #28)
> I don't understand the first one; is there a similar patch for Firefox?

None that I am aware of and that was just a suggestion: dropping it.

Comment 30

[Serge Gautherie (:sgautherie)]

Comment on attachment 418575 [details] [diff] [review]
(Hv1a-SM) Remove/Update (2 of 3) missed cases, Use waitForFocus()
[Checkin: Comment 30]


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

Comment 31

[Serge Gautherie (:sgautherie)]

Attachment: (Fv2a-SM) Remove last missed case [Checkin: Comment 37]

Comment 32

[Robert Kaiser]

Callek, the patch you talked about on IRC yesterday is part of "Gv1" here.

Comment 33

[Justin Wood (:Callek)]

Comment on attachment 418290 [details] [diff] [review]
(Gv1) Clean 'configure.in' up, the remaining part
[Checkin: Comment 35]

r+ on this... I don't think we really need sr here, but I did not clear request (been away a while so still catching up)

Comment 34

[Serge Gautherie (:sgautherie)]

Comment on attachment 418290 [details] [diff] [review]
(Gv1) Clean 'configure.in' up, the remaining part
[Checkin: Comment 35]


http://hg.mozilla.org/comm-central/rev/2303689fabf4

Comment 35

[Serge Gautherie (:sgautherie)]

Comment on attachment 418290 [details] [diff] [review]
(Gv1) Clean 'configure.in' up, the remaining part
[Checkin: Comment 35]


(In reply to comment #34)
> http://hg.mozilla.org/comm-central/rev/2303689fabf4

http://hg.mozilla.org/comm-central/rev/d8a60bb40b9b !

Comment 36

[Justin Wood (:Callek)]

Comment on attachment 418580 [details] [diff] [review]
(Fv2a-SM) Remove last missed case
[Checkin: Comment 37]

was just looking at this too!

Comment 37

[Serge Gautherie (:sgautherie)]

Comment on attachment 418580 [details] [diff] [review]
(Fv2a-SM) Remove last missed case
[Checkin: Comment 37]


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

Comment 38

[Robert Kaiser]

(In reply to comment #36)
> (From update of attachment 418580 [details] [diff] [review])
> was just looking at this too!

GRRRRRRRR!!!!!!!!!!!!!!!!!!!

Now we probably need to reintruduce this again if we need to ship from Lorentz!

This was why I held back this review :(