738380 - Remove nsINode::GetScriptTypeID/SetScriptTypeID

Status: RESOLVED FIXED

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

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

AFAICT, GetScriptTypeID will always end up returning nsIProgrammingLanguage::JAVASCRIPT. It would be good to remove the API, and the NODE_SCRIPT_TYPE_OFFSET / NODE_SCRIPT_TYPE_MASK stuff.

Comment 1

[Mark Capella [:capella]]

Attachment: Patch (v1)

I went through and made what I thought were the required changes to get rid off GetScriptTypeID / SetScriptTypeID() as the first part of this bug request. It builds nice and clean but crashes during startup.

If I monitor the WIN Task Manager I see WERFAULT.EXE doing something briefly ... so I guess I'm getting a >cool< abend somewhere ...

I did take some liberties during the coding of the changes, particularly in my substitutions where I do things like this:

-    nsCOMPtr<nsIScriptContext> scx = GetScriptContextInternal(
-                                timeout->mScriptHandler->GetScriptTypeID());
+    nsCOMPtr<nsIScriptContext> scx =
+      GetScriptContextInternal(nsIProgrammingLanguage::JAVASCRIPT);

So I have a few things to go on ...

Comment 2

[Mark Capella [:capella]]

Comment on attachment 608978 [details] [diff] [review]
Patch (v1)

Clobber build fixed the "problem" ... I'm cancelling the feedback request and moving on to the final part of the bug request.

Comment 3

[Mark Capella [:capella]]

Attachment: Patch (v2)

K - this should do it :)

Let me know if we can go forward with this, but the build was clean, FF runs w/no weird abends, mochitest-a11y runs w/no errors, mochitest-plain gives me the usual 9 suspects but nothing new ...

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 609054 [details] [diff] [review]
Patch (v2)

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

Just a few minor comments; thanks for doing this!

::: content/base/src/nsDocument.cpp
@@ -3112,5 @@
>        rv = NS_GetScriptRuntime(aData, getter_AddRefs(runtime));
>        if (NS_FAILED(rv) || runtime == nsnull) {
>          NS_WARNING("The script-type is unknown");
> -      } else {
> -        root->SetScriptTypeID(runtime->GetScriptTypeID());

I think this entire block can go now

::: content/base/src/nsScriptLoader.cpp
@@ +920,5 @@
>    mCurrentScript = oldCurrent;
>  
>    JSContext *cx = nsnull; // Initialize this to keep GCC happy.
> +  cx = context->GetNativeContext();
> +  ::JS_BeginRequest(cx);

This can use JSAutoRequest now

::: content/events/src/nsEventListenerManager.cpp
@@ +994,5 @@
> +   if (ls->mHandlerIsString) {
> +     CompileEventHandlerInternal(ls, true, nsnull);
> +   }
> + 
> +   *vp = OBJECT_TO_JSVAL(listener->GetHandler());

Did something go wrong with the indentation here?

::: content/xul/document/src/nsXULDocument.cpp
@@ +3912,5 @@
>  
>          // While merging, set the default script language of the element to be
>          // the language from the overlay - attributes will then be correctly
>          // hooked up with the appropriate language (while child nodes ignore
>          // the default language - they have it in their proto.

This comment can go too

::: dom/base/nsGlobalWindow.cpp
@@ +9379,5 @@
>  
>      // Get the script context (a strong ref to prevent it going away)
>      // for this timeout and ensure the script language is enabled.
> +    nsCOMPtr<nsIScriptContext> scx =
> +      GetScriptContextInternal(nsIProgrammingLanguage::JAVASCRIPT);

GetContextInternal(), I think

Comment 5

[Mark Capella [:capella]]

::: content/base/src/nsDocument.cpp
I think this entire block can go now

>>> Easy enough, gone.

::: content/base/src/nsScriptLoader.cpp
This can use JSAutoRequest now

>>> Researching ...

::: content/events/src/nsEventListenerManager.cpp
Did something go wrong with the indentation here?

>>> Looks like it ... I don't recall, but it's now fixed.

::: content/xul/document/src/nsXULDocument.cpp
This comment can go too

>>> Easy enough, gone.

::: dom/base/nsGlobalWindow.cpp
@@ +9379,5 @@
> +    nsCOMPtr<nsIScriptContext> scx =
> +      GetScriptContextInternal(nsIProgrammingLanguage::JAVASCRIPT);
GetContextInternal(), I think

>>> The original code was this. I can change, but researching ...
>>>-    nsCOMPtr<nsIScriptContext> scx = GetScriptContextInternal(
>>>-                                timeout->mScriptHandler->GetScriptTypeID());

Comment 6

[Mark Capella [:capella]]

Attachment: Patch (v3)

I made the (5) changes, two cosmetic, three code... the new version builds and tests ok still, but please verify the code changes for correctness.

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 609153 [details] [diff] [review]
Patch (v3)

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

Looks good, except for this one comment:

::: content/base/src/nsScriptLoader.cpp
@@ -930,5 @@
> -    NS_ASSERTION(!::JS_IsExceptionPending(cx),
> -                 "JS_ReportPendingException wasn't called in EvaluateString");
> -  }
> -
> -  context->SetProcessingScriptTag(oldProcessingScriptTag);

