Closed Bug 734023 Opened 8 years ago Closed 8 years ago

Remove language arguments from nsIScriptGlobalObject methods

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: Ms2ger, Assigned: capella)

References

Details

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

Attachments

(3 files, 2 obsolete files)

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
Attached patch Patch (v1) (obsolete) — Splinter Review
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)
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+
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);
Attached patch Patch (v2)Splinter Review
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)
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+
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)
Attached patch Patch (v3) (obsolete) — Splinter Review
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)
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.
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 ...
Attachment #607083 - Flags: feedback?(Ms2ger)
Attachment #608242 - Flags: feedback?(Ms2ger)
(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.
Depends on: 738380
Feel free to leave it alone for now; I've filed bug 738380 to clean up that mess.
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  :)
Attached patch Patch (v4)Splinter Review
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)
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+
Keywords: checkin-needed
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
Closed: 8 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.