Closed Bug 408370 (steel) Opened 14 years ago Closed 10 years ago



(Thunderbird :: Mail Window Front End, defect, P2)



(Not tracked)

Thunderbird 3.0b3


(Reporter: jminta, Assigned: jminta)




(Keywords: dev-doc-complete)


(3 files, 8 obsolete files)

Attached patch WIP (obsolete) — Splinter Review
This bug is to track the implementation of STEEL 0.1.  See for implementation plans, as well as to discuss interface design.

Attached is a WIP patch.  I just figured out how to get the message body, and that felt like a good point to checkpoint and save.  I haven't done anything with the event model yet, still trying to wrap my head around the various listeners.
Adds some documentation of various aspects of interfaces I've been using in the course of building STEEL.
Attached patch WIP2 (obsolete) — Splinter Review
This is a further work-in-progress patch.  It includes an implementation of the event-model, but this is largely untested.  I would highly discourage playing around with this patch at this point, since it's likely still very broken.  A couple of things remain before a final patch is ready:

-reply/forward functions of steelIMessage may be blocked by bug 408511.
-getHeader function of steelIMessage may be blocked by bug 364807.
-still stuck on how to enumerate attachments
-need some sort of reasonable testing framework

The blocking bugs can be "worked around" if necessary, by simply reimplementing the back-end code within the STEEL objects.  Depending on timing, this may be the path of least resistance, unfortunately.  (Also note that from my current understanding, solving attachments will require exactly the same re-implementation as solving headers.)

As far as a testing framework, I'm currently leaning towards adapting the code I wrote in bug 358985 to Thunderbird.  This and Firefox's mochikit testing framework are the only ways I know of to get a testing environment that includes the application's chrome (where STEEL lives).  There is the further complication that a testing framework would be inherently specific to a particular instance of Thunderbird, since there are very limited things to test without a particular mail account.  I'll likely include my tests here, but most will fail on other computers, since they'll be based on my own mailbox.
Attachment #293137 - Attachment is obsolete: true
Depends on: 409458
Attached patch WIP3 (obsolete) — Splinter Review
This is mostly the same as WIP2, but with a bajillion small bugfixes from lots of testing.  I'll attach my tests and say more about what works and what doesn't once bug 409458 has either been fixed or cleared as safe by the security experts.  Suffice it to say the issues from the previous WIP all still apply.
Attachment #294208 - Attachment is obsolete: true
I'm removing the dependency on the security bug, since I've found a way to use alternative code-paths, avoiding the issues there.
No longer depends on: 409458
Attached patch WIP4 (obsolete) — Splinter Review
More backing-up code dumping.

This WIP has most of the event-model tested and working, while avoiding the security bug.  It also includes some extra methods that it became clear the idl was missing.  Specifically, a steelIAccount now offers the ability to create and delete top-level folders for that account.  Also, regular steelIFolders allow for the deletion of child folders.

More importantly, this patch has an untested cut at headers and attachments.  I'm hoping to flesh out my tests later this weekend, which, assuming the code actually works, then leaves only the reply/forward functions that are blocked by the quoting bug (bug 408511).  Unfortunately, I haven't seen any movement on that bug, so it's looking like I'm going to need to reimplement those functions in the steel implementation.  Still, I think things are getting close, so that's good! :-)
Attachment #294437 - Attachment is obsolete: true
This is a rough testing framework that I've been using to test STEEL.  It's based largely on the browser mochikit testing framework.  To be generally useful, it still needs a defined place to copy all test files, and to iterate over those files at the beginning of the test-run.  Nonetheless, it's to a point where other front-end hackers should be able to easily modify it for their own local uses.

The tests you see here assume a clean profile.  I have a second set of tests that exercise more of the steelIMessage interface, but which rely on my own mailbox's characteristics.

There is currently 1 unfixed FAIL test, the deleteSubFolder test.  I'm still trying to track that one down.

The framework revealed a couple small bugs that I've fixed, but I don't think it's worth it to attach other WIP yet.
Attached patch WIP5 (obsolete) — Splinter Review
I learned me some js1.7 and 1.8 this weekend. :-)

This includes the bugfixes mentioned in the previous comment.  More importantly, it switches to using a bunch of nice new javascript features.  The most interesting of these is the iterator code I've added.  We may want to consider factoring this out into a js-module, for use in any front-end code that has to deal with the various array/enumerator classes.  For now, it just gives me smaller, cleaner code.

