Closed Bug 523820 Opened 10 years ago Closed 10 years ago

Remove old MOZILLA_1_9_1_BRANCH ifdefs from comm-central code

Categories

(MailNews Core :: Build Config, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.1a1

People

(Reporter: standard8, Assigned: sgautherie)

References

()

Details

(Keywords: fixed-seamonkey2.0.1)

Attachments

(8 files, 4 obsolete files)

31.68 KB, patch
kairo
: review+
Details | Diff | Splinter Review
26.78 KB, patch
standard8
: review-
Details | Diff | Splinter Review
6.98 KB, patch
standard8
: review+
standard8
: superreview+
Details | Diff | Splinter Review
8.49 KB, patch
standard8
: review+
Details | Diff | Splinter Review
22.24 KB, patch
kairo
: review+
Details | Diff | Splinter Review
5.03 KB, patch
Callek
: review+
Details | Diff | Splinter Review
3.36 KB, patch
neil
: review+
Details | Diff | Splinter Review
1.31 KB, patch
Callek
: review+
Details | Diff | Splinter Review
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.
Attached patch (Av1-SM) Remove obsolete cases (obsolete) — Splinter Review
Attachment #408165 - Flags: review?(kairo)
Depends on: 255503, 521293
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.
Av1-SM, with comment 2 suggestion(s).
Attachment #408165 - Attachment is obsolete: true
Attachment #408401 - Flags: review?(kairo)
Attachment #408165 - Flags: review?(kairo)
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.
Attachment #408401 - Flags: review?(kairo) → review+
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.
Attachment #408401 - Attachment description: (Av1a-SM) Remove obsolete cases → (Av1a-SM) Remove obsolete cases [Checkin: Comment 5]
Keywords: helpwanted
Whiteboard: [helpwanted: to do /calendar/* and /mail/*]
Depends on: 504029
Depends on: 525331
Depends on: 522713
Depends on: 521624
Blocks: 514665
Assignee: nobody → sgautherie.bz
Status: NEW → ASSIGNED
Attachment #412156 - Flags: review?(bugzilla)
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)...
We still have packaging issues on SM 2.0.x: this should make life easier in future.
Attachment #412218 - Flags: review?(bugzilla)
Attachment #412218 - Flags: approval-seamonkey2.0.1?
Attachment #412162 - Flags: superreview+
Attachment #412162 - Flags: review?(bugzilla)
Attachment #412162 - Flags: review+
Comment on attachment 412162 [details] [diff] [review]
(Cv1) Clean /mailnews/* up
[Checkin: See comment 11+18]


http://hg.mozilla.org/comm-central/rev/776a79a690ea
Attachment #412162 - Attachment description: (Cv1) Clean /mailnews/* up → (Cv1) Clean /mailnews/* up [Checkin: Comment 11]
Attachment #412164 - Flags: review?(bugzilla) → review+
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
Attachment #412164 - Attachment description: (Dv1) Clean "configure.in" up, the obvious part → (Dv1) Clean "configure.in" up, the obvious part [Checkin: Comment 12]
mailnews/imap/src/nsImapMailFolder.cpp
-  // Note: make cast in fillArrayWithKeys() match

Please leave this in, thanks.
(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 ?
> 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.
(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.)
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>;
Checked in, without reviews, because it's just restoring old code.
<http://hg.mozilla.org/comm-central/rev/d6dab51ecb69>
Attachment #412162 - Attachment description: (Cv1) Clean /mailnews/* up [Checkin: Comment 11] → (Cv1) Clean /mailnews/* up [Checkin: See comment 11+18]
Attachment #414749 - Flags: review?(kairo)
Depends on: 530010
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).
Attachment #412156 - Flags: review?(bugzilla) → review-
Attachment #412218 - Flags: review?(bugzilla) → review?(kairo)
Depends on: 531283
Attachment #412218 - Flags: review?(kairo)
Attachment #412218 - Flags: review+
Attachment #412218 - Flags: approval-seamonkey2.0.1?
Attachment #412218 - Flags: approval-seamonkey2.0.1+
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.
Attachment #414749 - Flags: review?(kairo) → review-
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
Attachment #412218 - Attachment description: (Ev1-SM-191) Clean /suite/installer/* up → (Ev1-SM-191) Clean /suite/installer/* up [Checkin: Comment 22]
(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...)
Depends on: 532573
Bug 530010 is doing TB part...
I filed bug 532573 about "Calendar".
Keywords: helpwanted
Whiteboard: [helpwanted: to do /calendar/* and /mail/*]
Depends on: 533627
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 :-|
Attachment #414749 - Attachment is obsolete: true
Attachment #417363 - Flags: review?(kairo)
waitForFocus(): I'm not fixing failures, I'm just being more safe/explicit.
Attachment #418554 - Flags: review?(neil)
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?
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.
Attachment #418554 - Attachment is obsolete: true
Attachment #418575 - Flags: review?(neil)
Attachment #418554 - Flags: review?(neil)
Attachment #418575 - Flags: review?(neil) → review+
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
Attachment #418575 - Attachment description: (Hv1a-SM) Remove/Update (2 of 3) missed cases, Use waitForFocus() → (Hv1a-SM) Remove/Update (2 of 3) missed cases, Use waitForFocus() [Checkin: Comment 30]
Attachment #417363 - Attachment is obsolete: true
Attachment #418580 - Flags: review?(kairo)
Attachment #417363 - Flags: review?(kairo)
Callek, the patch you talked about on IRC yesterday is part of "Gv1" here.
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)
Attachment #418290 - Flags: review?(bugzilla) → review+
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
Attachment #418290 - Attachment description: (Gv1) Clean 'configure.in' up, the remaining part → (Gv1) Clean 'configure.in' up, the remaining part [Checkin: Comment 33]
Attachment #418290 - Flags: superreview?(bugzilla)
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 !
Attachment #418290 - Attachment description: (Gv1) Clean 'configure.in' up, the remaining part [Checkin: Comment 33] → (Gv1) Clean 'configure.in' up, the remaining part [Checkin: Comment 35]
Comment on attachment 418580 [details] [diff] [review]
(Fv2a-SM) Remove last missed case
[Checkin: Comment 37]

was just looking at this too!
Attachment #418580 - Flags: review?(kairo) → review+
Comment on attachment 418580 [details] [diff] [review]
(Fv2a-SM) Remove last missed case
[Checkin: Comment 37]


http://hg.mozilla.org/comm-central/rev/fb6a673527df
Attachment #418580 - Attachment description: (Fv2a-SM) Remove last missed cases → (Fv2a-SM) Remove last missed case [Checkin: Comment 37]
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
(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 :(
Depends on: 541125
Duplicate of this bug: 516452
You need to log in before you can comment on or make changes to this bug.