Remove old MOZILLA_1_9_1_BRANCH ifdefs from comm-central code

RESOLVED FIXED in Thunderbird 3.1a1

Status

MailNews Core
Build Config
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: standard8, Assigned: sgautherie)

Tracking

({fixed-seamonkey2.0.1})

Trunk
Thunderbird 3.1a1
fixed-seamonkey2.0.1
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(8 attachments, 4 obsolete attachments)

31.68 KB, patch
Robert Kaiser
: 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
Robert Kaiser
: review+
Details | Diff | Splinter Review
5.03 KB, patch
Callek
: review+
Details | Diff | Splinter Review
3.36 KB, patch
neil@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
1.31 KB, patch
Callek
: review+
Details | Diff | Splinter Review
(Reporter)

Description

8 years ago
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.
(Assignee)

Updated

8 years ago
(Assignee)

Comment 1

8 years ago
Created attachment 408165 [details] [diff] [review]
(Av1-SM) Remove obsolete cases
Attachment #408165 - Flags: review?(kairo)
(Assignee)

Updated

8 years ago
Depends on: 255503, 521293

Comment 2

8 years ago
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.
(Assignee)

Comment 3

8 years ago
Created attachment 408401 [details] [diff] [review]
(Av1a-SM) Remove obsolete cases
[Checkin: Comment 5]

Av1-SM, with comment 2 suggestion(s).
Attachment #408165 - Attachment is obsolete: true
Attachment #408401 - Flags: review?(kairo)
Attachment #408165 - Flags: review?(kairo)

Comment 4

8 years ago
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+
(Assignee)

Comment 5

8 years ago
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]
(Assignee)

Updated

8 years ago
Keywords: helpwanted
Whiteboard: [helpwanted: to do /calendar/* and /mail/*]
(Assignee)

Updated

8 years ago
Depends on: 504029
(Assignee)

Updated

8 years ago
Depends on: 525331
(Assignee)

Updated

8 years ago
Depends on: 522713
(Assignee)

Updated

8 years ago
Depends on: 521624
(Assignee)

Updated

8 years ago
Blocks: 514665
(Assignee)

Comment 6

8 years ago
Created attachment 412156 [details] [diff] [review]
(Bv1-TB) Clean /mail/installer/* up, Improve sort order, Remove obsolete jsctypes.xpt.
Assignee: nobody → sgautherie.bz
Status: NEW → ASSIGNED
Attachment #412156 - Flags: review?(bugzilla)
(Assignee)

Comment 7

8 years ago
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)...
(Assignee)

Comment 8

8 years ago
Created attachment 412162 [details] [diff] [review]
(Cv1) Clean /mailnews/* up
[Checkin: See comment 11+18]
Attachment #412162 - Flags: review?(bugzilla)
(Assignee)

Comment 9

8 years ago
Created attachment 412164 [details] [diff] [review]
(Dv1) Clean "configure.in" up, the obvious part
[Checkin: Comment 12]
Attachment #412164 - Flags: review?(bugzilla)
(Assignee)

Comment 10

8 years ago
Created attachment 412218 [details] [diff] [review]
(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.
Attachment #412218 - Flags: review?(bugzilla)
Attachment #412218 - Flags: approval-seamonkey2.0.1?
(Reporter)

Updated

8 years ago
Attachment #412162 - Flags: superreview+
Attachment #412162 - Flags: review?(bugzilla)
Attachment #412162 - Flags: review+
(Assignee)

Comment 11

8 years ago
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]
(Reporter)

Updated

8 years ago
Attachment #412164 - Flags: review?(bugzilla) → review+
(Assignee)

Comment 12

8 years ago
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]

Comment 13

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

Please leave this in, thanks.
(Assignee)

Comment 14

8 years ago
(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

8 years ago
> 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.
(Assignee)

Comment 16

8 years ago
(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

8 years ago
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

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

Updated

8 years ago
Attachment #412162 - Attachment description: (Cv1) Clean /mailnews/* up [Checkin: Comment 11] → (Cv1) Clean /mailnews/* up [Checkin: See comment 11+18]
(Assignee)

Comment 19

8 years ago
Created attachment 414749 [details] [diff] [review]
(Fv1-SM) Remove/Update missed cases
Attachment #414749 - Flags: review?(kairo)
(Reporter)

Updated

8 years ago
Depends on: 530010
(Reporter)

Comment 20

8 years ago
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-
(Assignee)

Updated

8 years ago
Attachment #412218 - Flags: review?(bugzilla) → review?(kairo)
(Assignee)

Updated

8 years ago
Depends on: 531283

Updated

8 years ago
Attachment #412218 - Flags: review?(kairo)
Attachment #412218 - Flags: review+
Attachment #412218 - Flags: approval-seamonkey2.0.1?
Attachment #412218 - Flags: approval-seamonkey2.0.1+

Comment 21

8 years ago
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-
(Assignee)

Comment 22

8 years ago
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]
(Assignee)

Comment 23

8 years ago
(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...)
(Assignee)

Updated

8 years ago
Keywords: fixed-seamonkey2.0.1
(Assignee)

Updated

8 years ago
Depends on: 532573
(Assignee)

Comment 24

8 years ago
Bug 530010 is doing TB part...
I filed bug 532573 about "Calendar".
Keywords: helpwanted
Whiteboard: [helpwanted: to do /calendar/* and /mail/*]
(Assignee)

Updated

8 years ago
(Assignee)

Updated

8 years ago
Depends on: 533627
(Assignee)

Comment 25

8 years ago
Created attachment 417363 [details] [diff] [review]
(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 :-|
Attachment #414749 - Attachment is obsolete: true
Attachment #417363 - Flags: review?(kairo)
(Assignee)

Comment 26

8 years ago
Created attachment 418290 [details] [diff] [review]
(Gv1) Clean 'configure.in' up, the remaining part
[Checkin: Comment 35]
Attachment #418290 - Flags: superreview?(bugzilla)
Attachment #418290 - Flags: review?(bugzilla)
(Assignee)

Comment 27

8 years ago
Created attachment 418554 [details] [diff] [review]
(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.
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?
(Assignee)

Comment 29

8 years ago
Created attachment 418575 [details] [diff] [review]
(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.
Attachment #418554 - Attachment is obsolete: true
Attachment #418575 - Flags: review?(neil)
Attachment #418554 - Flags: review?(neil)

Updated

8 years ago
Attachment #418575 - Flags: review?(neil) → review+
(Assignee)

Comment 30

8 years ago
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]
(Assignee)

Comment 31

8 years ago
Created attachment 418580 [details] [diff] [review]
(Fv2a-SM) Remove last missed case
[Checkin: Comment 37]
Attachment #417363 - Attachment is obsolete: true
Attachment #418580 - Flags: review?(kairo)
Attachment #417363 - Flags: review?(kairo)

Comment 32

7 years ago
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+
(Assignee)

Comment 34

7 years ago
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)
(Assignee)

Comment 35

7 years ago
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+
(Assignee)

Comment 37

7 years ago
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]
(Assignee)

Updated

7 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Comment 38

7 years ago
(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 :(
(Assignee)

Updated

7 years ago
Depends on: 541125
(Assignee)

Updated

7 years ago
Duplicate of this bug: 516452
You need to log in before you can comment on or make changes to this bug.