Closed Bug 734023 Opened 8 years ago Closed 8 years ago
Remove language arguments from ns
IScript Global Object methods
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)
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);
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 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?
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 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.
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 :)
Ok.... all above addressed ... built clean, run all mochitest-plain. (I left the original patch with the r+ in place.)
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+
Whiteboard: [good_first_bug][mentor=Ms2ger][lang=C++] → [good first bug][mentor=Ms2ger][lang=C++]
Target Milestone: --- → mozilla14
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.