Closed Bug 479006 Opened 16 years ago Closed 5 years ago

get rid of crufty timeline service globals [dead code]

Categories

(MailNews Core :: Backend, task)

Product:
MailNews Core
Component:
Backend
Type:
task
Priority:
Not set
Severity:
minor

Tracking

(seamonkey2.49esr unaffected, seamonkey2.53 fixed, seamonkey2.57esr fixed, seamonkey2.63 wontfix)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 71.0
Tracking Flags:
Tracking Status
seamonkey2.49esr --- unaffected
seamonkey2.53 --- fixed
seamonkey2.57esr --- fixed
seamonkey2.63 --- wontfix

People

(Reporter: davida, Assigned: iannbugzilla)

References

Details

(Whiteboard: [patchlove])

Attachments

(3 files, 6 obsolete files)

Remove gTimeline code (esr) v1.3
7.21 KB, patch
frg
: review+
frg
: approval-comm-release+
Details | Diff | Splinter Review
Remove gTimeline code (cc) v1.2
6.43 KB, patch
frg
: review+
Details | Diff | Splinter Review
479006-remove_gTimeline_esr60.patch
6.46 KB, patch
frg
: review+
frg
: approval-comm-esr60+
Details | Diff | Splinter Review 
http://mxr.mozilla.org/comm-central/search?string=gtimeline&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central

I don't think we're using the timeline service at all as a perf tool.  gTimelineService and gTimelineEnabled should be excised from /mailnews.
Attached patch excision (obsolete) — — Splinter Review 
(obviously not to land until after branch, and taking seamonkey schedule into consideration)
Assignee: nobody → david.ascher
Status: NEW → ASSIGNED
Attachment #362854 - Flags: superreview?
Attachment #362854 - Flags: review?
Attachment #362854 - Flags: superreview?(bienvenu)
Attachment #362854 - Flags: superreview?
Attachment #362854 - Flags: review?(bienvenu)
Attachment #362854 - Flags: review?
Comment on attachment 362854 [details] [diff] [review]
excision

these are seamonkey-only files, with the possible exception of subscribe.js, so just running this past Neil.
Attachment #362854 - Flags: superreview?(bienvenu)
Attachment #362854 - Flags: superreview+
Attachment #362854 - Flags: review?(neil)
Attachment #362854 - Flags: review?(bienvenu)
Attachment #362854 - Flags: review?(neil) → review+
Comment on attachment 362854 [details] [diff] [review]
excision

Actually I think subscribe.js is forked too, although jcranmer wants to change that eventually.
hmm, i don't know if this patch was landed, but the mxr search is now showing a lot less stuff.
Not to me it's not: all the stuff your patch removes still exists, but after the bug 390262 disentanglement, comment 2 is more obviously correct, since they're now in suite/mailnews/ instead of mailnews/ (except for mail/base/content/SearchDialog.js, which your patch somehow missed back when it was a Thunderbird-only file living in mailnews/).

