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
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]
: Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ)
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:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

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 User image 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 User image Victor Porof [:vporof][:vp] 2012-02-07 01:19:10 PST
Created attachment 594951 [details] [diff] [review]

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 User image Victor Porof [:vporof][:vp] 2012-02-07 01:49:27 PST
Created attachment 594956 [details] [diff] [review]

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

+    // 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 User image 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 User image Victor Porof [:vporof][:vp] 2012-02-09 00:21:16 PST
Comment 6 User image Rob Campbell [:rc] (:robcee) 2012-02-24 06:49:21 PST
Comment 7 User image Tim Taubert [:ttaubert] 2012-02-27 01:11:33 PST

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