Closed Bug 244685 Opened 16 years ago Closed 15 years ago

Implement SourceURL for CF_HTML flavor for drag/drop clipboard object

Categories

(Core :: XUL, enhancement)

x86
Windows Server 2003
enhancement
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: David.R.Gardiner, Assigned: David.R.Gardiner)

References

Details

Attachments

(2 files, 9 obsolete files)

When you select a block of text in IE, and then drag it to Microsoft OneNote,
the text gets copied, and it also adds a reference to the URL of the page where
the text came from.

If you do the same with Firefox, then the text gets copied, but it doesn't
include the source URL, as I assume Firefox isn't including this in the
drag/drop information that OneNote sees.

-dave
Screengrab of OneNote 2003
I wrote a simple Windows app to dump what formats are provided when something is
dragged over it..

Here's the output for dragging selected text from IE:

====================================
Format: System.String
 
 
Learning that's out of this world
UniSA will help to host one of the most exciting space education events in the
world when the International Space University runs its 2004 summer session from
June 27 to August 27. more
====================================
Format: UnicodeText
 
 
Learning that's out of this world
UniSA will help to host one of the most exciting space education events in the
world when the International Space University runs its 2004 summer session from
June 27 to August 27. more
====================================
Format: Text
 
 
Learning that's out of this world
UniSA will help to host one of the most exciting space education events in the
world when the International Space University runs its 2004 summer session from
June 27 to August 27. more
====================================
Format: HTML Format
 
 
Version:1.0
StartHTML:000000187
EndHTML:000001648
StartFragment:000001312
EndFragment:000001588
StartSelection:000001312
EndSelection:000001584
SourceURL:http://www.unisa.edu.au/
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"
"http://www.w3c.org/TR/1999/REC-html401-19991224/loose.dtd">

<HTML><HEAD><TITLE>University of South Australia</TITLE><LINK media=screen 
href="commonfiles/stylesheets/other/NN4_styles.css" type=text/css 
rel=stylesheet>
<STYLE type=text/css>

@import url("commonfiles/stylesheets/layout.css");
@import url("commonfiles/stylesheets/layout_home.css");
@import url("commonfiles/stylesheets/text_style.css");
@import url("commonfiles/stylesheets/text_home.css");

</STYLE>
<!--<link rel="stylesheet" type="text/css"
href="commonfiles/stylesheets/print.css" media="print">-->
<SCRIPT type=text/javascript>
	function upgrade(){
	
window.open("detect.asp",null,"height=400,width=600,resizable=yes,status=yes,toolbar=yes,menubar=yes,location=no");
	}
	</SCRIPT>

<SCRIPT language=JavaScript src="ultimate_detect.js" type=text/javascript>
	</SCRIPT>

<STYLE type=text/css>
<!--
div.Section1
	{page:Section1;}
-->
</STYLE>
</HEAD>

<BODY>

<DIV id=content-holder>

<DIV id=content-home>

