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]
:
: Andrew Overholt [:overholt]
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 User image :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 User image 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 User image 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 User image Mark Capella [:capella] 2012-04-02 19:21:09 PDT
Ahhh... let's see if my L1 works ...
Comment 4 User image 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 User image 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 User image :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 User image 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 User image 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 User image Neil Deakin 2012-04-03 16:49:00 PDT
The cache is invalidated whenever a different build is run, so it shouldn't matter.
Comment 10 User image 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 User image 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 User image 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 User image Mark Capella [:capella] 2012-04-03 19:30:48 PDT
Comment on attachment 612053 [details] [diff] [review]
Patch (v5)

Please !
Comment 14 User image Johnny Stenback (:jst, jst@mozilla.com) 2012-04-05 12:21:19 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/633ddbb60390
Comment 15 User image 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.