744332 - Remove nsXULPrototypeScript::ScriptObjectHolder::mLangID

Status: RESOLVED FIXED

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

Description

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

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

Can't hurt to try ....

Comment 2

[Andrew McCreight [:mccr8]]

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

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

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

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

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

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

Attachment: Rebased patch

Rebased over my changes

Comment 9

[Mark Capella [:capella]]

This looks good. Will you run with it from here? Make sure it tests ... get it reviewed and pushed, etc?

Comment 10

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

Either way works for me; let me know?

Comment 11

[Mark Capella [:capella]]

Re-built and tested locally ... going for TRY run...

Comment 12

[Mozilla RelEng Bot]

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]

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/[e-mail]-f23ca2aed382

Comment 14

[Mark Capella [:capella]]

Comment on attachment 615174 [details] [diff] [review]
Rebased patch

TRY Builds all good ... requesting review from JST...

Comment 15

[Andrew McCreight [:mccr8]]

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

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

Comment on attachment 615625 [details] [diff] [review]
Rebased Patch + JS MACRO Nit

Looks good!

Comment 18

[Andrew McCreight [:mccr8]]

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

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

Landed by itself in case of any perf regressions.
https://hg.mozilla.org/integration/mozilla-inbound/rev/08008f60f4e5

Comment 21

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/08008f60f4e5