Last Comment Bug 744332 - Remove nsXULPrototypeScript::ScriptObjectHolder::mLangID
: Remove nsXULPrototypeScript::ScriptObjectHolder::mLangID
Status: RESOLVED FIXED
[good first bug][mentor=Ms2ger][lang=...
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Mark Capella [:capella]
:
Mentors:
Depends on: 749367
Blocks: 744103
  Show dependency treegraph
 
Reported: 2012-04-11 00:29 PDT by :Ms2ger (⌚ UTC+1/+2)
Modified: 2012-05-03 03:48 PDT (History)
6 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) very rough (39.11 KB, patch)
2012-04-12 21:41 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Rebased patch (31.75 KB, patch)
2012-04-15 09:46 PDT, :Ms2ger (⌚ UTC+1/+2)
no flags Details | Diff | Splinter Review
Rebased Patch + JS MACRO Nit (32.52 KB, patch)
2012-04-17 00:06 PDT, Mark Capella [:capella]
jst: review+
Details | Diff | Splinter Review

Description :Ms2ger (⌚ UTC+1/+2) 2012-04-11 00:29:28 PDT
See <http://mxr.mozilla.org/mozilla-central/source/content/xul/content/src/nsXULElement.h#343>. I'm not sure if it's GC-safe to remove the ScriptObjectHolder struct entirely.

