Open Bug 331109 Opened 16 years ago Updated 3 years ago

Code cleanup for nsGlobalWindow.cpp

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

ASSIGNED

People

(Reporter: alfred.peng, Assigned: alfred.peng)

Details

Attachments

(2 files, 5 obsolete files)

Code cleanup: nsGlobalWindow.cpp contains many copies of code to get the current JSContext, args and argv. This code should be shared in a helper function.
Attached patch Patch v1 for trunk (obsolete) — Splinter Review
Some functions in nsGlobalWindow.cpp depend on ncc, such as "SetTimeoutOrInterval" etc. So add ncc to the JSArgs structure.

Go through the nsCOMPtr user manual. Not quite sure whether the usage in the patch is right.

The function "GetJSContext" can only return NS_OK. Maybe we can change it to return the JSContext directly.
Attachment #215665 - Flags: review?(roc)
Mostly looks good.

+  nsCOMPtr<nsIXPCNativeCallContext> mNcc;

mNcc can be a regular pointer here. Where you call
+  rv = nsContentUtils::XPConnect()->
+    GetCurrentNativeCallContext(getter_AddRefs(ncc));

you need to call a new getter method that returns a regular pointer, which you need to add in js/src/xpconnect/idl/nsIXPConnect.idl to nsIXPCNativeCallContext.

You can add "[ptr] native nsIXPCNativeCallContextPtr(nsIXPCNativeCallContext);" and have an attribute that returns that.

Please add a document comment in nsContentUtils.h. You will need to mention that the JSArgs should NOT be used beyond the lifetime of the function that calls GetJSArgs.

+   NS_ENSURE_TRUE(argv, NS_ERROR_UNEXPECTED);

