We should freeze nsIWebBrowserStream, which was added as part of bug 205425.

so this interface has two issues, as I'm noticing (wish I had noticed that before): - can't pass binary data through |string| - appendToStream should use [const,array,size_is(aLen)] in octet aData - can't have IDN-using base uri - should use either nsIURI or AUTF8String as base uri I'd hate to see it frozen with these issues. fortunately it seems we didn't have any non-alpha release with it (I thought it was on 1.7 branch, but it doesn't seem to be)

yeah, we can safely hack this interface all we want. let's do it right! biesi: do you want to take a crack at fixing it up? would greatly appreciate your help :-)

Comment on attachment 162854 [details] [diff] [review] patch this doesn't mark the iface frozen just yet

I used this patch to gtkmozembed to test this patch. it is probably not suitable for checkin, since it doesn't eliminate the now unused members of EmbedPrivate and EmbedStream.*; and the (now bitrotted) patch in bug 205425 takes care of that anyway. I were also not sure whether I can change the signature of functions in gtkmozembed.h (although they are binary compatible. not source though) if people want, I can makes this a real patch to make gtkmozembed use this iface, though.

Comment on attachment 162854 [details] [diff] [review] patch I think we should allow the embedder to specify a content charset too, or we can allow them to pass "foo/bar; charset=xyz" .. that'll work with the code as is, so maybe that's sufficient. probably want to document it then. when verifying that the contentType is ASCII, might be good to return NS_ERROR_INVALID_ARG if it is not. and maybe use NS_ENSURE_TRUE so we get a warning in debug builds, not that embedders test against debug builds! ;) otherwise, the patch looks great. thanks! sr=darin

Comment on attachment 162854 [details] [diff] [review] patch This is really low on my radar until the ffox l10n stuff gets done, so it might take a couple weeks.

Comment on attachment 162854 [details] [diff] [review] patch ah, ok, I'll change to INVALID_ARG. I'll add a line to @param aContentType: This string may include a charset declaration, for example: text/html;charset=ISO-8859-1

NS_NewInputStreamChannel currently overwrites the charset with an empty string here. this patch fixes that.

biesi: another approach would be to have a separate version of NS_NewInputStreamChannel that does not take a contentCharset param, maybe? or, we could try setting the contentCharset before the contentType.

(In reply to comment #10) > biesi: another approach would be to have a separate version of > NS_NewInputStreamChannel that does not take a contentCharset param, maybe? hm... so I'd copy&paste most of the code from this function? is that worth it? > or, > we could try setting the contentCharset before the contentType. this would do the wrong thing for type "text/html;charset=iso-8859-1" and charset "UTF-8" it seems... this should probably use UTF-8 as charset, shouldn't it? (a bit of an edge case)

> > biesi: another approach would be to have a separate version of > > NS_NewInputStreamChannel that does not take a contentCharset param, maybe? > > hm... so I'd copy&paste most of the code from this function? is that worth it? exactly, or factor as appropriate. remember that this stuff is all inlined anyways, so it's better to inline less code. inlining with your patch would mean: "if (!EmptyString().IsEmpty()) { do stuff; }" ... the compiler will not be able to know that that branch can be removed.

OK, what about this? I verified that GCC optimizes the if (aContentType && ...) check away if the function w/o charset is called. I haven't verified that it optimizes the first part of the if away for the other caller (which it should be able to do, since a reference can't be NULL). I do think this check is needed there, though.

Comment on attachment 163133 [details] [diff] [review] charset work, v2 (checked in) >+ // XXX callers might like real error codes then they can write this code our by hand. most callers just want to know whether the thing succeeded or failed, so there's no point checking rv at every step of the way. r+sr=darin with that XXX comment removed.

this makes callers not pass an empty content charset to inputstreamchannels. some callers pass a null stream to this function... why they do that, I don't know - asyncOpen will fail on the resulting channel...

Comment on attachment 163299 [details] [diff] [review] charset work, addendum (checked in) sorry for not doing this in the previous patch

Comment on attachment 163299 [details] [diff] [review] charset work, addendum (checked in) r+sr=darin

Comment on attachment 162854 [details] [diff] [review] patch boris has agreed to review this patch. blizzard: feel free to chim in if you have comments :-)

Comment on attachment 162854 [details] [diff] [review] patch (With the changes darin requested) Btw if you want to turn your gtkmozembed test case in a real patch I'd happily review it.

(In reply to comment #19) > Btw if you want to turn your gtkmozembed test case in a real patch I'd happily > review it. oh, I were thinking you were doing that in bug 205425. but sure, if you aren't, I can do it here.

and now, with addressed review comments

Bug 205425 is a bit low in my queue. Also the patch is already bitrotten and, since the embed string changes is somewhat risky, I think it would be good to do the stream change here and eventually fix the left part of 205425 (embed strings) with a separate patch.

marco: see also bug 264274 (for better frozen string fu)

Comment on attachment 165812 [details] [diff] [review] make gtkmozembed use nsIWebBrowserStream mozilla/embedding/browser/gtk/src/gtkmozembed.h > void gtk_moz_embed_render_data (GtkMozEmbed *embed, >- const char *data, >+ const guint8 *data, > void gtk_moz_embed_append_data (GtkMozEmbed *embed, >- const char *data, guint32 len); >+ const guint8 *data, guint32 len); I think gtkmozembed is API frozen, so we cannot change the types on these functions. I think we will have to do the cast internally, when passing the data to nsIWebBrowserStream instead.

ok, weren't sure about that. this time, no gtkmozembed api changes.

Comment on attachment 165902 [details] [diff] [review] make gtkmozembed use nsIWebBrowserStream v2 (checked in) >Index: src/Makefile.in >=================================================================== >RCS file: /cvsroot/mozilla/embedding/browser/gtk/src/Makefile.in,v >retrieving revision 1.47 >diff -p -u -6 -r1.47 Makefile.in >--- src/Makefile.in 18 Apr 2004 22:00:39 -0000 1.47 >+++ src/Makefile.in 14 Nov 2004 16:46:46 -0000 >@@ -74,13 +74,13 @@ CPPSRCS = \ > EmbedPrivate.cpp \ > EmbedWindow.cpp \ > EmbedProgress.cpp \ > EmbedContentListener.cpp \ > EmbedEventListener.cpp \ > EmbedWindowCreator.cpp \ >- EmbedStream.cpp >+ $(NULL) Looks like $(NULL) should be aligned with Embed*

Comment on attachment 165902 [details] [diff] [review] make gtkmozembed use nsIWebBrowserStream v2 (checked in) >Index: src/EmbedPrivate.cpp > #include "nsIWidget.h" > #include "nsCRT.h" >+#include "nsNetUtil.h" Hmm... eventually, we'll want to remove these header files so that this code is built solely against the Gecko SDK, but we're not ready for that now, so that's fine. sr=darin

Comment on attachment 165902 [details] [diff] [review] make gtkmozembed use nsIWebBrowserStream v2 (checked in) "make gtkmozembed use nsIWebBrowserStream v2" checked in.

I couldn't resist a little bit of cleanup in nsEmbedStream

Comment on attachment 168442 [details] [diff] [review] Mark it frozen. looks great, sr=darin now, to teach eclipse to use this interface when compiling against the 1.4 Gecko SDK :-/

Darin: Does it mean that starting with 1.8 (>=beta), you encourage embedders to use this new nsIWebBrowserStream interface instead of the unfrozen nsIDocShell.LoadStream? If that's so, it sounds like the SWT Browser widget could do a query interface on nsIWebBrowser and if there is any answer use it instead of the old nsIDocShell way.

> Does it mean that starting with 1.8 (>=beta), you encourage embedders to use > this new nsIWebBrowserStream interface instead of the unfrozen > nsIDocShell.LoadStream? Yes, QI to nsIWebBrowserStream and if that works use it otherwise failover to the old nsIDocShell.loadStream approach.

Checking in embedding/browser/webBrowser/Makefile.in; /cvsroot/mozilla/embedding/browser/webBrowser/Makefile.in,v <-- Makefile.in new revision: 1.62; previous revision: 1.61 done Checking in embedding/browser/webBrowser/nsEmbedStream.cpp; /cvsroot/mozilla/embedding/browser/webBrowser/nsEmbedStream.cpp,v <-- nsEmbedStream.cpp new revision: 1.5; previous revision: 1.4 done Checking in embedding/browser/webBrowser/nsIWebBrowserStream.idl; /cvsroot/mozilla/embedding/browser/webBrowser/nsIWebBrowserStream.idl,v <-- nsIWebBrowserStream.idl new revision: 1.3; previous revision: 1.2 done