124465 - [meta] freeze specific necko APIs for mozilla 1.0

Status: VERIFIED FIXED

(Core :: Networking, defect, P3)

Description

[Darin Fisher]

[meta] freeze specific necko APIs for mozilla 1.0

Comment 1

[Darin Fisher]

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?

Comment 2

[Judson Valeski]

dougt has the latest cut at this list.

Comment 3

[Darin Fisher]

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?

Comment 4

[Bradley Baetz (:bbaetz)]

If you freeze nsIChannel, then maybe Open should be on a separate, nonfrozen,
interface? Do any of our current protocols support syncronous opening?

Comment 5

[Doug Turner (:dougt)]

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.

Comment 6

[Darin Fisher]

file, jar, and resource implement nsIChannel::Open

this is because these protocols can easily return a stream to file.

Comment 7

[Bradley Baetz (:bbaetz)]

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?

Comment 8

[Doug Turner (:dougt)]

there is NO reason why a protocol can not support open().  it is a bug that http
and ftp do not support it.  

Comment 9

[Darin Fisher]

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.

Comment 10

[Darin Fisher]

adding dependency bugs for nsIChannel and nsIProtocolHandler, since these will
most likely freeze whether we mark them as frozen or not :(

Comment 11

[rbs]

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|]

Comment 12

[Darin Fisher]

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 :(

Comment 13

[Darin Fisher]

adding nsIInputStream and nsIOutputStream to the interfaces being tracked by
this bug... i'll stamp them all FROZEN at the same time.

Comment 14

[rpotts (gone)]

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

Comment 15

[benc]

What needs to be done to verify the interface freeze bugs? This sounds like a
classic 'whitebox/code' issue.

Comment 16

[Chak Nanga]

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.

Comment 17

[Darin Fisher]

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.

Comment 18

[Darin Fisher]

Attachment: preliminary final patch (minus required mac project changes)

Comment 19

[Bradley Baetz (:bbaetz)]

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.

Comment 20

[Darin Fisher]

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.

Comment 21

[Darin Fisher]

nsC* is the convention for these header files.

Comment 22

[Doug Turner (:dougt)]

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 :-)

Comment 23

[Darin Fisher]

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.

Comment 24

[Darin Fisher]

Attachment: same changes w/o the nsC*.h files (minus mac project changes)

Comment 25

[Darin Fisher]

Attachment: interfaces to be frozen

Comment 26

[Darin Fisher]

i still need to address points 3 and 4 from comment #23

Comment 27

[Darin Fisher]

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 28

[Bradley Baetz (:bbaetz)]

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)

Comment 29

[scottputterman]

Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt.  If you have any
questions about this, please email [e-mail].  You can search for
"changing adt3 bugs" to quickly find and delete these bug mails.

Comment 30

[scottputterman]

Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt.  If you have any
questions about this, please email [e-mail].  You can search for
"changing adt3 bugs" to quickly find and delete these bug mails.

Comment 31

[Darin Fisher]

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.

Comment 32

[Asa Dotzler [:asa]]

mozilla1.0 needs this. adding a plus.

Comment 33

[Dave Barrowman]

Marking topembed-, embed per EDT triage.

Comment 34

[Darin Fisher]

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.

Comment 35

[Darin Fisher]

Attachment: final patch

Comment 36

[Darin Fisher]

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

Comment 37

[Darin Fisher]

landed on branch, marking FIXED

Comment 38

[Asa Dotzler [:asa]]

adding branch resolution keyword.

Comment 39

[benc]

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