326477 - Add more to nsIExpatSink/nsExpatDriver in order to support SAX

Status: RESOLVED FIXED

(Core :: XML, enhancement)

Description

[Robert Sayre]

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.

Comment 1

[Robert Sayre]

Attachment: Add some extra events to ExpatSink, to fill out SAX API

I don't think any of these will get called frequently enough to effect perf.

Comment 2

[Peter Van der Beken [:peterv]]

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.

Comment 3

[Robert Sayre]

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

Comment 4

[Peter Van der Beken [:peterv]]

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.

Comment 5

[Robert Sayre]

Attachment: add a new interface extending nsIExpatSink, and QI to it

OK, here's a patch that takes the first approach. Thanks for the clear instruction, I wasn't sure what you wanted.

Comment 6

[Robert Sayre]

Attachment: updated patch w/ new interface added

Comment 7

[Robert Sayre]

Attachment: fix idl typo

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 9

[Robert Sayre]

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

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 11

[Brendan Eich [:brendan]]

(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

Comment 12

[Robert Sayre]

Attachment: name change

Comment 13

[Peter Van der Beken [: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.

Comment 14

[Robert Sayre]

Attachment: Fix w/ peterv's comments

OK, this patch includes the IDL (forgot to cvs add it). Also sets mExtendedSink to nsnull in DidBuildModel.

Comment 15

[Peter Van der Beken [:peterv]]

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?

Comment 16

[Robert Sayre]

Attachment: patch w/ params documented

Comment 17

[Robert Sayre]

Attachment: fix JST Review Simulacrum nits

Comment 18

[Robert Sayre]

Attachment: remove extra whitespace line in xmlparse.c

Comment 19

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 20

[Robert Sayre]

Attachment: patch for the branch

Comment 21

[Robert Sayre]

Attachment: branch patch

missed one of the expat functions last time

Comment 22

[Peter Van der Beken [:peterv]]

Attachment: branch patch

Attachment 220749 [details] [diff] contains a chunk not for this bug.