Closed Bug 110689 Opened 18 years ago Closed Last year

code cleanup: CreateStartupUrl() see comment in code

Categories

(MailNews Core :: Backend, defect)

x86
Windows 2000
defect
Not set

Tracking

(thunderbird_esr6063+ fixed, thunderbird63 fixed)

RESOLVED FIXED
Thunderbird 63.0
Tracking Status
thunderbird_esr60 63+ fixed
thunderbird63 --- fixed

People

(Reporter: sspitzer, Assigned: neil)

Details

Attachments

(1 file, 3 obsolete files)

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.
Status: NEW → ASSIGNED
Target Milestone: --- → Future
Attached patch patch, v1 (obsolete) — Splinter Review
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...)
Blocks: 66098
Nominating for nsbeta1, this is needed by bug 66098.
Keywords: nsbeta1
Blocks: 117956
Seth, could you review the patch?
reassigning to adu@sparc.spb.su
Assignee: sspitzer → adu
Status: ASSIGNED → NEW
Keywords: nsbeta1nsbeta1-
Well, could anyone review this?
I'm behind in my reviews, give me a few days to get caught up please.
Attached patch a bit more surgery (obsolete) — Splinter Review
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.
Attachment #67247 - Attachment is obsolete: true
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...
Keywords: patch, review
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
No longer blocks: 66098
Keywords: patch, review
well, really reassign...
Assignee: adu → sspitzer
Attachment #70293 - Flags: needs-work+
No longer blocks: 117956
Product: MailNews → Core
sorry for the spam.  making bugzilla reflect reality as I'm not working on these bugs.  filter on FOOBARCHEESE to remove these in bulk.
Assignee: sspitzer → nobody
Filter on "Nobody_NScomTLD_20080620"
QA Contact: esther → backend
Product: Core → MailNews Core
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.
Assignee: nobody → kent
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.
Assignee: kent → nobody
Attached patch 110689.diff (obsolete) — Splinter Review
(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.
Assignee: nobody → neil
Attachment #70293 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8998799 - Flags: review?(acelists)
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.
Attachment #8998799 - Flags: review?(acelists) → review?(jorgk)
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]
Attachment #8998799 - Flags: review?(jorgk) → review-
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.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/d58a67d582ad
remove CreateStartupUrl() since it's unneeded. r=jorgk
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: Future → Thunderbird 63.0
Attachment #8999503 - Flags: review+
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
Attachment #8999503 - Flags: approval-comm-esr60?
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?
Attachment #8999503 - Flags: approval-comm-esr60? → approval-comm-esr60+
@Jörg: Thanks so much for accepting this! This was important for us to be able to save attachments.
You need to log in before you can comment on or make changes to this bug.