Last Comment Bug 310590 - Expose scriptable nsIINIParser wrapper for nsINIParser
: Expose scriptable nsIINIParser wrapper for nsINIParser
Status: RESOLVED FIXED
: fixed1.8
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: ---
Assigned To: Benjamin Smedberg [:bsmedberg]
:
Mentors:
Depends on:
Blocks: 300139
  Show dependency treegraph
 
Reported: 2005-09-30 09:57 PDT by Benjamin Smedberg [:bsmedberg]
Modified: 2005-10-11 03:16 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
expose nsIINIPaser (14.96 KB, patch)
2005-09-30 10:13 PDT, Benjamin Smedberg [:bsmedberg]
darin.moz: review+
Details | Diff | Splinter Review

Description Benjamin Smedberg [:bsmedberg] 2005-09-30 09:57:10 PDT
 
Comment 1 Benjamin Smedberg [:bsmedberg] 2005-09-30 10:13:45 PDT
Created attachment 198013 [details] [diff] [review]
expose nsIINIPaser
Comment 2 Darin Fisher 2005-09-30 11:44:18 PDT
Comment on attachment 198013 [details] [diff] [review]
expose nsIINIPaser

>Index: xpcom/build/nsXPComInit.cpp

>     if (registrar) {
>         for (int i = 0; i < components_length; i++)
>             RegisterGenericFactory(registrar, &components[i]);
>+
>+        nsCOMPtr<nsIFactory> iniParserFactory(new nsINIParserFactory());
>+        if (iniParserFactory)
>+            registrar->RegisterFactory(kINIParserFactoryCID, 
>+                                       "nsINIParserFactory",
>+                                       NS_INIPARSERFACTORY_CONTRACTID, 
>+                                       iniParserFactory);
>     }

Why this special case for the INI parser?  Why not register it the
same way as all the other built-in XPCOM components?


>Index: xpcom/ds/nsIINIParser.idl

>+[scriptable, uuid(7eb955f6-3e78-4d39-b72f-c1bf12a94bce)]
>+interface nsIINIParser : nsISupports
>+{
>+  /**
>+   * Enumerates the [section]s available in the INI file.
>+   */
>+  nsIUTF8StringEnumerator getSections();
>+
>+  /**
>+   * Enumerates the keys available within a section.
>+   */
>+  nsIUTF8StringEnumerator getKeys(in AUTF8String aSection);
>+
>+  /**
>+   * Get the value of a string for a particular section and key.
>+   */
>+  AUTF8String getString(in AUTF8String aSection, in AUTF8String aKey);
>+};
>+
>+[scriptable, uuid(ccae7ea5-1218-4b51-aecb-c2d8ecd46af9)]
>+interface nsIINIParserFactory : nsISupports
>+{
>+  /**
>+   * Create an iniparser instance from a local file.
>+   */
>+  nsIINIParser createINIParser(in nsILocalFile aINIFile);
>+};

Why not just have an init method on nsIINIParser?  The extra interface
for the factory just seems unnecessary to me.  Also, if this is meant to
be mainly used from script, then perhaps it should use AString instead
of AUTF8String.


>Index: xpcom/ds/nsINIParserImpl.cpp

>+NS_IMPL_ISUPPORTS2(nsINIParserFactory,
>+		   nsIINIParserFactory,
>+		   nsIFactory)

nit: indentation


>+NS_IMETHODIMP
>+nsINIParserFactory::CreateINIParser(nsILocalFile* aINIFile,
>+				    nsIINIParser* *aResult)

nit: indentation

>+  nsCOMPtr<nsINIParserImpl> p(new nsINIParserImpl());
>+  if (!p)
>+    return NS_ERROR_OUT_OF_MEMORY;
>+
>+  nsresult rv = p->Init(aINIFile);
>+
>+  if (NS_SUCCEEDED(rv))
>+    NS_ADDREF(*aResult = p);

you could use .swap here to save an AddRef call.