But as I said in bug 417267 when we removed the same stuff from the Thunderbird forks, it was pretty much dead and broken, so there's really no point in waiting until after we fork. I'd say you should just unrot your patch, include SearchDialog.js in it, and land it.
Blocks: 579571
(In reply to Phil Ringnalda (:philor) from comment #5)
> ...
> But as I said in bug 417267 when we removed the same stuff from the
> Thunderbird forks, it was pretty much dead and broken, so there's really no
> point in waiting until after we fork. I'd say you should just unrot your
> patch, include SearchDialog.js in it, and land it.
Assignee: davida → nobody
Status: ASSIGNED → NEW
Whiteboard: [patchlove]
Severity: trivial → minor
Type: defect → task
Summary: get rid of crufty timeline service globals → get rid of crufty timeline service globals [dead code]
Assignee: nobody → iann_bugzilla
Status: NEW → ASSIGNED
Attached patch Remove gTimeline code (cc) (obsolete) — — Splinter Review

Remove dead code (one global in TB, rest of code in SM)

Attachment #362854 - Attachment is obsolete: true
Attachment #9102755 - Flags: review?(frgrahl)
Attached patch Remove gTimeline code (esr) (obsolete) — — Splinter Review

Patch against older branches

Attachment #9102756 - Flags: review?(frgrahl)
Attachment #9102756 - Flags: approval-comm-release?
Attachment #9102756 - Flags: approval-comm-esr60?
Attachment #9102756 - Attachment description: 479006_remove_gTimeline_esr.patch → Remove gTimeline code (esr)
Attached patch Remove gTimeline code (esr) v1_1 (obsolete) — — Splinter Review

Missed subscribe.js and now includes header

Attachment #9102756 - Attachment is obsolete: true
Attachment #9102756 - Flags: review?(frgrahl)
Attachment #9102756 - Flags: approval-comm-release?
Attachment #9102756 - Flags: approval-comm-esr60?
Attachment #9102759 - Flags: review?(frgrahl)
Attachment #9102759 - Flags: approval-comm-release?
Attachment #9102759 - Flags: approval-comm-esr60?
Attached patch Remove gTimeline code (cc) v1_1 (obsolete) — — Splinter Review

Now includes a header

Attachment #9102755 - Attachment is obsolete: true
Attachment #9102755 - Flags: review?(frgrahl)
Attachment #9102760 - Flags: review?(frgrahl)
Attached patch Remove gTimeline code (esr) v1.2 (obsolete) — — Splinter Review

Rebased against a cleaner tree

Attachment #9102759 - Attachment is obsolete: true
Attachment #9102759 - Flags: review?(frgrahl)
Attachment #9102759 - Flags: approval-comm-release?
Attachment #9102759 - Flags: approval-comm-esr60?
Attachment #9102761 - Flags: review?(frgrahl)
Attachment #9102761 - Flags: approval-comm-release?
Attachment #9102761 - Flags: approval-comm-esr60?

Also remove the pref from mailnews.js

Attachment #9102761 - Attachment is obsolete: true
Attachment #9102761 - Flags: review?(frgrahl)
Attachment #9102761 - Flags: approval-comm-release?
Attachment #9102761 - Flags: approval-comm-esr60?
Attachment #9102767 - Flags: review?(frgrahl)
Attachment #9102767 - Flags: approval-comm-release?
Attachment #9102767 - Flags: approval-comm-esr60?

Also remove the pref from mailnews.js

Attachment #9102760 - Attachment is obsolete: true
Attachment #9102760 - Flags: review?(frgrahl)
Attachment #9102768 - Flags: review?(frgrahl)
Comment on attachment 9102768 [details] [diff] [review]
Remove gTimeline code (cc) v1.2

LGTM 

Touches TB code. I don't see any use there so I suspect dead and gone too. Jork do you want to check this in?

We have a ESR60 version. Do still care about this branch or can I check it in with DONTBUILD? Or do you prefer removal of the mail* changes for it.
Flags: needinfo?(jorgk)
Attachment #9102768 - Flags: review?(frgrahl) → review+
Comment on attachment 9102767 [details] [diff] [review]
Remove gTimeline code (esr) v1.3

r/a = me.

Mail* part for esr60 pending on answer from Jorgk
Attachment #9102767 - Flags: review?(frgrahl)
Attachment #9102767 - Flags: review+
Attachment #9102767 - Flags: approval-comm-release?
Attachment #9102767 - Flags: approval-comm-release+
Attachment #9102767 - Flags: approval-comm-esr60?
Attachment #9102767 - Flags: approval-comm-esr60+

Touches TB code. I don't see any use there so I suspect dead and gone too. Jork do you want to check this in?

Yes, please. Crufty indeed. Strangely the linter hasn't complained about the unused gTimelineEnabled.

Flags: needinfo?(jorgk)
Keywords: checkin-needed

Need to push Bug 1588876 first

Depends on: 1588876
Attachment #9102767 - Flags: approval-comm-esr60+

Rebased ESR60 version. subscribe.js was removed in Bug 1425962.

jorg could go into 60 if you don't mind. Otherwise we remove the TB part. We don't care currently about 68.

Attachment #9102816 - Flags: review+
Attachment #9102816 - Flags: approval-comm-esr60+

To me, TB 60 is EOL. We won't ship another version of it. If you want, add a=jorgk.

Since you're only removing dead code, there's no need to backport to 68. I'll land the C-C patch in the next 16 hours (before 12 noon on Monday).

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/b0211b821650
get rid of crufty timeline service globals [dead code]. r=frg

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 71.0
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: