Add more to nsIExpatSink/nsExpatDriver in order to support SAX

RESOLVED FIXED

Status

()

enhancement
RESOLVED FIXED
14 years ago
13 years ago

People

(Reporter: sayrer, Assigned: sayrer)

Tracking

({fixed1.8.1})

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 10 obsolete attachments)

I need a couple more events from Expat to fill out SAX support. They're pretty uncommon things, so they shouldn't have much effect on performance.
OS: Windows XP → All
Hardware: PC → All
Severity: normal → enhancement
I don't think any of these will get called frequently enough to effect perf.
Attachment #211208 - Flags: superreview?(peterv)
Attachment #211208 - Flags: review?(bugmail)
Blocks: 315826
Comment on attachment 211208 [details] [diff] [review]
Add some extra events to ExpatSink, to fill out SAX API

We really shouldn't register these handlers unless they're actually used by the sink. I think we should even consider adding a new interface that extends nsIExpatSink.
Attachment #211208 - Flags: superreview?(peterv)
Attachment #211208 - Flags: superreview-
Attachment #211208 - Flags: review?(bugmail)
(In reply to comment #2)
> (From update of attachment 211208 [details] [diff] [review] [edit])
> We really shouldn't register these handlers unless they're actually used by 
> the sink. I think we should even consider adding a new interface that extends
> nsIExpatSink.

Well, the XMLReader in bug 315826 does use and need these callbacks. I asked around, and was told this approach would be OK. I'm happy to do something different, but I would like to know exactly what that should be, so I can finish the XMLReader.
I know that you plan to use these callbacks in your sink. However, none of the other sinks need them (or will need them in the foreseeable future). So either you provide a new interface derived from nsIExpatSink that the driver can QI to, and then it registers the callbacks for the additional handlers if the QI was successful; or you add a way for the sink to signal which handlers it wants to be called. The first approach would limit the bloat by avoiding the addition of all the handlers in all the sinks (your sink would be the only one implementing the interface), both approaches would eliminate the performance hit of having to call all the handlers regardless of whether we do anything in them or not.
OK, here's a patch that takes the first approach. Thanks for the clear instruction, I wasn't sure what you wanted.
Attachment #211208 - Attachment is obsolete: true
Attachment #211262 - Flags: superreview?(peterv)
Attachment #211262 - Flags: review?(bugmail)
Attachment #211262 - Attachment is obsolete: true
Attachment #211264 - Flags: superreview?(peterv)
Attachment #211264 - Flags: review?(bugmail)
Attachment #211262 - Flags: superreview?(peterv)
Attachment #211262 - Flags: review?(bugmail)
Status: NEW → ASSIGNED
Posted patch fix idl typo (obsolete) — Splinter Review
Attachment #211264 - Attachment is obsolete: true
Attachment #211782 - Flags: superreview?(peterv)
Attachment #211782 - Flags: review?(bugmail)
Attachment #211264 - Flags: superreview?(peterv)
Attachment #211264 - Flags: review?(bugmail)
Comment on attachment 211782 [details] [diff] [review]
fix idl typo

Not sure if I like 'lexical'. How about 'extended' instead? I.e nsIExtendedExpatSink, mExtendedSink etc.

>Index: htmlparser/src/nsExpatDriver.cpp
...
>+nsExpatDriver::HandleStartNamespaceDecl(const PRUnichar* aPrefix,
>+                                        const PRUnichar* aUri)
>+{
>+  if (mLexicalSink) {
>+    mInternalState = mLexicalSink->HandleStartNamespaceDecl(aPrefix, 
>+                                                            aUri);
>+  }

You shouldn't need these mLexicalSink tests. Seems like we spottily do the same check in the other functions though so i guess it's safer to keep them.
Attachment #211782 - Flags: review?(bugmail) → review+
(In reply to comment #8)
> (From update of attachment 211782 [details] [diff] [review] [edit])
> Not sure if I like 'lexical'. How about 'extended' instead? I.e
> nsIExtendedExpatSink, mExtendedSink etc.

Works for me.

Actually, it might be a good idea to make nsIExtendedExpatSink inherit nsIExpatSink. I don't think it'll make a practical difference one way or the other, but it's a nice way to signal that if you implement one you should implement the other.
(In reply to comment #10)
> Actually, it might be a good idea to make nsIExtendedExpatSink inherit
> nsIExpatSink. I don't think it'll make a practical difference one way or the
> other, but it's a nice way to signal that if you implement one you should
> implement the other.

And it saves a vptr per instance (not a big cost given object population, but it is the right thing for several reasons as noted).

/be
Posted patch name change (obsolete) — Splinter Review
Attachment #211782 - Attachment is obsolete: true
Attachment #211814 - Flags: superreview?(peterv)
Attachment #211782 - Flags: superreview?(peterv)
Comment on attachment 211814 [details] [diff] [review]
name change

You should set mExtendedSink to nsnull in DidBuildModel (like we do for mSink). And I'd like to see the final .idl file too.
Attachment #211814 - Flags: superreview?(peterv) → superreview+
Posted patch Fix w/ peterv's comments (obsolete) — Splinter Review
OK, this patch includes the IDL (forgot to cvs add it). Also sets mExtendedSink to nsnull in DidBuildModel.
Attachment #211814 - Attachment is obsolete: true
Comment on attachment 213074 [details] [diff] [review]
Fix w/ peterv's comments

>Index: htmlparser/public/nsIExtendedExpatSink.idl
>===================================================================

Please document the arguments to functions (@param).

Method names should start with lowercase (see http://www.mozilla.org/scriptable/xpidl/idl-authors-guide/best-practices.html), although that makes it inconsistent with nsIExpatSink :-/.

>+  /**
>+   * Called when an xmlns attribute is encountered.
>+   */

This needs more documentation, for example that it is called before the startElement of the element that the attribute is attached to. Or you could say that it's called when a prefix mapping starts to be in-scope.

>+  void HandleStartNamespaceDecl(in wstring aPrefix,
>+                                in wstring aUri);
>+                              
>+  /**
>+   * Called when a prefix is no longer in-scope.

prefix mapping?
Posted patch patch w/ params documented (obsolete) — Splinter Review
Attachment #213074 - Attachment is obsolete: true
Attachment #214009 - Attachment is obsolete: true
Attachment #214010 - Attachment is obsolete: true
Attachment #214012 - Flags: superreview?(peterv)
Attachment #214012 - Flags: superreview?(peterv) → superreview+
Checking in parser/expat/lib/xmlparse.c;
/cvsroot/mozilla/parser/expat/lib/xmlparse.c,v  <--  xmlparse.c
new revision: 1.42; previous revision: 1.41
done
Checking in parser/htmlparser/public/Makefile.in;
/cvsroot/mozilla/parser/htmlparser/public/Makefile.in,v  <--  Makefile.in
new revision: 1.15; previous revision: 1.14
done
RCS file: /cvsroot/mozilla/parser/htmlparser/public/nsIExtendedExpatSink.idl,v
done
Checking in parser/htmlparser/public/nsIExtendedExpatSink.idl;
/cvsroot/mozilla/parser/htmlparser/public/nsIExtendedExpatSink.idl,v  <--  nsIExtendedExpatSink.idl
initial revision: 1.1
done
Checking in parser/htmlparser/src/nsExpatDriver.cpp;
/cvsroot/mozilla/parser/htmlparser/src/nsExpatDriver.cpp,v  <--  nsExpatDriver.cpp
new revision: 3.97; previous revision: 3.96
done
Checking in parser/htmlparser/src/nsExpatDriver.h;
/cvsroot/mozilla/parser/htmlparser/src/nsExpatDriver.h,v  <--  nsExpatDriver.h
new revision: 3.29; previous revision: 3.28
done
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Posted patch patch for the branch (obsolete) — Splinter Review
Attachment #220738 - Flags: approval-branch-1.8.1?(peterv)
Posted patch branch patch (obsolete) — Splinter Review
missed one of the expat functions last time
Attachment #220738 - Attachment is obsolete: true
Attachment #220749 - Flags: approval-branch-1.8.1?(peterv)
Attachment #220738 - Flags: approval-branch-1.8.1?(peterv)
Posted patch branch patchSplinter Review
Attachment 220749 [details] [diff] contains a chunk not for this bug.
Attachment #220749 - Attachment is obsolete: true
Attachment #220754 - Flags: approval-branch-1.8.1+
Attachment #220749 - Flags: approval-branch-1.8.1?(peterv)
You need to log in before you can comment on or make changes to this bug.