Last Comment Bug 738380 - Remove nsINode::GetScriptTypeID/SetScriptTypeID
: Remove nsINode::GetScriptTypeID/SetScriptTypeID
Status: RESOLVED FIXED
[good first bug][mentor=Ms2ger][lang=...
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Mark Capella [:capella]
:
Mentors:
Depends on: 762182
Blocks: 734023 744103
  Show dependency treegraph
 
Reported: 2012-03-22 11:46 PDT by :Ms2ger
Modified: 2012-06-06 12:11 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (31.64 KB, patch)
2012-03-24 03:16 PDT, Mark Capella [:capella]
no flags Details | Diff | Review
Patch (v2) (37.23 KB, patch)
2012-03-24 19:08 PDT, Mark Capella [:capella]
Ms2ger: feedback+
Details | Diff | Review
Patch (v3) (37.26 KB, patch)
2012-03-25 13:27 PDT, Mark Capella [:capella]
Ms2ger: feedback+
Details | Diff | Review
Patch (v4) (37.26 KB, patch)
2012-03-25 15:51 PDT, Mark Capella [:capella]
jst: review-
Ms2ger: feedback+
Details | Diff | Review
Patch (v5) (38.61 KB, patch)
2012-03-26 20:46 PDT, Mark Capella [:capella]
jst: review+
Details | Diff | Review
Patch (v6) (39.32 KB, patch)
2012-03-27 16:08 PDT, Mark Capella [:capella]
jst: checkin-
Details | Diff | Review
Patch (v7) (36.54 KB, patch)
2012-04-01 01:36 PDT, Mark Capella [:capella]
no flags Details | Diff | Review

Description :Ms2ger 2012-03-22 11:46:37 PDT
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] 2012-03-24 03:16:27 PDT
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 ...
Comment 2 Mark Capella [:capella] 2012-03-24 06:47:30 PDT
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] 2012-03-24 19:08:31 PDT
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 ...
Comment 4 :Ms2ger 2012-03-25 02:43:43 PDT
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] 2012-03-25 03:18:12 PDT
::: 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] 2012-03-25 13:27:49 PDT
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.
Comment 7 :Ms2ger 2012-03-25 13:31:39 PDT
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] 2012-03-25 13:34:09 PDT
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] 2012-03-25 15:51:56 PDT
Created attachment 609165 [details] [diff] [review]
Patch (v4)

Plays it safe, re-codes, re-compiles all, mochitest-plain all pass ...
Comment 10 Johnny Stenback (:jst, jst@mozilla.com) 2012-03-26 11:17:47 PDT
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] 2012-03-26 20:46:34 PDT
Created attachment 609602 [details] [diff] [review]
Patch (v5)

Created and added new _IID 's
Comment 12 Johnny Stenback (:jst, jst@mozilla.com) 2012-03-27 14:43:22 PDT
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] 2012-03-27 16:08:54 PDT
Created attachment 609934 [details] [diff] [review]
Patch (v6)

nsINode UUID added...
Comment 14 Mark Capella [:capella] 2012-03-29 05:41:16 PDT
Comment on attachment 609934 [details] [diff] [review]
Patch (v6)

Good to go?
Comment 15 Johnny Stenback (:jst, jst@mozilla.com) 2012-03-31 20:20:29 PDT
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] 2012-03-31 20:29:55 PDT
I'll rebase it this weekend and re-post it.
Comment 17 Mark Capella [:capella] 2012-04-01 01:36:05 PDT
Created attachment 611253 [details] [diff] [review]
Patch (v7)

Re-based, re-built, re-tested all mochitest-plain local.
Comment 18 Mozilla RelEng Bot 2012-04-01 01:41:23 PDT
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 2012-04-01 05:31:45 PDT
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
Comment 20 :Ms2ger 2012-04-01 07:14:11 PDT
Try run looked good, so I pushed to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/3eee3ceb400b
Comment 21 Matt Brubeck (:mbrubeck) 2012-04-02 11:04:39 PDT
https://hg.mozilla.org/mozilla-central/rev/3eee3ceb400b
Comment 22 Andrew McCreight [:mccr8] 2012-04-10 14:49:00 PDT
Hey, this is pretty cool!  I think it will let me tear out a bunch of gunk from the cycle collector.

Note You need to log in before you can comment on or make changes to this bug.