You seem to be removing this call; it should stay (after the JSAutoRequest).

Comment 8

[Mark Capella [:capella]]

great! I was wondering... it was sandwiched between the Start and End in the original code... wasn't sure to drop it or move it logically to the end

Comment 9

[Mark Capella [:capella]]

Attachment: Patch (v4)

Plays it safe, re-codes, re-compiles all, mochitest-plain all pass ...

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 609165 [details] [diff] [review]
Patch (v4)

 class nsIScriptContext : public nsIScriptContextPrincipal
 {
 public:
   NS_DECLARE_STATIC_IID_ACCESSOR(NS_ISCRIPTCONTEXT_IID)
 
-  /* Get the ID of this language. */
-  virtual PRUint32 GetScriptTypeID() = 0;

You're removing a virtual function here, that means you have to generate a new uuid for this interface (NS_ISCRIPTCONTEXT_IID).

 class nsIScriptRuntime : public nsISupports
 {
 public:
   NS_DECLARE_STATIC_IID_ACCESSOR(NS_ISCRIPTRUNTIME_IID)
-  /*
-   * Return the language ID of this script language
-   */
-  virtual PRUint32 GetScriptTypeID() = 0;

Same thing here, generate a new uuid for this interface as well.

 class nsIScriptTimeoutHandler : public nsISupports
 {
 public:
   NS_DECLARE_STATIC_IID_ACCESSOR(NS_ISCRIPTTIMEOUTHANDLER_IID)
 
-  // Get the script-type (language) implementing this timeout.
-  virtual PRUint32 GetScriptTypeID() = 0;

And here.

r- based on that alone, the rest looks great! Thanks for the cleanup patch!

Comment 11

[Mark Capella [:capella]]

Attachment: Patch (v5)

Created and added new _IID 's

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 609602 [details] [diff] [review]
Patch (v5)

Looks good! But I forgot to include nsINode in the list of interfaces that need a new UUID. Fix that, and this can land!

Comment 13

[Mark Capella [:capella]]

Attachment: Patch (v6)

nsINode UUID added...

Comment 14

[Mark Capella [:capella]]

Comment on attachment 609934 [details] [diff] [review]
Patch (v6)

Good to go?

Comment 15

[Johnny Stenback (:jst)]

Comment on attachment 609934 [details] [diff] [review]
Patch (v6)

I went to land this, but a bunch of other changes that touch some of this other code happened to land in the past several days, so this no longer applies :(

I don't imagine it'd be a ton of work to update this to apply any more, and I'll gladly land a new patch, or maybe even update this myself if nobody else beats me to it.

Comment 16

[Mark Capella [:capella]]

I'll rebase it this weekend and re-post it.

Comment 17

[Mark Capella [:capella]]

Attachment: Patch (v7)

Re-based, re-built, re-tested all mochitest-plain local.

Comment 18

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 611253
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=dcabacec61aa
Try run started, revision dcabacec61aa. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=dcabacec61aa

Comment 19

[Mozilla RelEng Bot]

Try run for dcabacec61aa is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=dcabacec61aa
Results (out of 15 total builds):
    success: 15
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-dcabacec61aa

Comment 20

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Try run looked good, so I pushed to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/3eee3ceb400b

Comment 21

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/3eee3ceb400b

Comment 22

[Andrew McCreight [:mccr8]]

Hey, this is pretty cool!  I think it will let me tear out a bunch of gunk from the cycle collector.