This could move inside GetJSArgs.
(In reply to comment #2)
> +   NS_ENSURE_TRUE(argv, NS_ERROR_UNEXPECTED);
> 
> This could move inside GetJSArgs.
> 

For the methods "Prompt", "Open", "OpenDialog" and "Find", there is no such error handling for argv and argc. If we move this to GetJSArgs, things will be changed. Do we really need to do that?
I think doing the extra check is OK.
(In reply to comment #2)
> +  nsCOMPtr<nsIXPCNativeCallContext> mNcc;
> 
> mNcc can be a regular pointer here. Where you call
> +  rv = nsContentUtils::XPConnect()->
> +    GetCurrentNativeCallContext(getter_AddRefs(ncc));
> 
> you need to call a new getter method that returns a regular pointer, which you
> need to add in js/src/xpconnect/idl/nsIXPConnect.idl to
> nsIXPCNativeCallContext.
Why do we need another new getter method? If we don't want to use nsCOMPtr, we can call the function this way:
  rv = nsContentUtils::XPConnect()->
    GetCurrentNativeCallContext(&ncc);

If we don't use nsCOMPtr, the reference count of the return value need to be released by the call function itself. Do we need to define the variable:
  nsCOMPtr<nsIXPCNativeCallContext> ncc = dont_AddRef(args.mNcc);
instead of:
  nsIXPCNativeCallContext *ncc = args.mNcc;
to handle the reference count issue?
We want to avoid ever adding a reference count, because that is wasteful.
Attached patch Patchv2 for Trunk (obsolete) — Splinter Review
Based on roc's comments, update the patch.

After changing the IDL file, I built in the js/src/xpconnect, dom, content and layout directory. Running seamonkey will generate a core dump.

Then I rebuilt the whole seamonkey, it works. It seems that I need to build in some other directories intead of rebuilding it. Anyone could give me a hint?
Attachment #215665 - Attachment is obsolete: true
Attachment #216281 - Flags: review?(roc)
Attachment #215665 - Flags: review?(roc)
When you changed nsIXPConnect.idl, you needed to rebuild in all directories that use nsIXPConnect.idl, which is probably a lot.
Comment on attachment 216281 [details] [diff] [review]
Patchv2 for Trunk

This looks OK to me. We should get feedback from jst on the (simple) XPConnect enhancement.

After this we should look for other places that use the JS args. I know there's one in nsCanvasRenderingContext2D.
Attachment #216281 - Flags: superreview+
Attachment #216281 - Flags: review?(roc)
Attachment #216281 - Flags: review?(jst)
Attachment #216281 - Flags: review+
I found there are some other places that use the JS args. And the usage are a little bit different between them.
1. There are some functions that use do_GetService(nsIXPConnect::GetCID(), &rv) to get the XPConnect. Do we need to change them to use nsContentUtil?
2. For nsHTMLDocument::ScriptWriteCommon, the function gets ncc first to determine whether it's called by JS. If so, get the actual JS args and write the args to buffer. Otherwise, call WriteCommon directly.
3. For nsHistory::Go, nsLocation::Reload and nsPluginArray::Refresh, the functions check the argc first. If it isn't bigger than 0, don't get JSContext and argv and take the corresponding action directly.
In the above situations, it's not that appropriate to use the function getCurrentJSArgs. Or maybe we need to change the function, to check the argc value before get JSContext and argv.
Status: NEW → ASSIGNED
(In reply to comment #10)
> 1. There are some functions that use do_GetService(nsIXPConnect::GetCID(),
> &rv) to get the XPConnect. Do we need to change them to use nsContentUtil?

Yes, if they're in content/, layout/ or dom/. You can only call nsContentUtils functions from there, so don't try to add callers to GetJSArgs from other places.

> 2. For nsHTMLDocument::ScriptWriteCommon, the function gets ncc first to
> determine whether it's called by JS. If so, get the actual JS args and write
> the args to buffer. Otherwise, call WriteCommon directly.

This can just call GetJSArgs and check for failure; if it fails we're not called from JS.

> 3. For nsHistory::Go, nsLocation::Reload and nsPluginArray::Refresh, the
> functions check the argc first. If it isn't bigger than 0, don't get
> JSContext and argv and take the corresponding action directly.
> In the above situations, it's not that appropriate to use the function
> getCurrentJSArgs. Or maybe we need to change the function, to check the argc
> value before get JSContext and argv.

Checking the argc first is not important. These functions do not get called often so it doesn't save any significant time. These can call JSArgs.
(In reply to comment #6)
> We want to avoid ever adding a reference count, because that is wasteful.

I'm all for decomtaminating code in favor of faster and smaller code where it matters, but this seems a bit over the top to me. Or is bumping the refcount on a callcontext more expensive than it seems (seems like it uses a normal impl of AddRef/Release, no magic).

If we truly need this, we at least need to change the IID for nsIXPCNativeCallContext.
(In reply to comment #12)
> (In reply to comment #6)
> > We want to avoid ever adding a reference count, because that is wasteful.
> 
> I'm all for decomtaminating code in favor of faster and smaller code where it
> matters, but this seems a bit over the top to me. Or is bumping the refcount
> on a callcontext more expensive than it seems (seems like it uses a normal
> impl of AddRef/Release, no magic).

There's also the codesize benefit of not having to release it anywhere.

But how about we just make nsXPConnect::GetCurrentNativeCallContext not add a reference, and redefine XPCCallContext::AddRef and Release to not bump the refcount? The refcount is meaingless anyway, it does not control the object's lifetime.

Actually, given that this is an object that only makes sense to access from C++, why is it an XPCOM object at all? Seems like we could deCOMtaminate the whole thing?
(In reply to comment #13)
> But how about we just make nsXPConnect::GetCurrentNativeCallContext not add a
> reference, and redefine XPCCallContext::AddRef and Release to not bump the
> refcount? The refcount is meaingless anyway, it does not control the object's
> lifetime.

Roc, you mean the caller needn't take care of the refcount? Do we have any chance to use a released pointer in this case?

> Actually, given that this is an object that only makes sense to access from
> C++, why is it an XPCOM object at all? Seems like we could deCOMtaminate the
> whole thing?

There seems to have something undecided yet. Maybe I need to wait for jst's opinion to go ahead.
Yes, we're waiting for jst's feedback.
(In reply to comment #13)
> There's also the codesize benefit of not having to release it anywhere.
> 
> But how about we just make nsXPConnect::GetCurrentNativeCallContext not add a
> reference, and redefine XPCCallContext::AddRef and Release to not bump the
> refcount? The refcount is meaingless anyway, it does not control the object's
> lifetime.
> 
> Actually, given that this is an object that only makes sense to access from
> C++, why is it an XPCOM object at all? Seems like we could deCOMtaminate the
> whole thing?

Either approach seems reasonable to me. If we end up deCOMtaminating the whole thing, we maybe want to consider leaving the XPCOM interface in, and just supply deCOMtaminated versions of the needed methods, depending on how nice we want to be to potential outside users of nsIXPConnect and nsIXPCNativeCallContext.
For the first approach, it's not a big change. To redefine AddRef and Release means that we don't need the MACRO NS_DECL_ISUPPORTS and make our own definition, is that right? I think that breaks the basic XPCOM rules. So we come to the second approach. If the performance between XPCOM and Class is a big difference, to deCOMtaminate the whole thing should be a good idea. But it maybe influence some other users.

roc, what't your idea?
(In reply to comment #17)
> For the first approach, it's not a big change. To redefine AddRef and Release
> means that we don't need the MACRO NS_DECL_ISUPPORTS and make our own
> definition, is that right? I think that breaks the basic XPCOM rules.

No, it's OK.

> So we
> come to the second approach. If the performance between XPCOM and Class is a
> big difference, to deCOMtaminate the whole thing should be a good idea. But it
> maybe influence some other users.
> 
> roc, what't your idea?

I think we should deCOMtaminate it. I think the new class should be called nsXPCNativeCallContext. All the attributes should be exposed as inline getter methods returning direct results. GetCallee should return a non-addrefed pointer. nsIXPConnect will have a new attribute to return the current native call context as an nsXPCNativeCallContext* (not addrefed). To maintain compatibility, nsXPCNativeCallContext should still implement nsIXPCNativeCallContext and provide the XPCOM methods; its implementation of AddRef and Release should do nothing (and return a refcount of 1).
I've worked on some parts of the code and was confused by some problems. I'd like to make them clear before I go ahead.

In my opinion, to deCOMtaminate XPCCallContext means we need to replace XPCCallContext with the new class "nsXPCNativeCallContext" to avoid the reference count issue. Since the object only makes sense to access from C++, can we just make the new class to provide the similar methods as XPCCallContext, and get rid of the XPCOM methods?

Another problem: Do we need to provide two different versions of GetCallee in the new class: 1) return the pointer directly; 2) return the pointer by argument if the new class still implements nsIXPCNativeCallContext?

All the inline getter(setter) methods will be put in xpcinline.h. And I'll make a new file to implement the ctor and dtor for nsXPCNativeCallContext.

Please correct me if I'm wrong. Thanks.
(In reply to comment #19)
> In my opinion, to deCOMtaminate XPCCallContext means we need to replace
> XPCCallContext with the new class "nsXPCNativeCallContext" to avoid the
> reference count issue. Since the object only makes sense to access from C++,
> can we just make the new class to provide the similar methods as
> XPCCallContext, and get rid of the XPCOM methods?

No, because there may be C++ XPCOM components that use nsIXPCCallContext that aren't in our tree.

> Another problem: Do we need to provide two different versions of GetCallee in
> the new class: 1) return the pointer directly; 2) return the pointer by
> argument if the new class still implements nsIXPCNativeCallContext?

Yes.

> All the inline getter(setter) methods will be put in xpcinline.h. And I'll make
> a new file to implement the ctor and dtor for nsXPCNativeCallContext.

not sure how this will look, but we can look at a patch
Attached patch Part of the patch v3 (obsolete) — Splinter Review
The definition of the new class "nsXPCNativeCallContext".

The XPCCallContext has already made some methods inline, so make some similiar ones for the new class. All the calls to the methods in XPCCallContext will be replaced by the methods in the new class.

This is just part of the patch. The rest still has some problem and I'll investigate further.

For the rest of the patch, I've made the following change to use the new class in my workspace.
1. replace all the XPCCallContext methods call by the new ones in the js/src/xpconnect/src directory.
2. replace GetCallContext with GetNativeCallContext.
3. add the new attribute to nsIXPConnect, update the uuid.
4. implement the getter(GetCurrentXPCNativeCallContext) by calling XPCPerThreadData::GetNativeCallContext() without adding reference.
5. update nsGlobalWindow.cpp/nsContentUtils.cpp/nsContentUtils.h according to the patch v2.

Please notify me if the patch is in the wrong direction.
Attachment #216281 - Attachment is obsolete: true
Attachment #216281 - Flags: review?(jst)
Attached patch Patch v4 part1 (obsolete) — Splinter Review
The patch doesn't contain the windows part. I'll post it later on.

Some issues:
1. The file xpccallcontext.cpp will be removed.
2. If we want to get rid of the interface nsIXPCNativeCallContext, maybe we should export the declaration of class nsXPCNativeCallContext.
3. Some methods in class nsXPCNativeCallContext aren't called by any callers. Should we remove them? For example: SetMethodIndex/GetContextPopRequired/GetCallerLanguage...
Attachment #230442 - Flags: review?(roc)
Attachment #229820 - Attachment is obsolete: true
Attached patch Patch v4 part2(Windows) (obsolete) — Splinter Review
No big change for Windows part. Just some class name replacement.
Attachment #230738 - Flags: review?(roc)
These should be reviewed by jst.

Please separate the JSArgs changes out into their own patch.
Sorry for the late response for the vacation.

DeCOMtaminate XPCCallContext, replace it with new class "nsXPCNativeCallContext" to avoid the reference count.
Attachment #230442 - Attachment is obsolete: true
Attachment #230738 - Attachment is obsolete: true
Attachment #235748 - Flags: review?(jst)
Attachment #230442 - Flags: review?(roc)
Attachment #230738 - Flags: review?(roc)
The JSArgs part for the codesize benefit.
Attachment #235749 - Flags: review?(jst)
QA Contact: ian → general
From the changes in part 2, only the code in nsJSTimeoutHandler still uses this stuff. Wontfix?
Comment on attachment 235748 [details] [diff] [review]
Patch v5 part1(the new class)

Clearing out old reviews. If this is still relevant, please re-request review for this patch.
Attachment #235748 - Flags: review?(jst)
Comment on attachment 235749 [details] [diff] [review]
Patch v5 part2(JSArgs)

Clearing out old reviews. If this is still relevant, please re-request review for this patch.
Attachment #235749 - Flags: review?(jst)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.