[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: nsIRequest nsIURI nsIURL and i believe these are also important: nsILoadGroup nsIHttpChannel (cringe!) 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... -- rick

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.

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: http://www.mozilla.org/projects/embedding/HowToFreeze.html 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 headers... 2. Definition of the Contract-id (and Class-id if necessary) for the component instance. 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 *always* paired. 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 paired. 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 nsIRequest::cancel). 5- sure, i'll attach a tarball of the revised interfaces.

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 >netwerk/base/public/nsIChannel.idl: > /** > * 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 >}; <nsIProtocolHandler> >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 don't 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? > >netwerk/base/public/nsIURI.idl: > >%{C++ >#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 they? > 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 adt@netscape.com. You can search for "changing adt3 bugs" to quickly find and delete these bug mails.

Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt. If you have any questions about this, please email adt@netscape.com. 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.

ok, times up... landed this patch on the trunk.

landed on branch, marking FIXED

adding branch resolution keyword.

VERIFIED: +1.0.0 If there are further concerns, please reopen.