Closed
Bug 458300
Opened 16 years ago
Closed 11 years ago
deCOMtaminate nsINameSpaceManager: part 2
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
1.4 S2 (28feb)
People
(Reporter: taras.mozilla, Assigned: reuben)
References
(Blocks 1 open bug)
Details
(Whiteboard: [systemsfe])
Attachments
(5 files, 5 obsolete files)
4.36 KB,
patch
|
jst
:
review+
|
Details | Diff | Splinter Review |
88.08 KB,
patch
|
jst
:
review+
|
Details | Diff | Splinter Review |
2.40 KB,
patch
|
hsivonen
:
review+
|
Details | Diff | Splinter Review |
23.98 KB,
patch
|
reuben
:
review+
|
Details | Diff | Splinter Review |
6.46 KB,
patch
|
hsivonen
:
review+
|
Details | Diff | Splinter Review |
rename nsINameSpaceManager and change the class to not inherit from nsISupports and s/nsCOMPtr/nsRefPtr/ for pointers containing it, etc
Comment 1•15 years ago
|
||
I'm looking at this bug, and I understand why s/nsCOMPtr/nsRefPtr is necessary. What else would be required for this bug? Are there calls to methods inherited from nsISupports that would need to be removed?
Comment 2•15 years ago
|
||
After making the basic changes described here, I am having trouble with do_GetService. Where it used to be possible to assign the resulting nsGetServiceByContractID directly into a nsCOMPtr, this does not seem to work for nsRefPtr. I know that for other assignments in to ncCOMPtr, changing to nsRefPtr requires adding a call to AddRef, or creating a new instance. However, I don't think that will work here.
I'm not sure how to extract a raw pointer to a nsINameSpaceManager from a nsGetServiceByContractID.
Attachment #414287 -
Flags: review-
Updated•15 years ago
|
Attachment #414287 -
Attachment description: Made the basic changes described in the bug. This has caused problems with do_GetService. → basic patch
Comment 3•15 years ago
|
||
Assignee | ||
Comment 4•11 years ago
|
||
Assignee: nobody → reuben.bmo
Assignee | ||
Comment 5•11 years ago
|
||
Assignee | ||
Comment 6•11 years ago
|
||
I tried to keep things sorted, but made no attempt to fix files that had unordered includes.
Assignee | ||
Comment 7•11 years ago
|
||
I removed the XPCOM registration code and added a GetInstance() method for users.
Assignee | ||
Comment 8•11 years ago
|
||
Forgot to qref before attaching.
Attachment #8364621 -
Attachment is obsolete: true
Assignee | ||
Comment 9•11 years ago
|
||
Those files have big "DO NOT EDIT, GENERATED" warnings, but I couldn't find the code that emits the boilerplate, so I assumed that part isn't generated. It's just a search and replace so I'm attaching this version anyway.
Assignee | ||
Comment 10•11 years ago
|
||
I had a green Try run for this code, but since I cleaned up some of this stuff and split it into smaller patches, here's a new run:
https://tbpl.mozilla.org/?tree=Try&rev=6178e929bee5
(old run is here: https://tbpl.mozilla.org/?tree=Try&rev=e68a581687df)
Assignee | ||
Updated•11 years ago
|
Attachment #8364626 -
Flags: review?(hsivonen)
Assignee | ||
Updated•11 years ago
|
Attachment #8364615 -
Flags: review?(jst)
Assignee | ||
Updated•11 years ago
|
Attachment #8364616 -
Flags: review?(jst)
Assignee | ||
Updated•11 years ago
|
Attachment #8364619 -
Flags: review?(jst)
Assignee | ||
Comment 11•11 years ago
|
||
Comment on attachment 8364623 [details] [diff] [review]
4 - Remove XPCOM goop
Flagging you on these because you reviewed bug 455595, feel free to redirect. Patches 1 and 3 are mechanical changes, mostly need a rubberstamp.
Attachment #8364623 -
Flags: review?(jst)
Comment 12•11 years ago
|
||
Comment on attachment 8364626 [details] [diff] [review]
0 - Remove unneeded includes in the HTML5 parser
Review of attachment 8364626 [details] [diff] [review]:
-----------------------------------------------------------------
::: parser/html/nsHtml5AttributeName.h
@@ +26,1 @@
> */
If you scroll up a little bit further, you'll see
* THIS IS A GENERATED FILE. PLEASE DO NOT EDIT.
Comment 13•11 years ago
|
||
Comment on attachment 8364623 [details] [diff] [review]
4 - Remove XPCOM goop
Review of attachment 8364623 [details] [diff] [review]:
-----------------------------------------------------------------
::: content/base/public/moz.build
@@ +58,5 @@
> 'nsINodeInfo.h',
> 'nsINodeList.h',
> 'nsIScriptElement.h',
> 'nsIStyleSheetLinkingElement.h',
> + 'nsLineBreaker.h',
Wrong patch
::: content/base/src/nsNameSpaceManager.cpp
@@ +37,5 @@
>
> +StaticAutoPtr<nsNameSpaceManager> nsNameSpaceManager::sInstance;
> +
> +/* static */
> +nsNameSpaceManager* nsNameSpaceManager::GetInstance() {
/* static */ nsNameSpaceManager*
nsNameSpaceManager::GetInstance()
{
@@ +44,5 @@
> + if (sInstance->Init()) {
> + ClearOnShutdown(&sInstance);
> + } else {
> + delete sInstance;
> + sInstance = nullptr;
Looks like a double free to me.
Comment on attachment 8364626 [details] [diff] [review]
0 - Remove unneeded includes in the HTML5 parser
This is generated code, so it would be good to identify what pattern in the generator is useless. It seems like an accident on my part that the generator puts the same set of includes in both foo.h and foo.cpp that anyway includes foo.h. However, it's intentional for simplicity that except for the tree builder and the tokenizer, all the generated files have the same set of includes.
Will the nsINameSpaceManager.h header continue to be need to be included in .cpp files under parser/html/? Which ones?
Assignee | ||
Comment 15•11 years ago
|
||
(In reply to Henri Sivonen (:hsivonen) from comment #14)
> Will the nsINameSpaceManager.h header continue to be need to be included in
> .cpp files under parser/html/? Which ones?
Yes, the ones touched by patch 3:
parser/html/nsHtml5AttributeName.cpp
parser/html/nsHtml5ElementName.cpp
parser/html/nsHtml5HtmlAttributes.cpp
parser/html/nsHtml5MetaScanner.cpp
parser/html/nsHtml5StackNode.cpp
parser/html/nsHtml5StateSnapshot.cpp
parser/html/nsHtml5TreeBuilder.cpp
parser/html/nsHtml5UTF16Buffer.cpp
(In reply to Reuben Morais [:reuben] from comment #15)
> (In reply to Henri Sivonen (:hsivonen) from comment #14)
> > Will the nsINameSpaceManager.h header continue to be need to be included in
> > .cpp files under parser/html/? Which ones?
>
> Yes, the ones touched by patch 3:
OK. I suggest just renaming the include in the other generated files as well and filing a separate bug about the redundant includes in the generated code. Unless fixed in the generator, the changes will be overwritten in the future.
Comment on attachment 8364626 [details] [diff] [review]
0 - Remove unneeded includes in the HTML5 parser
r- for the reason stated in the previous comment.
Attachment #8364626 -
Flags: review?(hsivonen) → review-
Updated•11 years ago
|
Attachment #8364615 -
Flags: review?(jst) → review+
Comment 18•11 years ago
|
||
Comment on attachment 8364616 [details] [diff] [review]
2 - Merge nsINameSpaceManager and NameSpaceManagerImpl
- In nsContentUtils::Shutdown():
NS_IF_RELEASE(sConsoleService);
sXPConnect = nullptr;
NS_IF_RELEASE(sSecurityManager);
- NS_IF_RELEASE(sNameSpaceManager);
This causes a leak AFAICT.
The rest looks good, but r- due to the leak.
Attachment #8364616 -
Flags: review?(jst) → review-
Comment 19•11 years ago
|
||
Comment on attachment 8364619 [details] [diff] [review]
3 - Rename nsINameSpaceManager.h to nsNameSpaceManager.h
- In editor/composer/src/nsComposeTxtSrvFilter.cpp:
#include "nsError.h" // for NS_OK
#include "nsIContent.h" // for nsIContent
#include "nsIDOMNode.h" // for nsIDOMNode
-#include "nsINameSpaceManager.h" // for kNameSpaceID_None
+#include "nsNameSpaceManager.h" // for kNameSpaceID_None
Make the comment line up again, please.
- In editor/libeditor/base/nsEditor.cpp:
#include "nsIHTMLDocument.h" // for nsIHTMLDocument
#include "nsIInlineSpellChecker.h" // for nsIInlineSpellChecker, etc
#include "nsIMEStateManager.h" // for nsIMEStateManager
-#include "nsINameSpaceManager.h" // for kNameSpaceID_None, etc
+#include "nsNameSpaceManager.h" // for kNameSpaceID_None, etc
Here too...
- In editor/libeditor/html/nsHTMLEditUtils.cpp:
#include "nsIAtom.h" // for nsIAtom
#include "nsIDOMHTMLAnchorElement.h" // for nsIDOMHTMLAnchorElement
#include "nsIDOMNode.h" // for nsIDOMNode
-#include "nsINameSpaceManager.h" // for kNameSpaceID_None
+#include "nsNameSpaceManager.h" // for kNameSpaceID_None
... and here.
Attachment #8364619 -
Flags: review?(jst) → review+
Comment 20•11 years ago
|
||
Comment on attachment 8364623 [details] [diff] [review]
4 - Remove XPCOM goop
- In content/base/src/nsNameSpaceManager.cpp:
+StaticAutoPtr<nsNameSpaceManager> nsNameSpaceManager::sInstance;
Ah, this fixes the leak in the earlier patch. Please move the removal of the release of sNameSpaceManager to this patch.
r=jst with that.
Attachment #8364623 -
Flags: review?(jst) → review+
Comment 21•11 years ago
|
||
Comment on attachment 8364616 [details] [diff] [review]
2 - Merge nsINameSpaceManager and NameSpaceManagerImpl
r+ on this one too with my previous comments addressed.
Attachment #8364616 -
Flags: review- → review+
Updated•11 years ago
|
Whiteboard: [systemsfe]
Target Milestone: --- → 1.4 S1 (14feb)
Updated•11 years ago
|
Target Milestone: 1.4 S1 (14feb) → 1.4 S2 (28feb)
Assignee | ||
Comment 22•11 years ago
|
||
Doing this in this bug since it's a minor thing.
Attachment #414287 -
Attachment is obsolete: true
Attachment #8364626 -
Attachment is obsolete: true
Attachment #8381075 -
Flags: review?(hsivonen)
Assignee | ||
Comment 23•11 years ago
|
||
I merged the two patches because it's pointless to add extra code to make the tree build and work with the first patch just to remove it in the second one. Carrying r=jst.
Attachment #8364616 -
Attachment is obsolete: true
Attachment #8364623 -
Attachment is obsolete: true
Attachment #8381078 -
Flags: review+
Assignee | ||
Comment 24•11 years ago
|
||
Attachment #8381079 -
Flags: review?(hsivonen)
Attachment #8381075 -
Flags: review?(hsivonen) → review+
Attachment #8381079 -
Flags: review?(hsivonen) → review+
Assignee | ||
Comment 25•11 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/2a3ff17ca99e
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/5837f5e79047
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/1b5ee8c5491a
https://hg.mozilla.org/projects/htmlparser/rev/8987599c49b3
Flags: needinfo?
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?
Comment 26•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2a3ff17ca99e
https://hg.mozilla.org/mozilla-central/rev/5837f5e79047
https://hg.mozilla.org/mozilla-central/rev/1b5ee8c5491a
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•