Closed Bug 309525 Opened 15 years ago Closed 14 years ago

<object> loading doesn't do stream conversion

Categories

(Core :: DOM: Core & HTML, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9alpha1

People

(Reporter: Biesinger, Assigned: Biesinger)

References

()

Details

Attachments

(5 files, 4 obsolete files)

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.
Priority: -- → P2
Target Milestone: --- → mozilla1.9alpha
Attached patch netwerk/ part (obsolete) — Splinter Review
Attachment #202911 - Flags: superreview?(bzbarsky)
Attachment #202911 - Flags: review?(darin)
Attached patch uriloader/ part (obsolete) — Splinter Review
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)
Attached patch content/base/src part (obsolete) — Splinter Review
I wonder if I should expose this on nsIWebNavigation, and call that...
Attachment #202913 - Flags: superreview?(bzbarsky)
Attachment #202913 - Flags: review?(bzbarsky)
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 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 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... ;)
(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
> 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.




> 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.
OK.  But then I think it's worth saying what it gets replaced with...
Attachment #202911 - Attachment is obsolete: true
Attachment #203663 - Flags: review?(darin)
Attachment #202911 - Flags: review?(darin)
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)
Comment on attachment 203663 [details] [diff] [review]
netwerk/ part, v2

r=darin
Attachment #203663 - Flags: review?(darin) → review+
Comment on attachment 203672 [details] [diff] [review]
uriloader/ part, v2

I'll sr this after boris has finished his review of it.
Comment on attachment 203672 [details] [diff] [review]
uriloader/ part, v2

Thanks for the comment improvements!  r=bzbarsky
Attachment #203672 - Flags: review?(bzbarsky) → review+
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 on attachment 203673 [details] [diff] [review]
uriloader/ part, v2, as diff -b

sr=darin
Attachment #203673 - Flags: superreview+
Attachment #203672 - Flags: superreview?(darin)
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)
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)
Attached patch content/base/src part, v2 (obsolete) — Splinter Review
Attachment #202913 - Attachment is obsolete: true
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
the comment change in v2 was wrong... this one is right (also has the correct diff parameters)
Attachment #206187 - Attachment is obsolete: true
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: 14 years ago
Resolution: --- → FIXED
this seems interesting functionality.

what it is supposed the eml testcase to do (i get FAIL on latest trunk)?
requires seamonkey with mailnews. you should see the mail message in the <object>.
I think this fix caused bug 320732 - perhaps mailnews is doing something wrong...investigating
Depends on: 382113
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.