734023 - Remove language arguments from nsIScriptGlobalObject methods

Status: RESOLVED FIXED

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

Description

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

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]]

Attachment: 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 (he/him; ⌚ UTC+1/+2)]

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]]

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]]

Attachment: 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 (he/him; ⌚ UTC+1/+2)]

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)]

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]]

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]]

Attachment: 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 (he/him; ⌚ UTC+1/+2)]

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]]

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 (he/him; ⌚ UTC+1/+2)]

(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 (he/him; ⌚ UTC+1/+2)]

Feel free to leave it alone for now; I've filed bug 738380 to clean up that mess.

Comment 13

[Mark Capella [:capella]]

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]]

Attachment: 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 (he/him; ⌚ UTC+1/+2)]

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]]

Attachment: Patch (v5) Final for CHECKIN with proper review message

Comment 17

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/52825f4226e6

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/52825f4226e6