deCOMtaminate nsINameSpaceManager: part 2

RESOLVED FIXED in 1.4 S2 (28feb)

Status

()

defect
RESOLVED FIXED
11 years ago
3 months ago

People

(Reporter: taras.mozilla, Assigned: reuben)

Tracking

(Blocks 1 bug)

Trunk
1.4 S2 (28feb)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [systemsfe])

Attachments

(5 attachments, 5 obsolete attachments)

Reporter

Description

11 years ago
rename nsINameSpaceManager and change the class to not inherit from nsISupports and s/nsCOMPtr/nsRefPtr/ for pointers containing it, etc

Updated

10 years ago
Component: Content → DOM
QA Contact: content → general

Comment 1

10 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

10 years ago
Posted patch basic patch (obsolete) — Splinter Review
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

10 years ago
Attachment #414287 - Attachment description: Made the basic changes described in the bug. This has caused problems with do_GetService. → basic patch
Blocks: deCOM
Assignee

Comment 4

6 years ago
Assignee: nobody → reuben.bmo
Assignee

Comment 6

6 years ago
I tried to keep things sorted, but made no attempt to fix files that had unordered includes.
Assignee

Comment 7

6 years ago
Posted patch 4 - Remove XPCOM goop (obsolete) — Splinter Review
I removed the XPCOM registration code and added a GetInstance() method for users.
Assignee

Comment 8

6 years ago
Posted patch 4 - Remove XPCOM goop (obsolete) — Splinter Review
Forgot to qref before attaching.
Attachment #8364621 - Attachment is obsolete: true
Assignee

Comment 9

6 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

6 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

6 years ago
Attachment #8364626 - Flags: review?(hsivonen)
Assignee

Updated

6 years ago
Attachment #8364615 - Flags: review?(jst)
Assignee

Updated

6 years ago
Attachment #8364616 - Flags: review?(jst)
Assignee

Updated

6 years ago
Attachment #8364619 - Flags: review?(jst)
Assignee

Comment 11

6 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 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 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

6 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-
Attachment #8364615 - Flags: review?(jst) → review+
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 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 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 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+
Whiteboard: [systemsfe]
Target Milestone: --- → 1.4 S1 (14feb)
Target Milestone: 1.4 S1 (14feb) → 1.4 S2 (28feb)
Assignee

Comment 22

5 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

5 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+
Attachment #8381075 - Flags: review?(hsivonen) → review+
Attachment #8381079 - Flags: review?(hsivonen) → review+
Assignee

Updated

5 years ago
Flags: needinfo?
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.