Closed Bug 326477 Opened 15 years ago Closed 15 years ago

Add more to nsIExpatSink/nsExpatDriver in order to support SAX

Categories

(Core :: XML, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: sayrer, Assigned: sayrer)

References

Details

(Keywords: fixed1.8.1)

Attachments

(2 files, 10 obsolete files)

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
Attached 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
Attached 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+
Attached 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?
Attached patch patch w/ params documented (obsolete) — Splinter Review
Attachment #213074 - Attachment is obsolete: true
Attached patch fix JST Review Simulacrum nits (obsolete) — Splinter Review
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: 15 years ago
Resolution: --- → FIXED
Depends on: 323908
Attached patch patch for the branch (obsolete) — Splinter Review
Attachment #220738 - Flags: approval-branch-1.8.1?(peterv)
Attached 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)
Attached 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.