Mark, feel free to take if you feel like it, else I can have a look myself.
Comment 1 Mark Capella [:capella] 2012-04-11 00:40:27 PDT
Can't hurt to try ....
Comment 2 Andrew McCreight [:mccr8] 2012-04-11 07:42:52 PDT
I started looking at this yesterday.  It ends up unraveling into at least one or two other classes.  nsScriptObjectHolder has a getScriptTypeID method that ends up returning JAVASCRIPT all of the time.  It is a templated class, but as far as I can see, it is only safe to use when the argument is a GCable class, so maybe it should be removed entirely.  I'm not sure if that is going to be easy or not.
Comment 3 :Ms2ger (⌚ UTC+1/+2) 2012-04-11 09:41:16 PDT
(In reply to Andrew McCreight [:mccr8] from comment #2)
> I started looking at this yesterday.  It ends up unraveling into at least
> one or two other classes.  nsScriptObjectHolder has a getScriptTypeID method
> that ends up returning JAVASCRIPT all of the time.  It is a templated class,
> but as far as I can see, it is only safe to use when the argument is a
> GCable class, so maybe it should be removed entirely.  I'm not sure if that
> is going to be easy or not.

It's only templated because it used to have void*-that's-always-JSObject*, which turned into void*-that's-usually-JSObject*-but-sometimes-JSScript* (thanks, Igor!). I'd be happy to see it go, but I don't understand the GC constraints well enough to figure out how to do that.
Comment 4 Mark Capella [:capella] 2012-04-12 21:41:48 PDT
Created attachment 614678 [details] [diff] [review]
Patch (v1) very rough

Ok,, I took a great big ax and came up with this ... I haven't killed the whole ScriptObjectHolder structure ... just lost the mLangID var ...

Also, I didn't do a lot of analysis, just made sure it built and mochitest-plain ran it locally. I haven't TRY tested it yet, as I wanted a pair of eyes to reality-review the while thing before I went off too far ... there may yet be room for improvements / enhancements.

--mark
Comment 5 Andrew McCreight [:mccr8] 2012-04-15 00:15:34 PDT
Ms2ger, jst, how much should we be concerned about breaking PyXPCOM with these kinds of changes?  I'm not opposed to doing it if that's necessary to really improve the code, but I don't know if accidentally breaking it just to clean out some weirdness is necessarily good.  I looked over the PyXPCOM code before and I think it was using at least nsScriptObjectHolder.  Of course, it seems to only be updated to 10, and isn't using the new templated stuff anyways...
Comment 6 Johnny Stenback (:jst, jst@mozilla.com) 2012-04-15 01:13:26 PDT
At this point, not at all. Ant using PyXPCOM to access XPCOM objects in Mozilla is very different from executing python code in a script *element* in Mozilla. IOW, AFAIK the script type stuff doesn't matter much, if at all, to PyXPCOM.
Comment 7 Mark Capella [:capella] 2012-04-15 01:24:21 PDT
Comment on attachment 614678 [details] [diff] [review]
Patch (v1) very rough

Removing feedback for now ... Bug 742837 - Remove dead code and cleanup around nsDOMScriptObjectFactory changes have to be absorbed before I can repost
Comment 8 :Ms2ger (⌚ UTC+1/+2) 2012-04-15 09:46:27 PDT
Created attachment 615174 [details] [diff] [review]
Rebased patch

Rebased over my changes
Comment 9 Mark Capella [:capella] 2012-04-15 19:58:01 PDT
This looks good. Will you run with it from here? Make sure it tests ... get it reviewed and pushed, etc?
Comment 10 :Ms2ger (⌚ UTC+1/+2) 2012-04-16 10:00:33 PDT
Either way works for me; let me know?
Comment 11 Mark Capella [:capella] 2012-04-16 13:29:54 PDT
Re-built and tested locally ... going for TRY run...
Comment 12 Mozilla RelEng Bot 2012-04-16 13:34:41 PDT
Autoland Patchset:
	Patches: 615174
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=f23ca2aed382
Try run started, revision f23ca2aed382. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=f23ca2aed382
Comment 13 Mozilla RelEng Bot 2012-04-16 18:02:33 PDT
Try run for f23ca2aed382 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f23ca2aed382
Results (out of 15 total builds):
    success: 15
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-f23ca2aed382
Comment 14 Mark Capella [:capella] 2012-04-16 18:07:10 PDT
Comment on attachment 615174 [details] [diff] [review]
Rebased patch

TRY Builds all good ... requesting review from JST...
Comment 15 Andrew McCreight [:mccr8] 2012-04-16 18:15:25 PDT
Comment on attachment 615174 [details] [diff] [review]
Rebased patch

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

::: content/xul/content/src/nsXULElement.cpp
@@ +2573,5 @@
>      }
>      else if (tmp->mType == nsXULPrototypeNode::eType_Script) {
>          nsXULPrototypeScript *script =
>              static_cast<nsXULPrototypeScript*>(tmp);
> +        NS_IMPL_CYCLE_COLLECTION_TRACE_CALLBACK(nsIProgrammingLanguage::JAVASCRIPT,

Not a big deal, but please instead use NS_IMPL_CYCLE_COLLECTION_TRACE_JS_CALLBACK and then drop the JAVASCRIPT argument.
Comment 16 Mark Capella [:capella] 2012-04-17 00:06:40 PDT
Created attachment 615625 [details] [diff] [review]
Rebased Patch + JS MACRO Nit

Last patch plus requested JS MACRO change, Rebased against nightly, rebuilt locally, all mochitest-plain run.
Comment 17 Johnny Stenback (:jst, jst@mozilla.com) 2012-04-17 17:25:37 PDT
Comment on attachment 615625 [details] [diff] [review]
Rebased Patch + JS MACRO Nit

Looks good!
Comment 18 Andrew McCreight [:mccr8] 2012-04-18 17:40:37 PDT
This is just a refactoring that is not fixing a regression or whatnot.  I think it  can just land on birch.
Comment 19 Ryan VanderMeulen [:RyanVM] 2012-04-18 17:45:08 PDT
mccr8:	well, a similar previous patch caused a perf regression so I don't know how low risk it is

Good enough reason to wait IMO.
Comment 20 Ryan VanderMeulen [:RyanVM] 2012-04-24 15:41:33 PDT
Landed by itself in case of any perf regressions.
https://hg.mozilla.org/integration/mozilla-inbound/rev/08008f60f4e5

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