Closed
Bug 309525
Opened 19 years ago
Closed 19 years ago
<object> loading doesn't do stream conversion
Categories
(Core :: DOM: Core & HTML, defect, P2)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha1
People
(Reporter: Biesinger, Assigned: Biesinger)
References
()
Details
Attachments
(5 files, 4 obsolete files)
1.05 KB,
message/rfc822
|
Details | |
4.54 KB,
patch
|
darin.moz
:
review+
|
Details | Diff | Splinter Review |
23.16 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
17.20 KB,
patch
|
darin.moz
:
superreview+
|
Details | Diff | Splinter Review |
5.14 KB,
patch
|
Details | Diff | Splinter Review |
Loading stuff via <object> doesn't do any stream conversions, because it directly calls nsIURIContentListener::DoContent. This includes making nsObjectLoadingContent recognize types only supported by stream converters as valid document types.
Assignee | ||
Comment 1•19 years ago
|
||
Assignee | ||
Updated•19 years ago
|
Assignee | ||
Updated•19 years ago
|
Priority: -- → P2
Target Milestone: --- → mozilla1.9alpha
Assignee | ||
Comment 2•19 years ago
|
||
Attachment #202911 -
Flags: superreview?(bzbarsky)
Attachment #202911 -
Flags: review?(darin)
Assignee | ||
Comment 3•19 years ago
|
||
Includes a necessary patch for rdf/chrome, so that IsPending works correctly (this is already fixed in chrome/).
Attachment #202912 -
Flags: superreview?(darin)
Attachment #202912 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 4•19 years ago
|
||
I wonder if I should expose this on nsIWebNavigation, and call that...
Attachment #202913 -
Flags: superreview?(bzbarsky)
Attachment #202913 -
Flags: review?(bzbarsky)
Comment 5•19 years ago
|
||
Comment on attachment 202911 [details] [diff] [review] netwerk/ part >Index: netwerk/streamconv/src/nsStreamConverterService.cpp >+nsStreamConverterService::CanConvert(const char* >+ *_retval = NS_SUCCEEDED(rv) != PR_FALSE; Isn't that the same as: *_retval = NS_SUCCEEDED(rv); ? sr=bzbarsky
Attachment #202911 -
Flags: superreview?(bzbarsky) → superreview+
Comment 6•19 years ago
|
||
Comment on attachment 202912 [details] [diff] [review] uriloader/ part >Index: uriloader/base/nsIURILoader.idl >+ /* @{ */ I assume that indicates to something that these two constants belong together? >+ /** >+ * Loads data from a channel. This differs from openURI in that the channel >+ * may already be opened, and that it returns a stream listener into which the >+ * caller should pump data. I'm not sure I follow this. What does it mean for this to return a stream listener? What will this do if the channel is _not_ opened? Still return a stream listener? What data is the caller expected to pump into the listener this returns? >+ * Note: If the channel already has a loadgroup, it will be replaced with a >+ * different one. Which one? Or can we not document that in a reasonable way? >+ * If the current listener refuses the load immediately Which one is the "current listener"? What does it mean to refuse the load immediately? >, this method will >+ * return NS_ERROR_WONT_HANDLE_CONTENT. If desired, the caller should cancel >+ * the channel (this method won't). s/If desired etc/At that point, the caller should probably cancel the channel if it's already open (this method will not cancel the channel)./ Or something like that, perhaps? >+ * If flags include DONT_RETARGET, but the current listener refuses the load, Again, what is "current listener", and how does "refuses the load" differ from "refuses the load immediately"? >+ * @param aChannel >+ * The channel that should be loaded. The channel may already be >+ * opened. May it also already be closed? eg. a channel that's done loading? >+ * @param aIsContentPreferred aFlags, you mean? >+ * Should the content be displayed in a container that prefers the >+ * content-type, or will any container do. And fix this comment, please? >Index: uriloader/base/nsURILoader.cpp I think the diff to this file would be a lot more readable as a diff -b (or diff -w). Could you post a diff like that, please?
Comment 7•19 years ago
|
||
Comment on attachment 202913 [details] [diff] [review] content/base/src part I need to think about this a bit more, but comments off top of my head: 1) We don't really want to change nsIWebNavigation -- we want to freeze it more or less as-is. 2) >@@ -1007,15 +1003,30 @@ nsObjectLoadingContent::IsSupportedDocum >+ if (supported == nsIWebNavigationInfo::UNSUPPORTED) ... >+ rv = convServ->CanConvert(aMimeType.get(), "*/*", &canConvert); >+ return NS_SUCCEEDED(rv) && canConvert; I'm not sure I buy that... Just because we can convert the type to something doesn't mean the something will be a supported document, does it? Consider, eg, the case when the type is application/x-unknown-content-type and the unknown decoder detects it as PDF. If there's something I'm missing, it should probably be documented here with this code... ;)
Assignee | ||
Comment 8•19 years ago
|
||
(In reply to comment #5) > Isn't that the same as: > > *_retval = NS_SUCCEEDED(rv); Hm, yes, looks like it is for NS_SUCCEEDED (not for NS_FAILED though) (In reply to comment #6) > I assume that indicates to something that these two constants belong together? Yeah, doxygen will group them together with a "Flags for opening URIs." header. > What data is the caller expected to pump into the listener > this returns? Well, the point is that this stream listener will receive the data from the channel. I wanted to allow both doing something like: var chan = newChannel(...); var listener = uriLoader.openChannel(chan); return chan.asyncOpen(listener, nsnull); (which openURI now does) And the other approach, calling it inside a listener's onStartRequest method. I should probably clarify the comment. > Which one? Or can we not document that in a reasonable way? does anyone care which one it is? :-) > Which one is the "current listener"? The one from aWindowContext... I should document this better > What does it mean to refuse the load immediately? You mean I should document that this calls onStartURIOpen? >Again, what is "current listener", and how does "refuses the load" differ from >"refuses the load immediately"? It differs in that the caller only refuses it in onStartRequest :-) (canHandleContent etc)... hm.. probably needs rewording too. > May it also already be closed? eg. a channel that's done loading? ohh, that's an interesting question. I'm pretty sure the impl would screw this up, with that isPending check. so, no :-) (In reply to comment #7) > 1) We don't really want to change nsIWebNavigation -- we want to freeze it > more or less as-is. Yes, I meant as an nsIWebNavigation2 method or so. > I'm not sure I buy that... Just because we can convert the type to something > doesn't mean the something will be a supported document, does it? Consider, > eg, the case when the type is application/x-unknown-content-type and the > unknown decoder detects it as PDF. Yeah, hm, but we can only detect that in onStartRequest. At that point, uriloader will call canHandleContent, which fails, uriloader returns NS_ERROR_WONT_HANDLE_CONTENT, which means onStartRequest fails, which means we fall back. So I think we're ok here. I should probably test this.
Status: NEW → ASSIGNED
Comment 9•19 years ago
|
||
> I should probably clarify the comment. Yes, exactly. > does anyone care which one it is? :-) I do, if we're putting this in an api comment. Why bother mentioning it at all otherwise? > The one from aWindowContext... I should document this better Yes, please. > You mean I should document that this calls onStartURIOpen? For example, yes. :) But seriously, I didn't have any idea what that comment was talking about. Note that "listener" in this comment can mean either nsIStreamListener or nsIURIContentListener depending on the use; it would be good to disambiguate... > probably needs rewording too. Right. > Yeah, hm, but we can only detect that in onStartRequest. At that point, > uriloader will call canHandleContent, which fails, uriloader returns > NS_ERROR_WONT_HANDLE_CONTENT, which means onStartRequest fails, which means we > fall back. So I think we're ok here. I should probably test this. Please. And document.
Assignee | ||
Comment 10•19 years ago
|
||
> Why bother mentioning it at all otherwise?
The reason I mentioned is that the caller might assume that the loadgroup he set might be the one that's used. I wanted to make it clear that it will get replaced.
Comment 11•19 years ago
|
||
OK. But then I think it's worth saying what it gets replaced with...
Assignee | ||
Comment 12•19 years ago
|
||
Attachment #202911 -
Attachment is obsolete: true
Attachment #203663 -
Flags: review?(darin)
Attachment #202911 -
Flags: review?(darin)
Assignee | ||
Comment 13•19 years ago
|
||
Attachment #202912 -
Attachment is obsolete: true
Attachment #203672 -
Flags: superreview?(darin)
Attachment #203672 -
Flags: review?(bzbarsky)
Attachment #202912 -
Flags: superreview?(darin)
Attachment #202912 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 14•19 years ago
|
||
Comment 15•19 years ago
|
||
Comment on attachment 203663 [details] [diff] [review] netwerk/ part, v2 r=darin
Attachment #203663 -
Flags: review?(darin) → review+
Comment 16•19 years ago
|
||
Comment on attachment 203672 [details] [diff] [review] uriloader/ part, v2 I'll sr this after boris has finished his review of it.
Comment 17•19 years ago
|
||
Comment on attachment 203672 [details] [diff] [review] uriloader/ part, v2 Thanks for the comment improvements! r=bzbarsky
Attachment #203672 -
Flags: review?(bzbarsky) → review+
Comment 18•19 years ago
|
||
Comment on attachment 202913 [details] [diff] [review] content/base/src part >Index: content/base/src/nsObjectLoadingContent.cpp >+ rv = uriLoader->OpenChannel(chan, nsIURILoader::IS_CONTENT_PREFERRED | >+ nsIURILoader::DONT_RETARGET, req, Why nsIURILoader::IS_CONTENT_PREFERRED ? That should only be set for link clicks, no? That's certainly what docshell does it passes mLoadType == LOAD_LINK for the aIsContentPreferred arg of nsIURILoader::LoadURI. r+sr=bzbarsky with that fixed.
Attachment #202913 -
Flags: superreview?(bzbarsky)
Attachment #202913 -
Flags: superreview+
Attachment #202913 -
Flags: review?(bzbarsky)
Attachment #202913 -
Flags: review+
Comment 19•19 years ago
|
||
Comment on attachment 203673 [details] [diff] [review] uriloader/ part, v2, as diff -b sr=darin
Attachment #203673 -
Flags: superreview+
Updated•19 years ago
|
Attachment #203672 -
Flags: superreview?(darin)
Assignee | ||
Comment 20•19 years ago
|
||
hm... I did use to pass true to the isContentPreferred argument: - rv = listener->DoContent(mContentType.get(), PR_TRUE, aRequest, I guess that was wrong. I'll remove that flag before checking in. (note to self: still need to do aforementioned testing)
Assignee | ||
Comment 21•19 years ago
|
||
Testcase for a stream converter that converts to something unsupported: data:text/html,<object%20data="data:application/x-unknown-content-type,FAIL%2500">PASS</object> (Verification that this does what one would expect: data:text/html,<object%20data="data:application/x-unknown-content-type,Text%20content">Alternate</object> ) Works fine with the patch: The first one shows "PASS", the second one "Text content" (as expected)
Assignee | ||
Comment 22•19 years ago
|
||
Attachment #202913 -
Attachment is obsolete: true
Assignee | ||
Comment 23•19 years ago
|
||
netwerk part: Checking in netwerk/streamconv/public/nsIStreamConverterService.idl; /cvsroot/mozilla/netwerk/streamconv/public/nsIStreamConverterService.idl,v <-- nsIStreamConverterService.idl new revision: 1.11; previous revision: 1.10 done Checking in netwerk/streamconv/src/nsStreamConverterService.cpp; /cvsroot/mozilla/netwerk/streamconv/src/nsStreamConverterService.cpp,v <-- nsStreamConverterService.cpp new revision: 1.67; previous revision: 1.66 done uriloader part: Checking in rdf/chrome/src/nsChromeProtocolHandler.cpp; /cvsroot/mozilla/rdf/chrome/src/nsChromeProtocolHandler.cpp,v <-- nsChromeProtocolHandler.cpp new revision: 1.108; previous revision: 1.107 done Checking in uriloader/base/nsIURILoader.idl; /cvsroot/mozilla/uriloader/base/nsIURILoader.idl,v <-- nsIURILoader.idl new revision: 1.34; previous revision: 1.33 done Checking in uriloader/base/nsURILoader.cpp; /cvsroot/mozilla/uriloader/base/nsURILoader.cpp,v <-- nsURILoader.cpp new revision: 1.138; previous revision: 1.137 done
Assignee | ||
Comment 24•19 years ago
|
||
the comment change in v2 was wrong... this one is right (also has the correct diff parameters)
Attachment #206187 -
Attachment is obsolete: true
Assignee | ||
Comment 25•19 years ago
|
||
content part: Checking in content/base/src/Makefile.in; /cvsroot/mozilla/content/base/src/Makefile.in,v <-- Makefile.in new revision: 1.84; previous revision: 1.83 done Checking in content/base/src/nsObjectLoadingContent.cpp; /cvsroot/mozilla/content/base/src/nsObjectLoadingContent.cpp,v <-- nsObjectLoadingContent.cpp new revision: 1.18; previous revision: 1.17 done
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Comment 26•19 years ago
|
||
this seems interesting functionality. what it is supposed the eml testcase to do (i get FAIL on latest trunk)?
Assignee | ||
Comment 27•19 years ago
|
||
requires seamonkey with mailnews. you should see the mail message in the <object>.
Comment 28•19 years ago
|
||
I think this fix caused bug 320732 - perhaps mailnews is doing something wrong...investigating
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•