Remove nsXULPrototypeElement::mScriptTypeID

RESOLVED FIXED in mozilla14

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Ms2ger, Assigned: capella)

Tracking

Trunk
mozilla14
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
Mark, it would be nice if you had time for this; if not, I can do it myself
(Assignee)

Comment 1

5 years ago
Looks like a follow-up to Bug 738380 - Remove nsINode::GetScriptTypeID/SetScriptTypeID

I'm gonna run with it.
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
(Assignee)

Comment 2

5 years ago
Created attachment 611693 [details] [diff] [review]
Patch (v1)

Took a big axe and started chopping ... thought I'd have to make changes into nsXULPrototypeCache.* but it didn't seem to be required. 

Let me know if otherwise, or if there's any other obvious errors, but it built and ran mochitest-plain and mochitest-a11y locally just fine.
Attachment #611693 - Flags: review?(Ms2ger)
(Assignee)

Comment 3

5 years ago
Ahhh... let's see if my L1 works ...
Whiteboard: [autoland-try]

Updated

5 years ago
Whiteboard: [autoland-try] → [autoland-in-queue]

Comment 4

5 years ago
Autoland Patchset:
	Patches: 611693
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=d732f6d97452
Try run started, revision d732f6d97452. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=d732f6d97452

Comment 5

5 years ago
Try run for d732f6d97452 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=d732f6d97452
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-d732f6d97452

Updated

5 years ago
Whiteboard: [autoland-in-queue]
(Reporter)

Comment 6

5 years ago
Comment on attachment 611693 [details] [diff] [review]
Patch (v1)

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

Looks great!
Attachment #611693 - Flags: review?(jst)
Attachment #611693 - Flags: review?(Ms2ger)
Attachment #611693 - Flags: feedback+
Comment on attachment 611693 [details] [diff] [review]
Patch (v1)

- In nsXULPrototypeElement::Serialize():

     // Write script language
-    rv |= aStream->Write32(mScriptTypeID);
+    PRUint32 scriptId = 0;
+    rv |= aStream->Write32(scriptId);

This ends up caching a zero for no reason. We should stop writing this out and stop reading it completely rather than writing dummy values to disk here.

r=jst for this patch, but I'd like to see this fixed up before this lands.
Attachment #611693 - Flags: review?(jst) → review+
Neil, the attached patch here removes the need to store the value of a member of XUL elements in the startup cache. What's the versioning scheme or what not for the startup cache here, can we just stop writing out this data, or do we need to bump a version number or something else to have this code working correctly if we change what data is being cached?
The cache is invalidated whenever a different build is run, so it shouldn't matter.
Perfect, thanks! Mark, that means you can simply take out the code that reads/writes the script type to the startup cache.
(Assignee)

Comment 11

5 years ago
Created attachment 612053 [details] [diff] [review]
Patch (v5)

New version, minus the read/write32 bytes...
Comment on attachment 612053 [details] [diff] [review]
Patch (v5)

Looks good! And this looks ready for someone to check in, no?
Attachment #612053 - Flags: review+
(Assignee)

Comment 13

5 years ago
Comment on attachment 612053 [details] [diff] [review]
Patch (v5)

Please !
Attachment #612053 - Flags: checkin?(jst)
https://hg.mozilla.org/integration/mozilla-inbound/rev/633ddbb60390
https://hg.mozilla.org/mozilla-central/rev/633ddbb60390
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
Depends on: 744297

Updated

5 years ago
Attachment #612053 - Flags: checkin?(jst) → checkin+
You need to log in before you can comment on or make changes to this bug.