remove nsSimpleUnicharStreamFactory

RESOLVED FIXED in Firefox 46

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: froydnj, Assigned: u546977, Mentored)

Tracking

unspecified
mozilla46
Points:
---

Firefox Tracking Flags

(firefox46 fixed)

Details

(Whiteboard: [lang=c++])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
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

3 years ago
Hi Nathan I would really like to work on this bug. How would you recommend testing this when working on it though?
Flags: needinfo?(nfroyd)
(Reporter)

Comment 2

3 years ago
(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.
Flags: needinfo?(nfroyd)
(Assignee)

Comment 3

3 years ago
Created attachment 8706065 [details] [diff] [review]
Removed nsSimpleUnicharStreamFactory because it is no longer being used
Attachment #8706065 - Flags: review?(nfroyd)
(Assignee)

Comment 4

3 years ago
(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.
(Reporter)

Comment 5

3 years ago
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.
Attachment #8706065 - Flags: review?(nfroyd) → feedback+
(Reporter)

Updated

3 years ago
Blocks: 1238545
(Assignee)

Comment 6

3 years ago
Created attachment 8706684 [details] [diff] [review]
Remove nsSimpleUnicharStreamFactory
Attachment #8706684 - Flags: review?(nfroyd)
(Assignee)

Comment 7

3 years ago
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.
(Reporter)

Comment 8

3 years ago
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!
Attachment #8706684 - Flags: review?(nfroyd) → review+
(Reporter)

Updated

3 years ago
Attachment #8706065 - Attachment is obsolete: true
(Reporter)

Updated

3 years ago
Assignee: nobody → wilmer.paulino

Comment 10

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2b91c5f6f820
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.