[meta] freeze specific necko APIs for mozilla 1.0
can someone please list the full set of necko APIs that must be frozen for
mozilla 1.0... i already have the following under way:
and i believe these are also important:
are there others?
dougt has the latest cut at this list.
nsIRequestObserver needs to be added to this list as well.
there's also been some talk about freezing nsIProtocolHandler and nsIChannel.
do we really want to freeze these for moz 1.0?
If you freeze nsIChannel, then maybe Open should be on a separate, nonfrozen,
interface? Do any of our current protocols support syncronous opening?
that is what darin and I discussed a few times, but there are callers that want
to open a resource synchronously. We shouldn't minimize the API because a few
callers (noted the big ones) don't implement it.
file, jar, and resource implement nsIChannel::Open
this is because these protocols can easily return a stream to file.
No, but you could make an nsISynchronousChannel. I just get a bad feeling abuot
freezing behaviour that most of the protocol handlers don't implement - who
actually calls Open on those protocols?
there is NO reason why a protocol can not support open(). it is a bug that http
and ftp do not support it.
some RDF and maybe XBL resources are loaded synchronously at startup. there
have always been plans to support sync HTTP ... it just got severely delayed.
adding dependency bugs for nsIChannel and nsIProtocolHandler, since these will
most likely freeze whether we mark them as frozen or not :(
Probably too late, but the names |nsIRequest| / |nsIChannel| have always seemed
to be too generic to me (e.g., they give the impression of being pretty much in
the same category as |nsISupports|).
[Alternatives could perhaps have been |nsIConnectRequest|, |nsIConnectChannel|]
i'd agree that the names are a bit overly generic, but i'd also agree that it is
late in the game... so, no love probably :(
adding nsIInputStream and nsIOutputStream to the interfaces being tracked by
this bug... i'll stamp them all FROZEN at the same time.
the name nsIRequest was meant to be generic. currently, nsIRequests are used to
represent 'async operations'. these include layout reflows as well as network
operations... basically, an nsIRequest can be anything that is asynchronous and
should be cancelable, etc...
that's why we tried to keep nsIRequest as generic as possible... nsIChannel is
another story :-) yeah, nsIChannel is kinda vague... but at this point i think
we're just going to live with it...
What needs to be done to verify the interface freeze bugs? This sounds like a
classic 'whitebox/code' issue.
All that needs to be done is to make sure that frozen interfaces follow the
guidelines at http://www.mozilla.org/projects/embedding/HowToFreeze.html
In addition to proper comments, they should be marked with "@status FROZEN" keyword.
i'll be updating the IIDs of all the necko interfaces that i have touched before
marking them frozen. this will help avoid binary conflicts with 3rd party code
that references the old interfaces.
Created attachment 78659 [details] [diff] [review]
preliminary final patch (minus required mac project changes)
Why the nsC* stuff? Can't they all go in one combined header file, such that the
header file itsself isn't frozen, but the stuff in there will continue to be the
same? You could have a spearate header file for frozen vs non frozen interfaces
if you want. Although we're only freezing contract ids, not classids anyway,
aren't we, since we're freezing interfaces, not concrete implementation.
bbaetz: take a look at the bottom of this document:
For each frozen service or public component create a corresponding header
file which has the following:
1. #includes for each of the interfaces it supports. This allows consumers to
simply include the 'component' header file when using
a component without having to explicitly include all of the interface
2. Definition of the Contract-id (and Class-id if necessary) for the
3. Documentation about the component[Please follow the guidelines in "How to
mark an interface as frozen"
mentioned above as applicable]
4. Any other component specific code and/or defines.
nsC* is the convention for these header files.
Comment on attachment 78659 [details] [diff] [review]
preliminary final patch (minus required mac project changes)
You created a lot of nsCFoo.h files which contain the ContractID, CID, and
class name. Did you think about just having one file which contains all of
these declarations? Do we really need seperate files?
do we really have to have this warrning in the nsIChannel interface:
+ * method. Moreover, since this method may block the calling thread, it
+ * should not be called on a thread that processes UI events.
in the nsIRequestObserver, you should also assert that onStart and onStop are
This comment scares me:
+ * An exception thrown from onStopRequest is generally ignored.
When is it not ignored?
the changes you had to make are minor and I am okay with them. However, I
still wanna review the all of the complete interfaces which you are freezeing.
Could you tar up all of the frozen interfaces and attach them. This way, I can
comment on the complete interface a bit easier than sorting through this
unified diff :-)
1- nsCFoo is "the way to freeze a component" no? chak, valeski?
2- we don't need to have the warning i suppose, but it certainly should be
documented somewhere, wouldn't you agree? what better place then nsIChannel.idl?
3- i'll add a comment to nsIRequestObserver about onStart/onStop always being
4- hmm, yeah.. i suppose i should say that an exception resulting from
OnStopRequest may be ignored (e.g., it will not result in a call to
5- sure, i'll attach a tarball of the revised interfaces.
Created attachment 79579 [details] [diff] [review]
same changes w/o the nsC*.h files (minus mac project changes)
Created attachment 79581 [details]
interfaces to be frozen
i still need to address points 3 and 4 from comment #23
given that this latest patch doesn't add any nsC*.h files, does anyone have any
other concerns that need to be addressed by this patch?
dougt, bbaetz: i'd really appreciate it if you two could review this patch. thx!
Comment on attachment 79581 [details]
interfaces to be frozen
> * This flag is set to indicate that onStopRequest may be followed by
> * another onStartRequest/onStopRequest pair. This flag is, for example,
> * used by the multipart/replace stream converter.
> const unsigned long LOAD_REPLACE = 1 << 18;
tabs... You have these in a few other places, too
>interface nsIProtocolHandler : nsISupports
> * The scheme of this protocol (e.g., "file").
> readonly attribute ACString scheme;
> * The default port is the port that this protocol normally uses.
> * If a port does not make sense for the protocol (e.g., "about:")
> * then -1 will be returned.
> readonly attribute long defaultPort;
> * Returns the protocol specific flags (see flag definitions below).
> readonly attribute unsigned long protocolFlags;
This is going to be semi-controversial (I remember the last time it was brought
up, late last year):
"Implementations MUST ignore any flags they do not understand."
this allows us to add a new flag later, provided that implementations which
understand the flag are unchanged.
OTOH, that disucssions sort of tapered off, rather than arrive at a conclusion.
Maybe we should add the comment anyway (since doing so is good programming
practice, and won't hurt),
and keep the arguments until we actually need/want to..
>interface nsIRequest : nsISupports
> * The name of the request. Often this is the URI of the request.
> readonly attribute AUTF8String name;
> * @return TRUE if the request has yet to reach completion.
> * @return FALSE if the request has reached completion (e.g., after
> * OnStopRequest has fired).
> * Suspended requests are still considered pending.
> boolean isPending();
Why isn't this a readonly attribute?
>#undef GetPort // XXX Windows!
>#undef SetPort // XXX Windows!
That looks rather, um, fragile. Can we do better, somehow?
>interface nsIURI : nsISupports
> * An optimization to do scheme checks without requiring the users of nsIURI
> * to GetScheme, thereby saving extra allocating and freeing. Returns true if
> * the schemes match (case ignored).
Would this be better off as an |in nsAUTF8String| (Actually, scheme names are
always ASCII, aren't
> boolean schemeIs(in string scheme);
We're not freezing nsIFileURL, correct?
[nsIInputStream & nsIOutputStream]
> * @return true if stream is non-blocking
> boolean isNonBlocking();
Why isn't this a readonly attribute (in both cases)
Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt. If you have any
questions about this, please email firstname.lastname@example.org. You can search for
"changing adt3 bugs" to quickly find and delete these bug mails.
bbaetz: thx for reviewing the patch...
1- will fix the indentation problems
2- agreed... will add that comment to the patch
3- already had the argument over isNonBlocking... isPending follows suite. i
personally prefer IsPending() over GetIsPending(). see the discussion in bug
99165 if you're interested. anyways, i don't want to change the binary
signature of these interfaces at this point. looks like it's going to be hard
enough to convince ADT to accept this patch on the branch... wouldn't want there
to be actual code changes muddling the picture ;-) ...all kidding aside, let's
not break binary compatibility _again_ if we can help it... apparently, there
are plugin vendors using these interfaces.
4- #if defined(XP_WIN) && defined(GetPort) ?! heck, we've lived with this for a
long time... why change it now?
5- RFC2396 defines an URL scheme as a subset of ASCII.
6- we could freeze nsIFileURL but no one has frankly asked for it. anyways,
i've never liked the fact that you can QI nsStandardURL to nsIFileURL when it
doesn't make sense. was hoping to one day remedy that by adding an
initialization flag to nsIStandardURL::Init. until something like that happens,
we'll always have to explain to people that QI'ing to nsIFileURL is not enough
to determine that it is a file:// URL, which greatly confuses most necko newbies.
mozilla1.0 needs this. adding a plus.
Marking topembed-, embed per EDT triage.
FYI: these "comment changes" (plus revisions for bbaetz's points #1 and #2) are
landing sometime tomorrow on both the trunk and the 1.0 branch.
Created attachment 82179 [details] [diff] [review]
ok, times up... landed this patch on the trunk.
landed on branch, marking FIXED
adding branch resolution keyword.
If there are further concerns, please reopen.