<DIV id=content-home-text><!--StartFragment--><H1>Learning that's out of this
world</H1>
<P>UniSA will help to host one of the most exciting space education events in 
the world when the International Space University runs its 2004 summer session 
from June 27 to August 27. <A
href="promo/2004/rocket.asp">more</A></P><!--EndFragment--></DIV>
</DIV>
</DIV>
</BODY>
</HTML>
====================================
Format: Rich Text Format
 
 
{\rtf1\ansi\ansicpg-1\deff0\deflang3081{\fonttbl{\f0\froman\fcharset0 Times New
Roman;}{\f1\ftech\fcharset0 Symbol;}{\f2\fswiss\fcharset0
Arial;}{\f3\fswiss\fcharset0 Courier New;}{\f4\ftech\fcharset0
Wingdings;}}{\colortbl\red0\green0\blue0;\red0\green0\blue255;\red0\green255\blue255;\red0\green255\blue0;\red255\green0\blue255;\red255\green0\blue0;\red255\green255\blue0;\red255\green255\blue255;\red0\green0\blue128;\red0\green128\blue128;\red0\green128\blue0;\red128\green0\blue128;\red128\green0\blue0;\red128\green128\blue0;\red128\green128\blue128;\red192\green192\blue192;}{\stylesheet{\s1\sa100\sb100
Normal;}{\s2\sbasedon1\snext3 Definition Term;}{\s3\sbasedon1\li360\snext2
Definition List;}{\*\cs4\i1\additive
Definition;}{\s5\sbasedon1\sa100\sb100\b1\fs48\kerning36\keepn\snext1\outlinelevel1
H1;}{\s6\sbasedon1\sa100\sb100\fs36\b1\keepn\snext1\outlinelevel2
H2;}{\s7\sbasedon1\sa100\sb100\fs28\b1\keepn\snext1\outlinelevel3
H3;}{\s8\sbasedon1\sa100\sb100\b1\fs24\keepn\snext1\outlinelevel4
H4;}{\s9\sbasedon1\sa100\sb100\fs20\b1\keepn\snext1\outlinelevel5
H5;}{\s10\sbasedon1\sa100\sb100\fs16\b1\keepn\snext1\outlinelevel6
H6;}{\s11\sbasedon1\i1\snext1 Address;}{\s12\sbasedon1\sa100\sb100\li360\ri360
Blockquote;}{\*\cs13\i1\additive CITE;}{\*\cs14\f3\fs20\additive
CODE;}{\*\cs15\i1\ulnone\additive Emphasis;}{\*\cs16\cf1\ul\additive
Hyperlink;}{\*\cs17\cf11\ul\additive
FollowedHyperlink;}{\*\cs18\f3\b1\fs20\additive
Keyboard;}{\s19\sbasedon1\f3\sa0\sb0\fs20\tx0\tx959\tx1918\tx2877\tx3836\tx4795\tx5754\tx6713\tx7672\tx8631\tx9590
Preformatted;}{\s20\f2\snext1\shidden\qc\fs16\v1\brdrt\brdrdb\brdrw6\brdrcf0
z-Bottom of Form;}{\s21\f2\snext1\shidden\qc\v1\fs16\brdrb\brdrdb\brdrw6\brdrcf0
z-Top of Form;}{\*\cs22\f3\additive Sample;}{\*\cs23\b1\additive
Strong;}{\*\cs24\f3\fs20\additive Typewriter;}{\*\cs25\i1\additive
Variable;}{\*\cs26\v1\cf5\additive HTML Markup;}{\*\cs27\v1\additive
Comment;}}{\*\listtable{\list{\listname
;}\listid1{\listlevel\levelstartat1\leveljc0\levelnfc0\li720\fi-360{\leveltext\'02\'00.;}{\levelnumbers\'01;}}{\listlevel\levelstartat1\leveljc0\levelnfc0\li1440\fi-360{\leveltext\'02\'01.;}{\levelnumbers\'01;}}{\listlevel\levelstartat1\leveljc0\levelnfc0\li2160\fi-360{\leveltext\'02\'02.;}{\levelnumbers\'01;}}{\listlevel\levelstartat1\leveljc0\levelnfc0\li2880\fi-360{\leveltext\'02\'03.;}{\levelnumbers\'01;}}{\listlevel\levelstartat1\leveljc0\levelnfc0\li3600\fi-360{\leveltext\'02\'04.;}{\levelnumbers\'01;}}{\listlevel\levelstartat1\leveljc0\levelnfc0\li4320\fi-360{\leveltext\'02\'05.;}{\levelnumbers\'01;}}{\listlevel\levelstartat1\leveljc0\levelnfc0\li5040\fi-360{\leveltext\'02\'06.;}{\levelnumbers\'01;}}{\listlevel\levelstartat1\leveljc0\levelnfc0\li5760\fi-360{\leveltext\'02\'07.;}{\levelnumbers\'01;}}}}{\*\listoverridetable}{\info}{\*\userprops{{\propname
HTML}\proptype11{\staticval 1}}{{\propname
DocumentEncoding}\proptype30{\staticval
utf-8}}}\margl1273\margr1273\margt1417\margb1134\sl-240\slmult0\deftab720\headery1440\footery1440\fet2\pgwsxn11906\pghsxn16838\viewkind5\ansicpg-1\pgwsxn11906\pghsxn16838\pard\s1\sa100\sb100\marglsxn1273\margrsxn1273\s5\sa100\sb100\keepn\outlinelevel1\plain\fs24\b1\fs48\kerning36
Learning that's out of this world\par\pard\s1\sa100\sb100\li0\plain\fs24 UniSA
will help to host one of the most exciting space education events in the world
when the International Space University runs its 2004 summer session from June
27 to August 27. {\field{\*\fldinst HYPERLINK
"promo/2004/rocket.asp"}{\fldrslt\pard\s1\sa100\sb100\li0\plain\fs24\cs16\cf1\ul
more}}\plain\fs24\plain\fs24\par\pard}


Here is what Firefox provides:

====================================
Format: text/_moz_htmlcontext
 
 
System.IO.MemoryStream====================================
Format: text/_moz_htmlinfo
 
 
System.IO.MemoryStream====================================
Format: text/html
 
 
System.IO.MemoryStream====================================
Format: HTML Format
 
 
Version:0.9
StartHTML:00000097
EndHTML:00000472
StartFragment:00000111
EndFragment:00000436
<html><body>
<!--StartFragment -->
<h1>Learning that's out of this world</h1>
<p>UniSA will help to host one of the most exciting space education events in 
the world when the International Space University runs its 2004 summer session 
from June 27 to August 27. <a
href="http://www.unisa.edu.au/promo/2004/rocket.asp">more</a></p>
<!--EndFragment-->
</body>
</html>====================================
Format: System.String
 
 
Learning that's out of this world

UniSA will help to host one of the most exciting space education events in the
world when the International Space University runs its 2004 summer session from
June 27 to August 27. more====================================
Format: UnicodeText
 
 
Learning that's out of this world

UniSA will help to host one of the most exciting space education events in the
world when the International Space University runs its 2004 summer session from
June 27 to August 27. more====================================
Format: Text
 
 
Learning that's out of this world

UniSA will help to host one of the most exciting space education events in the
world when the International Space University runs its 2004 summer session from
June 27 to August 27. more
==========================


The difference that looks significant to me is the "HTML Format" formats. Apart
from the version difference (0.9 vs. 1.0), IE also includes a "SourceURL" line.
I'm guessing this is what OneNote must use.

-dave



Microsoft's documentation on the CF_HTML clipboard format:

http://msdn.microsoft.com/workshop/networking/clipboard/htmlclipboard.asp

This includes a comment regarding how IE includes a SourceURL property.

-dave
Update title to reflect what the patched code will do (patch coming shortly)

-dave
Component: OS Integration → XP Toolkit/Widgets
Product: Firefox → Browser
Summary: Dragging text from Firefox and dropping into OneNote should include the URL of the document → Implement SourceURL for CF_HTML flavor for drag/drop clipboard object
Version: unspecified → Trunk
Assignee: bugs → jag
QA Contact: jrgmorrison
Here is a patch to implement this feature.

It is currently compiled against source pulled from AVIARY_1_0_20040515_BRANCH,
but i'm not sure that too much has changed here on the trunk since then.

I haven't done a lot of patching of Mozilla, so i'm sure there are some things
that i've forgotten to do in my patch. Constructive critisim accepted gladly.

-dave
Revised patch:
. Fixed tabs/whitespace
. Use nsCAutoString
Attachment #149459 - Attachment is obsolete: true
* Add contributer comments
* Fix tab that I missed earlier

I'd love someone to review this patch!

-dave
Attachment #149813 - Attachment is obsolete: true
*** Bug 235986 has been marked as a duplicate of this bug. ***
Comment on attachment 149814 [details] [diff] [review]
Patch to implement SourceURL (against trunk) 

>RCS file: /cvsroot/mozilla/widget/src/windows/nsClipboard.cpp,v
>@@ -130,7 +131,16 @@
>   nsDataObj * dataObj;
>-  dataObj = new nsDataObj();
>+  if (uri) {
...
>+  	dataObj = new nsDataObj(spec);
>+  } else {
>+    dataObj = new nsDataObj();
>+  }

I know you're just copying code, but please null check dataObj.

>   dataObj->AddRef();

>@@ -259,7 +269,7 @@
>   ::OleSetClipboard(NULL);
> 
>   IDataObject * dataObj;
>-  if ( NS_SUCCEEDED(CreateNativeDataObject(mTransferable, &dataObj)) ) { // this add refs
>+  if ( NS_SUCCEEDED(CreateNativeDataObject(mTransferable, &dataObj, NULL)) ) { // this add refs

With so many parameters, it'd be nice if the comment indicated which was being
addref'd.

>RCS file: /cvsroot/mozilla/widget/src/windows/nsDataObj.cpp,v
>@@ -89,16 +90,27 @@

>-nsDataObj::nsDataObj()
>+void nsDataObj::Init()

This is a silly change

> {
>-	m_cRef	        = 0;
>+  m_cRef          = 0;
>   mTransferable   = nsnull;

these two really should be initialized in the initializer section of the
constructor.

>   mDataFlavors    = new nsVoidArray();
> 
>   m_enumFE = new CEnumFormatEtc(32);

However, if you're going to do it, make Init return an nsresult and handle
failure to alloc mDataFlavors and m_enumFE.

>-  m_enumFE->AddRef();  
>+  m_enumFE->AddRef();
>+}
Attachment #149814 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #149814 - Flags: review?(dean_tessman)
Assignee: jag → david.gardiner
This patch is against the current trunk.

I've tried to make the changes as suggested. Let me know if i've misunderstood
something or missed something.

thanks,
-dave
Attachment #149814 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Just to note that this patch works for drag and drop, but does not work for
copy/paste.

The copy and paste bit looks tricky, as from what i can figure out by tracing
the code, by the time it gets to the Win32-specific code, the copy/paste code
doesn't know the IDocument where it came from, so I can't easily determine the
URL of the document to add to the clipboard data.

I haven't given up, but i'd welcome any clues/suggestions.

-dave
I've figured out a way to pass the URL when doing a copy/paste.

Basically, I add the kURLMime "Flavour" with the document's URI (and also the
title, as that is what other code assumes for this flavour).

I then updated nsDataObj::BuildPlatformHTML so that it looks for this flavour
in addition to using the mSourceURL that is set via drag/drop.

so, i think this does everything now - drag/drop and also copy/paste!

I suspect that my new code for the copy/paste might need some reviewing, as I'm
sure i haven't done it the best way.

-dave
Attachment #155396 - Attachment is obsolete: true
Just tested that last patch and it is retrieving the wrong data from the
clipboard when you paste into notepad.

Might be something to do with the order in which flavours are tried.. Anyway,
i'll see what I can do to fix that.

-dave
By making sure that a text/unicode flavour is provided, the code now works
properly when pasting content into Notepad (which isn't expecting anything
fancy), as well as OneNote (which can use the text/html and the SourceURL
property)

So, we are "good to go".

Comments and feedback on the quality of the patch is welcome.

-dave
Attachment #157250 - Attachment is obsolete: true
Attachment #158522 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #158522 - Flags: review?(dean_tessman)
Attachment #149814 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #149814 - Flags: review?(dean_tessman)
Comment on attachment 158522 [details] [diff] [review]
Patch to implement SourceURL (against trunk)

>Index: widget/src/windows/nsClipboard.cpp

>+#include "nsIDocument.h"

I could be missing something, but why do you need this file?

>+    uri->GetSpec(spec);
>+  	dataObj = new nsDataObj(spec);

Replace the tab on the second line with spaces so it indents properly.

>Index: widget/src/windows/nsClipboard.h
>+                                         IDataObject ** aDataObj,
>+										 nsIURI			* uri);

Again, spaces instead of tabs.

>Index: widget/src/windows/nsDataObj.cpp
>===================================================================
> nsDataObj::nsDataObj()
>+: m_cRef(0), mTransferable(nsnull)

Indent two spaces.

>+nsDataObj::nsDataObj(const nsACString& SourceURL)
>+: m_cRef(0), mTransferable(nsnull)

Again here.

Can we avoid the duplication of the common code in the two constructors?

>+      PRNTDEBUG2("  mime type: %s\n", df->get());

Remove this.

>+      "SourceURL:");
>+
>+  // insert source url
>+  strcat(buf, PromiseFlatCString(mSourceURL).get());
>+
>+  strcat(buf, 
>+      "\r\n"

Once this goes in, make sure your other clipboard patch adds SourceURL as well.

>Index: widget/src/windows/nsDataObj.h
>===================================================================
>RCS file: /cvsroot/mozilla/widget/src/windows/nsDataObj.h,v
>retrieving revision 1.23
>diff -u -r1.23 nsDataObj.h
>--- widget/src/windows/nsDataObj.h	16 Jul 2004 12:17:18 -0000	1.23
>+++ widget/src/windows/nsDataObj.h	11 Sep 2004 09:19:16 -0000
>@@ -74,8 +74,9 @@
> class nsDataObj : public IDataObject
> {
> 	public: // construction, destruction
>-		nsDataObj();
>-		~nsDataObj();
>+    nsDataObj();
>+    nsDataObj(const nsACString& SourceURL);
>+    ~nsDataObj();

Ugh, this file is ugly.  It's using tabs for the first half and spaces for the
second half.  Can you file a new bug to clean it up, please?

>Index: content/base/src/nsCopySupport.cpp

>   PRUint32 flags = 0;
>+  PRUint32 flagsText = 0;
>   nsAutoString mimeType;
>+  nsAutoString mimeTypeText;

...

>+  mimeTypeText.AssignLiteral(kUnicodeMime);
>+  flagsText |= nsIDocumentEncoder::OutputBodyOnly | nsIDocumentEncoder::OutputPreformatted;
>+
>+  if (bIsHTMLCopy) {
>     mimeType.AssignLiteral(kHTMLMime);
>+  }
>   else
>   {
>     flags |= nsIDocumentEncoder::OutputBodyOnly | nsIDocumentEncoder::OutputPreformatted;
>     mimeType.AssignLiteral(kUnicodeMime);
>   }

Can't this be done with one nsIDocumentEncoder, one flags variable, and one
mimeType variable?  Wouldn't we want the same flags for both selections that
are being encoded?

>+  if (NS_FAILED(rv)) return rv;
>+  rv = docEncoderText->SetSelection(aSel);
>+  if (NS_FAILED(rv)) return rv;

I know you're just copying existing code, but please put the "return"s on a
separate line.

>   if (bIsHTMLCopy)
>   {
>+
>     // encode the selection as html with contextual info
>     rv = docEncoder->EncodeToStringWithContext(buffer, parents, info);
>     if (NS_FAILED(rv)) 
>       return rv;
>+
>   }

No need to add those blank lines.

If I read the changes to nsCopySupport.cpp right, most of it is just moving
chunks of code around.

r=me with the changes mentioned and questions answered.
Attachment #158522 - Flags: review?(dean_tessman) → review+
(In reply to comment #15)
>(From update of attachment 158522 [details] [diff] [review])
>Can we avoid the duplication of the common code in the two constructors?
Sure, something along these lines:

class nsDataObj: public IDataObject {
public:
nsDataObj(const nsACString& aSourceURL = EmptyCString());

N.B. nsACString is actually probably not the correct string class to use here.
Comment on attachment 158522 [details] [diff] [review]
Patch to implement SourceURL (against trunk)

+  NS_ASSERTION(dataObj, "Could not create nsDataObj");

you can't assert that out-of-memory does not happen, you must handle it. it is
not a programming error if there is no memory. an assertion is the wrong thing.

same in other places

+      mSourceURL.Assign(NS_ConvertUTF16toUTF8(url));

CopyUTF16toUTF8(url, mSourceURL);

+  // insert source url
+  strcat(buf, PromiseFlatCString(mSourceURL).get());

this looks like a buffer overflow waiting to happen... if you don't want to
create the string in another way, maybe you should add mSourceURL.Length() to
the allocation size?

+	 nsIURI *uri = nsnull;
+	 uri = aDoc->GetDocumentURI();

hmm. not my module, and I'm doing a drive-by review here, but I would merge
these lines... (I'd also move the nsresult data_rv etc decls to their first
usage)

+	 shortcut = NS_ConvertUTF8toUTF16(spec);

again, CopyUTF8...


+	 const nsAFlatString title = aDoc->GetDocumentTitle();

this looks wrong... are you missing an & here?
Comment on attachment 158522 [details] [diff] [review]
Patch to implement SourceURL (against trunk)

>   nsDataObj * dataObj;
>-  dataObj = new nsDataObj();
>+  if (uri) {
>+
>+    // A URI was obtained, so pass this through to the DataObject
>+    // so it can create a SourceURL for CF_HTML flavour
>+    nsCAutoString spec;
>+    uri->GetSpec(spec);
>+  	dataObj = new nsDataObj(spec);
>+  } else {
>+    dataObj = new nsDataObj();
>+  }
But after reading this, what you want is nsDataObj* dataObj = new
nsDataObj(uri); and in the constructor use if (uri) uri->GetSpec(mSourceURL);

>+  nsresult rv = NS_ERROR_FAILURE;
Unused?

>+    if (!url.IsEmpty()) {
>+      mSourceURL.Assign(NS_ConvertUTF16toUTF8(url));
>+    }
Not sure why you need the extra test, but I think you should be using
AppendUTF16toUTF8 here.

>   // Create temporary buffer for HTML header... 
>   char *buf = NS_REINTERPRET_CAST(char*, nsMemory::Alloc(400 + strlen(inOurHTML)));
>   if( !buf )
>@@ -1190,6 +1217,13 @@
>       "EndHTML:00000000\r\n"
>       "StartFragment:00000000\r\n"
>       "EndFragment:00000000\r\n"
>+      "SourceURL:");
>+
>+  // insert source url
>+  strcat(buf, PromiseFlatCString(mSourceURL).get());
I don't think you've allowed extra buffer space for this URL. Mind you, this
code is pretty cheezy.

>+        nsIURI *uri = nsnull;
>+        uri = aDoc->GetDocumentURI();
Lose the ^M (and the other three); also combine these into one line.

>+        shortcut = NS_ConvertUTF8toUTF16(spec);
AppendUTF8toUTF16(spec, shortcut);

>+        shortcut.Append(NS_LITERAL_STRING("\n"));
Use Append(PRUnichar('\n')) for a single character.

>+        // and get document title
>+        const nsAFlatString title = aDoc->GetDocumentTitle();
>+
>+        if (!title.IsEmpty()) {
>+          shortcut.Append(title);
>+        }
Why not just Append(aDoc->GetDocumentTitle())?
Attachment #158522 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview-
(In reply to comment #18)
> >+  strcat(buf, PromiseFlatCString(mSourceURL).get());
> I don't think you've allowed extra buffer space for this URL. Mind you, this
> code is pretty cheezy.
...
> >+        shortcut = NS_ConvertUTF8toUTF16(spec);
> AppendUTF8toUTF16(spec, shortcut);

Sorry, I apparently glazed over too much of the string work.  Regarding the
cheeziness, this code is being re-written over in bug 157994 to get rid of all
the strcat()s.
Thanks for everyones feedback.

I believe i've addressed all the concerns with this patch (please let me know
if  I've missed something though!)

It works properly now with dragging and dropping text to OneNote, and also
copy/paste to OneNote and to Notepad.

-dave
Attachment #158522 - Attachment is obsolete: true
Hope you can fix this.
Attachment #159711 - Flags: superreview?(neil.parkwaycc.co.uk)
Is there a chance that this bug will be fixed in a Firefox update soon? As a
User of Clipmate by Thornsoft Development I hope it can happen ASAP
This patches nsDataObj.cpp which was recently modified by bug 157994.

I need to revise the patch so that it patches the current trunk. Once I've
generated the new patch, then it should be ready for review and hopefully
checked in.

-dave
Depends on: 157994
Refreshed against latest code in trunk.. Please review this patch.

-david
Attachment #159711 - Attachment is obsolete: true
Comment on attachment 159711 [details] [diff] [review]
Patch to implement SourceURL (against trunk)

>+  if (uri) {
>+
>+    // A URI was obtained, so pass this through to the DataObject
>+    // so it can create a SourceURL for CF_HTML flavour
>+    nsCAutoString spec;
>+    uri->GetSpec(spec);
>+    dataObj = new nsDataObj(spec);
>+  } else {
>+    dataObj = new nsDataObj();
>+  }
Did you not like my idea of dataObj = new nsDataObj(uri); (more...)

>-nsDataObj::nsDataObj()
>+nsDataObj::nsDataObj(const nsACString& SourceURL)
(...continued) nsDataObj::nsDataObj(nsIURI *uri) (more...)

>+  if (!SourceURL.IsEmpty()) {
>+    mSourceURL.Assign(SourceURL);
>+  }
(...continued) if (uri) uri->GetSpec(mSourceURL); (more...)
[Or at least remove the IsEmpty check, mSourceURL is already empty]

>+    if (!url.IsEmpty()) {
>+      mSourceURL.Assign(NS_ConvertUTF16toUTF8(url));
>+    }
You changed one of these but missed this one.
[And I don't get the point of the IsEmpty check]

>+    nsDataObj(const nsACString& SourceURL = EmptyCString());
(...continued) nsDataObj(nsIURI *uri = nsnull);
I certainly don't like the call to EmptyCString() here. Didn't you have two
constructors before, or something?

>+  nsCOMPtr<nsIDocument> doc(do_QueryInterface(mSourceDocument, &rv));
>+  if (NS_SUCCEEDED(rv)) {
>+    uri = doc->GetDocumentURI();
>+  }
Possible alternative is
nsCOMPtr<nsIDocument> doc(do_QueryInterface(mSourceDocument));
if (doc) {
  uri = doc->GetDocumentURI();
}
Attachment #159711 - Flags: superreview?(neil.parkwaycc.co.uk)
(In reply to comment #25)

Thanks Neil for your feedback..

> (From update of attachment 159711 [details] [diff] [review])
> >+  if (uri) {
> >+
> >+    // A URI was obtained, so pass this through to the DataObject
> >+    // so it can create a SourceURL for CF_HTML flavour
> >+    nsCAutoString spec;
> >+    uri->GetSpec(spec);
> >+    dataObj = new nsDataObj(spec);
> >+  } else {
> >+    dataObj = new nsDataObj();
> >+  }
> Did you not like my idea of dataObj = new nsDataObj(uri); (more...)

Sorry, I think i must have got a bit confused between your two previous comments
- #16 and #18. I think I implemented the first suggestion instead of the second
one :-(

> >+    nsDataObj(const nsACString& SourceURL = EmptyCString());
> (...continued) nsDataObj(nsIURI *uri = nsnull);
> I certainly don't like the call to EmptyCString() here. Didn't you have two
> constructors before, or something?

I did, but I thought you suggested in #16 that I fold them into one :-(

I'll update my patch with your suggestions.

-dave
Attachment #167632 - Attachment is obsolete: true
Comment on attachment 168005 [details] [diff] [review]
Patch to implement SourceURL (against trunk) 

I hope I've addressed all your suggestions. Please let me know if I've missed
something.

thanks,
-david
Attachment #168005 - Flags: superreview?(neil.parkwaycc.co.uk)
Comment on attachment 168005 [details] [diff] [review]
Patch to implement SourceURL (against trunk) 

>+  //nsCOMPtr<nsIDocument> doc(do_QueryInterface(mSourceDocument, &rv));
>+  //if (NS_SUCCEEDED(rv)) {
>+  //  uri = doc->GetDocumentURI();
>+  //}
>+
This needs to be removed before checkin.
Attachment #168005 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
Final patch, ready for someone to commit to trunk!

Should this go on any branches too?
-dave
Attachment #168005 - Attachment is obsolete: true
> Should this go on any branches too?

No. All significant new releases will come from the trunk.
Note: this might've caused bug 273657
In fact, this broke plaintext copy altogether -- the plaintext is now parsed as
HTML before being copied.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
*** Bug 284311 has been marked as a duplicate of this bug. ***
Duplicate of this bug: 279037
Depends on: 453151
No longer depends on: 453151
Duplicate of this bug: 281873
You need to log in before you can comment on or make changes to this bug.