>+NS_IMETHODIMP
>+nsINIParserFactory::CreateInstance(nsISupports* aOuter,
>+				   REFNSIID aIID,
>+				   void **aResult)
>+{
>+  NS_ENSURE_NO_AGGREGATION(aOuter);
>+
>+  // We are our own singleton.
>+  return QueryInterface(aIID, aResult);
>+}
>+
>+NS_IMETHODIMP
>+nsINIParserFactory::LockFactory(PRBool aLock)
>+{
>+  return NS_OK;
>+}
>+

yeah, generic factory is more overhead than this, but it seems
like more headache to split from the normal way of dealing with
factories.


>+NS_IMPL_ISUPPORTS1(nsINIParserImpl,
>+		   nsIINIParser)

nit: whitespace


>+NS_IMETHODIMP
>+nsINIParserImpl::GetSections(nsIUTF8StringEnumerator* *aResult)
>+{
>+  nsCStringArray *strings = new nsCStringArray;
>+  if (!strings)
>+    return NS_ERROR_OUT_OF_MEMORY;
>+
>+  nsresult rv = mParser.GetSections(SectionCB, strings);
>+  if (NS_SUCCEEDED(rv))
>+    rv = NS_NewUTF8StringEnumerator(aResult, strings);

hmm... does it make sense to cache the nsCStringArray as a
member variable?

you could have a nsCStringArray member variable that is not
a pointer, and then you could use the aOwner version of the
NS_NewUTF8StringEnumerator function.


>+NS_IMETHODIMP
>+nsINIParserImpl::GetKeys(const nsACString& aSection,
>+			 nsIUTF8StringEnumerator* *aResult)
>+{
>+  nsCStringArray *strings = new nsCStringArray;
>+  if (!strings)
>+    return NS_ERROR_OUT_OF_MEMORY;
>+
>+  nsresult rv = mParser.GetStrings(PromiseFlatCString(aSection).get(),
>+				   KeyCB, strings);
>+  if (NS_SUCCEEDED(rv))
>+    rv = NS_NewUTF8StringEnumerator(aResult, strings);

oh, ic... if you cache the sections, then you wouldn't necessarily
want to cache the keys... or would you?  that would require a hash
table... hrm... not so much fun.  maybe optimizing this is not worth
it.


>Index: xpcom/ds/nsINIParserImpl.h

>+#include "nsIINIParser.h"
>+
>+#include "nsIFactory.h"

nit: kill this newline :)
Comment 3 Benjamin Smedberg [:bsmedberg] 2005-09-30 11:53:07 PDT
> Why this special case for the INI parser?  Why not register it the
> same way as all the other built-in XPCOM components?

Really I think most services ought to be registered this way, but beside that
you can get the factory object directly without having to go through the
getservice call.

> Why not just have an init method on nsIINIParser?  The extra interface

I hate COM init methods. I don't think in general we should be handing out
objects that aren't ready-to-use. See bug 295430.

> be mainly used from script, then perhaps it should use AString instead
> of AUTF8String.

Then I'd have to do utf16->utf8 conversions, which xpconnect can do for me.

> you could use .swap here to save an AddRef call.

If you use swap you have to null *aResult anyway because COM doesn't guarantee
that's it's null coming in.
Comment 4 Darin Fisher 2005-09-30 14:05:08 PDT
(In reply to comment #3)
> > Why this special case for the INI parser?  Why not register it the
> > same way as all the other built-in XPCOM components?
> 
> Really I think most services ought to be registered this way, but beside that
> you can get the factory object directly without having to go through the
> getservice call.

I think this way of registering is "nice" because you wanted to use
getClassObject to expose the factory interface that you've created.


> > Why not just have an init method on nsIINIParser?  The extra interface
> 
> I hate COM init methods. I don't think in general we should be handing out
> objects that aren't ready-to-use. See bug 295430.

An init method has the advantage of allowing the object to be re-used.  Perhaps
that does not outweigh the advantages of being able to ensure that an object is
never in an uninitialized state.  nsIScriptableInputStream.init is nice because
it allows you to reuse the same scriptable input stream object, which does come
in handy.  See connection-xpcom.js in chatzilla, for example.

Anyways, init methods are quite common in our codebase.  There's something to be
said about convention and consistency, but of course there are times when the
conventions aren't so hot.


> > be mainly used from script, then perhaps it should use AString instead
> > of AUTF8String.
> 
> Then I'd have to do utf16->utf8 conversions, which xpconnect can do for me.

Right, I know... but your nsStringArray would then hold ref-counted UTF-16
buffers, which would be shared by JavaScript.  So, if you were to ever optimize
this code to cache the arrays, then there'd be some advantage to doing the
charset conversion once as well.


> > you could use .swap here to save an AddRef call.
> 
> If you use swap you have to null *aResult anyway because COM doesn't guarantee
> that's it's null coming in.

Nulling *aResult is a lot more efficient than calling AddRef! ;-)
Comment 5 Darin Fisher 2005-09-30 18:09:20 PDT
bsmedberg: also, why did you decide to return enumerators instead of having the
consumer specify a callback?  the enumerator interface seems to force you into
having to build up these arrays, which could be rather large.  obviously, the
nsINIParser implementation could be modified to return an efficient enumerator
if we found that it was needed.  hmm.
Comment 6 Benjamin Smedberg [:bsmedberg] 2005-10-03 07:10:28 PDT
Comment on attachment 198013 [details] [diff] [review]
expose nsIINIPaser

I can't use .swap() because the it's a nsCOMPtr<nsINIParserImpl> and the result
is nsIINIParser**

nsINIParser (the C++ class) cannot be reinitialized.

I don't ever envision this code being in a critical codepath so that I would
need to cache the string arrays, and caching them introduces significant extra
codesize+complexity.
Comment 7 Benjamin Smedberg [:bsmedberg] 2005-10-03 07:18:26 PDT
And as for the callback, I would love generic visitor interfaces in XPCOM

interface nsIUTF8StringVisitor : nsISupports
{
  void visitString(AUTF8String aString);
};

interface nsISupportsVisitor : nsISupports
{
  void visitSupports(nsISupports aSupports);
};

And we could use this interfaces for nsIArray visiting as well. But I don't want
to do that now, as I would like to get a form of this patch landed on trunk
(under xpcom/) and on 1.8 branch (under xulrunner/).
Comment 8 Darin Fisher 2005-10-03 18:53:10 PDT
(In reply to comment #7)
> And as for the callback, I would love generic visitor interfaces in XPCOM
> 
> interface nsIUTF8StringVisitor : nsISupports
> {
>   void visitString(AUTF8String aString);
> };
> 
> interface nsISupportsVisitor : nsISupports
> {
>   void visitSupports(nsISupports aSupports);
> };

Yes, we should define those interfaces post-1.8


> And we could use this interfaces for nsIArray visiting as well. But I don't 
> want to do that now, as I would like to get a form of this patch landed on 
> trunk (under xpcom/) and on 1.8 branch (under xulrunner/).

OK.  Like I said before, we could always implement nsIINIParser using a more
efficient back-end in the future.
Comment 9 Darin Fisher 2005-10-03 19:07:09 PDT
It's too bad the Components.classes['...'] object does not QI to nsIFactory and
any other interfaces supported by the identified class object.
Comment 10 Darin Fisher 2005-10-03 19:07:49 PDT
Comment on attachment 198013 [details] [diff] [review]
expose nsIINIPaser

r=darin with nits picked
Comment 11 Benjamin Smedberg [:bsmedberg] 2005-10-04 10:21:47 PDT
Fixed on trunk. I'm going to land identical code in mozilla/xulrunner/utils on
the 1.8 branch (except for build-fu of course).
Comment 12 Benjamin Smedberg [:bsmedberg] 2005-10-07 08:34:27 PDT
Fixed xulrunner-only for 1.8.

Note You need to log in before you can comment on or make changes to this bug.