Remove nsINode::GetScriptTypeID/SetScriptTypeID

RESOLVED FIXED in mozilla14

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Ms2ger, Assigned: capella)

Tracking

Trunk
mozilla14
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=Ms2ger][lang=c++])

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
(Assignee)

Comment 1

5 years ago
Created attachment 608978 [details] [diff] [review]
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 ...
Attachment #608978 - Flags: feedback?(Ms2ger)
(Assignee)

Comment 2

5 years ago
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.
Attachment #608978 - Flags: feedback?(Ms2ger)
(Assignee)

Comment 3

5 years ago
Created attachment 609054 [details] [diff] [review]
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 ...
Attachment #608978 - Attachment is obsolete: true
Attachment #609054 - Flags: feedback?(Ms2ger)
(Reporter)

Comment 4

5 years ago
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
Attachment #609054 - Flags: feedback?(Ms2ger) → feedback+
(Assignee)

Comment 5

5 years ago
::: 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());
(Assignee)

Comment 6

5 years ago
Created attachment 609153 [details] [diff] [review]
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.
Attachment #609054 - Attachment is obsolete: true
Attachment #609153 - Flags: feedback?(Ms2ger)
(Reporter)

Comment 7

5 years ago
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).
Attachment #609153 - Flags: feedback?(Ms2ger) → feedback+
(Assignee)

Comment 8

5 years ago
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
(Assignee)

Comment 9

5 years ago
Created attachment 609165 [details] [diff] [review]
Patch (v4)

Plays it safe, re-codes, re-compiles all, mochitest-plain all pass ...
Attachment #609153 - Attachment is obsolete: true
Attachment #609165 - Flags: feedback?(Ms2ger)
(Reporter)

Updated

5 years ago
Attachment #609165 - Flags: feedback?(Ms2ger) → feedback+
(Assignee)

Updated

5 years ago
Attachment #609165 - Flags: review?(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!
Attachment #609165 - Flags: review?(jst) → review-
(Assignee)

Comment 11

5 years ago
Created attachment 609602 [details] [diff] [review]
Patch (v5)

Created and added new _IID 's
Attachment #609165 - Attachment is obsolete: true
Attachment #609602 - Flags: review?(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!
Attachment #609602 - Flags: review?(jst) → review+
(Assignee)

Comment 13

5 years ago
Created attachment 609934 [details] [diff] [review]
Patch (v6)

nsINode UUID added...
(Assignee)

Comment 14

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

Good to go?
Attachment #609934 - Flags: checkin?(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.
Attachment #609934 - Flags: checkin?(jst) → checkin-
(Assignee)

Comment 16

5 years ago
I'll rebase it this weekend and re-post it.
(Assignee)

Comment 17

5 years ago
Created attachment 611253 [details] [diff] [review]
Patch (v7)

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

Updated

5 years ago
Whiteboard: [good first bug][mentor=Ms2ger][lang=c++] → [autoland-try:611253][good first bug][mentor=Ms2ger][lang=c++]

Updated

5 years ago
Whiteboard: [autoland-try:611253][good first bug][mentor=Ms2ger][lang=c++] → [autoland-in-queue][good first bug][mentor=Ms2ger][lang=c++]

Comment 18

5 years ago
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

5 years ago
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/autolanduser@mozilla.com-dcabacec61aa

Updated

5 years ago
Whiteboard: [autoland-in-queue][good first bug][mentor=Ms2ger][lang=c++] → [good first bug][mentor=Ms2ger][lang=c++]
(Reporter)

Comment 20

5 years ago
Try run looked good, so I pushed to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/3eee3ceb400b
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/3eee3ceb400b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Hey, this is pretty cool!  I think it will let me tear out a bunch of gunk from the cycle collector.
Blocks: 744103
Depends on: 762182
You need to log in before you can comment on or make changes to this bug.