Nothing new in terms of implementation features here though.
Attachment #294841 - Attachment is obsolete: true
Depends on: 418490
Attached file preview extension (obsolete) —
This is an extension to make it easier for people to play with the API and provide some feedback.  At this point, I've made rough cut attempts at all of the interfaces defined in the previous WIP patch.  You should use the steelIApplication.idl and extIApplication.idl in that patch as your guide to the api.  I fully expect lots of broken-ness though, and would appreciate reports of any such problems here.

After installing the extension, you can confirm its existence, and play around with basics bits in the Error Console.  Try "Application.accounts.all[0].folders[0].name" for instance.  You'll probably need to write basic extensions yourself to do anything more complex than that though.
Two feedback items I've gotten so far, that I don't want to lose:
  - the Application object doesn't fire an event for new-mail, only folders do. We should make both fire the event
  - there is no way to get the cc-list of an email.

Both will be fixed for the actual 0.1 patch.
Depends on: 424024
Attached patch patch for initial review (obsolete) — Splinter Review
This patch includes a lot of fixes from the feedback I got with the preview extension.  I'm confident enough in it to ask for an initial review, to hopefully get the ball rolling in time for this to make a1.  Obviously the iterator code at the end of the file can disappear if/when the iterator-utils patch lands.
Attachment #295663 - Attachment is obsolete: true
Attachment #312928 - Flags: superreview?(dmose)
Attachment #312928 - Flags: review?(dmose)
Note to self here: the accounts bit will probably need a bug 41133 hack too.  I'll address that once the initial review is done.
Bug 420614 has bit rotted this I think.
Comment on attachment 312928 [details] [diff] [review]
patch for initial review

This has blocked waiting for my review to long.  Neil and bienvenu both have broader experience across the mailnews/ codebase, so you'll get more useful feedback to boot!
Attachment #312928 - Flags: superreview?(dmose)
Attachment #312928 - Flags: superreview?(bienvenu)
Attachment #312928 - Flags: review?(neil)
Attachment #312928 - Flags: review?(dmose)
The patch has also bitrotted because of nsIMsgFolderListener interface changes.
I thought I'd mentioned that already, but obviously I didn't ;-), so:
Is this TB-centric "enough" to "hide" it inside /mail?
Wouldn't it make more sense to put it into /mailnews, so that can be shared?
This should probably block 3.0b1; though we may need to reconsider later.  Joey, given that code freeze is currently September 9th, should we reassign this to someone else to drive?
Flags: blocking-thunderbird3.0b1+
Another breakage is because of changes in the getMsgTextFromStream interface.
Switching for b1 flags to target milestones, to avoid flag churn.
Target Milestone: --- → Thunderbird 3.0b1
Whiteboard: [needs review neil][needs review bienvenu
Whiteboard: [needs review neil][needs review bienvenu → [needs review neil][needs review bienvenu]
Joey's super busy at the moment, so re-assigning to nobody to better reflect a reality.  We need to find a driver still.
Assignee: jminta → nobody
Whiteboard: [needs review neil][needs review bienvenu] → [needs owner][needs updated patch][needs review neil][needs review bienvenu]
Priority: -- → P2
Comment on attachment 312928 [details] [diff] [review]
patch for initial review

bienvenu has expressed interest in delegating his review to asuth, since he's spent lots of time in the front-end code, albeit not (yet) in-tree.  Andrew, does this work for you?
Attachment #312928 - Flags: superreview?(neil)
Attachment #312928 - Flags: superreview?(bienvenu)
Attachment #312928 - Flags: review?(neil)
Attachment #312928 - Flags: review?(bugmail)
Whiteboard: [needs owner][needs updated patch][needs review neil][needs review bienvenu] → [needs owner][needs updated patch][needs review asuth][needs review neil]
Will review.
As one might expect given the amount of time this patch has been waiting on
review, there's some bit-rot.  The most extensive rot is in the Folder class's
keep-state-up-to-date logic.  (Bug 439225 changed up the way
nsIMsgFolderNotificationService/nsIMsgFolderListener works rather extensively.)

I have done various other de-rotting already, but I think we should probably
figure out the gloda/STEEL interaction before Joey or I make the effort to
update this code.

The Folder class listens to the various notifications so it can keep its list
of sub-folders, contained messages, and unread messages accurate.  It does this
somewhat expensively, as every Folder instance registers as a
catch-all-listener, ignoring events (or erroneously failing to ignore events)
that don't concern it.  (STEEL attempts to proactively create a Folder instance
for every folder for every account, so it will really add up.)

Gloda's indexing mechanism already handles the event-stream in a more efficient
fashion, and its query mechanisms allows filtering based on folder as well as
updating the resulting collection as the contents of the folder change.

As such, I propose:
1) We let gloda handle the folder/message indexing.

