Closed Bug 398810 Opened 12 years ago Closed 12 years ago

Remove MOZILLA_1_8_BRANCH ifdefs from core on trunk

Categories

(Core :: General, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.1a1

People

(Reporter: bzbarsky, Assigned: glandium)

Details

Attachments

(2 files, 1 obsolete file)

Summary: Remove MOZILLA_1_8_BRANCH interfaces from core on trunk → Remove MOZILLA_1_8_BRANCH ifdefs from core on trunk
Attached patch patchSplinter Review
This should do
Attachment #317962 - Flags: review?(gavin.sharp)
Attached patch patch for extensions (obsolete) — Splinter Review
I don't know if this belongs here or to bug #398811
Attachment #317963 - Flags: review?(gavin.sharp)
Mike, if you wouldn't mind, please check the "Take bug" checkbox when you attach a patch that fixes a bug so that the bug can be assigned to you. :)
Assignee: nobody → mh+mozilla
OS: Linux → All
Hardware: PC → All
You should get the relevant module owners to sign off on these changes. I'm not sure that all the code in /extensions doesn't still rely on being buildable on trunk or branch, for example. I think most of the core code is probably OK (though it would be good to verify why the ifdefs were added in the first place, and confirm that it was because of the plans to do automatic mirroring).
OS: All → Linux
Hardware: All → PC
Pretty sure canvas3d is trunk-only, and I know extensions/metrics/ is trunk only.
Attachment #317962 - Flags: review?(jonas)
Attachment #317962 - Flags: review?(gavin.sharp)
Attachment #317962 - Flags: review?(brendan)
Attachment #317962 - Flags: review?(benjamin)
(In reply to comment #4)
> You should get the relevant module owners to sign off on these changes. I'm not
> sure that all the code in /extensions doesn't still rely on being buildable on
> trunk or branch, for example. I think most of the core code is probably OK
> (though it would be good to verify why the ifdefs were added in the first
> place, and confirm that it was because of the plans to do automatic mirroring).

Nothing for these extensions on http://www.mozilla.org/owners.html :-/
Comment on attachment 317962 [details] [diff] [review]
patch

r=me for the js/src changes.

/be
Attachment #317962 - Flags: review?(brendan) → review+
Attachment #317963 - Flags: review?(pete)
Attachment #317963 - Flags: review?(pete) → review+
Attachment #317963 - Flags: review?(vladimir)
Attachment #317963 - Flags: review?(mozilla)
Attachment #317963 - Flags: review?(jonas)
Attachment #317963 - Flags: review?(jonas)
Attachment #317963 - Flags: review?(gavin.sharp)
Comment on attachment 317963 [details] [diff] [review]
patch for extensions

looks fine here, though I plan on cvs rm'ing extensions/canvas3d shortly (moving the repo elsewhere)
Attachment #317963 - Flags: review?(vladimir) → review+
Comment on attachment 317963 [details] [diff] [review]
patch for extensions

Please leave the CCK code in the tree.

The CCK, while built on the trunk, can still build CCKs that work in FF 2.
Attachment #317963 - Flags: review?(mozilla) → review-
Same as previous, without changes to cck, which means it should be review+ for the rest.
Attachment #317963 - Attachment is obsolete: true
Attachment #317962 - Flags: review?(benjamin) → review+
Any progress on this bug?
Comment on attachment 317962 [details] [diff] [review]
patch

Removes unneeded MOZILLA_1_8_BRANCH |#define|s from the trunk.
Attachment #317962 - Flags: approval1.9?
Comment on attachment 318188 [details] [diff] [review]
patch for extensions v2

Removes unneeded MOZILLA_1_8_BRANCH |#define|s from the trunk.
Attachment #318188 - Flags: approval1.9?
Comment on attachment 318188 [details] [diff] [review]
patch for extensions v2

Only taking showstopper issues at this point.  If this is one of those please re-nom with why.
Attachment #318188 - Flags: approval1.9? → approval1.9-
Comment on attachment 317962 [details] [diff] [review]
patch

Only taking showstopper issues at this point.  If this is one of those please
re-nom with why.
Attachment #317962 - Flags: approval1.9? → approval1.9-
Comment on attachment 318188 [details] [diff] [review]
patch for extensions v2

See bug 347910, comment #2. bz _really_ wants this. Patch is very safe, as it's just removing code that was only used on the branch and nothing more.
Attachment #318188 - Flags: approval1.9- → approval1.9?
Comment on attachment 317962 [details] [diff] [review]
patch

See bug 347910, comment #2. bz _really_ wants this. Patch is very safe, as it's just removing code that was only used on the branch and nothing more.
Attachment #317962 - Flags: approval1.9- → approval1.9?
One of those zero-risk issues, since we don't use this code at all.
We should take this to get rid of 1.8 cruft in the hg.mozilla.org and 1.9.x worlds, ASAP.

/be
Comment on attachment 317962 [details] [diff] [review]
patch

Per discussion on IRC with shaver, gavin, reed, we don't have an urgent need for this on 1.9.   Land away on Hg.
Attachment #317962 - Flags: approval1.9? → approval1.9-
Comment on attachment 318188 [details] [diff] [review]
patch for extensions v2

Actually, this is all NPOTB. I'll just land this (minus the content changes that are really in the other patch).
Attachment #318188 - Flags: approval1.9?
Comment on attachment 318188 [details] [diff] [review]
patch for extensions v2

Per discussion on IRC with shaver, gavin, reed, we don't have an urgent need for this on 1.9.   Land away on Hg.
Attachment #318188 - Flags: approval1.9-
Comment on attachment 318188 [details] [diff] [review]
patch for extensions v2

NPOTB (minus the content/ changes that are also in the other patch).
Attachment #318188 - Flags: approval1.9-
We have tons of dead code in the tree, and shouldn't be injecting noise into the end-game by landing patches that are _designed_to_do_nothing_.

The _interfaces_ are already gone, which is the most important issue as per bz's original bug, and so don't represent an attractive nuisance for code consumers.

In hg, let's by all means remove dead code for 1.9.x and Mozilla 2.  This is just the tip of the iceberg, I'm sure.

http://mxr.mozilla.org/firefox/search?string=MWContext !
Checking in extensions/metrics/build/nsMetricsModule.cpp;
/cvsroot/mozilla/extensions/metrics/build/nsMetricsModule.cpp,v  <--  nsMetricsModule.cpp
new revision: 1.5; previous revision: 1.4
done
Checking in extensions/metrics/src/nsLoadCollector.cpp;
/cvsroot/mozilla/extensions/metrics/src/nsLoadCollector.cpp,v  <--  nsLoadCollector.cpp
new revision: 1.25; previous revision: 1.24
done
Checking in extensions/metrics/src/nsMetricsService.cpp;
/cvsroot/mozilla/extensions/metrics/src/nsMetricsService.cpp,v  <--  nsMetricsService.cpp
new revision: 1.46; previous revision: 1.45
done
Checking in extensions/metrics/test/TestMetricsConfig.cpp;
/cvsroot/mozilla/extensions/metrics/test/TestMetricsConfig.cpp,v  <--  TestMetricsConfig.cpp
new revision: 1.4; previous revision: 1.3
done
Status: NEW → ASSIGNED
Keywords: checkin-needed
Whiteboard: [needs landing after 1.9]
Pushed in 15888:5fba7bea3723.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing after 1.9]
Target Milestone: --- → mozilla1.9.1a1
You need to log in before you can comment on or make changes to this bug.