Last Comment Bug 761790 - make import not depend on nsIDOMNode and nsIDOMHTMLImageElement for importing messages with images.
: make import not depend on nsIDOMNode and nsIDOMHTMLImageElement for importing...
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Import (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: Thunderbird 16.0
Assigned To: David :Bienvenu
:
:
Mentors:
: 684466 762501 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-05 14:18 PDT by David :Bienvenu
Modified: 2012-06-22 17:14 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
wip (45.20 KB, patch)
2012-06-06 14:05 PDT, David :Bienvenu
no flags Details | Diff | Splinter Review
proposed fix (74.07 KB, patch)
2012-06-06 16:36 PDT, David :Bienvenu
no flags Details | Diff | Splinter Review
forgot to add a file (79.08 KB, patch)
2012-06-06 17:33 PDT, David :Bienvenu
no flags Details | Diff | Splinter Review
fix addressing comments (74.50 KB, patch)
2012-06-12 13:33 PDT, David :Bienvenu
neil: review+
Details | Diff | Splinter Review
move embeddedimage object into import code (76.22 KB, patch)
2012-06-15 15:28 PDT, David :Bienvenu
standard8: superreview+
Details | Diff | Splinter Review
Fix external linkage builds (921 bytes, patch)
2012-06-22 16:52 PDT, neil@parkwaycc.co.uk
mozilla: review+
Details | Diff | Splinter Review

Description David :Bienvenu 2012-06-05 14:18:20 PDT
see https://bugzilla.mozilla.org/show_bug.cgi?id=761747#c7
Comment 1 David :Bienvenu 2012-06-05 15:55:56 PDT
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.
Comment 2 Joshua Cranmer [:jcranmer] 2012-06-06 04:23:49 PDT

*** This bug has been marked as a duplicate of bug 684466 ***
Comment 3 David :Bienvenu 2012-06-06 14:03:49 PDT
*** Bug 684466 has been marked as a duplicate of this bug. ***
Comment 4 David :Bienvenu 2012-06-06 14:05:35 PDT
Created attachment 630711 [details] [diff] [review]
wip

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
Comment 5 David :Bienvenu 2012-06-06 16:36:40 PDT
Created attachment 630767 [details] [diff] [review]
proposed fix

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 :-)
Comment 6 David :Bienvenu 2012-06-06 17:33:22 PDT
Created attachment 630777 [details] [diff] [review]
forgot to add a file
Comment 7 Joshua Cranmer [:jcranmer] 2012-06-07 08:18:26 PDT
*** Bug 762501 has been marked as a duplicate of this bug. ***
Comment 8 neil@parkwaycc.co.uk 2012-06-10 08:06:28 PDT
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?
Comment 9 David :Bienvenu 2012-06-12 13:33:18 PDT
Created attachment 632387 [details] [diff] [review]
fix addressing comments
Comment 10 neil@parkwaycc.co.uk 2012-06-13 16:14:25 PDT
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?
Comment 11 David :Bienvenu 2012-06-13 16:26:32 PDT
(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.
Comment 12 David :Bienvenu 2012-06-15 15:28:44 PDT
Created attachment 633701 [details] [diff] [review]
move embeddedimage object into import code
Comment 13 Mark Banner (:standard8) 2012-06-18 02:28:35 PDT
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
Comment 14 David :Bienvenu 2012-06-18 17:27:08 PDT
fixed on trunk, with comment addressed. http://hg.mozilla.org/comm-central/rev/5edf3019ddac
Comment 15 neil@parkwaycc.co.uk 2012-06-22 16:52:40 PDT
Created attachment 635973 [details] [diff] [review]
Fix external linkage builds

Note You need to log in before you can comment on or make changes to this bug.