Last Comment Bug 723937 - Make sure all the obviously private properties in Tilt start with an underscore
: Make sure all the obviously private properties in Tilt start with an underscore
Status: RESOLVED FIXED
[tilt]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: 12 Branch
: All All
: -- normal (vote)
: Firefox 13
Assigned To: Victor Porof [:vporof][:vp]
:
: Patrick Brosset <:pbro>
Mentors:
Depends on: 723588
Blocks: 726634
  Show dependency treegraph
 
Reported: 2012-02-03 07:30 PST by Victor Porof [:vporof][:vp]
Modified: 2012-02-27 01:11 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (56.31 KB, patch)
2012-02-07 01:19 PST, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v1.1 (56.37 KB, patch)
2012-02-07 01:49 PST, Victor Porof [:vporof][:vp]
rcampbell: review+
Details | Diff | Splinter Review

Description Victor Porof [:vporof][:vp] 2012-02-03 07:30:56 PST
Even if this rather useless "refactoring" may seem like a waste of time, right now there are some horrible inconsistencies across Tilt's codebase, which may cause confusions. Also, plenty things are obviously not to be messed with, like functions or flags used only for tests, so it would be a good idea to clean those up at some point.
Comment 1 Victor Porof [:vporof][:vp] 2012-02-07 01:19:10 PST
Created attachment 594951 [details] [diff] [review]
v1

Apart from the symmetrical chunks of lines where properties are now starting with an underscore, I moved a "let finalize = ..." function declaration outside from T_destroy to keep it as clean as possible, moved the LayoutHelpers import into a temp scope from one of the tests and fixed some javascript errors triggered only when running the arcball-reset tests.
Comment 2 Victor Porof [:vporof][:vp] 2012-02-07 01:49:27 PST
Created attachment 594956 [details] [diff] [review]
v1.1

It seems Dão already made the LayoutHelpers change, which will make a hunk fail for the v1 patch. Updated.
Comment 3 Rob Campbell [:rc] (:robcee) 2012-02-08 13:00:35 PST
Comment on attachment 594956 [details] [diff] [review]
v1.1

+    // if the visualizer is destroyed or destroying, don't do anything
+    if (!this.visualizers[aId] || this._isDestroying) {
+      return;
+    }

this sounds like a little extra from just adding some underscores to method names. Were you getting races on shutdown?

The rest looks as expected so r+ with a try run.
Comment 4 Rob Campbell [:rc] (:robcee) 2012-02-08 14:12:38 PST
yes, I am dumb for not reading comment #1. I inferred it from the code ALL BY MYSELF!
Comment 5 Victor Porof [:vporof][:vp] 2012-02-09 00:21:16 PST
https://tbpl.mozilla.org/?tree=Try&rev=843a6a70c862
Comment 6 Rob Campbell [:rc] (:robcee) 2012-02-24 06:49:21 PST
https://hg.mozilla.org/integration/fx-team/rev/e1658e01d6c4
Comment 7 Tim Taubert [:ttaubert] 2012-02-27 01:11:33 PST
https://hg.mozilla.org/mozilla-central/rev/e1658e01d6c4

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