Closed Bug 761790 Opened 12 years ago Closed 12 years ago

make import not depend on nsIDOMNode and nsIDOMHTMLImageElement for importing messages with images.

Categories

(MailNews Core :: Import, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 16.0

People

(Reporter: Bienvenu, Assigned: Bienvenu)

References

Details

Attachments

(2 files, 4 obsolete files)

unfortunately, nsMsgAttachmentData doesn't have quite the right fields (in particular, contentId). I'm probably going to have to invent a small interface that just has a url string and a content id. I could combine the strings in an nsISupportsString, but that's kinda hacky.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → DUPLICATE
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Attached patch wip (obsolete) — Splinter Review
this patch builds, though I haven't tried running it yet. I also got rid of all the c++ class definitions in nsIMsgSend.idl, by moving them to a new file, nsMsgAttachmentData.h
Attached patch proposed fix (obsolete) — Splinter Review
This adds a new embeddedImageData interface and object so that potentially other extensions or importers can create messages with embedded images. It moves the c++ classes out of the idl, which I'm hoping will make Neil happy :-)
Attachment #630711 - Attachment is obsolete: true
Attachment #630767 - Flags: superreview?(mbanner)
Attachment #630767 - Flags: review?(neil)
Attached patch forgot to add a file (obsolete) — Splinter Review
Attachment #630767 - Attachment is obsolete: true
Attachment #630767 - Flags: superreview?(mbanner)
Attachment #630767 - Flags: review?(neil)
Attachment #630777 - Flags: superreview?(mbanner)
Attachment #630777 - Flags: review?(neil)
Status: REOPENED → ASSIGNED
Comment on attachment 630777 [details] [diff] [review]
forgot to add a file

>+          nsCOMPtr<nsIMsgEmbeddedImageData> imageData =
>+           do_QueryElementAt(mEmbeddedObjectList, i, &rv);
>+           if (!imageData)
>+             continue;
>+           acceptObject = true;
Nit: strange indentation.

>+      contentId = NS_LossyConvertUTF16toASCII(nodeValue);
Nit: LossyCopyUTF16toASCII(nodeValue, contentId);

>       if (!contentId.IsEmpty())
>-        m_attachments[i].m_contentId = NS_LossyConvertUTF16toASCII(contentId);
>+        m_attachments[i].m_contentId = contentId;
>       else
[Would it make more sense to fetch the contentId directly into m_attachments[i].m_contentId and then use
if (m_attachments[i].m_contentId.IsEmpty()) ?]

>+      nsMsgEmbeddedImageData *imageData =
>+        new nsMsgEmbeddedImageData(embeddedImageURL, NS_LossyConvertUTF16toASCII(cid), EmptyCString());
[Add a 2-arg constructor for nsMsgEmbeddedImageData?]

>       // Append the embedded image node to the list
>-      (*aNodeList)->AppendElement(imageNode);
>+      (*aNodeList)->AppendElement(imageData);
[Would using m_EmbeddedObjectList be clearer?]

>+        nsCString uriSpec;
>+        uri->GetSpec(uriSpec);
>+        nsCOMPtr<nsIMsgEmbeddedImageData> imageData =
>+          new nsMsgEmbeddedImageData(uriSpec, nsDependentCString(cid),
>+                                     nsDependentCString(name));
>+        embeddedObjects->AppendElement(imageData);
ProcessMultipartRelated just turns this spec back into a URI... would it make more sense to store this as a URI in the first place?
Attached patch fix addressing comments (obsolete) — Splinter Review
Attachment #630777 - Attachment is obsolete: true
Attachment #630777 - Flags: superreview?(mbanner)
Attachment #630777 - Flags: review?(neil)
Attachment #632387 - Flags: superreview?(mbanner)
Attachment #632387 - Flags: review?(neil)
Comment on attachment 632387 [details] [diff] [review]
fix addressing comments

This is almost perfect, but it would be helpful for nsMsgEmbeddedImageData to live in either mailnews/base/util or somewhere under mailnews/import so that we can link to it in external linkage builds; any ideas?
Attachment #632387 - Flags: review?(neil) → review+
(In reply to neil@parkwaycc.co.uk from comment #10)
> Comment on attachment 632387 [details] [diff] [review]
> fix addressing comments
> 
> This is almost perfect, but it would be helpful for nsMsgEmbeddedImageData
> to live in either mailnews/base/util or somewhere under mailnews/import so
> that we can link to it in external linkage builds; any ideas?

Ah, I forgot the part of the patch that adds this to nsMailModule.cpp, so code can create this object via xpcom. But it's such a trivial class that that's not important. So mailnews/import/src is probably the better place for it. I'll have a whack at that later today.
Attachment #632387 - Attachment is obsolete: true
Attachment #632387 - Flags: superreview?(mbanner)
Attachment #633701 - Flags: superreview?(mbanner)
Comment on attachment 633701 [details] [diff] [review]
move embeddedimage object into import code

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

::: mailnews/import/eudora/src/nsEudoraEditor.cpp
@@ -286,5 @@
>    return bHasEmbeddedContent;
>  }
>  
> -
> -nsEudoraHTMLImageElement::nsEudoraHTMLImageElement(const nsAString & aSrc,

I think you're missing the equivalent removal from nsEudoraEditor.h
Attachment #633701 - Flags: superreview?(mbanner) → superreview+
fixed on trunk, with comment addressed. http://hg.mozilla.org/comm-central/rev/5edf3019ddac
Status: ASSIGNED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 16.0
Attachment #635973 - Flags: review?(dbienvenu)
Attachment #635973 - Flags: review?(dbienvenu) → review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: