[meta] freeze specific necko APIs for mozilla 1.0

VERIFIED FIXED in mozilla1.0

Status

()

Core
Networking
P3
major
VERIFIED FIXED
15 years ago
15 years ago

People

(Reporter: Darin Fisher, Assigned: Darin Fisher)

Tracking

({arch, embed, topembed-})

Trunk
mozilla1.0
arch, embed, topembed-
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt3])

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

15 years ago
[meta] freeze specific necko APIs for mozilla 1.0
(Assignee)

Comment 1

15 years ago
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?
Severity: normal → major
Status: NEW → ASSIGNED
Depends on: 124030, 124031
Priority: -- → P2
Target Milestone: --- → mozilla1.0

Comment 2

15 years ago
dougt has the latest cut at this list.
(Assignee)

Comment 3

15 years ago
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?
(Assignee)

Updated

15 years ago
Keywords: mozilla1.0
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

15 years ago
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.
(Assignee)

Comment 6

15 years ago
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?

Comment 8

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

Comment 9

15 years ago
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.
(Assignee)

Comment 10

15 years ago
adding dependency bugs for nsIChannel and nsIProtocolHandler, since these will
most likely freeze whether we mark them as frozen or not :(
Depends on: 128508, 128509
(Assignee)

Updated

15 years ago
No longer depends on: 128508
(Assignee)

Updated

15 years ago
Depends on: 128508
(Assignee)

Updated

15 years ago
Keywords: arch, nsbeta1, topembed

Updated

15 years ago
Keywords: nsbeta1+

Updated

15 years ago
Keywords: nsbeta1
(Assignee)

Updated

15 years ago
Priority: P2 → P3

Comment 11

15 years ago
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|]
(Assignee)

Comment 12

15 years ago
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 :(
(Assignee)

Updated

15 years ago
Depends on: 99165
(Assignee)

Comment 13

15 years ago
adding nsIInputStream and nsIOutputStream to the interfaces being tracked by
this bug... i'll stamp them all FROZEN at the same time.
No longer depends on: 99165

Comment 14

15 years ago
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

Updated

15 years ago
Keywords: topembed → topembed+

Updated

15 years ago
Whiteboard: [adt3]

Comment 15

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

Comment 16

15 years ago
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.
(Assignee)

Comment 17

15 years ago
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.
(Assignee)

Comment 18

15 years ago
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.
(Assignee)

Comment 20

15 years ago
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.
(Assignee)

Comment 21

15 years ago
nsC* is the convention for these header files.

Comment 22

15 years ago
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 :-)
(Assignee)

Comment 23

15 years ago
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.
(Assignee)

Comment 24

15 years ago
Created attachment 79579 [details] [diff] [review]
same changes w/o the nsC*.h files (minus mac project changes)
Attachment #78659 - Attachment is obsolete: true
(Assignee)

Comment 25

15 years ago
Created attachment 79581 [details]
interfaces to be frozen
(Assignee)

Comment 26

15 years ago
i still need to address points 3 and 4 from comment #23
(Assignee)

Comment 27

15 years ago
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)

Comment 29

15 years ago
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.
Keywords: nsbeta1-

Comment 30

15 years ago
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.
Keywords: nsbeta1+
(Assignee)

Comment 31

15 years ago
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

15 years ago
mozilla1.0 needs this. adding a plus.
Keywords: mozilla1.0 → mozilla1.0+

Comment 33

15 years ago
Marking topembed-, embed per EDT triage.
Keywords: topembed+ → embed, topembed-
(Assignee)

Comment 34

15 years ago
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.
(Assignee)

Comment 35

15 years ago
Created attachment 82179 [details] [diff] [review]
final patch
Attachment #79579 - Attachment is obsolete: true
(Assignee)

Comment 36

15 years ago
ok, times up... landed this patch on the trunk.
(Assignee)

Comment 37

15 years ago
landed on branch, marking FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 38

15 years ago
adding branch resolution keyword.
Keywords: fixed1.0.0

Comment 39

15 years ago
VERIFIED: +1.0.0
If there are further concerns, please reopen.
Status: RESOLVED → VERIFIED
Keywords: fixed1.0.0 → verified1.0.0
You need to log in before you can comment on or make changes to this bug.