Port |Bug 1432519 - Make nsIURL attributes readonly| to mailnews

RESOLVED FIXED in Thunderbird 60.0

Status

enhancement
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: jorgk, Assigned: jorgk)

Tracking

Thunderbird 60.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Bug 1432519 will make 
nsIURL.directory
nsIURL.fileName
nsIURL.fileBaseName
nsIURL.fileExtension
read-only, that means that the Set methods will go away.

Since nsMsgMailNewsUrl extends nsIURL, nsMsgMailNewsUrl.cpp/h will need a bit of work. For now, we will use the *Internal() methods.

It's a little harder to see whether the setters are used in JS code.
With the patch from bug 1432519 applied, this compiles. Better to work it out now than under bustage-fix pressure.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8947998 - Flags: review?(valentin.gosu)
Comment on attachment 8947998 [details] [diff] [review]
1434687-nsIURL-readonly.patch (v1)

Review of attachment 8947998 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/compose/src/nsMsgSend.cpp
@@ +2058,5 @@
> +              rv = NS_MutateURI(NS_STANDARDURLMUTATOR_CONTRACTID)
> +                     .Apply<nsIURLMutator>(&nsIURLMutator::SetFileName,
> +                                           m_attachments[newLoc]->m_realName,
> +                                           nullptr)
> +                     .Finalize(fileUrl);

For this to work properly, you need to do something else too.
Make nsMsgMailNewsUrl::Mutator implement nsIURLMutator (also make NS_ISUPPORTS_IMPL(Mutator..., nsIURLMutator) at nsMsgMailNewsUrl.cpp#1084 ) and forward Mutator::SetFileName to SetFileNameInternal.
Attachment #8947998 - Flags: review?(valentin.gosu) → review+
Posted file function.cpp
Thanks for the quick review.

> For this to work properly, you need to do something else too.
Really? That snippet in nsMsgSend.cpp isn't related to nsMsgMailNewsUrl at all. As far as I can tell, all it does is to initialise a standard URL with a file name to get an extension. What am I missing? I've included the entire function with the patch applied for your reference.
Ok, right, that is an nsStandardURL. Serves me right to review patches after FOSDEM beer :)
This is as a followup, towards removing the internal method from the IDL. Once you do that, instead of calling msgUrl->SetFileNameInternal you can use the mutator.
In bug 1432204 I plan to straighten out the nsMsgMailNewsUrl processing. It's on the to-do list, but due to a lot of bustage in the past week, I haven't gotten to it yet. Here I'm just making sure we're OK once you land bug 1432519.

nsMsgMailNewsUrl::SetFileNameInternal() and nsMsgMailNewsUrl::GetFileName() are somewhat weird, since the don't pass the call to the underlying nsIURL in all cases:

NS_IMETHODIMP nsMsgMailNewsUrl::GetFileName(nsACString &aFileName)
{
  if (!mAttachmentFileName.IsEmpty())
  {
    aFileName = mAttachmentFileName;
    return NS_OK;
  }
  return m_baseURL->GetFileName(aFileName);
}

nsresult nsMsgMailNewsUrl::SetFileNameInternal(const nsACString &aFileName)
{
  mAttachmentFileName = aFileName;
  return NS_OK;
}
Just some white-space changes in a comment.
Attachment #8948486 - Flags: review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/fc0902ee6890
Port bug 1432519 to mailnews: Make nsIURL attributes readonly. r=valentin
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 60.0
Attachment #8948486 - Attachment is obsolete: true
Depends on: 1446679
You need to log in before you can comment on or make changes to this bug.