code cleanup: CreateStartupUrl() see comment in code // XXX fix this, so that base doesn't depend on imap, local or news. // we can't do NS_NewURI(uri, aUrl), because these are imap_message://, mailbox_message://, news_message:// uris. // GetMessageServiceFromURI() to get the service, and then have the service create the appropriate nsI*Url, and // then QI to nsIURI, and return it.

I'm moving to future, but if this affects something important, you can move it back in.

Use message server to create nsURI

This is not just clean up, it also fixes problem of being unable to get message folder charset when composing fwd inline/draft/template, see bug 66098. (nsMailboxUrl expect message number in form ?number=<num>, but with current CreateStartupUrl it gets msg number in form #<num>. So, msg key if left at default 0xffffffff, and .... Seth, does #-notation comes from rdf stuff? (otherwise I cannot imagine why not to always use only ?number= notation...)

Nominating for nsbeta1, this is needed by bug 66098.

Seth, could you review the patch?

reassigning to adu@sparc.spb.su

Well, could anyone review this?

I'm behind in my reviews, give me a few days to get caught up please.

Removed unneeded |#include|s and |NS_DEFINE_CID|s as well...

Denis, with the current code, does GetMsgDBHdrFromURI fail to get a message header for the draft case?

Yes, assertion still here: WARNING: NS_ENSURE_TRUE(msg) failed, file nsMailboxUrl.cpp, line 485 WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed, file nsMailboxUrl.cpp, line 497

I set a break point at CreateStartupUrl() and tried forward as inline. The URI my case, "mailbox-message://nobody@Local%20Folders/smoketest#39278?fetchCompleteMessage=true" it was handled by the line below and returned without error. else if (PL_strncasecmp(uri, "mailbox", 7) == 0) The change in the patch of using GetMessageServiceFromURI, what does it make different? I know it fixes the folder charset problem but not clear the reason.

Naoki, take a look at the code where assertion happens. I don't have mozilla sources at the moment, but relevant pieces is where msg key is parsed. Currently, we fail to get msg key from url which contains msg key in form '#msgkey'. Using GetMessageServiceFromUri() allows us to successfully parse it. I'll able to provide more details tomorrow, when I'll get to work.

GetUri (I checked nsMailboxUrl::GetUri and nsImapUrl::GetUri) behave differently for forward inline case. Usually, GetUri returns mURI if not empty, something like this, for a local message, "mailbox-message://nobody@Local%20Folders/smoketest#39278" But for inline forward mURI is empty and it generates something like this for the same local message, "mailbox-message:/#-1" Note that the folder name 'smoketest' is not available, so we cannot get information like a folder charset.

When we call SetSpec, we eventually get to nsMailboxUrl::ParseSearhPart() method, which parse msg key in the following way: char * messageKey = extractAttributeValue(searchPart, "number="); if (messageKey) m_messageKey = atol(messageKey); // convert to a long... So, when we don't have "?number=<...>" in URL, we'll get -1 as msg key. When we use (as in my patch) GetMessageServiceFromURI and then message service's GetUrlForUri, we'll get in PrepareMessageUrl, which will parse #<msgkey> notation for us.

I set a break point at nsMailboxUrl::ParseSearhPart(). http://lxr.mozilla.org/seamonkey/source/mailnews/local/src/nsMailboxUrl.cpp#374 it has "?number=<...>" for forward inline too. So still not clear about the problem. By the change to call GetUrlForUri() which calls PrepareMessageUrl() then eventually SetUri() is called. So the later call to GetUri can return the stored URI (so GetFolderCharset does not fail). I think the patch does the right thing for forward inline but not known about other cases. Could you make the change specific to forward inline (and ohter draft related)? I think we can check if the URI ends with "fetchCompleteMessage=true". Then attach the patch to bug 117956 or bug 66098.

Is it unreal to get sspitzer to review this? CreateStartupUrl() is used in only one place besides draft operations (in save attachment in nsMessenger), so I think draft-specific handling would be overkill...

But save attachment does not work with the patch. I tried both IMAP and local mail, msgService->GetUrlForUri returned error. nsParseLocalMessageURI returns error if '#' is not found. I prefer changing it for draft specific at least for now.

Some XPCOM voodoo magic... I don't understand, why QueryInterface on nsIURI returned from GetUrlForUi (back to nsIMailNewsUrl) doesn't work. I don't see much difference between old CreateStartupUrl() and XXXMessageService::GetUrlForUri()....

>why QueryInterface on nsIURI returned from GetUrlForUi (back to nsIMailNewsUrl) >doesn't work. The problem is not that. With the patch, it generates URI without '#' which does not work for the exisiting code nsParseLocalMessageURI().

OK, will post patch for draft operations to 66098. But I still don't understand, why is two url naming convensions needed. Reassigning back to sspitzer

well, really reassign...

sorry for the spam. making bugzilla reflect reality as I'm not working on these bugs. filter on FOOBARCHEESE to remove these in bulk.

Filter on "Nobody_NScomTLD_20080620"

This issue prevents my exchange web services code from being able to implement save attachment. I'll probably need to submit a fix for this.

rkent, will you finish this?

(In reply to :aceman from comment #26) > rkent, will you finish this? Hmmm, that comment 25 was three years ago. I developed a workaround for this, which reduces my incentive to fix this. So I don't have any plans for the near future to get to this, so I should probably just remove my name from the assign.

(In reply to :aceman from comment #26) > rkent, will you finish this? It seems that, despite its comment to the contrary, the entire function is unnecessary and can be replaced with NS_NewURI. The function is used in three places: * Opening an attachment to a message in a local folder * Saving an attachment * Saving a message as text or HTML I wasn't able to discern any problem with those three functions with these changes applied. One immediate advantage of this is that you can then save ExQuilla messages as text. It would also remove ExQuilla's need to hook into the attachment saving code. (Obviously its use by the local folder code is not relevant to ExQuilla.) Naturally this would most benefit ExQuilla if it could be uplifted to Thunderbird ESR.

Comment on attachment 8998799 [details] [diff] [review] 110689.diff How have we suffered with CreateStartupUrl() through all the nsIURI changes :-( - Happy to see it go. I'll look into it.

Comment on attachment 8998799 [details] [diff] [review] 110689.diff Review of attachment 8998799 [details] [diff] [review]: ----------------------------------------------------------------- https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=118f7cfa6c669e450f8736555dd80fb8804c8493 ::: mailnews/local/src/nsMailboxService.cpp @@ +354,5 @@ > urlString += "&type="; > urlString += aContentType; > urlString += "&filename="; > urlString += aFileName; > + NS_NewURI(getter_AddRefs(URL), urlString); No error checking here?

Comment on attachment 8998799 [details] [diff] [review] 110689.diff Not compiling on Mac: /builds/worker/workspace/build/src/comm/mailnews/base/util/nsMsgUtils.cpp:119:22: error: unused variable 'kImapUrlCID' [-Werror,-Wunused-const-variable] /builds/worker/workspace/build/src/comm/mailnews/base/util/nsMsgUtils.cpp:120:22: error: unused variable 'kCMailboxUrl' [-Werror,-Wunused-const-variable] /builds/worker/workspace/build/src/comm/mailnews/base/util/nsMsgUtils.cpp:121:22: error: unused variable 'kCNntpUrlCID' [-Werror,-Wunused-const-variable]

I forgot to run tests, so here again: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=44a4f413cfb5e3f7e6e708ffa4dde7ba9f61905f

This is green (only showing the failures we're currently expecting). Neil, please address comment #30 and comment #31.

Looking at this again, what about the comment in the hunk that's going to be removed? // XXX fix this, so that base doesn't depend on imap, local or news. // we can't do NS_NewURI(uri, aUrl), because these are imap-message://, mailbox-message://, news-message:// uris. While we're here, do you know where and when the *-message URLs are produced and used? I've seen them around but I don't know the complete picture.

Rebased for bug 1482720 and removed the unused variables: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=26cb41f62233b4ab7003e33079c41ed2cd899308

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/d58a67d582ad remove CreateStartupUrl() since it's unneeded. r=jorgk

Sorry for not following the bug closely. (In reply to Jorg K (GMT+2) from comment #34) > While we're here, do you know where and when the *-message URLs are produced > and used? I've seen them around but I don't know the complete picture. Way back when in the early days of Gecko, before even Outliner, so about 2000ish, the thread pane was generated using RDF. As such, every message had two URLs, an RDF URI which was the folder URI + '#' + a message key that was used to build the thread pane, and then a Necko-capable -message: URL that could be loaded into an nsIDocShell. (As you know, folders still use RDF, although these days I believe you like to build the folder pane in JS.) Sadly there are still some remnants of the RDF message URIs around, although they're completely unnecessary. In fact I think the -message: suffix is also unnecessary, although again there are still some remnants around.

Comment on attachment 8999503 [details] [diff] [review] 110689.diff (v4) [Approval Request Comment] Testing completed (on c-c, etc.): Baked on c-c for several weeks Risk to taking this patch (and alternatives if risky): Low, basically code removal

Why all those uplift requests? What are we gaining in TB 60? Usually the ESR is about stability and security fixes. We've already added "annoying bugs" to that list, but why uplift internal refactoring?

(In reply to Jorg K from comment #39) > Why all those uplift requests See bug 1493121 comment 6. Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=894dfd0facd5704f29df5413977afe86003a87f8

TB 60.3 ESR: https://hg.mozilla.org/releases/comm-esr60/rev/9976c780c366

@Jörg: Thanks so much for accepting this! This was important for us to be able to save attachments.