1237668 - remove nsSimpleUnicharStreamFactory

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nathan Froyd [:froydnj]]

nsSimpleUnicharStreamFactory is a bit of mess.  It is exposed to JavaScript, but exists solely as a singleton:

http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsUnicharInputStream.cpp#439

At least in C++-land, everybody uses it for CreateInstanceFromUTF8Stream:

http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsUnicharInputStream.cpp#422

which doesn't actually depend on any internal state of the object.  That means we have this useless class sitting around, taking up codesize and whatnot.  And addons don't even use any of this functionality anyway.

We should fix this, making the smallest amount of functionality available to C++ clients, and ditching the JS support.

1. Create a NS_NewUnicharInputStream function that is an exact copy of CreateInstanceFromUTF8Stream.  Declare it in nsUnicharInputStream.h.
2. Update current call sites of CreateInstanceFromUTF8Stream to call NS_NewUnicharInputStream instead.
3. Delete everything from nsUnicharInputStream.h except the declaration added in step 1, and delete nsSimpleUnicharStreamFactory from nsUnicharInputStream.cpp.  This will also involve deleting a few things from xpcom/build/XPCOMInit.cpp.

Comment 1

[Tyler Steiman]

Hi Nathan I would really like to work on this bug. How would you recommend testing this when working on it though?

Comment 2

[Nathan Froyd [:froydnj]]

(In reply to Tyler Steiman from comment #1)
> Hi Nathan I would really like to work on this bug. How would you recommend
> testing this when working on it though?

Cool!

I'm not sure that we have great test coverage for some of this code.  I see we have mochitests in parser/htmlparser/tests/mochitest/ which might cover the changes necessary in parser/htmlparser/nsExpatDriver.cpp.  The tests under extensions/spellcheck/ don't look particular helpful, and I don't think anything tests nsPersistentProperties.cpp.

So my recommendation would be running:

  mach mochitest parser/htmlparser/tests/mochitest/

after your changes build.  If you wanted to go the extra mile of writing tests (I think testing nsPersistentProperties would be the easiest), that would be great, but given how small this refactoring is, I'm not certain it's worth the effort at this time.

Comment 3

[u546977]

Attachment: Removed nsSimpleUnicharStreamFactory because it is no longer being used

Comment 4

[u546977]

(In reply to Wilmer Paulino [:wpaulino] from comment #3)
> Created attachment 8706065 [details] [diff] [review]
> Removed nsSimpleUnicharStreamFactory because it is no longer being used

Just realized I added the same header twice in nsUnicharInputStream.h. I'll wait for the review just in case there are any other issues.

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8706065 [details] [diff] [review]
Removed nsSimpleUnicharStreamFactory because it is no longer being used

Review of attachment 8706065 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch!  Just a few minor things to change below.  Please upload a new patch and flag me for review.

::: xpcom/io/nsUnicharInputStream.cpp
@@ +379,5 @@
>    aValidUTF8bytes = c - aBuffer;
>    aValidUTF16CodeUnits = utf16length;
>  }
>  
> +NS_IMETHODIMP NS_NewUnicharInputStream(nsIInputStream* aStreamToWrap,

This should use |nsresult| as the return type, rather than |NS_IMETHODIMP|.

::: xpcom/io/nsUnicharInputStream.h
@@ +8,5 @@
>  #define nsUnicharInputStream_h__
>  
>  #include "nsISimpleUnicharStreamFactory.h"
>  #include "nsIUnicharInputStream.h"
> +#include "nsIUnicharInputStream.h"

As you've already noted, this duplicated include should be removed.

@@ +13,2 @@
>  
> +NS_IMETHODIMP NS_NewUnicharInputStream(nsIInputStream* aStreamToWrap,

This should use |nsresult| as the return type, rather than NS_IMETHODIMP.

Comment 6

[u546977]

Attachment: Remove nsSimpleUnicharStreamFactory

Comment 7

[u546977]

Nathan, I see that you've linked a related bug in the comments. Once this goes through, I'd like to work on that as well.

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 8706684 [details] [diff] [review]
Remove nsSimpleUnicharStreamFactory

Review of attachment 8706684 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!  I'll take care of committing the patch from here!

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b91c5f6f820

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2b91c5f6f820