Last Comment Bug 734023 - Remove language arguments from nsIScriptGlobalObject methods
: Remove language arguments from nsIScriptGlobalObject methods
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: 738380
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-08 01:37 PST by :Ms2ger
Modified: 2012-03-24 14:06 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (25.29 KB, patch)
2012-03-18 18:06 PDT, Mark Capella [:capella]
Ms2ger: feedback+
Details | Diff | Review
Patch (v2) (25.83 KB, patch)
2012-03-19 01:22 PDT, Mark Capella [:capella]
jst: review+
Details | Diff | Review
Patch (v3) (25.84 KB, patch)
2012-03-21 23:54 PDT, Mark Capella [:capella]
no flags Details | Diff | Review
Patch (v4) (25.98 KB, patch)
2012-03-22 13:58 PDT, Mark Capella [:capella]
Ms2ger: feedback+
Details | Diff | Review
Patch (v5) Final for CHECKIN with proper review message (25.99 KB, patch)
2012-03-22 14:23 PDT, Mark Capella [:capella]
no flags Details | Diff | Review

Description :Ms2ger 2012-03-08 01:37:34 PST
In a time long gone, we "supported" non-JavaScript scripting languages. In order to support this, the methods on the nsIScriptGlobalObject interface [1] take a PRUint32 argument for the language involved. Now we've given up on DOM-agnosticism, these arguments can be removed. (If the implementations still use them, nsIProgrammingLanguage::JAVASCRIPT can be used instead.)

