Closed Bug 244685 Opened 21 years ago Closed 20 years ago

Implement SourceURL for CF_HTML flavor for drag/drop clipboard object

Categories

(Core :: XUL, enhancement)

Product:
Core
Component:
XUL
Platform:
x86
Windows Server 2003
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

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

References

Details

Attachments

(2 files, 9 obsolete files)

Screengrab of OneNote showing identical text dragged from IE and FireFox
17.67 KB, image/png
Details
Patch to implement SourceURL (against trunk)
17.72 KB, patch
Details | Diff | Splinter Review
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.
In fact, it did cause it.
Depends on: 273741
In fact, this broke plaintext copy altogether -- the plaintext is now parsed as HTML before being copied.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
*** Bug 284311 has been marked as a duplicate of this bug. ***
Depends on: 453151
No longer depends on: 453151
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: