Freeze nsIWebBrowserStream

RESOLVED FIXED in mozilla1.8beta1

Status

()

RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: darin.moz, Assigned: Biesinger)

Tracking

Trunk
mozilla1.8beta1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(5 attachments, 5 obsolete attachments)

6.70 KB, patch
darin.moz
: review+
darin.moz
: superreview+
Details | Diff | Splinter Review
9.42 KB, patch
darin.moz
: review+
darin.moz
: superreview+
Details | Diff | Splinter Review
10.32 KB, patch
Details | Diff | Splinter Review
19.77 KB, patch
mpgritti
: review+
darin.moz
: superreview+
Details | Diff | Splinter Review
4.24 KB, patch
mpgritti
: review+
darin.moz
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

14 years ago
We should freeze nsIWebBrowserStream, which was added as part of bug 205425.
(Reporter)

Updated

14 years ago
Blocks: 248683
(Reporter)

Updated

14 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.8beta
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)
(Reporter)

Comment 2

14 years ago
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 :-)
Created attachment 162854 [details] [diff] [review]
patch
Assignee: darin → cbiesinger
Comment on attachment 162854 [details] [diff] [review]
patch

this doesn't mark the iface frozen just yet
Attachment #162854 - Flags: superreview?(darin)
Attachment #162854 - Flags: review?(bsmedberg)
Created attachment 162855 [details] [diff] [review]
patch I used to test this

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.
(Reporter)

Comment 6

14 years ago
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
Attachment #162854 - Flags: superreview?(darin) → superreview+

Comment 7

14 years ago
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
Attachment #162854 - Flags: review?(bsmedberg) → review?(blizzard)
Created attachment 163019 [details] [diff] [review]
make that charset-as-part-of-content-type work

NS_NewInputStreamChannel currently overwrites the charset with an empty string
here. this patch fixes that.
Attachment #163019 - Flags: superreview?(darin)
Attachment #163019 - Flags: review?(darin)
(Reporter)

Comment 10

14 years ago
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)
(Reporter)

Comment 12

14 years ago
> > 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.
Created attachment 163133 [details] [diff] [review]
charset work, v2 (checked in)

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.
Attachment #163019 - Attachment is obsolete: true
Attachment #163133 - Flags: superreview?(darin)
Attachment #163133 - Flags: review?(darin)
Attachment #163019 - Flags: superreview?(darin)
Attachment #163019 - Flags: review?(darin)
(Reporter)

Comment 14

14 years ago
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.
Attachment #163133 - Flags: superreview?(darin)
Attachment #163133 - Flags: superreview+
Attachment #163133 - Flags: review?(darin)
Attachment #163133 - Flags: review+
Attachment #163133 - Attachment description: charset work, v2 → charset work, v2 (checked in)
Created attachment 163299 [details] [diff] [review]
charset work, addendum (checked in)

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
Attachment #163299 - Flags: superreview?
Attachment #163299 - Flags: review?(darin)
(Reporter)

Comment 17

14 years ago
Comment on attachment 163299 [details] [diff] [review]
charset work, addendum (checked in)

r+sr=darin
Attachment #163299 - Flags: superreview?
Attachment #163299 - Flags: superreview+
Attachment #163299 - Flags: review?(darin)
Attachment #163299 - Flags: review+
Attachment #163299 - Attachment description: charset work, addendum → charset work, addendum (checked in)
(Reporter)

Comment 18

14 years ago
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 :-)
Attachment #162854 - Flags: review?(blizzard) → review?(bzbarsky)
(Reporter)

Updated

14 years ago
Blocks: 268520
Attachment #162854 - Flags: review?(bzbarsky) → review?(marco)

Comment 19

14 years ago
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.
Attachment #162854 - Flags: review?(marco) → review+
(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.
Created attachment 165742 [details] [diff] [review]
patch merged to trunk
Attachment #162854 - Attachment is obsolete: true
Created attachment 165744 [details] [diff] [review]
patch merged to trunk with review comments (checked in)

and now, with addressed review comments
Attachment #165742 - Attachment is obsolete: true
Attachment #165744 - Attachment description: patch merged to trunk with review comments → patch merged to trunk with review comments (checked in)

Comment 23

14 years ago
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.
(Reporter)

Comment 24

14 years ago
marco: see also bug 264274 (for better frozen string fu)
Created attachment 165812 [details] [diff] [review]
make gtkmozembed use nsIWebBrowserStream
Attachment #162855 - Attachment is obsolete: true
Attachment #165812 - Flags: review?(marco)

Comment 26

14 years ago
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.
Attachment #165812 - Flags: review?(marco) → review-
Created attachment 165902 [details] [diff] [review]
make gtkmozembed use nsIWebBrowserStream v2 (checked in)

ok, weren't sure about that. this time, no gtkmozembed api changes.
Attachment #165812 - Attachment is obsolete: true
Attachment #165902 - Flags: review?(marco)

Comment 28

14 years ago
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*
Attachment #165902 - Flags: review?(marco) → review+
Attachment #165902 - Flags: superreview?(darin)
(Reporter)

Comment 29

14 years ago
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
Attachment #165902 - Flags: superreview?(darin) → superreview+
Comment on attachment 165902 [details] [diff] [review]
make gtkmozembed use nsIWebBrowserStream v2 (checked in)

"make gtkmozembed use nsIWebBrowserStream v2" checked in.
Attachment #165902 - Attachment description: make gtkmozembed use nsIWebBrowserStream v2 → make gtkmozembed use nsIWebBrowserStream v2 (checked in)
Created attachment 168442 [details] [diff] [review]
Mark it frozen.

I couldn't resist a little bit of cleanup in nsEmbedStream
Attachment #168442 - Flags: superreview?(darin)
Attachment #168442 - Flags: review?(marco)
(Reporter)

Comment 32

14 years ago
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 :-/
Attachment #168442 - Flags: superreview?(darin) → superreview+

Comment 33

14 years ago
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.

Updated

14 years ago
Attachment #168442 - Flags: review?(marco) → review+
(Reporter)

Comment 34

14 years ago
> 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
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.