Make sure all the obviously private properties in Tilt start with an underscore

RESOLVED FIXED in Firefox 13

Status

()

Firefox
Developer Tools: Inspector
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: vporof, Assigned: vporof)

Tracking

12 Branch
Firefox 13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tilt])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Whiteboard: [tilt]
(Assignee)

Updated

5 years ago
Depends on: 723588
(Assignee)

Comment 1

5 years ago
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.
Attachment #594951 - Flags: review?(rcampbell)
(Assignee)

Comment 2

5 years ago
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.
(Assignee)

Updated

5 years ago
Attachment #594956 - Flags: review?(rcampbell)
(Assignee)

Updated

5 years ago
Attachment #594951 - Flags: review?(rcampbell)
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.
Attachment #594956 - Flags: review?(rcampbell) → review+
Whiteboard: [tilt] → [tilt][land-pending-try]
yes, I am dumb for not reading comment #1. I inferred it from the code ALL BY MYSELF!
(Assignee)

Comment 5

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=843a6a70c862
Whiteboard: [tilt][land-pending-try] → [tilt]
(Assignee)

Updated

5 years ago
Blocks: 726634
(Assignee)

Updated

5 years ago
Whiteboard: [tilt] → [tilt][land-in-fx-team]
(Assignee)

Updated

5 years ago
Attachment #594951 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/e1658e01d6c4
Whiteboard: [tilt][land-in-fx-team] → [tilt][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/e1658e01d6c4
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [tilt][fixed-in-fx-team] → [tilt]
Target Milestone: --- → Firefox 13
You need to log in before you can comment on or make changes to this bug.