Last Comment Bug 741580 - Remove nsXULPrototypeElement::mScriptTypeID
: Remove nsXULPrototypeElement::mScriptTypeID
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Mark Capella [:capella]
:
:
Mentors:
Depends on: 744297
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-02 14:30 PDT by :Ms2ger (⌚ UTC+1/+2)
Modified: 2012-04-25 09:43 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (15.49 KB, patch)
2012-04-02 19:12 PDT, Mark Capella [:capella]
jst: review+
Ms2ger: feedback+
Details | Diff | Splinter Review
Patch (v5) (15.67 KB, patch)
2012-04-03 18:36 PDT, Mark Capella [:capella]
jst: review+
jst: checkin+
Details | Diff | Splinter Review

Description :Ms2ger (⌚ UTC+1/+2) 2012-04-02 14:30:10 PDT
Mark, it would be nice if you had time for this; if not, I can do it myself
Comment 1 Mark Capella [:capella] 2012-04-02 14:33:55 PDT
Looks like a follow-up to Bug 738380 - Remove nsINode::GetScriptTypeID/SetScriptTypeID

I'm gonna run with it.
Comment 2 Mark Capella [:capella] 2012-04-02 19:12:59 PDT
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.
Comment 3 Mark Capella [:capella] 2012-04-02 19:21:09 PDT
Ahhh... let's see if my L1 works ...
Comment 4 Mozilla RelEng Bot 2012-04-02 19:23:56 PDT
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 Mozilla RelEng Bot 2012-04-02 23:31:33 PDT
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
Comment 6 :Ms2ger (⌚ UTC+1/+2) 2012-04-03 00:31:23 PDT
Comment on attachment 611693 [details] [diff] [review]
Patch (v1)

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

Looks great!
Comment 7 Johnny Stenback (:jst, jst@mozilla.com) 2012-04-03 16:40:39 PDT
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.
Comment 8 Johnny Stenback (:jst, jst@mozilla.com) 2012-04-03 16:44:52 PDT
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?
Comment 9 Neil Deakin (away until Oct 3) 2012-04-03 16:49:00 PDT
The cache is invalidated whenever a different build is run, so it shouldn't matter.
Comment 10 Johnny Stenback (:jst, jst@mozilla.com) 2012-04-03 16:53:19 PDT
Perfect, thanks! Mark, that means you can simply take out the code that reads/writes the script type to the startup cache.
Comment 11 Mark Capella [:capella] 2012-04-03 18:36:59 PDT
Created attachment 612053 [details] [diff] [review]
Patch (v5)

New version, minus the read/write32 bytes...
Comment 12 Johnny Stenback (:jst, jst@mozilla.com) 2012-04-03 19:29:23 PDT
Comment on attachment 612053 [details] [diff] [review]
Patch (v5)

Looks good! And this looks ready for someone to check in, no?
Comment 13 Mark Capella [:capella] 2012-04-03 19:30:48 PDT
Comment on attachment 612053 [details] [diff] [review]
Patch (v5)

Please !
Comment 14 Johnny Stenback (:jst, jst@mozilla.com) 2012-04-05 12:21:19 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/633ddbb60390
Comment 15 Matt Brubeck (:mbrubeck) 2012-04-06 11:33:03 PDT
https://hg.mozilla.org/mozilla-central/rev/633ddbb60390

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