Remove language arguments from nsIScriptGlobalObject methods

RESOLVED FIXED in mozilla14

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Ms2ger, Assigned: capella)

Tracking

Trunk
mozilla14
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

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

Comment 1

6 years ago
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
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attachment #607039 - Flags: feedback?(Ms2ger)
(Reporter)

Comment 2

6 years ago
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.
Attachment #607039 - Flags: review?(jst)
Attachment #607039 - Flags: feedback?(Ms2ger)
Attachment #607039 - Flags: feedback+
(Assignee)

Comment 3

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

Comment 4

6 years ago
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
Attachment #607039 - Attachment is obsolete: true
Attachment #607083 - Flags: review?(Ms2ger)
Attachment #607039 - Flags: review?(jst)
(Reporter)

Comment 5

6 years ago
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.
Attachment #607083 - Flags: review?(Ms2ger) → review?(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!
Attachment #607083 - Flags: review?(jst) → review+
(Assignee)

Comment 7

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

Comment 8

6 years ago
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?
Attachment #608242 - Flags: feedback?(Ms2ger)
(Reporter)

Comment 9

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

Comment 10

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

Updated

6 years ago
Attachment #607083 - Flags: feedback?(Ms2ger)
(Assignee)

Updated

6 years ago
Attachment #608242 - Flags: feedback?(Ms2ger)
(Reporter)

Comment 11

6 years ago
(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.
(Reporter)

Updated

6 years ago
Depends on: 738380
(Reporter)

Comment 12

6 years ago
Feel free to leave it alone for now; I've filed bug 738380 to clean up that mess.
(Assignee)

Comment 13

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

Comment 14

6 years ago
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.)
Attachment #608242 - Attachment is obsolete: true
Attachment #608462 - Flags: feedback?(Ms2ger)
(Reporter)

Comment 15

6 years ago
Comment on attachment 608462 [details] [diff] [review]
Patch (v4)

Ship it. (But make sure the commit message says r=jst :))
Attachment #608462 - Flags: feedback?(Ms2ger) → feedback+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
(Assignee)

Comment 16

6 years ago
Created attachment 608476 [details] [diff] [review]
Patch (v5) Final for CHECKIN with proper review message
http://hg.mozilla.org/integration/mozilla-inbound/rev/52825f4226e6
Keywords: checkin-needed
Whiteboard: [good_first_bug][mentor=Ms2ger][lang=C++] → [good first bug][mentor=Ms2ger][lang=C++]
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/52825f4226e6
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.