Closed Bug 1012320 Opened 6 years ago Closed 6 years ago

Move the code that defines window.netscape into nsJSContext

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Details

Attachments

(2 files, 1 obsolete file)

Attached patch Patch v1 (obsolete) — Splinter Review
There doesn't seem to be a good reason to go through nsScriptNameSpaceManager. I tried to move it into WebIDL, but that was a pain.
Attachment #8424439 - Flags: review?(bobbyholley)
Attached patch Patch v1.1Splinter Review
Attachment #8424439 - Attachment is obsolete: true
Attachment #8424439 - Flags: review?(bobbyholley)
Attachment #8424443 - Flags: review?(bobbyholley)
Comment on attachment 8424443 [details] [diff] [review]
Patch v1.1

Review of attachment 8424443 [details] [diff] [review]:
-----------------------------------------------------------------

nsJSContext needs to go away, and I don't want to add more stuff to it. Here's what I'd request:

* Move this stuff into InitGlobalObject in nsXPConnect.
* Make it conditional WindowOrNull(global) returning non-null.
* Stop checking if the property is already defined.
* Use JS_GetObjectPrototype instead of the while() loop.
Attachment #8424443 - Flags: review?(bobbyholley) → review-
xpc::InitGlobalObject feels like a weird place for something window-specific; I was actually thinking of moving it into nsGlobalWindow later. I guess I'll do it if you feel that's the right place, though.
(In reply to :Ms2ger from comment #3)
> xpc::InitGlobalObject feels like a weird place for something
> window-specific; I was actually thinking of moving it into nsGlobalWindow
> later. I guess I'll do it if you feel that's the right place, though.

I'd also be ok with putting it in CreateNativeGlobalForInner.
Attached patch Patch v2Splinter Review
Attachment #8425712 - Flags: review?(bobbyholley)
Comment on attachment 8425712 [details] [diff] [review]
Patch v2

Review of attachment 8425712 [details] [diff] [review]:
-----------------------------------------------------------------

r=bholley with the fixes below.

::: dom/base/nsGlobalWindow.cpp
@@ +2194,5 @@
>           !Preferences::GetBool("dom.use_xbl_scopes_for_remote_xul", false);
>  }
>  
> +static bool
> +netscape_security_enablePrivilege(JSContext* cx, unsigned argc, JS::Value* vp)

Might as well call this EnablePrivilege.

@@ +2200,5 @@
> +  Telemetry::Accumulate(Telemetry::ENABLE_PRIVILEGE_EVER_CALLED, true);
> +  return xpc::EnableUniversalXPConnect(cx);
> +}
> +
> +static const JSFunctionSpec PrivilegeManager_static_methods[] = {

EnablePrivilegeSpec.

@@ +2211,5 @@
> +{
> +  JSAutoCompartment ac(aCx, aGlobal);
> +
> +  JS::Rooted<JSObject*> obj(aCx, JS_GetObjectPrototype(aCx, aGlobal));
> +  const JSClass *objectClass = JS_GetClass(obj);

As it turns out, this isn't necessary at all. You can just pass nullptr as the clasp to JS_DefineObject and it'll default to JSObject::class_.

@@ +2216,5 @@
> +
> +  obj = JS_DefineObject(aCx, aGlobal, "netscape", objectClass);
> +  NS_ENSURE_TRUE(obj, false);
> +
> +  obj = JS_DefineObject(aCx, obj, "security", objectClass);

Please add a comment somewhere in here mentioning bug 791526.

@@ +2299,5 @@
>    // Set the location information for the new global, so that tools like
>    // about:memory may use that information
>    xpc::SetLocationForGlobal(aGlobal, aURI);
>  
> +  if (!InitializeEnablePrivilege(aCx, aGlobal)) {

Let's call this "InitializeLegacyNetscapeObject".

::: dom/base/nsJSEnvironment.cpp
@@ +40,5 @@
>  #include "nsIXULRuntime.h"
>  #include "nsTextFormatter.h"
>  
>  #include "xpcpublic.h"
> +#include "xpcprivate.h" // for EnableUniversalXPConnect

This should go away.
Attachment #8425712 - Flags: review?(bobbyholley) → review+
https://hg.mozilla.org/mozilla-central/rev/5680d55b9ec6
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.