2) We assume that gloda's event-generating query-based collections are
sufficient to cover the use-cases that would have been served by STEEL's
folder/message events.  For those we don't cover, I think extensions may be
sufficiently well off just using nsIMsgFolderNotificationService and converting
things back into gloda/STEEL space as needed.

3) We fuse STEEL's Folder and Message classes with those of gloda.  (Actually,
gloda doesn't have a real object representation for folders yet.)  This
primarily means taking the mutating methods and extensive helper functions
(reply/forward/etc) to the existing gloda indexing-based attribute-exposures. 
Then we go through and make sure that any convenience filters (ex: unread
messages for a folder) are exposed as convenience queries.

4) We use the STEEL Application/Accounts/Account classes as is, exposing the
new hybrid gloda/steel folders.

Flags: blocking-thunderbird3+
Pushing off to b2 based on discussions in IRC.
Target Milestone: Thunderbird 3.0b1 → Thunderbird 3.0b2
Flags: blocking-thunderbird3.0b1+
Target Milestone: Thunderbird 3.0b1 → Thunderbird 3.0b2
I'm going to be pushing for the remainder of this to get some traction for the b2 time frame.  I think andrew's comment #22 has laid out exactly what needs to be done.
Whiteboard: [needs owner][needs updated patch][needs review asuth][needs review neil] → [needs owner][needs updated patch][see comment #22]
Until we can get someone to own this it will continue to slip.  I'd like to block the next release for this, be that rc1.  STEEL is a necessary part of any add-on story for future Thunderbird's.
Target Milestone: Thunderbird 3.0b2 → Thunderbird 3.0rc1
Why is this important enough to block RC1 (especially since there seems so little discussion about it on the wiki and there are not even proposals to replace most of the existing mail interfaces) yet nobody apparently cares about providing documentation for the current XPCOM mail interfaces?

The Mozilla Developer Center only has documentation on core and glue XPCOM interfaces. XUL Planet states its documentation is very old, out of date and its front page states the web site is being phased out in favor of the MDC. 

I don't know if I'm part of your target audience. I'm a experienced COM programmer who is working on his first extension. But what I read about STEEL just creates FUD.
Eric, you're right that that needs to happen too.  However, there are a limited number of resources available for Thunderbird, so the theory is that getting clean, well-documented interfaces for the small subset of very frequent tasks is significantly more bang for the buck than spending an equivalent amount of effort making a smaller dent in the much larger set of hard-to-use interface.
My impression from a discussion with Andrew a while back is that he's signed up to do this work.
Assignee: nobody → bugmail
Pushing on this for b3 now that we have that target created; rc1 would be too late in the tb3 timeframe.
Whiteboard: [needs owner][needs updated patch][see comment #22] → [needs updated patch][see comment #22]
Target Milestone: Thunderbird 3.0rc1 → Thunderbird 3.0b3
(In reply to comment #27)
> the theory is that getting clean, well-documented interfaces for the small subset of very frequent tasks

Dmose, with asuth's work on gloda being "the big thing" for 3.0b3 - which cannot be allowed to slip - I fear that STEEL will be lost if asuth can't get to it.

I don't want to jinx anything, but STEEL missed both b1 and b2 target. So in the context of gloda, I wonder if it is worth evaluating whether to adjust the schedule or assignment for STEEL?  And, should this be part of B3UX?  Seems like extension authors might welcome being included in that way :)
There is indeed risk that STEEL won't make it.  This isn't really a "User eXperience" bug, but assigning it to a milestone sounds worthwhile.  Andrew, do you have thoughts about the likelihood of this happening?
STEEL is like an onion; it has many layers.  Perhaps the right course of action is to land the FUEL parity parts of STEEL real-soon-now.  We then lightly port the account things and land them ('lightly' in this case meaning stripping out the folder abstraction stuff and making sure things work but not enhancing anything).  The folder/message gloda augments are going to need more love than I can provide at the current time and we punt on them.
Attachment #312928 - Attachment is obsolete: true
Attachment #312928 - Flags: superreview?(neil)
Attachment #312928 - Flags: review?(bugmail)
That sounds like a fine plan to me; some STEEL goodness is better than none!
realistically, we won't block on this, even though we really want it.
Flags: wanted-thunderbird3+
Flags: blocking-thunderbird3-
Flags: blocking-thunderbird3+
Just wanted to note that from an extension developers perspective.

The Extension object of the FUEL/STEEL library has the only code that one really, really wants.  It takes care of some of the really crappy parts of extension writing that could be covered by docs but is just annoying.  

Having a built-in "first run" is really handy.

I needed the following code to handle uninstall of an extension, something that FUEL/STEEL does very nicely in their events object via an "uninstall" event.

The observe event is where it really gets annoyingly peculiar

The other pieces of the library are nice, but I could live without them.  However the Extension piece is pretty necessary for easy development.  

I know this comment probably means I have to drive this bug now, so if anyone is willing to help out I could really use it.
Apologies if this isn't (or is no longer) the right venue for more technical suggestions for STEEL, but:

At a suggestion was made that seems like a really cool extension: hook up Gloda's ease of identifying contacts with minimal logic for utterly formulaic emails, to make those emails much easier to use.

I've tinkered around with this idea a bit (literally only one afternoon!), and have the outlines of this extension implemented (after poking around for some Gloda API documentation, it was easy).  But I'm stuck at the last step: actually *changing* the reply-to header of the message to reflect the contact we've identified.  I've tried using nsIMsgBDHdr.setStringProperty, which works in the sense that a subsequent getStringProperty returns the value I set.  But it fails in that the displayed headers' reply-to field does not change---the currentHeaderData does not get updated, even if I reload the message.  (More accurately, it reloads whatever MIME data exists when the message was downloaded, which was not changed by setStringProperty.)

I suspect I'm not doing anything wrong, but rather that messages, once downloaded, are very tricky if not impossible to modify. there any (eventual) place in STEEL for an API to persistently modify headers of received messages?  I only found one extension that purported to do so (TB Header Tools), but its code is scary and also only works in TB1.0(!)

Is this a common enough and/or hard enough to merit STEEL simplification?
(BTW -- I'm not trying to advocate for feature creep; if this doesn't belong to STEEL at all, or belongs in STEEL 2.0, that's fine... ;-) thanks!)
Ben, modifying message bodies is made basically impossible by the IMAP case.  (We would need to completely add a new message that is the modified copy, then delete the original, as I understand it.)  Also, I would call that a major hack.

However, the message reader and reply-to process could be made more flexible.

As an aside (which shouldn't really be followed-up here, the newsgroup is most appropriate), gloda was designed to actually support doing these things to gloda's understanding.  For example, the bugzilla plugin for gloda makes it seem like the message is 'from' the person who took the action on the bug.  One could easily write an extension that did the same for facebook (maybe you did?)  The trick is that only exptoolbar ever displays this information for now.
Thanks for the quick reply.  I had a hunch the IMAP case makes things tricky, but figured I'd check first.  And yeah, it's a hack all right :)

So far, all I've used Gloda for was to query(NOUN_CONTACT).name(<the string I'd scraped>) -- very simple to figure out how to use, and perhaps a trivial ability on Gloda's part :)  For the reply-to stuff, I went straight to the nsIMsgDBHdr, because, as you said, only the exptoolbar currently consumes Gloda data, not the 3-pane interface...

Anyway, I'll follow up on m.d.a.tb...

Separate aside: I'd mentioned, in bug #468684, that I had an extended version of Lightning Nightly Updater that would track exptoolbar and glodabook as well.  I've heard nothing back from LNU's author, so that extension has just been sitting on my computer, doing nobody any extra good.  If there's another extension (you mentioned the bugzilla-for-gloda) I should track too, I'd be happy to add that, and publish the revised LNU where it would do more good...
(In reply to comment #38)
> Ben, modifying message bodies is made basically impossible by the IMAP case. 
> (We would need to completely add a new message that is the modified copy, then
> delete the original, as I understand it.)  Also, I would call that a major
> hack.

Isn't this exactly what we do for deleting attachments? :-)

In this case though, it's not necessary for the updates to be persisted to the IMAP data store. We just need them in the UI (and, thereby, acted upon by things like the Reply command). Is it possible to patch the in-memory data? Or is the data object too closely tied to the IMAP copy? (Waves hands)

gerv, I created a thread in to continue this.
Attached patch toolkit parts only (obsolete) — Splinter Review
The reason I originally abstracted out the exthelper stuff was so that all toolkit apps could do this minimal embedding without much effort. This patch contains the smallest amount of code possible to get something like FUEL into thunderbird. It seems, given resource constraints, that just getting this barebones, toolkit functions only, version of steel in is better than nothing. This will allow extensions that use the toolkit stuff to be cross-compatible with Thunderbird 3.

We can then work through the other, mail-specific stuff as time allows.

Hoping dmose has time to get to this...
Attachment #377698 - Flags: superreview?(dmose)
Attachment #377698 - Flags: review?(dmose)
Comment on attachment 377698 [details] [diff] [review]
toolkit parts only

Remove the 'observe' method from Application from steelApplication.js:

that patch is roughly:
-  observe: function app_observe(aSubject, aTopic, aData) {
-    this.__proto__.__proto__.observe(aSubject, aTopic, aData);
-  },

I checked the FUEL code and it does this to be able to call the super-class method and add its own behavior.  Since no behavior is added, it does not need to be there.  Also, the FUEL version uses call so that the 'this' does not become inconsistent.

Without this change, an exception gets produced on shutdown because of the 'this' mis-match:

[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIObserverService.removeObserver]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: mozilla/dist/bin/components/steelApplication.js :: app_observe :: line 737"  data: no]

r=asuth with that change, but it needs dmose sign-off too, even if it just deferral to me.

Thank you for the stick-to-itiveness on this bug/patch.
Attachment #377698 - Flags: review+
Assignee: bugmail → jminta
Attachment #377698 - Flags: superreview?(dmose)
Attachment #377698 - Flags: review?(dmose)
Comment on attachment 377698 [details] [diff] [review]
toolkit parts only

Piecemeal is indeed much better than nothing; a fine strategy.  I'll happily delegate my review bits to asuth, so once you make the changes he suggests, feel free to land.  Bonus points for writing a unit test or three.
Whiteboard: [needs updated patch][see comment #22] → [needs revised patch][has review]
Joey, do you have time to update the patch with the change and land it, or would you like me to do that?
Alias: steel
Blocks: 497564
Joey, I can't take the suspense anymore! :-D

I've updated the patch to address asuth's comment #43 suggestion.  I'm not going to carry the review forward since this isn't my patch but it should be easy to see from the attachment diff and get a quick r=asuth + checkin-needed keyword.

I'm really glad to see this get in, I think Thunderbird 3 is going to be a great space for add-on developers.  Once this lands we can get jenzed into full swing with add-on docs.
Attachment #377698 - Attachment is obsolete: true
Attachment #383584 - Flags: review+
Comment on attachment 383584 [details] [diff] [review]
[checked in] toolkit parts with updates addressed

> I'm not going to carry the review forward

umm... whoops
Attachment #383584 - Flags: review+
(In reply to comment #46)
> I've updated the patch to address asuth's comment #43 suggestion.  I'm not
> going to carry the review forward since this isn't my patch but it should be
> easy to see from the attachment diff and get a quick r=asuth + checkin-needed
> keyword.

I'm jumping in here. The patch is exactly what I'd have done, and what asuth told me needed doing. The only additional thing I've changed is to remove the duplicate addition of exthelper.xpt in packages-static. I think we can therefore land this and move forward.
Comment on attachment 383584 [details] [diff] [review]
[checked in] toolkit parts with updates addressed

Landed in time for today's nightlies:
Attachment #383584 - Attachment description: toolkit parts with updates addressed → [checked in] toolkit parts with updates addressed
Adding dev-doc-needed, and cc'ing Jen. Quick summary for Jen:

The patch we've just landed implements an "empty" steelIApplication interface which inherits from extIApplication which is part of toolkit.

Looking at devmo, the FUEL documentation ( appears to be out of date - much of FUEL moved to toolkit in the last cycle and interfaces got renamed, this doesn't appear to have been updated - I added the dev-doc-needed keyword to bug 407963 as well which implemented those changes.

So far the documentation we have for steel is here: but we've only landed a small subset of that at the moment.

I think only main devmo page that references steel is the Thunderbird one:

It would be great to start knocking the Fuel & Steel documentation into shape :-)

Keywords: dev-doc-needed
Whiteboard: [needs revised patch][has review]
I've created an initial page for STEEL here:

If anyone has time to improve the documentation on that and/or help update the FUEL documentation (see previous update) it would be very much appreciated.
Joey, please add the .xpi and its version info to either the or AMO (or better to both). This will allow people to locate it easier.
(In reply to comment #52)
> Joey, please add the .xpi and its version info to either the
> or AMO (or better to both). This will
> allow people to locate it easier.

The current xpi will be out of date wrt what has landed and most likely the xpi isn't up to date with the current interfaces in mailnews.

I'm also not convinced you can easily make the remaining interfaces work as an extension given what has already landed in the builds.
Attachment #309579 - Attachment is obsolete: true
I think as we did land some of steel ages ago, we can call this fixed. If we go the route of including more interfaces in future, then we can do that in new bugs.
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.