The default bug view has changed. See this FAQ.

Remove nsXULPrototypeScript::ScriptObjectHolder::mLangID

RESOLVED FIXED in mozilla15

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Ms2ger, Assigned: capella)

Tracking

Trunk
mozilla15
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

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

Comment 1

5 years ago
Can't hurt to try ....
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
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.
(Reporter)

Comment 3

5 years ago
(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.
No longer blocks: 744103
(Assignee)

Comment 4

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

Updated

5 years ago
Whiteboard: [good first bug][mentor=Ms2ger][lang=c++] → [autoland][good first bug][mentor=Ms2ger][lang=c++]
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...
(Assignee)

Updated

5 years ago
Whiteboard: [autoland][good first bug][mentor=Ms2ger][lang=c++] → [good first bug][mentor=Ms2ger][lang=c++]
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.
(Assignee)

Comment 7

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

Comment 8

5 years ago
Created attachment 615174 [details] [diff] [review]
Rebased patch

Rebased over my changes
Attachment #614678 - Attachment is obsolete: true
(Assignee)

Comment 9

5 years ago
This looks good. Will you run with it from here? Make sure it tests ... get it reviewed and pushed, etc?
(Reporter)

Comment 10

5 years ago
Either way works for me; let me know?
(Assignee)

Comment 11

5 years ago
Re-built and tested locally ... going for TRY run...
Whiteboard: [good first bug][mentor=Ms2ger][lang=c++] → [autoland-try][good first bug][mentor=Ms2ger][lang=c++]

Updated

5 years ago
Whiteboard: [autoland-try][good first bug][mentor=Ms2ger][lang=c++] → [autoland-in-queue][good first bug][mentor=Ms2ger][lang=c++]

Comment 12

5 years ago
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

5 years ago
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

Updated

5 years ago
Whiteboard: [autoland-in-queue][good first bug][mentor=Ms2ger][lang=c++] → [good first bug][mentor=Ms2ger][lang=c++]
(Assignee)

Comment 14

5 years ago
Comment on attachment 615174 [details] [diff] [review]
Rebased patch

TRY Builds all good ... requesting review from JST...
Attachment #615174 - Flags: review?(jst)
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.
(Assignee)

Comment 16

5 years ago
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.
Attachment #615174 - Attachment is obsolete: true
Attachment #615174 - Flags: review?(jst)
Attachment #615625 - Flags: review?(jst)
Comment on attachment 615625 [details] [diff] [review]
Rebased Patch + JS MACRO Nit

Looks good!
Attachment #615625 - Flags: review?(jst) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Attachment #615625 - Flags: approval-mozilla-central?
This is just a refactoring that is not fixing a regression or whatnot.  I think it  can just land on birch.
Attachment #615625 - Flags: approval-mozilla-central?
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.
Landed by itself in case of any perf regressions.
https://hg.mozilla.org/integration/mozilla-inbound/rev/08008f60f4e5
Flags: in-testsuite-
Keywords: checkin-needed
Target Milestone: --- → mozilla15
Blocks: 744103
https://hg.mozilla.org/mozilla-central/rev/08008f60f4e5
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 749367
You need to log in before you can comment on or make changes to this bug.