Closed
Bug 1237668
Opened 9 years ago
Closed 9 years ago
remove nsSimpleUnicharStreamFactory
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: froydnj, Assigned: u546977, Mentored)
References
Details
(Whiteboard: [lang=c++])
Attachments
(1 file, 1 obsolete file)
8.13 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
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•9 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•9 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)
Attachment #8706065 -
Flags: review?(nfroyd)
(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•9 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+
Attachment #8706684 -
Flags: review?(nfroyd)
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•9 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•9 years ago
|
Attachment #8706065 -
Attachment is obsolete: true
Reporter | ||
Updated•9 years ago
|
Assignee: nobody → wilmer.paulino
Comment 10•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 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.
Description
•