[1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsIScriptGlobalObject.h
Comment 1 Mark Capella [:capella] 2012-03-18 18:06:18 PDT
Created attachment 607039 [details] [diff] [review]
Patch (v1)

My c++ Classes aren't where I'd like them to be yet, so I thought this'd be a good challenge.

I removed the language PRUInt32 parms from three methods: EnsureScriptEnvironment(), GetScriptContext(), and SetScriptContext() in the nsIScriptGlobalObject.h file, then modified modules as required / attached.

The build works fine locally, and all mochitest-plain and mochitest-a11y (where I do most of my patches) pass locally except for a few that never seem to work.

Let me know if there's other tests I could run, or if I'm way off base. I think I saw a few things that might use improvement -- mark
Comment 2 :Ms2ger 2012-03-19 00:24:28 PDT
Comment on attachment 607039 [details] [diff] [review]
Patch (v1)

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

This is great, thanks! Just a few small nits:

::: content/xbl/src/nsXBLDocumentInfo.cpp
@@ +105,3 @@
>  
>    nsCOMPtr<nsIScriptContext> mScriptContext;
>    JSObject *mJSObject;    // XXX JS language rabies bigotry badness

This comment can go now, I think.

@@ +291,2 @@
>  {
>    // This impl still assumes JS

And this

::: content/xbl/src/nsXBLPrototypeHandler.cpp
@@ +302,5 @@
>    if (!boundGlobal)
>      return NS_OK;
>  
>    nsIScriptContext *boundContext =
> +    boundGlobal->GetScriptContext();

This can probably go on one line now.

::: content/xul/document/src/nsXULPrototypeDocument.cpp
@@ +761,5 @@
>    if (NS_FAILED(rv)) {
>      NS_ERROR("Failed to setup script language");
>      return NULL;
>    }
>    // Note that EnsureScriptEnvironment has validated lang_id

Look like I forgot to remove this comment when I last touched this code; could you remove it?

::: docshell/base/nsDocShell.cpp
@@ +10803,5 @@
>      win->SetDocShell(static_cast<nsIDocShell *>(this));
>  
>      // Ensure the script object is set to run javascript - other languages
>      // setup on demand.
>      // XXXmarkh - should this be setup to run the default language for this doc?

Maybe turn this comment into "Ensure the script object is set up to run script." or something like that.
Comment 3 Mark Capella [:capella] 2012-03-19 00:37:46 PDT
Quick check on the last item ... replace all three comments for one?

    win->SetDocShell(static_cast<nsIDocShell *>(this));

    // Ensure the script object is set up to run script.
    nsresult rv;
    rv = mScriptGlobal->EnsureScriptEnvironment();
    NS_ENSURE_SUCCESS(rv, rv);
Comment 4 Mark Capella [:capella] 2012-03-19 01:22:31 PDT
Created attachment 607083 [details] [diff] [review]
Patch (v2)

Nits addressed ... assumes the last one is as you wanted ... rebuilt again w/no. problems.

If review passes, let me know how to proceed ... some mentors like to move it forward from that point, others want me to do things like autoland/try and checkin-needed to finish it
Comment 5 :Ms2ger 2012-03-19 02:16:24 PDT
Comment on attachment 607083 [details] [diff] [review]
Patch (v2)

Yep, that's what I meant. I can't review this myself; asking jst.

Please set checkin-needed when the patch is ready. If nobody beats me to it, I'll land it.
Comment 6 Johnny Stenback (:jst, jst@mozilla.com) 2012-03-21 11:56:54 PDT
Comment on attachment 607083 [details] [diff] [review]
Patch (v2)

- In nsXULDocument::ExecuteScript():

     PRUint32 stid = aScript->mScriptObject.mLangID;
 
     nsresult rv;
-    rv = mScriptGlobalObject->EnsureScriptEnvironment(stid);
+    rv = mScriptGlobalObject->EnsureScriptEnvironment();
     NS_ENSURE_SUCCESS(rv, rv);
 
     nsCOMPtr<nsIScriptContext> context =
-      mScriptGlobalObject->GetScriptContext(stid);
+      mScriptGlobalObject->GetScriptContext();

Looks like you're removing all uses of stid here, so stid can go too! Maybe capture the output of compiling the changed cpp files here and see if the compiler catches any other similar cases? I didn't while reviewing, but I could've missed some.

r=jst with that, thanks for the patch!
Comment 7 Mark Capella [:capella] 2012-03-21 19:40:22 PDT
Comment on attachment 607083 [details] [diff] [review]
Patch (v2)

Well dang ... in the meantime I corrupted my mercurial queues and lost the patch ... Is there a way to re-create it other than manually so I can add the tweak that :jst requested re:stid?
Comment 8 Mark Capella [:capella] 2012-03-21 23:54:51 PDT
Created attachment 608242 [details] [diff] [review]
Patch (v3)

Todays lesson was >QIMPORT<....  :)

Here's the revised patch minus the orphaned stid VAR ... how you spotted that is amazing!

This built clean, mochitest-plain and -a11y were happy ... can we checkin-needed this or do I need to get it reviewed again?
Comment 9 :Ms2ger 2012-03-22 06:26:51 PDT
Comment on attachment 608242 [details] [diff] [review]
Patch (v3)

Can you search the patch for other 'stid' variables that are unused now? I think there's one in nsXULDocument::OnStreamComplete, in particular, but there may be others.
Comment 10 Mark Capella [:capella] 2012-03-22 06:59:47 PDT
It looks like one more can go in nsXULDocument.cpp ...

In nsScriptLoader.cpp nsScriptLoader::EvaluateScript() the block of code might be simplified ... I'll have to read it better ... it amounts to whether:

   nsCOMPtr<nsIContent> scriptContent(do_QueryInterface(aRequest->mElement))->GetScriptTypeID() always defaults to nsIProgrammingLanguage::JAVASCRIPT ...
Comment 11 :Ms2ger 2012-03-22 11:40:32 PDT
(In reply to Mark Capella [:capella] from comment #10)
> It looks like one more can go in nsXULDocument.cpp ...
> 
> In nsScriptLoader.cpp nsScriptLoader::EvaluateScript() the block of code
> might be simplified ... I'll have to read it better ... it amounts to
> whether:
> 
>    nsCOMPtr<nsIContent>
> scriptContent(do_QueryInterface(aRequest->mElement))->GetScriptTypeID()
> always defaults to nsIProgrammingLanguage::JAVASCRIPT ...

I think that's the case, but tracking it down makes my head hurt.
Comment 12 :Ms2ger 2012-03-22 11:47:48 PDT
Feel free to leave it alone for now; I've filed bug 738380 to clean up that mess.
Comment 13 Mark Capella [:capella] 2012-03-22 11:50:56 PDT
Lmao ... you read my mind re: "my brain is full" ... I already have a new build going with the first fix and NOT the second  :)
Comment 14 Mark Capella [:capella] 2012-03-22 13:58:57 PDT
Created attachment 608462 [details] [diff] [review]
Patch (v4)

Ok.... all above addressed ... built clean, run all mochitest-plain. (I left the original patch with the r+ in place.)
Comment 15 :Ms2ger 2012-03-22 14:04:07 PDT
Comment on attachment 608462 [details] [diff] [review]
Patch (v4)

Ship it. (But make sure the commit message says r=jst :))
Comment 16 Mark Capella [:capella] 2012-03-22 14:23:45 PDT
Created attachment 608476 [details] [diff] [review]
Patch (v5) Final for CHECKIN with proper review message
Comment 18 Ed Morley [:emorley] 2012-03-24 14:06:17 PDT
https://hg.mozilla.org/mozilla-central/rev/52825f4226e6

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